[DNM] sidecar: fix assert in sidecar node creation flow by babayet2 · Pull Request #4044 · microsoft/openvmm

babayet2 · 2026-07-27T19:07:49Z

The sidecar client initialization flow assumes that sidecar nodes are contiguous, and that the nodes' CPU range begin at CPU 0. This does not always hold; in scale tests in which we stress the IO path, most VPs need a full linux kernel, and the CPU range for the first sidecar client in nonzero.

This PR relaxes the assert; we're now only asserting that sidecar nodes are ordered by CPU number, not contiguous.

github-actions · 2026-07-27T19:08:14Z

⚠️ Unsafe Code Detected

This PR modifies files containing unsafe Rust code. Extra scrutiny is required during review.

For more on why we check whole files, instead of just diffs, check out the Rustonomicon

mattkur · 2026-07-27T19:09:36Z

                Err(err) => return Err(err),
            };
-            assert_eq!(node.cpus.start, expected_base);
+            assert!(node.cpus.start >= expected_base);


Let's add a petri test that would evoke this scenario

Because, as far as I understand, this case comes about when we're doing a restore with the NUMA's base excluded from sidecar (right)? That should be deterministic, and will help illustrate the point.

Thanks for making this change!

Copilot

Pull request overview

Relaxes a sidecar client initialization invariant so sidecar nodes no longer need to be contiguous or start at CPU 0, addressing scale-test configurations where the first sidecar node’s CPU range begins at a nonzero CPU.

Changes:

Loosened the node CPU-range invariant from “must start exactly at expected base” to “must start at or after expected base” (allowing gaps).
Preserved monotonic ordering by updating the expected base to the end of each node’s CPU range.

mattkur · 2026-07-27T19:14:32Z

                Err(err) => return Err(err),
            };
-            assert_eq!(node.cpus.start, expected_base);
+            assert!(node.cpus.start >= expected_base);


mattkur · 2026-07-27T19:15:40Z

@babayet2: Thanks for making the fix! I added the correct backport label. After we fork a branch, we keep track of fixes targeting (and eventually backported) into those forked branches. We have some scripts that catch up the cherry picks, etc. So it's good to proactively label any fixes that you think need to go back.

github-actions · 2026-07-27T20:47:09Z

1 Petri tests failed (1 unstable)

Address PR microsoft#4044 review feedback: - Add a petri test (`servicing_keepalive_sidecar_first_node_nonzero_start`) that evokes the scenario the relaxed assert guards against: on a servicing restore where every AP of the first NUMA node had outstanding IO at save time, openhcl_boot skips that node's sidecar node, so the first sidecar node the client observes starts at a nonzero CPU. Uses 4 VPs across 2 NUMA nodes with delayed NVMe IO pinned to the first node's CPUs, then verifies OpenHCL restores without panicking and all VPs come online. - Restore the values dropped when the assert was changed from assert_eq! to assert!: the panic message now prints the node index, node.cpus.start, and expected_base so failures stay actionable. Co-authored-by: Copilot <[email protected]> Copilot-Session: 87928837-c8d6-42d8-a55b-3c8508a3c66e

mattkur · 2026-07-27T22:00:20Z

This looks related to #4051 . Which is going to be the fix?

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

+    assert!(
+        boot_logs_str.contains("node at base VP 0")
+            && boot_logs_str.contains("kernel-starting all of them"),
+        "first NUMA node's sidecar node was not skipped; the nonzero-start \
+         scenario was not evoked (its APs likely did not all have outstanding \
+         IO). Boot logs: {}",
+        boot_logs_str
+    );


github-actions · 2026-07-27T23:27:42Z

2 Petri tests failed (1 unstable)

The first pushed version of servicing_keepalive_sidecar_first_node_nonzero_start passed locally but failed in CI (x64-windows-intel): with max_io_queues=2, boot enumeration IO consumed both queue slots on the two NUMA base VPs, so node 0's AP (VP 1) never got its own NVMe IO queue and node 0 was not skipped. Rewrite the test to be deterministic: - A CPU is excluded from sidecar iff it has an NVMe IO queue in the persisted state (queue existence alone, no outstanding IO required), so drop the delayed-IO fault machinery. - Only node 0's AP (VP 1) needs a queue to skip node 0; node 1's AP (VP 3) must stay queue-free so a sidecar node survives at a nonzero base VP. - Set max_io_queues = vp_count so boot IO cannot exhaust the queue slots. - Deterministically create a queue on VP 1 with run_cpu_pinned_io, then verify the precondition (VP 1 has a queue, VP 3 does not) with find_cpus_with_io_issuers before saving, so the test fails loudly instead of passing vacuously. - Remove the now-unused spawn_pinned_in_flight_io helper. Co-authored-by: Copilot <[email protected]> Copilot-Session: 87928837-c8d6-42d8-a55b-3c8508a3c66e

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

babayet2 · 2026-07-28T23:34:45Z

@mattkur

