bhouse-nexthop commented on code in PR #14131:
URL: https://github.com/apache/cloudstack/pull/14131#discussion_r4208822665
##########
server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java:
##########
@@ -2342,6 +2382,9 @@ public LoadBalancer
updateLoadBalancerRule(UpdateLoadBalancerRuleCmd cmd) {
lb.setCidrList(cidrListStr);
}
+ // lb.getId() rather than the id off the command, which is a Long and
unboxes badly
+ boolean settingsChanged =
updateLoadBalancerConnectionSettings(lb.getId(), cmd.getKeepAlive(),
cmd.getIdleTimeout(), cmd.getKeepAliveTimeout());
Review Comment:
Agreed, the comment was wrong: `lb` is found by `lbRuleId` and is non-null
here, so the unboxing is safe. Switched to `lbRuleId` in b5ff105b8e, matching
`applyLoadBalancerConfig(lbRuleId)` further down. They are the same id though:
`LoadBalancerVO` extends `FirewallRuleVO` and joins `load_balancing_rules` on
`id`, so `lb.getId()` is the `firewall_rules` id, i.e. the rule id. A cleanup,
not a behaviour change.
##########
server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java:
##########
@@ -2285,6 +2289,42 @@ public List<LbDestination> getExistingDestinations(long
lbId) {
return dstList;
}
+ /**
+ * Haproxy rejects a negative timeout, and a rejected file leaves every
rule on the router
+ * running its previous config. Refuse the value here rather than let it
reach the VR.
+ */
+ protected void validateConnectionTimeout(String name, Long value) {
+ if (value != null && value < 0) {
+ throw new InvalidParameterValueException(String.format("%s must be
0 or greater, got [%s]. 0 means no timeout.", name, value));
+ }
+ }
+
+ @Override
+ public boolean updateLoadBalancerConnectionSettings(long lbRuleId, Boolean
keepAlive, Long idleTimeout, Long keepAliveTimeout) {
+ validateConnectionTimeout(ApiConstants.IDLE_TIMEOUT, idleTimeout);
+ validateConnectionTimeout(ApiConstants.KEEPALIVE_TIMEOUT,
keepAliveTimeout);
+
+ boolean changed = storeDetail(lbRuleId, LoadBalancer.KEEPALIVE,
keepAlive == null ? null : keepAlive.toString());
+ changed |= storeDetail(lbRuleId, LoadBalancer.IDLE_TIMEOUT,
idleTimeout == null ? null : idleTimeout.toString());
+ changed |= storeDetail(lbRuleId, LoadBalancer.KEEPALIVE_TIMEOUT,
keepAliveTimeout == null ? null : keepAliveTimeout.toString());
+ return changed;
+ }
+
+ private boolean storeDetail(long lbRuleId, String key, String value) {
+ if (value == null) {
Review Comment:
Right, there was no way back: leaving it out keeps the value, negatives are
refused, and keepalive is a boolean. e974792cfa adds
`cleanupconnectionsettings` to updateLoadBalancerRule, following
`cleanupdetails` on updateVirtualMachine/updateTemplate. It drops the rule's
own keepalive/idletimeout/keepalivetimeout, then sets again any passed in the
same call, so one call can clear one and keep the rest. 07af93829d makes the
edit dialog send it when a field the rule had set is cleared.
##########
server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java:
##########
@@ -2342,6 +2382,9 @@ public LoadBalancer
updateLoadBalancerRule(UpdateLoadBalancerRuleCmd cmd) {
lb.setCidrList(cidrListStr);
}
+ // lb.getId() rather than the id off the command, which is a Long and
unboxes badly
+ boolean settingsChanged =
updateLoadBalancerConnectionSettings(lb.getId(), cmd.getKeepAlive(),
cmd.getIdleTimeout(), cmd.getKeepAliveTimeout());
Review Comment:
Yes, 5ae177450c restores the previous settings along with
name/description/algorithm/cidrs when the apply fails. Same commit moves the
settings write after `validateLbRule`: they went straight to the details table
before validation, so a rule the provider rejected kept them. Unit tests cover
both.
--
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]
