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 ed163787b112..5c63fcc0a2ba 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 @@ -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 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" + + "