Copilot commented on code in PR #12826:
URL: https://github.com/apache/cloudstack/pull/12826#discussion_r2945425637
##########
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;
+ }
+ }
+
+ logger.warn("No free PCI slots available, letting libvirt
auto-assign");
+ return null;
+ } catch (LibvirtException e) {
+ logger.warn("Failed to get domain XML for PCI slot calculation,
letting libvirt auto-assign", e);
+ return null;
+ }
Review Comment:
This change introduces new PCI-slot selection behavior (including fallbacks
on XML-read failure and slot exhaustion), but there is no unit test coverage
for `LibvirtPlugNicCommandWrapper`. Other KVM wrapper classes in this module do
have Mockito-based tests (e.g., `LibvirtReplugNicCommandWrapperTest`), so
adding similar tests here would help prevent regressions and validate expected
slot selection and fallback behavior.
##########
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");
+ }
Review Comment:
The slot-selection logic will still return a high/non-sequential slot when
intermediate slots are already consumed by other PCI devices (e.g.,
virtio-serial/disk/memballoon/watchdog). In that common layout (also reflected
in existing test XMLs like LibvirtReplugNicCommandWrapperTest where non-NIC
devices occupy the slots immediately after NICs), `maxNicSlot + 1` will be
used/blocked and the loop will skip ahead (e.g. to 0x09), so the guest will
still see `ens9` rather than the expected next NIC name. If this PR is meant to
fully “fix #12825”/ensure sequential naming, it likely also needs the
complementary change to place non-NIC devices at higher PCI slots during VM
creation (or an alternative strategy that guarantees contiguous NIC slots).
##########
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:
`findNextAvailablePciSlot` parses the libvirt domain XML using a broad regex
(`slot='0x...'`). This is brittle (depends on exact XML formatting and can
match non-PCI-slot attributes) and makes the behavior hard to reason about
across libvirt/QEMU variations. Consider using an XML parser (ideally the same
safer DocumentBuilderFactory approach already used in `LibvirtDomainXMLParser`)
and restricting matches to `<address type='pci' ...>` elements so only actual
PCI addresses are considered.
--
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]
