sidecar: remove 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 removes the assert.

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 <223556219+Copilot@users.noreply.github.com> 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)

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)

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)

Copilot

Pull request overview

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

Comments suppressed due to low confidence (1)

openhcl/sidecar_client/src/lib.rs:158

This info log’s message assumes the gap means earlier nodes were "skipped (no sidecar-started APs)", but with the assert removed the supported/expected case is also that early CPUs simply aren’t sidecar-managed (e.g., first node base_cpu is nonzero). Consider making the message neutral so it doesn’t imply a specific cause that may be wrong in scale tests.

                    "sidecar node follows a gap; earlier node(s) skipped (no sidecar-started APs)"
                );

mattkur

@babayet2 wasn't happy with the state of the petri test, so we're going to take this change without the test. Emilijan will take up the task of improving test coverage in this area (likely with Steven and Chris' help)

github-actions · 2026-07-30T20:09:03Z

1 Petri tests failed (0 unstable)

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

remove broken assert

a816db5

Copilot AI review requested due to automatic review settings July 30, 2026 18:27

babayet2 force-pushed the sidecar-assert branch from c4c55ca to a816db5 Compare July 30, 2026 18:27

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

Copilot started reviewing on behalf of babayet2 July 30, 2026 18:28 View session

babayet2 changed the title ~~sidecar: fix assert in sidecar node creation flow~~ sidecar: remove assert in sidecar node creation flow Jul 30, 2026

Copilot AI reviewed Jul 30, 2026

View reviewed changes

mattkur approved these changes Jul 30, 2026

View reviewed changes

babayet2 enabled auto-merge (squash) July 30, 2026 18:48

babayet2 merged commit 2542acf into microsoft:main Jul 30, 2026
99 of 101 checks passed

babayet2 mentioned this pull request Jul 30, 2026

sidecar: remove assert in sidecar node creation flow (#4044) #4101

Open

Uh oh!

Conversation

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

Uh oh!

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mattkur left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

babayet2 commented Jul 27, 2026 •

edited

Loading

babayet2 commented Jul 28, 2026 •

edited

Loading