Copilot commented on code in PR #14333:
URL: https://github.com/apache/cloudstack/pull/14333#discussion_r4260654746
##########
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtGetVmIpAddressCommandWrapperTest.java:
##########
@@ -58,6 +58,24 @@ public class LibvirtGetVmIpAddressCommandWrapperTest {
" Loopback Pseudo-Interface 1 ipv6 ::1/128\n" + //
" - - ipv4 127.0.0.1/8\n";
+ // Windows guest that has not obtained a DHCP lease yet and self-assigned
an APIPA address
+ private static String VIRSH_DOMIF_OUTPUT_WINDOWS_APIPA = " Name MAC
address Protocol Address\n" + //
+
"-------------------------------------------------------------------------------\n"
+ //
+ " Ethernet 02:01:00:d0:00:87 ipv6
fe80::df84:2984:e396:8ac3%6/64\n" + //
+ " - - ipv4
169.254.149.120/16\n" + //
+ " Loopback Pseudo-Interface 1 ipv6
::1/128\n" + //
+ " - - ipv4 127.0.0.1/8\n";
+
+ // Same Windows guest once the DHCP lease has been obtained
+ private static String VIRSH_DOMIF_OUTPUT_WINDOWS_DHCP = " Name MAC
address Protocol Address\n" + //
+
"-------------------------------------------------------------------------------\n"
+ //
+ " Ethernet 02:01:00:d0:00:87 ipv6
fd00:ac19:0:1910:0:d3fc:4177:2c32/128\n" + //
+ " - - ipv6
fe80::df84:2984:e396:8ac3%6/64\n" + //
+ " - - ipv4 172.25.16.236/22\n"
+ //
+ " - - ipv4
169.254.149.120/16\n" + //
+ " Loopback Pseudo-Interface 1 ipv6
::1/128\n" + //
+ " - - ipv4 127.0.0.1/8\n";
Review Comment:
These test fixtures are constants and should be declared `private static
final` to prevent accidental reassignment and better communicate intent.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtGetVmIpAddressCommandWrapper.java:
##########
@@ -130,10 +130,20 @@ private Pair<String, String> getIpAddresses(String
output, String macAddress) {
found = true;
String ipFamily = parts[parts.length - 2];
String ipPart = parts[parts.length - 1].split("/")[0];
+ // Skip link-local addresses (e.g. Windows APIPA 169.254.0.0/16
before a DHCP lease is obtained),
+ // they are not routable and must not be reported as the instance
IP
if (ipFamily.equals("ipv4")) {
- ipv4 = ipPart;
+ if (NetUtils.isLinkLocalIp4(ipPart)) {
+ logger.debug(String.format("Ignoring link-local ipv4 %s
with mac address %s", ipPart, macAddress));
+ } else {
+ ipv4 = ipPart;
+ }
} else if (ipFamily.equals("ipv6")) {
- ipv6 = ipPart;
+ if (NetUtils.isLinkLocalIp6(ipPart)) {
+ logger.debug(String.format("Ignoring link-local ipv6 %s
with mac address %s", ipPart, macAddress));
+ } else {
+ ipv6 = ipPart;
+ }
Review Comment:
Avoid `String.format(...)` inside `logger.debug(...)` since it formats
eagerly even when debug logging is disabled. Prefer parameterized logging
(e.g., `logger.debug(\"... {} ... {}\", ipPart, macAddress)`) to reduce
overhead.
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -2522,6 +2533,13 @@ private void loadVmDetailsInMapForExternalDhcpIp() {
List<NicVO> nics = _nicDao.listByNetworkId(network.getId());
for (NicVO nic : nics) {
+ // a link-local (APIPA) address recorded before the guest
obtained its DHCP lease is not a valid instance IP,
+ // clear it so it is no longer exposed and the NIC is
retried as one without an IP
+ if (NetUtils.isLinkLocalIp4(nic.getIPv4Address())) {
+ logger.debug("Clearing link-local IP {} recorded for
NIC {}", nic.getIPv4Address(), nic);
+ nic.setIPv4Address(null);
+ _nicDao.update(nic.getId(), nic);
+ }
Review Comment:
The ‘clear APIPA and persist’ logic is duplicated with the retry loop
earlier in this class. Consider extracting a small helper (e.g.,
`clearIpv4IfLinkLocal(NicVO nic)`) to centralize the behavior and reduce the
chance of future divergence (e.g., logging/DAO update behavior changes in one
location but not the other).
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -808,6 +808,17 @@ protected void runInContext() {
nic.setIPv4Address(null);
_nicDao.update(nicId, nic);
}
+ } else if (NetUtils.isLinkLocalIp4(vmIp)) {
+ // a link-local (APIPA) address means the guest has
not obtained a DHCP lease yet; do not record it,
+ // clear any previously recorded link-local address
and keep retrying without consuming the retry budget
+ // (consistent with KVM, which reports this case as a
failed answer), so a late DHCP lease is still picked up
+ logger.debug("Instance [ID: {}, UUID: {}, name: {}] -
ignoring link-local IP {}, DHCP lease not obtained yet",
+ vmId, vmUuid, vmName, vmIp);
+ decrementCount = false;
+ if (nic != null &&
NetUtils.isLinkLocalIp4(nic.getIPv4Address())) {
+ nic.setIPv4Address(null);
+ _nicDao.update(nicId, nic);
+ }
Review Comment:
Setting `decrementCount = false` on APIPA can result in effectively
unbounded retries if a guest never obtains a DHCP lease (e.g., external DHCP
misconfiguration). Consider still decrementing after a bounded number of APIPA
sightings, or adding a maximum elapsed time/backoff cap for this case to
prevent long-running/stuck retry loops.
--
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]