@babayet2: Thanks for making the fix! I added the correct backport label. After we fork a branch, we keep track of fixes targeting (and eventually backported) into those forked branches. We have some scripts that catch up the cherry picks, etc. So it's good to proactively label any fixes that you think need to go back.

Thanks! I'll make sure to check the labels on future PRs

This looks related to #4051 . Which is going to be the fix?

We'll take this one.

Because, as far as I understand, this case comes about when we're doing a restore with the NUMA's base excluded from sidecar (right)? That should be deterministic, and will help illustrate the point.

Yes, right. I've pushed up a test that will run now in CI - my opinion is that it has value in proving out the bug in this PR, but is bloat as a vmm-test. I intend to rip it out before merging after I see a pass, unless you see some reason to keep it that I'm missing.

github-actions · 2026-07-29T03:39:43Z

3 Petri tests failed (1 unstable)

The 2-node version of servicing_keepalive_sidecar_first_node_nonzero_start flaked on AMD CI: guest-driven device discovery (SCSI MODE_SENSE from get_device_paths) issues a small amount of IO that the guest scheduler places on an arbitrary CPU, lazily creating a stray NVMe queue there. When that stray landed on node 1's only AP (VP 3), node 1 was emptied and sidecar disabled, tripping the precondition assert. Rework the test to be robust: - 8 VPs / 4 NUMA nodes (2 VPs each) instead of 4 VPs / 2 nodes, giving three candidate survivor nodes so a handful of stray queues cannot empty them all. - Drain node 0's AP (VP 1) deterministically so node 0 is always skipped. - Verify the precondition adaptively (some later node's AP is clean) rather than hard-coding a specific survivor. - Assert the invariant from the restore boot logs (node 0 skipped AND some node kept at a nonzero base VP, sidecar not disabled), independent of which node survives, removing a snapshot-vs-save race. Co-authored-by: Copilot <[email protected]> Copilot-Session: 87928837-c8d6-42d8-a55b-3c8508a3c66e

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

vmm_tests/vmm_tests/tests/tests/multiarch/openhcl_servicing.rs:419

numa_mem_sizes is setting 2 GiB per NUMA node (4 nodes => 8 GiB total), which is double the default Petri VM memory (4 GiB) and could make this test significantly heavier / more prone to resource-related flakes. Since the test only needs multiple vNUMA nodes (not necessarily 2 GiB each), consider distributing the default 4 GiB evenly across nodes (1 GiB each) to keep resource usage consistent with other tests.

        MemoryConfig {
            numa_mem_sizes: Some(vec![2 * 1024 * 1024 * 1024; NUM_NODES as usize]),
            ..Default::default()

vmm_tests/vmm_tests/tests/tests/multiarch/openhcl_servicing.rs:381

fault_start_updater is never mutated in this test, so the mut is unnecessary and slightly misleading (it suggests the updater toggles during the test, but it doesn't).

    let mut fault_start_updater = CellUpdater::new(false);

github-actions · 2026-07-29T09:30:41Z

5 Petri tests failed (4 unstable)

fix assert in sidecar client

eaa94ff

babayet2 assigned emirceski Jul 27, 2026

Copilot AI review requested due to automatic review settings July 27, 2026 19:07

babayet2 requested a review from a team as a code owner July 27, 2026 19:07

github-actions Bot added the unsafe Related to unsafe code label Jul 27, 2026

Copilot started reviewing on behalf of babayet2 July 27, 2026 19:08 View session

mattkur added the backport_1.8.2607 Change should be backported to the release/1.8.2607 branch label Jul 27, 2026

mattkur reviewed Jul 27, 2026

View reviewed changes

Copilot AI reviewed Jul 27, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 27, 2026 22:00

Copilot started reviewing on behalf of babayet2 July 27, 2026 22:00 View session

Copilot AI reviewed Jul 27, 2026

View reviewed changes

chris-oo approved these changes Jul 27, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 28, 2026 23:28

Copilot started reviewing on behalf of babayet2 July 28, 2026 23:29 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

babayet2 changed the title ~~sidecar: fix assert in sidecar node creation flow~~ [DNM] sidecar: fix assert in sidecar node creation flow Jul 29, 2026

Copilot AI review requested due to automatic review settings July 29, 2026 07:52

Copilot started reviewing on behalf of babayet2 July 29, 2026 07:53 View session

Copilot AI reviewed Jul 29, 2026

View reviewed changes

Uh oh!

Conversation

babayet2 commented Jul 27, 2026

Uh oh!

github-actions Bot commented Jul 27, 2026

Uh oh!

mattkur Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

mattkur Jul 27, 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!

mattkur Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

mattkur commented Jul 27, 2026

Uh oh!

github-actions Bot commented Jul 27, 2026

Uh oh!

mattkur commented Jul 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

github-actions Bot commented Jul 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

babayet2 commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 29, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

github-actions Bot commented Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

babayet2 commented Jul 28, 2026 •

edited

Loading