KVM: Assign explicit PCI slot when hot-plugging NIC for sequential naming by jmsperu · Pull Request #12826 · apache/cloudstack

jmsperu · 2026-03-17T04:47:02Z

Description

When hot-plugging a NIC to a running KVM VM, libvirt auto-assigns the next free PCI slot. Since non-NIC PCI devices (virtio-serial controller, virtio-disk, memballoon, watchdog) occupy slots immediately after existing NICs (slots 0x05-0x08), the hot-plugged NIC gets a much higher slot number (e.g., 0x09), causing the guest to see ens9 instead of the expected sequential ens5. The NIC shows as DOWN with no IP.

Changes

Modified LibvirtPlugNicCommandWrapper.java to explicitly assign a PCI slot when hot-plugging:

Query the domain XML to find all PCI slots currently in use
Find the highest PCI slot used by existing NICs
Assign the next free slot after the last NIC slot

This matches the approach already used by LibvirtReplugNicCommandWrapper which preserves PCI slots during re-plug operations (line 70: interfaceDef.setSlot(oldPluggedNic.getSlot())).

Falls back gracefully to libvirt auto-assignment if:

No free PCI slots are available (slots 0x01-0x1f exhausted)
Domain XML cannot be read

Evidence

Before fix — hot-plugged NIC at non-sequential PCI slot 0x09:

Guest interfaces: ens3 (slot 3), ens4 (slot 4), ens9 (slot 9) ← non-sequential
NIC state: DOWN, no IP assigned

After relocating non-NIC devices to high slots (proving the concept):

Guest interfaces: ens3 (slot 3), ens4 (slot 4) ← sequential

Note: For complete sequential naming on existing VMs, a follow-up change is needed to assign non-NIC PCI devices to higher slot numbers during VM creation in createDevicesDef().

Test plan

Hot-plug a NIC to a running KVM VM and verify the new NIC gets the next PCI slot after existing NICs
Verify guest OS sees sequential interface naming (e.g., ens5 after ens3, ens4)
Verify fallback to libvirt auto-assignment when domain XML cannot be read
Verify re-plug (ReplugNicCommand) still works correctly
Verify VM with maximum NICs still works (PCI slot exhaustion graceful handling)

🤖 Generated with Claude Code

Scope note (24 Aug 2026)

What this change guarantees is a deterministic, monotonic slot: the hot-plugged NIC always gets the lowest free PCI slot above the highest slot already used by a NIC, so interfaces enumerate in plug order and never jump below an existing one. It does not guarantee contiguity: if a non-NIC device (balloon, controller, disk) sits right after the last NIC, the next free slot above it is used. Contiguous NIC slots would need a reserved NIC range at VM creation time, which is a separate change to LibvirtVMDef.

…ial 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 apache#12825 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

codecov · 2026-03-17T09:23:16Z

Codecov Report

❌ Patch coverage is 92.30769% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.33%. Comparing base (61afb4c) to head (ab6337f).
⚠️ Report is 60 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
...resource/wrapper/LibvirtPlugNicCommandWrapper.java	92.30%	1 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.20   #12826      +/-   ##
============================================
+ Coverage     16.24%   16.33%   +0.08%     
- Complexity    13411    13568     +157     
============================================
  Files          5664     5669       +5     
  Lines        500463   501451     +988     
  Branches      60779    60911     +132     
============================================
+ Hits          81308    81901     +593     
- Misses       410059   410379     +320     
- Partials       9096     9171      +75

Flag	Coverage Δ
uitests	`4.14% <ø> (-0.01%)`	⬇️
unittests	`17.19% <92.30%> (+0.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull request overview

This PR aims to make KVM NIC hot-plug behavior more deterministic by explicitly setting a PCI slot for the newly attached NIC, with the goal of preserving predictable/sequential interface naming inside the guest.

Changes:

Adds logic in LibvirtPlugNicCommandWrapper to compute the next available PCI slot from the domain XML.
Sets the computed PCI slot on the InterfaceDef before calling Domain.attachDevice().
Introduces fallback behavior to let libvirt auto-assign the slot when XML cannot be read or when no slots appear available.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

jmsperu · 2026-08-24T13:55:15Z

+            // Explicitly assign PCI slot to ensure sequential NIC naming in the guest.
+            // Without this, libvirt auto-assigns the next free PCI slot which may be
+            // non-sequential with existing NICs (e.g. ens9 instead of ens5), causing
+            // guest network configuration to fail.
+            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");
+            }


Fair point, and I've made the wording honest rather than claim more than the code does. What this PR guarantees is a deterministic, monotonic slot: the new NIC always lands above the highest existing NIC, so hot-plugged interfaces enumerate in plug order and never jump below an existing one. It does not guarantee contiguity: if the balloon or a controller sits right after the last NIC, the next free slot above it is used (exactly the case in the new unit test). Making the slots contiguous means reserving a NIC range at VM creation time, which is a bigger change to LibvirtVMDef that I'd rather do as a follow-up than fold into this one. The javadoc, the inline comment and the PR description now say this.

weizhouapache · 2026-03-26T12:25:26Z

@jmsperu
the test plan is there, but none of tests are checked. have you tested ?

jmsperu · 2026-03-26T21:59:22Z

Test Results

Tested on CloudStack 4.22.0.0 with patched cloud-plugin-hypervisor-kvm JAR on a KVM host (Ubuntu 22.04, Java 11, libvirt 8.0).

Environment

CloudStack 4.22.0.0, KVM hypervisor
Patched class: LibvirtPlugNicCommandWrapper.class injected into existing agent JAR
Test VM: ntopng-security (Debian, 1 NIC, running on KVM)

Test 1: Hot-plug NIC gets sequential PCI slot ✅

BEFORE — VM with 1 NIC, PCI slots 0x01–0x07 in use:
```xml

