From a4d3c664823f428abc1ca402772861e937928345 Mon Sep 17 00:00:00 2001 From: James Peru Mmbono Date: Fri, 11 Sep 2026 12:51:00 +0300 Subject: [PATCH 01/41] KVM: assign the hot-plugged NIC the next PCI slot above existing NICs (monotonic interface naming) (#12826) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * 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 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
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 --------- Signed-off-by: James Peru Co-authored-by: James Peru Co-authored-by: Claude Opus 4.6 Co-authored-by: jmsperu --- .../wrapper/LibvirtPlugNicCommandWrapper.java | 105 +++++++++++++ .../LibvirtComputingResourceTest.java | 9 ++ .../LibvirtPlugNicCommandWrapperTest.java | 147 ++++++++++++++++++ 3 files changed, 261 insertions(+) create mode 100644 plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPlugNicCommandWrapperTest.java 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 b0950376a93d..acf95edeb954 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.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 { + /** 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 Answer execute(final PlugNicCommand command, final LibvirtComputingResour 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 Answer execute(final PlugNicCommand command, final LibvirtComputingResour } } } + + /** + * 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 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 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
} element in the domain XML, + * whichever device or bus they belong to. Returns {@code null} if the XML cannot be parsed. + */ + protected Set getUsedPciSlots(final String domXml) { + final Set 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 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 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 4683a76fac9a..b7d66e66542d 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 void testPlugNicCommandNoMatchMack() { 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 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( + "" + + "
" + + ""); + 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 000000000000..979ffda2f279 --- /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 = + "\n" + + " i-2-42-VM\n" + + " \n" + + " \n" + + " \n" + + "
\n" + + " \n" + + " \n" + + " \n" + + "
\n" + + " \n" + + " \n" + + "
\n" + + " \n" + + " \n" + + "
\n" + + " \n" + + " \n" + + " \n" + + "
\n" + + " \n" + + " \n" + + " \n" + + "
\n" + + " \n" + + " \n" + + "
\n" + + " \n" + + "