Skip to content

Commit a4d3c66

Browse files
jmsperuJames Peruclaude
authored
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 <noreply@anthropic.com> * 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 <jmsperu@gmail.com> --------- Signed-off-by: James Peru <jmsperu@gmail.com> Co-authored-by: James Peru <jamesperu@Jamess-Mac-mini.local> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: jmsperu <jmsperu@users.noreply.github.com>
1 parent b93f603 commit a4d3c66

3 files changed

Lines changed: 261 additions & 0 deletions

File tree

‎plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPlugNicCommandWrapper.java‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,15 +30,29 @@
3030
import com.cloud.resource.CommandWrapper;
3131
import com.cloud.resource.ResourceWrapper;
3232
import com.cloud.vm.VirtualMachine;
33+
import org.apache.cloudstack.utils.security.ParserUtils;
3334
import org.libvirt.Connect;
3435
import org.libvirt.Domain;
3536
import org.libvirt.LibvirtException;
37+
import org.w3c.dom.Document;
38+
import org.w3c.dom.Element;
39+
import org.w3c.dom.NodeList;
40+
import org.xml.sax.InputSource;
41+
import org.xml.sax.SAXException;
3642

43+
import javax.xml.parsers.DocumentBuilder;
44+
import javax.xml.parsers.ParserConfigurationException;
45+
import java.io.IOException;
46+
import java.io.StringReader;
47+
import java.util.HashSet;
3748
import java.util.List;
49+
import java.util.Set;
3850