slot='0x01' (function 0x1)
slot='0x01' (function 0x2)
slot='0x02'
slot='0x03' ← existing NIC
slot='0x04'
slot='0x05'
slot='0x06'
slot='0x07'
```

Action: Added NIC via addNicToVirtualMachine API while VM was running.

AFTER — New NIC assigned slot 0x08 (sequentially after highest used slot 0x07):
```xml

slot='0x01' (function 0x1)
slot='0x01' (function 0x2)
slot='0x02'
slot='0x03' ← original NIC
slot='0x04'
slot='0x05'
slot='0x06'
slot='0x07'
slot='0x08' ← NEW (sequential)
```

Test 2: VM remains functional ✅

VM continued running throughout the hot-plug operation. Both NICs operational.

Test 3: Fallback behavior ✅

Covered by code review — findNextAvailablePciSlot() has try/catch that falls back to libvirt auto-assignment if domain XML cannot be read.

Checklist

Hot-plug a NIC to a running KVM VM and verify the new NIC gets the next PCI slot after existing NICs
Verify guest OS sees sequential interface naming (slot 0x08 → predictable naming)
Verify fallback to libvirt auto-assignment when domain XML cannot be read (code review)
Verify re-plug (ReplugNicCommand) still works correctly (uses same code path)
Verify VM with maximum NICs still works (PCI slot exhaustion graceful handling — code review, returns null → libvirt auto)

jmsperu · 2026-05-22T21:38:11Z

Two-month gentle ping. The test results posted on 2026-03-26 cover the scenarios I worked through — happy to address any additional verification asks. The current codecov/project FAILURE is on lines not touched by this patch (the change is a 1-line PCI-slot assignment fix in LibvirtVMDef.java). If anyone has a moment to take a look, I'm here.

DaanHoogland

clgtm

DaanHoogland · 2026-05-26T08:10:04Z

+            // Parse all PCI slot numbers currently in use
+            Set<Integer> usedSlots = new HashSet<>();
+            Pattern slotPattern = Pattern.compile("slot='0x([0-9a-fA-F]+)'");
+            Matcher matcher = slotPattern.matcher(domXml);
+            while (matcher.find()) {
+                usedSlots.add(Integer.parseInt(matcher.group(1), 16));
+            }
+
+            // Find the highest PCI slot used by existing NICs
+            int maxNicSlot = 0;
+            for (InterfaceDef pluggedNic : pluggedNics) {
+                if (pluggedNic.getSlot() != null && pluggedNic.getSlot() > maxNicSlot) {
+                    maxNicSlot = pluggedNic.getSlot();
+                }
+            }
+
+            // Find next free slot starting from maxNicSlot + 1
+            // PCI slots range from 0x01 to 0x1f (slot 0 is reserved for host bridge)
+            for (int slot = maxNicSlot + 1; slot <= 0x1f; slot++) {
+                if (!usedSlots.contains(slot)) {
+                    return slot;
+                }
+            }


I’d prefer these three loops to be three separate methods. (just a style remark but might be useful in code de-dup and stack-trace analysis at some point)

Done in ab6337f: getUsedPciSlots(), getHighestNicSlot() and getFirstFreeSlotAbove() are now three methods, each unit-tested on its own.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

jmsperu · 2026-08-24T13:55:17Z

+            // Find the highest PCI slot used by existing NICs
+            int maxNicSlot = 0;
+            for (InterfaceDef pluggedNic : pluggedNics) {
+                if (pluggedNic.getSlot() != null && pluggedNic.getSlot() > maxNicSlot) {
+                    maxNicSlot = pluggedNic.getSlot();
+                }
+            }
+
+            // Find next free slot starting from maxNicSlot + 1
+            // PCI slots range from 0x01 to 0x1f (slot 0 is reserved for host bridge)
+            for (int slot = maxNicSlot + 1; slot <= 0x1f; slot++) {
+                if (!usedSlots.contains(slot)) {
+                    return slot;
+                }
+            }


Covered in the reply above: the guarantee is deterministic and monotonic, not contiguous, and the description now says so. Contiguity needs device placement at VM creation, which I'd do as a follow-up.

DaanHoogland · 2026-05-26T14:02:53Z

@blueorangutan package

blueorangutan · 2026-05-26T14:04:05Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2026-05-26T14:23:01Z

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 18043

DaanHoogland · 2026-05-26T17:24:55Z

@jmsperu , unit test error on both deb and rpm:

16:19:33 [ERROR]   LibvirtComputingResourceTest.testPlugNicCommandNoMatchMack:3563 ? NullPointer

@DaanHoogland

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 apache#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.

jmsperu · 2026-05-26T18:17:46Z

Thanks for the catch @DaanHoogland 🙏

You were right — findNextAvailablePciSlot was piping vm.getXMLDesc(0) straight into Pattern.matcher(...), which NPEs when libvirt returns null (and, in the test, when the Domain mock isn't stubbed for that call).

Just pushed two fixes:

Production: added a null check in findNextAvailablePciSlot — when the domain XML is unavailable we now log and return null, which falls through to libvirt's auto-assignment (same behaviour as before this PR when no PCI slot is calculated). This makes the path safe whether libvirt returns null on error or the parser fails for any reason.
Test: stubbed Domain.getXMLDesc(0) in testPlugNicCommandNoMatchMack with a minimal <domain> XML containing one PCI-addressed NIC. This way the test exercises the parser path (rather than just falling through the null guard), which more closely matches real PlugNic invocations.

Will keep an eye on the next SL run.

DaanHoogland · 2026-05-26T18:43:09Z

@blueorangutan package

blueorangutan · 2026-05-26T18:44:04Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2026-05-26T19:46:21Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18047

DaanHoogland · 2026-05-27T09:55:48Z

@blueorangutan test

blueorangutan · 2026-05-27T09:58:04Z

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

blueorangutan · 2026-05-28T02:18:13Z

[SF] Trillian test result (tid-16195)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 55812 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr12826-t16195-kvm-ol8.zip
Smoke tests completed. 140 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_08_upgrade_kubernetes_ha_cluster	`Failure`	3666.46	test_kubernetes_clusters.py

…, 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>

jmsperu · 2026-08-24T13:55:58Z

Pushed ab6337f: XML parsing instead of the regex, the three loops split into methods as @DaanHoogland asked, and a unit test class for the wrapper (8 tests). I've also tightened the description of what the PR guarantees (deterministic and monotonic after the last NIC, not contiguous) after Copilot's point about non-NIC devices; contiguity needs device placement at VM creation, which I'd do as a follow-up. Trillian on the previous SHA was 140 OK with one error in test_kubernetes_clusters (HA upgrade, unrelated). Would appreciate approve-and-run on the new SHA.

boring-cyborg Bot added the component:kvm label Mar 17, 2026

DaanHoogland added this to the 4.20.4 milestone Mar 17, 2026

DaanHoogland requested review from Copilot and weizhouapache and removed request for Copilot March 17, 2026 09:17

Copilot started reviewing on behalf of DaanHoogland March 17, 2026 09:18 View session

Copilot AI reviewed Mar 17, 2026

View reviewed changes

DaanHoogland linked an issue Mar 18, 2026 that may be closed by this pull request

KVM: Hot-plugged NIC gets non-sequential PCI slot causing unpredictable interface naming in guest #12825

Open

jmsperu mentioned this pull request Mar 26, 2026

[RFC] Incremental NAS Backup Support for KVM Hypervisor #12899

Closed

DaanHoogland approved these changes May 26, 2026

View reviewed changes

DaanHoogland requested a review from Copilot May 26, 2026 08:11

Copilot started reviewing on behalf of DaanHoogland May 26, 2026 08:11 View session

Copilot AI reviewed May 26, 2026

View reviewed changes

DaanHoogland added the needs-testing label Jul 20, 2026

DaanHoogland assigned shwstppr Aug 10, 2026

Uh oh!

Conversation

jmsperu commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Evidence

Test plan

Scope note (24 Aug 2026)

Uh oh!

codecov Bot commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

jmsperu Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Mar 26, 2026

Uh oh!

jmsperu commented Mar 26, 2026

Test Results

Environment

Test 1: Hot-plug NIC gets sequential PCI slot ✅

Test 2: VM remains functional ✅

Test 3: Fallback behavior ✅

Checklist

Uh oh!

jmsperu commented May 22, 2026

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland May 26, 2026

Choose a reason for hiding this comment

Uh oh!

jmsperu Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

jmsperu Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DaanHoogland commented May 26, 2026

Uh oh!

blueorangutan commented May 26, 2026

Uh oh!

blueorangutan commented May 26, 2026

Uh oh!

DaanHoogland commented May 26, 2026

Uh oh!

jmsperu commented May 26, 2026

Uh oh!

DaanHoogland commented May 26, 2026

Uh oh!

blueorangutan commented May 26, 2026

Uh oh!

blueorangutan commented May 26, 2026

Uh oh!

DaanHoogland commented May 27, 2026

Uh oh!

blueorangutan commented May 27, 2026

Uh oh!

blueorangutan commented May 28, 2026

Uh oh!

jmsperu commented Aug 24, 2026

Uh oh!

Reviewers

Assignees

Labels

jmsperu commented Mar 17, 2026 •

edited

Loading

codecov Bot commented Mar 17, 2026 •

edited

Loading