KVM: Assign explicit PCI slot when hot-plugging NIC for sequential naming

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<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 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<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;
+    }
 }

plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java

@@ -3548,6 +3548,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 <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);
 

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))));
+    }
+}