Copilot commented on code in PR #10968:
URL: https://github.com/apache/cloudstack/pull/10968#discussion_r2310149707
##########
engine/schema/src/main/java/com/cloud/network/dao/LoadBalancerVO.java:
##########
@@ -136,4 +136,8 @@ public Scheme getScheme() {
public String getCidrList() {
return cidrList;
}
+
Review Comment:
The setCidrList method should include a JavaDoc comment explaining its
purpose, parameters, and any constraints, especially since this is a new public
API method.
##########
server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java:
##########
@@ -2282,6 +2298,9 @@ public LoadBalancer
updateLoadBalancerRule(UpdateLoadBalancerRuleCmd cmd) {
if (lbBackup.getAlgorithm() != null) {
lb.setAlgorithm(lbBackup.getAlgorithm());
}
+ if (lbBackup.getCidrList() != null || lb.getCidrList() !=
null) { // Added for cidrlist rollback
+ lb.setCidrList(lbBackup.getCidrList());
+ }
Review Comment:
The rollback condition `(lbBackup.getCidrList() != null || lb.getCidrList()
!= null)` will always execute the rollback when either value is non-null, even
when no change occurred. This should be removed and the rollback should always
restore the backup value: `lb.setCidrList(lbBackup.getCidrList());`
```suggestion
lb.setCidrList(lbBackup.getCidrList());
```
##########
server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java:
##########
@@ -2260,8 +2270,14 @@ public LoadBalancer
updateLoadBalancerRule(UpdateLoadBalancerRuleCmd cmd) {
LoadBalancerVO tmplbVo = _lbDao.findById(lbRuleId);
boolean success = _lbDao.update(lbRuleId, lb);
- // If algorithm is changed, have to reapply the lb config
- if ((algorithm != null) &&
(tmplbVo.getAlgorithm().compareTo(algorithm) != 0)){
+ // Check if algorithm or cidrlist has changed, and reapply the lb
config if needed
+ boolean algorithmChanged = (algorithm != null) &&
(tmplbVo.getAlgorithm().compareTo(algorithm) != 0);
+ boolean cidrListChanged = (tmplbVo.getCidrList() == null &&
lb.getCidrList() != null) ||
+ (tmplbVo.getCidrList() != null &&
lb.getCidrList() == null) ||
+ (tmplbVo.getCidrList() != null &&
lb.getCidrList() != null &&
+
!tmplbVo.getCidrList().equals(lb.getCidrList()));
Review Comment:
The CIDR list change detection logic is complex and hard to read. Consider
using Objects.equals() for a cleaner null-safe comparison: `boolean
cidrListChanged = !Objects.equals(tmplbVo.getCidrList(), lb.getCidrList());`
--
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: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