3951
@ResourceWrapper(handles = PlugNicCommand.class)
4052
public final class LibvirtPlugNicCommandWrapper extends CommandWrapper<PlugNicCommand, Answer, LibvirtComputingResource> {
4153

54+
/** Highest PCI slot number on a bus (0x00 is the host bridge). */
55+
private static final int MAX_PCI_SLOT = 0x1f;
4256

4357
@Override
4458
public Answer execute(final PlugNicCommand command, final LibvirtComputingResource libvirtComputingResource) {
@@ -65,6 +79,15 @@ public Answer execute(final PlugNicCommand command, final LibvirtComputingResour
6579
if (command.getDetails() != null) {
6680
libvirtComputingResource.setInterfaceDefQueueSettings(command.getDetails(), null, interfaceDef);
6781
}
82+
83+
// Pin the PCI slot to the lowest free one above the existing NICs so the guest sees a
84+
// deterministic, monotonic NIC order across hot-plugs (see findNextAvailablePciSlot).
85+
Integer nextSlot = findNextAvailablePciSlot(vm, pluggedNics);
86+
if (nextSlot != null) {
87+
interfaceDef.setSlot(nextSlot);
88+
logger.debug("Assigning PCI slot 0x" + String.format("%02x", nextSlot) + " to hot-plugged NIC");
89+
}
90+
6891
vm.attachDevice(interfaceDef.toString());
6992

7093
// apply default network rules on new nic
@@ -96,4 +119,86 @@ public Answer execute(final PlugNicCommand command, final LibvirtComputingResour
96119
}
97120
}
98121
}
122+
123+
/**
124+
* Picks the PCI slot for the NIC being hot-plugged: the lowest free slot above the highest slot
125+
* already used by a NIC. The choice is deterministic and monotonic with respect to the NICs that
126+
* are already present (a new NIC never lands below an existing one), which is what keeps the
127+
* guest's predictable interface names stable across hot-plugs. It does not guarantee contiguity:
128+
* slots between the last NIC and the new one may already be taken by other devices (disks,
129+
* controllers, balloon), in which case the next free slot above them is used.
130+
*
131+
* @return the slot to assign, or {@code null} to let libvirt auto-assign (domain XML unavailable
132+
* or unparseable, or no free slot left).
133+
*/
134+
protected Integer findNextAvailablePciSlot(final Domain vm, final List<InterfaceDef> pluggedNics) {
135+
try {
136+
final String domXml = vm.getXMLDesc(0);
137+
// getXMLDesc can return null on certain libvirt error paths; fall back to libvirt's own choice.
138+
if (domXml == null) {
139+
logger.debug("Domain XML unavailable, letting libvirt auto-assign PCI slot");
140+
return null;
141+
}
142+
final Set<Integer> usedSlots = getUsedPciSlots(domXml);
143+
if (usedSlots == null) {
144+
return null;
145+
}
146+
final Integer slot = getFirstFreeSlotAbove(getHighestNicSlot(pluggedNics), usedSlots);
147+
if (slot == null) {
148+
logger.warn("No free PCI slots available, letting libvirt auto-assign");
149+
}
150+
return slot;
151+
} catch (final LibvirtException e) {
152+
logger.warn("Failed to get domain XML for PCI slot calculation, letting libvirt auto-assign", e);
153+
return null;
154+
}
155+
}
156+
157+
/**
158+
* Collects the slot numbers of every {@code <address type='pci' .../>} element in the domain XML,
159+
* whichever device or bus they belong to. Returns {@code null} if the XML cannot be parsed.
160+
*/
161+
protected Set<Integer> getUsedPciSlots(final String domXml) {
162+
final Set<Integer> usedSlots = new HashSet<>();
163+
try {
164+
final DocumentBuilder builder = ParserUtils.getSaferDocumentBuilderFactory().newDocumentBuilder();
165+
final Document doc = builder.parse(new InputSource(new StringReader(domXml)));
166+
final NodeList addresses = doc.getElementsByTagName("address");
167+
for (int i = 0; i < addresses.getLength(); i++) {
168+
final Element address = (Element) addresses.item(i);
169+
if (!"pci".equals(address.getAttribute("type")) || address.getAttribute("slot").isEmpty()) {
170+
continue;
171+
}
172+
usedSlots.add(Integer.decode(address.getAttribute("slot")));
173+
}
174+
} catch (final ParserConfigurationException | SAXException | IOException | NumberFormatException e) {
175+
logger.warn("Failed to parse domain XML for PCI slot calculation, letting libvirt auto-assign", e);
176+
return null;
177+
}
178+
return usedSlots;
179+
}
180+
181+
/** Highest PCI slot used by an existing NIC, or 0 when no NIC carries a slot. */
182+
protected static int getHighestNicSlot(final List<InterfaceDef> pluggedNics) {
183+
int highest = 0;
184+
for (final InterfaceDef pluggedNic : pluggedNics) {
185+
if (pluggedNic.getSlot() != null && pluggedNic.getSlot() > highest) {
186+
highest = pluggedNic.getSlot();
187+
}
188+
}
189+
return highest;
190+
}
191+
192+
/**
193+
* Lowest slot strictly above {@code from} (and no higher than {@link #MAX_PCI_SLOT}) that is not in
194+
* {@code usedSlots}; {@code null} when the range is exhausted. Slot 0 is reserved for the host bridge.
195+
*/
196+
protected static Integer getFirstFreeSlotAbove(final int from, final Set<Integer> usedSlots) {
197+
for (int slot = from + 1; slot <= MAX_PCI_SLOT; slot++) {
198+
if (!usedSlots.contains(slot)) {
199+
return slot;
200+
}
201+
}
202+
return null;
203+
}
99204
}

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3549,6 +3549,15 @@ public void testPlugNicCommandNoMatchMack() {
35493549
when(vifDriver.plug(nic, "Other PV", "", null)).thenReturn(interfaceDef);
35503550
when(interfaceDef.toString()).thenReturn("Interface");
35513551

3552+
// Stub vm.getXMLDesc(0) so findNextAvailablePciSlot can scan the domain XML
3553+
// for in-use PCI slots. Returning a minimal <domain> with a single NIC at
3554+
// slot 0x03 exercises the production parser without forcing the production
3555+
// code into its null-fallback path.
3556+
when(vm.getXMLDesc(0)).thenReturn(
3557+
"<domain><devices><interface type='bridge'>" +
3558+
"<address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>" +
3559+
"</interface></devices></domain>");
3560+
35523561
final String interfaceDefStr = interfaceDef.toString();
35533562
doNothing().when(vm).attachDevice(interfaceDefStr);
35543563

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
18+
package com.cloud.hypervisor.kvm.resource.wrapper;
19+
20+
import static org.junit.Assert.assertEquals;
21+
import static org.junit.Assert.assertNull;
22+
import static org.mockito.Mockito.when;
23+
24+
import java.util.Arrays;
25+
import java.util.Collections;
26+
import java.util.List;
27+
import java.util.Set;
28+
29+
import org.junit.Test;
30+
import org.junit.runner.RunWith;
31+
import org.libvirt.Domain;
32+
import org.libvirt.LibvirtException;
33+
import org.mockito.Mock;
34+
import org.mockito.Mockito;
35+
import org.mockito.junit.MockitoJUnitRunner;
36+
37+
import com.cloud.hypervisor.kvm.resource.LibvirtVMDef.InterfaceDef;
38+
39+
@RunWith(MockitoJUnitRunner.class)
40+
public class LibvirtPlugNicCommandWrapperTest {
41+
42+
/**
43+
* Typical q35-less layout: NICs at 0x03 and 0x06, other PCI devices at 0x01, 0x02, 0x04, 0x05 and 0x07,
44+
* plus non-PCI addresses (drive, usb) that must be ignored.
45+
*/
46+
private static final String DOMAIN_XML =
47+
"<domain type='kvm'>\n"
48+
+ " <name>i-2-42-VM</name>\n"
49+
+ " <devices>\n"
50+
+ " <disk type='file' device='disk'>\n"
51+
+ " <target dev='vda' bus='virtio'/>\n"
52+
+ " <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/>\n"
53+
+ " </disk>\n"
54+
+ " <disk type='file' device='cdrom'>\n"
55+
+ " <target dev='hdc' bus='ide'/>\n"
56+
+ " <address type='drive' controller='0' bus='1' target='0' unit='0'/>\n"
57+
+ " </disk>\n"
58+
+ " <controller type='usb' index='0'>\n"
59+
+ " <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>\n"
60+
+ " </controller>\n"
61+
+ " <controller type='virtio-serial' index='0'>\n"
62+
+ " <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>\n"
63+
+ " </controller>\n"
64+
+ " <interface type='bridge'>\n"
65+
+ " <mac address='02:00:7c:98:00:01'/>\n"
66+
+ " <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>\n"
67+
+ " </interface>\n"
68+
+ " <interface type='bridge'>\n"
69+
+ " <mac address='02:00:7c:98:00:02'/>\n"
70+
+ " <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/>\n"
71+
+ " </interface>\n"
72+
+ " <channel type='unix'>\n"
73+
+ " <address type='virtio-serial' controller='0' bus='0' port='1'/>\n"
74+
+ " </channel>\n"
75+
+ " <video>\n"
76+
+ " <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>\n"
77+
+ " </video>\n"
78+
+ " <memballoon model='virtio'>\n"
79+
+ " <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/>\n"
80+
+ " </memballoon>\n"
81+
+ " </devices>\n"
82+
+ "</domain>\n";
83+
84+
@Mock
85+
private Domain domain;
86+
87+
private final LibvirtPlugNicCommandWrapper wrapper = new LibvirtPlugNicCommandWrapper();
88+
89+
private static InterfaceDef nicAtSlot(final Integer slot) {
90+
final InterfaceDef nic = new InterfaceDef();
91+
nic.setSlot(slot);
92+
return nic;
93+
}
94+
95+
private static List<InterfaceDef> nicsFromXml() {
96+
return Arrays.asList(nicAtSlot(0x03), nicAtSlot(0x06));
97+
}
98+
99+
@Test
100+
public void getUsedPciSlotsOnlyCountsPciAddresses() {
101+
final Set<Integer> used = wrapper.getUsedPciSlots(DOMAIN_XML);
102+
assertEquals(Set.of(0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07), used);
103+
}
104+
105+
@Test
106+
public void getUsedPciSlotsReturnsNullOnMalformedXml() {
107+
assertNull(wrapper.getUsedPciSlots("<domain><devices><interface>"));
108+
}
109+
110+
@Test
111+
public void getHighestNicSlotIgnoresNicsWithoutAddress() {
112+
assertEquals(0x06, LibvirtPlugNicCommandWrapper.getHighestNicSlot(Arrays.asList(nicAtSlot(0x03), nicAtSlot(null), nicAtSlot(0x06))));
113+
assertEquals(0, LibvirtPlugNicCommandWrapper.getHighestNicSlot(Collections.emptyList()));
114+
}
115+
116+
@Test
117+
public void getFirstFreeSlotAboveSkipsOccupiedSlotsAndStopsAtBusEnd() {
118+
assertEquals(Integer.valueOf(0x08), LibvirtPlugNicCommandWrapper.getFirstFreeSlotAbove(0x06, Set.of(0x07)));
119+
assertEquals(Integer.valueOf(0x07), LibvirtPlugNicCommandWrapper.getFirstFreeSlotAbove(0x06, Collections.emptySet()));
120+
assertNull(LibvirtPlugNicCommandWrapper.getFirstFreeSlotAbove(0x1f, Collections.emptySet()));
121+
}
122+
123+
@Test
124+
public void findNextAvailablePciSlotPicksLowestFreeSlotAboveLastNic() throws LibvirtException {
125+
when(domain.getXMLDesc(0)).thenReturn(DOMAIN_XML);
126+
// 0x07 is taken by the balloon, so the NIC goes to 0x08: monotonic after the last NIC, not contiguous.
127+
assertEquals(Integer.valueOf(0x08), wrapper.findNextAvailablePciSlot(domain, nicsFromXml()));
128+
}
129+
130+
@Test
131+
public void findNextAvailablePciSlotFallsBackWhenXmlUnavailable() throws LibvirtException {
132+
when(domain.getXMLDesc(0)).thenReturn(null);
133+
assertNull(wrapper.findNextAvailablePciSlot(domain, nicsFromXml()));
134+
}
135+
136+
@Test
137+
public void findNextAvailablePciSlotFallsBackWhenLibvirtFails() throws LibvirtException {
138+
when(domain.getXMLDesc(0)).thenThrow(Mockito.mock(LibvirtException.class));
139+
assertNull(wrapper.findNextAvailablePciSlot(domain, nicsFromXml()));
140+
}
141+
142+
@Test
143+
public void findNextAvailablePciSlotFallsBackWhenBusIsFull() throws LibvirtException {
144+
when(domain.getXMLDesc(0)).thenReturn(DOMAIN_XML);
145+
assertNull(wrapper.findNextAvailablePciSlot(domain, Collections.singletonList(nicAtSlot(0x1f))));
146+
}
147+
}

0 commit comments

Comments
 (0)