bhouse-nexthop commented on code in PR #14131:
URL: https://github.com/apache/cloudstack/pull/14131#discussion_r4273543852
##########
server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java:
##########
@@ -2348,6 +2421,12 @@ public LoadBalancer
updateLoadBalancerRule(UpdateLoadBalancerRuleCmd cmd) {
throw new
InvalidParameterValueException(String.format("Modifications in lb rule %s are
not supported.", lb));
}
+ // The settings are written straight to the details table, unlike the
fields above that wait
+ // for _lbDao.update, so they are stored only once nothing else can
reject the update
+ Map<String, String> settingsBackup = getConnectionSettings(lbRuleId);
+ boolean settingsChanged =
updateLoadBalancerConnectionSettings(lbRuleId, cmd.getKeepAlive(),
cmd.getIdleTimeout(), cmd.getKeepAliveTimeout(),
Review Comment:
Fixed in 4433e91187: the test now sets the rule id on the command and stubs
`findById`/`update` with it. It also mocks `FirewallRuleDetailsDao`, which the
settings are read from. I left the production code as it is: `id` is `required
= true` on the API, and `lbRuleId` here is what @DaanHoogland asked for
earlier, so only the test could reach the null. `testRuleNotValidated` used to
pass only because the null id missed the lookup. It now finds the rule and
fails on validation, as it was meant to. The other red checks were Sonar (same
test) and a simulator smoke failure where the SSVM never reached Running, which
this PR does not touch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
