This is an automated email from the ASF dual-hosted git repository.
shwstppr pushed a commit to branch 4.20
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/4.20 by this push:
new a4d3c664823 KVM: assign the hot-plugged NIC the next PCI slot above
existing NICs (monotonic interface naming) (#12826)
a4d3c664823 is described below
commit a4d3c664823f428abc1ca402772861e937928345
Author: James Peru Mmbono <[email protected]>
AuthorDate: Fri Sep 11 12:51:00 2026 +0300
KVM: assign the hot-plugged NIC the next PCI slot above existing NICs
(monotonic interface naming) (#12826)
* KVM: assign explicit PCI slot when hot-plugging NIC to ensure sequential
naming
When hot-plugging a NIC to a running VM, libvirt auto-assigns the next
free PCI slot. Since non-NIC devices (virtio-serial, disk, balloon,
watchdog) occupy slots immediately after existing NICs, the hot-plugged
NIC gets a much higher slot number (e.g. 0x09 instead of 0x05), causing
the guest to see non-sequential interface names (ens9 instead of ens5).
This fix queries the domain XML to find all used PCI slots and assigns
the next free slot after the highest existing NIC slot. This matches
the approach already used by LibvirtReplugNicCommandWrapper which
preserves PCI slots during re-plug operations.
Fixes #12825
Co-Authored-By: Claude Opus 4.6 <[email protected]>
* fix(kvm): NPE in PlugNic when libvirt domain XML is unavailable
LibvirtPlugNicCommandWrapper.findNextAvailablePciSlot calls
vm.getXMLDesc(0) and pipes the result straight into Pattern.matcher,
which NPEs if libvirt returned null (or, in the
LibvirtComputingResourceTest.testPlugNicCommandNoMatchMack unit test,
when the Domain mock isn't stubbed for getXMLDesc). Reported by
@DaanHoogland after the SL packaging run on #12826.
Defensive null check returns null from findNextAvailablePciSlot when
the domain XML can't be parsed, which falls through to libvirt's
auto-assignment of the PCI slot — same behaviour as before this PR
when nextSlot is null.
Also stubs Domain.getXMLDesc(0) in testPlugNicCommandNoMatchMack with
a minimal <domain> XML that exercises the parser path (rather than
just relying on the null-fallback), so the test continues to cover
the happy path of the new logic.
* address review (#12826): parse PCI addresses with an XML parser, split
slot selection into helpers, add unit tests
- getUsedPciSlots() parses the domain XML with the safer
DocumentBuilderFactory
and only considers <address type='pci'> elements, replacing the regex.
- getHighestNicSlot() and getFirstFreeSlotAbove() are separate methods.
- The javadoc now states the guarantee precisely: deterministic and
monotonic
after the last NIC, not contiguous when other devices sit in between.
- LibvirtPlugNicCommandWrapperTest covers parsing, selection and the
fallbacks.
Signed-off-by: James Peru <[email protected]>
---------
Signed-off-by: James Peru <[email protected]>
Co-authored-by: James Peru <[email protected]>
Co-authored-by: Claude Opus 4.6 <[email protected]>
Co-authored-by: jmsperu <[email protected]>
---
.../wrapper/LibvirtPlugNicCommandWrapper.java | 105 +++++++++++++++
.../kvm/resource/LibvirtComputingResourceTest.java | 9 ++
.../wrapper/LibvirtPlugNicCommandWrapperTest.java | 147 +++++++++++++++++++++
3 files changed, 261 insertions(+)
diff --git
a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPlugNicCommandWrapper.java
b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPlugNicCommandWrapper.java
index b0950376a93..acf95edeb95 100644
---
a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPlugNicCommandWrapper.java
+++
b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPlugNicCommandWrapper.java
@@ -30,15 +30,29 @@ import com.cloud.hypervisor.kvm.resource.VifDriver;
import com.cloud.resource.CommandWrapper;
import com.cloud.resource.ResourceWrapper;
import com.cloud.vm.VirtualMachine;
+import org.apache.cloudstack.utils.security.ParserUtils;
import org.libvirt.Connect;
import org.libvirt.Domain;
import org.libvirt.LibvirtException;
+import org.w3c.dom.Document;
+import org.w3c.dom.Element;
+import org.w3c.dom.NodeList;
+import org.xml.sax.InputSource;
+import org.xml.sax.SAXException;
+import javax.xml.parsers.DocumentBuilder;
+import javax.xml.parsers.ParserConfigurationException;
+import java.io.IOException;
+import java.io.StringReader;
+import java.util.HashSet;
import java.util.List;
+import java.util.Set;
@ResourceWrapper(handles = PlugNicCommand.class)
public final class LibvirtPlugNicCommandWrapper extends
CommandWrapper<PlugNicCommand, Answer, LibvirtComputingResource> {
+ /** Highest PCI slot number on a bus (0x00 is the host bridge). */
+ private static final int MAX_PCI_SLOT = 0x1f;
@Override
public Answer execute(final PlugNicCommand command, final
LibvirtComputingResource libvirtComputingResource) {
@@ -65,6 +79,15 @@ public final class LibvirtPlugNicCommandWrapper extends
CommandWrapper<PlugNicCo
if (command.getDetails() != null) {
libvirtComputingResource.setInterfaceDefQueueSettings(command.getDetails(),
null, interfaceDef);
}
+
+ // Pin the PCI slot to the lowest free one above the existing NICs
so the guest sees a
+ // deterministic, monotonic NIC order across hot-plugs (see
findNextAvailablePciSlot).
+ 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());
// apply default network rules on new nic
@@ -96,4 +119,86 @@ public final class LibvirtPlugNicCommandWrapper extends
CommandWrapper<PlugNicCo
}
}
}
+
+ /**
+ * Picks the PCI slot for the NIC being hot-plugged: the lowest free slot
above the highest slot
+ * already used by a NIC. The choice is deterministic and monotonic with
respect to the NICs that
+ * are already present (a new NIC never lands below an existing one),
which is what keeps the
+ * guest's predictable interface names stable across hot-plugs. It does
not guarantee contiguity:
+ * slots between the last NIC and the new one may already be taken by
other devices (disks,
+ * controllers, balloon), in which case the next free slot above them is
used.
+ *
+ * @return the slot to assign, or {@code null} to let libvirt auto-assign
(domain XML unavailable
+ * or unparseable, or no free slot left).
+ */
+ protected Integer findNextAvailablePciSlot(final Domain vm, final
List<InterfaceDef> pluggedNics) {
+ try {
+ final String domXml = vm.getXMLDesc(0);
+ // getXMLDesc can return null on certain libvirt error paths; fall
back to libvirt's own choice.
+ if (domXml == null) {
+ logger.debug("Domain XML unavailable, letting libvirt
auto-assign PCI slot");
+ return null;
+ }
+ final Set<Integer> usedSlots = getUsedPciSlots(domXml);
+ if (usedSlots == null) {
+ return null;
+ }
+ final Integer slot =
getFirstFreeSlotAbove(getHighestNicSlot(pluggedNics), usedSlots);
+ if (slot == null) {
+ logger.warn("No free PCI slots available, letting libvirt
auto-assign");
+ }
+ return slot;
+ } catch (final LibvirtException e) {
+ logger.warn("Failed to get domain XML for PCI slot calculation,
letting libvirt auto-assign", e);
+ return null;
+ }
+ }
+
+ /**
+ * Collects the slot numbers of every {@code <address type='pci' .../>}
element in the domain XML,
+ * whichever device or bus they belong to. Returns {@code null} if the XML
cannot be parsed.
+ */
+ protected Set<Integer> getUsedPciSlots(final String domXml) {
+ final Set<Integer> usedSlots = new HashSet<>();
+ try {
+ final DocumentBuilder builder =
ParserUtils.getSaferDocumentBuilderFactory().newDocumentBuilder();
+ final Document doc = builder.parse(new InputSource(new
StringReader(domXml)));
+ final NodeList addresses = doc.getElementsByTagName("address");
+ for (int i = 0; i < addresses.getLength(); i++) {
+ final Element address = (Element) addresses.item(i);
+ if (!"pci".equals(address.getAttribute("type")) ||
address.getAttribute("slot").isEmpty()) {
+ continue;
+ }
+ usedSlots.add(Integer.decode(address.getAttribute("slot")));
+ }
+ } catch (final ParserConfigurationException | SAXException |
IOException | NumberFormatException e) {
+ logger.warn("Failed to parse domain XML for PCI slot calculation,
letting libvirt auto-assign", e);
+ return null;
+ }
+ return usedSlots;
+ }
+
+ /** Highest PCI slot used by an existing NIC, or 0 when no NIC carries a
slot. */
+ protected static int getHighestNicSlot(final List<InterfaceDef>
pluggedNics) {
+ int highest = 0;
+ for (final InterfaceDef pluggedNic : pluggedNics) {
+ if (pluggedNic.getSlot() != null && pluggedNic.getSlot() >
highest) {
+ highest = pluggedNic.getSlot();
+ }
+ }
+ return highest;
+ }
+
+ /**
+ * Lowest slot strictly above {@code from} (and no higher than {@link
#MAX_PCI_SLOT}) that is not in
+ * {@code usedSlots}; {@code null} when the range is exhausted. Slot 0 is
reserved for the host bridge.
+ */
+ protected static Integer getFirstFreeSlotAbove(final int from, final
Set<Integer> usedSlots) {
+ for (int slot = from + 1; slot <= MAX_PCI_SLOT; slot++) {
+ if (!usedSlots.contains(slot)) {
+ return slot;
+ }
+ }
+ return null;
+ }
}
diff --git
a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java
b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java
index 4683a76fac9..b7d66e66542 100644
---
a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java
+++
b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java
@@ -3549,6 +3549,15 @@ public class LibvirtComputingResourceTest {
when(vifDriver.plug(nic, "Other PV", "",
null)).thenReturn(interfaceDef);
when(interfaceDef.toString()).thenReturn("Interface");
+ // Stub vm.getXMLDesc(0) so findNextAvailablePciSlot can scan the
domain XML
+ // for in-use PCI slots. Returning a minimal <domain> with a
single NIC at
+ // slot 0x03 exercises the production parser without forcing the
production
+ // code into its null-fallback path.
+ when(vm.getXMLDesc(0)).thenReturn(
+ "<domain><devices><interface type='bridge'>" +
+ "<address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>" +
+ "</interface></devices></domain>");
+
final String interfaceDefStr = interfaceDef.toString();
doNothing().when(vm).attachDevice(interfaceDefStr);
diff --git
a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPlugNicCommandWrapperTest.java
b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPlugNicCommandWrapperTest.java
new file mode 100644
index 00000000000..979ffda2f27
--- /dev/null
+++
b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPlugNicCommandWrapperTest.java
@@ -0,0 +1,147 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.mockito.Mockito.when;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.libvirt.Domain;
+import org.libvirt.LibvirtException;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import com.cloud.hypervisor.kvm.resource.LibvirtVMDef.InterfaceDef;
+
+@RunWith(MockitoJUnitRunner.class)
+public class LibvirtPlugNicCommandWrapperTest {
+
+ /**
+ * Typical q35-less layout: NICs at 0x03 and 0x06, other PCI devices at
0x01, 0x02, 0x04, 0x05 and 0x07,
+ * plus non-PCI addresses (drive, usb) that must be ignored.
+ */
+ private static final String DOMAIN_XML =
+ "<domain type='kvm'>\n"
+ + " <name>i-2-42-VM</name>\n"
+ + " <devices>\n"
+ + " <disk type='file' device='disk'>\n"
+ + " <target dev='vda' bus='virtio'/>\n"
+ + " <address type='pci' domain='0x0000' bus='0x00'
slot='0x05' function='0x0'/>\n"
+ + " </disk>\n"
+ + " <disk type='file' device='cdrom'>\n"
+ + " <target dev='hdc' bus='ide'/>\n"
+ + " <address type='drive' controller='0' bus='1' target='0'
unit='0'/>\n"
+ + " </disk>\n"
+ + " <controller type='usb' index='0'>\n"
+ + " <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'/>\n"
+ + " </controller>\n"
+ + " <controller type='virtio-serial' index='0'>\n"
+ + " <address type='pci' domain='0x0000' bus='0x00'
slot='0x04' function='0x0'/>\n"
+ + " </controller>\n"
+ + " <interface type='bridge'>\n"
+ + " <mac address='02:00:7c:98:00:01'/>\n"
+ + " <address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>\n"
+ + " </interface>\n"
+ + " <interface type='bridge'>\n"
+ + " <mac address='02:00:7c:98:00:02'/>\n"
+ + " <address type='pci' domain='0x0000' bus='0x00'
slot='0x06' function='0x0'/>\n"
+ + " </interface>\n"
+ + " <channel type='unix'>\n"
+ + " <address type='virtio-serial' controller='0' bus='0'
port='1'/>\n"
+ + " </channel>\n"
+ + " <video>\n"
+ + " <address type='pci' domain='0x0000' bus='0x00'
slot='0x02' function='0x0'/>\n"
+ + " </video>\n"
+ + " <memballoon model='virtio'>\n"
+ + " <address type='pci' domain='0x0000' bus='0x00'
slot='0x07' function='0x0'/>\n"
+ + " </memballoon>\n"
+ + " </devices>\n"
+ + "</domain>\n";
+
+ @Mock
+ private Domain domain;
+
+ private final LibvirtPlugNicCommandWrapper wrapper = new
LibvirtPlugNicCommandWrapper();
+
+ private static InterfaceDef nicAtSlot(final Integer slot) {
+ final InterfaceDef nic = new InterfaceDef();
+ nic.setSlot(slot);
+ return nic;
+ }
+
+ private static List<InterfaceDef> nicsFromXml() {
+ return Arrays.asList(nicAtSlot(0x03), nicAtSlot(0x06));
+ }
+
+ @Test
+ public void getUsedPciSlotsOnlyCountsPciAddresses() {
+ final Set<Integer> used = wrapper.getUsedPciSlots(DOMAIN_XML);
+ assertEquals(Set.of(0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07), used);
+ }
+
+ @Test
+ public void getUsedPciSlotsReturnsNullOnMalformedXml() {
+ assertNull(wrapper.getUsedPciSlots("<domain><devices><interface>"));
+ }
+
+ @Test
+ public void getHighestNicSlotIgnoresNicsWithoutAddress() {
+ assertEquals(0x06,
LibvirtPlugNicCommandWrapper.getHighestNicSlot(Arrays.asList(nicAtSlot(0x03),
nicAtSlot(null), nicAtSlot(0x06))));
+ assertEquals(0,
LibvirtPlugNicCommandWrapper.getHighestNicSlot(Collections.emptyList()));
+ }
+
+ @Test
+ public void getFirstFreeSlotAboveSkipsOccupiedSlotsAndStopsAtBusEnd() {
+ assertEquals(Integer.valueOf(0x08),
LibvirtPlugNicCommandWrapper.getFirstFreeSlotAbove(0x06, Set.of(0x07)));
+ assertEquals(Integer.valueOf(0x07),
LibvirtPlugNicCommandWrapper.getFirstFreeSlotAbove(0x06,
Collections.emptySet()));
+ assertNull(LibvirtPlugNicCommandWrapper.getFirstFreeSlotAbove(0x1f,
Collections.emptySet()));
+ }
+
+ @Test
+ public void findNextAvailablePciSlotPicksLowestFreeSlotAboveLastNic()
throws LibvirtException {
+ when(domain.getXMLDesc(0)).thenReturn(DOMAIN_XML);
+ // 0x07 is taken by the balloon, so the NIC goes to 0x08: monotonic
after the last NIC, not contiguous.
+ assertEquals(Integer.valueOf(0x08),
wrapper.findNextAvailablePciSlot(domain, nicsFromXml()));
+ }
+
+ @Test
+ public void findNextAvailablePciSlotFallsBackWhenXmlUnavailable() throws
LibvirtException {
+ when(domain.getXMLDesc(0)).thenReturn(null);
+ assertNull(wrapper.findNextAvailablePciSlot(domain, nicsFromXml()));
+ }
+
+ @Test
+ public void findNextAvailablePciSlotFallsBackWhenLibvirtFails() throws
LibvirtException {
+
when(domain.getXMLDesc(0)).thenThrow(Mockito.mock(LibvirtException.class));
+ assertNull(wrapper.findNextAvailablePciSlot(domain, nicsFromXml()));
+ }
+
+ @Test
+ public void findNextAvailablePciSlotFallsBackWhenBusIsFull() throws
LibvirtException {
+ when(domain.getXMLDesc(0)).thenReturn(DOMAIN_XML);
+ assertNull(wrapper.findNextAvailablePciSlot(domain,
Collections.singletonList(nicAtSlot(0x1f))));
+ }
+}
