jmsperu commented on code in PR #12826:
URL: https://github.com/apache/cloudstack/pull/12826#discussion_r3844111103
##########
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:
Done in ab6337f: `getUsedPciSlots()` parses the domain XML with
`ParserUtils.getSaferDocumentBuilderFactory()` (the same factory
LibvirtDomainXMLParser uses) and only looks at `<address type='pci'>` elements.
Slots are collected regardless of bus, so a slot in use on any bus is never
handed out; conservative, and it keeps the helper simple.
##########
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:
Done in ab6337f, see the reply on the earlier thread: XML parser, `<address
type='pci'>` only.
##########
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:
Done: `LibvirtPlugNicCommandWrapperTest` (8 tests) covers PCI-only parsing,
malformed XML, the lowest-free-slot-above-the-last-NIC rule, a full bus, and
the fallbacks when libvirt throws or returns no XML.
##########
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:
Done, `LibvirtPlugNicCommandWrapperTest` added in ab6337f.
##########
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:
Fair point, and I've made the wording honest rather than claim more than the
code does. What this PR guarantees is a deterministic, monotonic slot: the new
NIC always lands above the highest existing NIC, so hot-plugged interfaces
enumerate in plug order and never jump below an existing one. It does not
guarantee contiguity: if the balloon or a controller sits right after the last
NIC, the next free slot above it is used (exactly the case in the new unit
test). Making the slots contiguous means reserving a NIC range at VM creation
time, which is a bigger change to LibvirtVMDef that I'd rather do as a
follow-up than fold into this one. The javadoc, the inline comment and the PR
description now say this.
##########
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:
Covered in the reply above: the guarantee is deterministic and monotonic,
not contiguous, and the description now says so. Contiguity needs device
placement at VM creation, which I'd do as a follow-up.
##########
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:
Done in ab6337f: `getUsedPciSlots()`, `getHighestNicSlot()` and
`getFirstFreeSlotAbove()` are now three methods, each unit-tested on its own.
--
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]
