Copilot commented on code in PR #12826:
URL: https://github.com/apache/cloudstack/pull/12826#discussion_r3302286973
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPlugNicCommandWrapper.java:
##########
@@ -96,4 +111,46 @@ public Answer execute(final PlugNicCommand command, final
LibvirtComputingResour
}
}
}
+
+ /**
+ * Finds the next available PCI slot for a hot-plugged NIC by examining
+ * all PCI slots currently in use by the domain. This ensures the new NIC
+ * gets a sequential PCI address relative to existing NICs, resulting in
+ * predictable interface naming in the guest OS (e.g. ens5 instead of
ens9).
+ */
+ private Integer findNextAvailablePciSlot(final Domain vm, final
List<InterfaceDef> pluggedNics) {
+ try {
+ String domXml = vm.getXMLDesc(0);
+
+ // Parse all PCI slot numbers currently in use
+ Set<Integer> usedSlots = new HashSet<>();
+ Pattern slotPattern = Pattern.compile("slot='0x([0-9a-fA-F]+)'");
+ Matcher matcher = slotPattern.matcher(domXml);
+ while (matcher.find()) {
+ usedSlots.add(Integer.parseInt(matcher.group(1), 16));
+ }
+
+ // Find the highest PCI slot used by existing NICs
+ int maxNicSlot = 0;
+ for (InterfaceDef pluggedNic : pluggedNics) {
+ if (pluggedNic.getSlot() != null && pluggedNic.getSlot() >
maxNicSlot) {
+ maxNicSlot = pluggedNic.getSlot();
+ }
+ }
+
+ // Find next free slot starting from maxNicSlot + 1
+ // PCI slots range from 0x01 to 0x1f (slot 0 is reserved for host
bridge)
+ for (int slot = maxNicSlot + 1; slot <= 0x1f; slot++) {
+ if (!usedSlots.contains(slot)) {
+ return slot;
+ }
+ }
Review Comment:
The current slot-selection algorithm will still skip PCI slots already used
by non-NIC devices. In the scenario described in the PR/issue (non-NIC devices
occupying slots 0x05–0x08), this method will return 0x09 (same as libvirt
auto-assignment), so the hot-plugged NIC will still appear as ens9 rather than
ens5. Either adjust the PR’s stated behavior/expectations, or pair this with
device address placement changes during VM creation (so slots after the last
NIC are actually free) to achieve sequential NIC naming.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPlugNicCommandWrapper.java:
##########
@@ -96,4 +111,46 @@ public Answer execute(final PlugNicCommand command, final
LibvirtComputingResour
}
}
}
+
+ /**
+ * Finds the next available PCI slot for a hot-plugged NIC by examining
+ * all PCI slots currently in use by the domain. This ensures the new NIC
+ * gets a sequential PCI address relative to existing NICs, resulting in
+ * predictable interface naming in the guest OS (e.g. ens5 instead of
ens9).
+ */
+ private Integer findNextAvailablePciSlot(final Domain vm, final
List<InterfaceDef> pluggedNics) {
+ try {
+ String domXml = vm.getXMLDesc(0);
+
+ // Parse all PCI slot numbers currently in use
+ Set<Integer> usedSlots = new HashSet<>();
+ Pattern slotPattern = Pattern.compile("slot='0x([0-9a-fA-F]+)'");
+ Matcher matcher = slotPattern.matcher(domXml);
+ while (matcher.find()) {
+ usedSlots.add(Integer.parseInt(matcher.group(1), 16));
+ }
Review Comment:
Parsing the libvirt domain XML with a regex is brittle and harder to
maintain (XML formatting/quoting/attribute ordering can change, and this also
ignores bus/function). Consider parsing with an XML parser (similar to
LibvirtDomainXMLParser) and collecting <address type='pci'> elements,
optionally filtering by bus/domain, to make slot detection more robust.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPlugNicCommandWrapper.java:
##########
@@ -65,6 +69,17 @@ public Answer execute(final PlugNicCommand command, final
LibvirtComputingResour
if (command.getDetails() != null) {
libvirtComputingResource.setInterfaceDefQueueSettings(command.getDetails(),
null, interfaceDef);
}
+
+ // Explicitly assign PCI slot to ensure sequential NIC naming in
the guest.
+ // Without this, libvirt auto-assigns the next free PCI slot which
may be
+ // non-sequential with existing NICs (e.g. ens9 instead of ens5),
causing
+ // guest network configuration to fail.
+ Integer nextSlot = findNextAvailablePciSlot(vm, pluggedNics);
+ if (nextSlot != null) {
+ interfaceDef.setSlot(nextSlot);
+ logger.debug("Assigning PCI slot 0x" + String.format("%02x",
nextSlot) + " to hot-plugged NIC");
+ }
+
vm.attachDevice(interfaceDef.toString());
Review Comment:
This change adds non-trivial behavior (calculating/forcing a PCI slot and
fallback paths) but there is no unit test covering the new slot-selection
logic. There are existing wrapper tests under
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper
(e.g., for ReplugNic); adding a similar test for PlugNic would help prevent
regressions (e.g., ensuring the chosen slot is the lowest free slot after the
last NIC, and verifying fallback when getXMLDesc throws).
--
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]
