review comments and cleanup

Author: Daan Hoogland

api/src/main/java/org/apache/cloudstack/api/response/RouterHealthCheckResultResponse.java

@@ -19,7 +19,7 @@
 
 import java.util.Date;
 
-import com.cloud.network.VirtualNetworkApplianceService;
+import com.cloud.network.VirtualNetworkApplianceService.RouterHealthStatus;
 import org.apache.cloudstack.api.ApiConstants;
 import org.apache.cloudstack.api.BaseResponse;
 
@@ -37,7 +37,7 @@ public class RouterHealthCheckResultResponse extends BaseResponse {
 
     @SerializedName(ApiConstants.SUCCESS)
     @Param(description = "result of the health check")
-    private VirtualNetworkApplianceService.RouterHealthStatus result;
+    private RouterHealthStatus result;
 
     @SerializedName(ApiConstants.LAST_UPDATED)
     @Param(description = "the date this VPC was created")
@@ -55,7 +55,7 @@ public String getCheckType() {
         return checkType;
     }
 
-    public VirtualNetworkApplianceService.RouterHealthStatus getResult() {
+    public RouterHealthStatus getResult() {
         return result;
     }
 
@@ -75,7 +75,7 @@ public void setCheckType(String checkType) {
         this.checkType = checkType;
     }
 
-    public void setResult(VirtualNetworkApplianceService.RouterHealthStatus result) {
+    public void setResult(RouterHealthStatus result) {
         this.result = result;
     }
 

server/src/main/java/com/cloud/network/router/CommandSetupHelper.java

@@ -547,10 +547,9 @@ public void createFirewallRulesCommands(final List<? extends FirewallRule> rules
         String systemRule = null;
         Boolean defaultEgressPolicy = false;
         if (rules != null) {
-            if (!rules.isEmpty()) {
-                if (rules.get(0).getTrafficType() == FirewallRule.TrafficType.Egress && rules.get(0).getType() == FirewallRule.FirewallRuleType.System) {
-                    systemRule = String.valueOf(FirewallRule.FirewallRuleType.System);
-                }
+            boolean isSystemFirewallEgressRule = !rules.isEmpty() && rules.get(0).getTrafficType() == FirewallRule.TrafficType.Egress && rules.get(0).getType() == FirewallRule.FirewallRuleType.System;
+            if (isSystemFirewallEgressRule) {
+                systemRule = String.valueOf(FirewallRule.FirewallRuleType.System);
             }
             for (final FirewallRule rule : rules) {
                 _rulesDao.loadSourceCidrs((FirewallRuleVO) rule);

server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java

@@ -74,6 +74,7 @@
 import org.apache.cloudstack.utils.CloudStackVersion;
 import org.apache.cloudstack.utils.identity.ManagementServerNode;
 import org.apache.cloudstack.utils.usage.UsageUtils;
+import org.apache.commons.collections4.CollectionUtils;
 import org.apache.commons.lang3.ObjectUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.jetbrains.annotations.NotNull;
@@ -811,9 +812,10 @@ protected void updateSite2SiteVpnConnectionState(final List<DomainRouterVO> rout
             }
             final String privateIP = router.getPrivateIpAddress();
             final HostVO host = _hostDao.findById(router.getHostId());
-            if ( !(host == null || host.getState() != Status.Up)
+            boolean hostAvailable =  !(host == null || host.getState() != Status.Up)
                     && (host.getManagementServerId() == ManagementServerNode.getManagementServerId())
-                    && (privateIP != null)) {
+                    && (privateIP != null);
+            if (hostAvailable) {
                 final CheckS2SVpnConnectionsCommand command = new CheckS2SVpnConnectionsCommand(ipList);
                 command.setAccessDetail(NetworkElementCommand.ROUTER_IP, _routerControlHelper.getRouterControlIp(router.getId()));
                 command.setAccessDetail(NetworkElementCommand.ROUTER_NAME, router.getInstanceName());
@@ -1041,12 +1043,7 @@ protected void runInContext() {
                         continue;
                     }
 
-                    DomainRouterVO router;
-                    if (router0.getId() < router1.getId()) {
-                        router = router0;
-                    } else {
-                        router = router1;
-                    }
+                    DomainRouterVO router = (router0.getId() < router1.getId()) ? router0 : router1;
                     // && router.getState() == VirtualMachine.State.Stopped
                     if (router.getHostId() == null && router.getState() == VirtualMachine.State.Running) {
                         logger.debug("Skip router pair (" + router0.getInstanceName() + "," + router1.getInstanceName() + ") due to can't find host");
@@ -1156,7 +1153,7 @@ private List<String> getFailingChecks(DomainRouterVO router, GetRouterMonitorRes
     }
 
     private void handleFailingChecks(DomainRouterVO router, List<String> failingChecks) {
-        if (failingChecks == null || failingChecks.isEmpty()) {
+        if (CollectionUtils.isEmpty(failingChecks)) {
             return;
         }
 
@@ -1404,7 +1401,6 @@ private void updateDbHealthChecksFromRouterResponse(final DomainRouterVO router,
         } catch (JsonSyntaxException ex) {
             logger.error("Unable to parse the result of health checks due to " + ex.getLocalizedMessage(), ex);
         }
-        return;
     }
 
     private GetRouterMonitorResultsAnswer fetchAndUpdateRouterHealthChecks(DomainRouterVO router, boolean performFreshChecks) {
@@ -1420,7 +1416,7 @@ private GetRouterMonitorResultsAnswer fetchAndUpdateRouterHealthChecks(DomainRou
             try {
                 final Answer answer = _agentMgr.easySend(router.getHostId(), command);
 
-                logger.info("Got health check results from router {}: {}", router.getHostName(), answer != null ? answer.getDetails() : "null answer");
+                logger.debug("Got health check results from router {}: {}", router.getHostName(), answer != null ? answer.getDetails() : "null answer");
                 if (answer == null) {
                     logger.warn("Unable to fetch monitoring results data from router {}", router.getHostName());
                     return null;
@@ -1484,7 +1480,7 @@ public Pair<Boolean, String> performRouterHealthChecks(long routerId) {
             throw new CloudRuntimeException("Router health checks are not enabled for router: " + router);
         }
 
-        logger.info("Running health check results for router " + router.getUuid());
+        logger.debug("Running health check results for router " + router.getUuid());
 
         GetRouterMonitorResultsAnswer answer;
         String resultDetails = "";
@@ -1493,11 +1489,11 @@ public Pair<Boolean, String> performRouterHealthChecks(long routerId) {
         // Step 1: Perform basic tests to check the connectivity and file system on router
         answer = performBasicTestsOnRouter(router);
         if (answer == null) {
-            logger.debug("No results received for the basic tests on router: " + router);
+            logger.info("No results received for the basic tests on router: " + router);
             resultDetails = "Basic tests results unavailable";
             success = false;
         } else if (!answer.getResult()) {
-            logger.debug("Basic tests failed on router: " + router);
+            logger.warn("Basic tests failed on router: " + router);
             resultDetails = "Basic tests failed - " + answer.getMonitoringResults();
             success = false;
         } else {
@@ -1624,15 +1620,16 @@ private boolean updateRouterHealthChecksConfig(DomainRouterVO router) {
     }
 
     private String getSystemThresholdsHealthChecksData(final DomainRouterVO router) {
-        return "minDiskNeeded=" + RouterHealthChecksFreeDiskSpaceThreshold.valueIn(router.getDataCenterId()) +
-                ",maxCpuUsage=" + RouterHealthChecksMaxCpuUsageThreshold.valueIn(router.getDataCenterId()) +
-                ",maxMemoryUsage=" + RouterHealthChecksMaxMemoryUsageThreshold.valueIn(router.getDataCenterId()) + ";";
+        return String.format("minDiskNeeded=%s,maxCpuUsage=%s,maxMemoryUsage=%s;",
+                RouterHealthChecksFreeDiskSpaceThreshold.valueIn(router.getDataCenterId()),
+                RouterHealthChecksMaxCpuUsageThreshold.valueIn(router.getDataCenterId()),
+                RouterHealthChecksMaxMemoryUsageThreshold.valueIn(router.getDataCenterId()));
     }
 
     private String getRouterVersionHealthChecksData(final DomainRouterVO router) {
         if (router.getTemplateVersion() != null && router.getScriptsVersion() != null) {
-            return "templateVersion=" + router.getTemplateVersion() +
-                    ",scriptsVersion=" + router.getScriptsVersion();
+            return String.format("templateVersion=%s,scriptsVersion=%s", router.getTemplateVersion(),
+                    router.getScriptsVersion());
         }
         return null;
     }

server/src/test/java/com/cloud/vpc/MockConfigurationManagerImpl.java

@@ -96,14 +96,6 @@
 
 @Component
 public class MockConfigurationManagerImpl extends ManagerBase implements ConfigurationManager, ConfigurationService {
-    public static final ConfigKey<Integer> NETWORK_LB_HAPROXY_MAX_CONN = new ConfigKey<>(
-            "Network",
-            Integer.class,
-            "network.loadbalancer.haproxy.max.conn",
-            "4096",
-            "Load Balancer(haproxy) maximum number of concurrent connections(global max)",
-            true,
-            ConfigKey.Scope.Global);
     @Inject
     NetworkOfferingDaoImpl _ntwkOffDao;