Re: [PR] Management ip address resolution [cloudstack]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #13603:
URL: https://github.com/apache/cloudstack/pull/13603#discussion_r3749239179


##########
agent/src/main/java/com/cloud/agent/properties/AgentProperties.java:
##########
@@ -108,6 +108,13 @@ public class AgentProperties{
      */
     public static final Property<String> PRIVATE_NETWORK_DEVICE = new Property<>("private.network.device", "cloudbr1");
 
+    /**
+     * Private NIC device address. If this property is commented, it will be autodetected on service startup.<br>
+     * Data type: String.<br>
+     * Default value: <code>cloudbr1</code>

Review Comment:
   The Javadoc for PRIVATE_NETWORK_DEVICE_ADDRESS says the default value is cloudbr1, but the property default is actually null (and cloudbr1 is the default for private.network.device, not an IP address). This can mislead operators configuring the new setting.



##########
core/src/main/java/com/cloud/resource/ServerResourceBase.java:
##########
@@ -106,8 +117,74 @@ protected void defineResourceNetworkInterfaces(Map<String, Object> params) {
         this.storageNic2 = NetUtils.getNetworkInterface(storageNic2);
     }
 
+    private void checkForPrivateInterfaceDefinedByIp(Map<String, Object> params) {
+        final String ifAddr = (String) params.get("private.network.address");
+        if (ifAddr != null) {
+            logger.debug(String.format("Trying to use private address to resolve interface: [%s]", ifAddr));
+            try {
+                InetAddress rawAddr = InetAddress.getByAddress(InetAddress.getByName(ifAddr).getAddress());
+                final NetworkInterface nic = NetworkInterface.getByInetAddress(rawAddr);
+                if (nic != null) {
+                    logger.info(String.format("Using NIC [%s] as private NIC. Source: InterfaceAddress [%s]", nic, ifAddr));
+                    privateNic = nic;
+                } else {
+                    logger.info(String.format("Unable to found private NIC with defined ip [%s]", ifAddr));
+                }
+            } catch (Throwable e) {
+                 // Logging only, if this method was unable to find a valid interface, iteration will be tested
+                logger.info(String.format("Unable to use private address to get the management interface: [%s]", e.getMessage()));
+            }

Review Comment:
   Catching Throwable here can swallow serious JVM errors and makes troubleshooting harder. Also, the log message has a grammatical error ("Unable to found"). Prefer catching Exception and include the exception as a parameter so the stack trace is available in logs.
   
   This issue also appears on line 179 of the same file.



##########
core/src/main/java/com/cloud/resource/ServerResourceBase.java:
##########
@@ -106,8 +117,74 @@ protected void defineResourceNetworkInterfaces(Map<String, Object> params) {
         this.storageNic2 = NetUtils.getNetworkInterface(storageNic2);
     }
 
+    private void checkForPrivateInterfaceDefinedByIp(Map<String, Object> params) {
+        final String ifAddr = (String) params.get("private.network.address");
+        if (ifAddr != null) {
+            logger.debug(String.format("Trying to use private address to resolve interface: [%s]", ifAddr));

Review Comment:
   checkForPrivateInterfaceDefinedByIp should treat blank/whitespace values as unset. In Java, InetAddress.getByName("") resolves to the loopback address, which could cause the loopback NIC to be selected if the property is present but empty.



##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -1007,6 +1007,8 @@ protected String getDefaultTungstenScriptsDir() {
 
     @Override
     public boolean configure(final String name, final Map<String, Object> params) throws ConfigurationException {
+        params.put(AgentProperties.HOST.getName(), AgentPropertiesFileHandler.getPropertyValue(AgentProperties.HOST));
+        params.put(AgentProperties.PRIVATE_NETWORK_DEVICE_ADDRESS.getName(), AgentPropertiesFileHandler.getPropertyValue(AgentProperties.PRIVATE_NETWORK_DEVICE_ADDRESS));

Review Comment:
   These unconditional params.put(...) calls override any command-line parameters that Agent.java intentionally merged into params (e.g., starting the agent with host=... or private.network.address=...). They can also overwrite a provided value with a default (e.g., private.network.address -> null), effectively disabling overrides. Prefer only setting these when absent.



##########
core/src/main/java/com/cloud/resource/ServerResourceBase.java:
##########
@@ -75,6 +80,12 @@ public boolean configure(final String name, Map<String, Object> params) throws C
 
         defineResourceNetworkInterfaces(params);
 
+        if (privateNic == null) {
+            checkForPrivateInterfaceDefinedByIp(params);
+        }
+        if (privateNic == null) {
+            tryToAutoDiscoverResourcePrivateNetworkInterfaceByRouteLookup(params);
+        }
         if (privateNic == null) {
             tryToAutoDiscoverResourcePrivateNetworkInterface();
         }

Review Comment:
   The new private-NIC resolution paths (explicit IP / route lookup) only run when privateNic is null. For KVM, defineResourceNetworkInterfaces() typically returns a non-null NIC even when it has no usable IP (e.g., bridge without an address), so these new methods will be skipped and configure() will still fail later when NetUtils.getNetworkParams(privateNic) returns null. Consider treating a NIC with no usable address as “unresolved” and/or always honoring an explicitly configured private.network.address.
   
   This issue also appears on line 83 of the same file.



-- 
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-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org