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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
