test(minvmd): vsock stub and bridge end-to-end test - #350
Conversation
…R3.5) Closes #328 - Add tests/bridge_e2e.rs: boots a VM via minvmd boot --foreground, waits for vm-up, then opens 5 concurrent host UDS connections to the minimald bridge socket; each writes a distinct payload and reads it back through the libkrun UDS↔vsock bridge — all 5 must succeed (R3.3, R3.4). XDG_RUNTIME_DIR is isolated per run via tempfile::TempDir. - Add TSI ~62-concurrent-connection cap comment to vm.rs near the krun_add_vsock_port2 call (R3.5). The boot→READY round-trip (R2.4) is verified by the pre-existing tests/boot_e2e.rs, which was moved from #326. The guest-side vsock stub (/sbin/minvmd-stub-init) source is already embedded in scripts/build-rootfs.sh: it sends READY\n to vsock port 7350 and then exec-s socat VSOCK-LISTEN:2222,fork EXEC:cat for the bridge (R3.4). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a macOS-only end-to-end test that boots ChangesBridge E2E Test and TSI Documentation
Sequence Diagram(s)sequenceDiagram
participant TestBinary
participant minvmd
participant HostUDS
participant GuestVsock
TestBinary->>minvmd: spawn `minvmd boot --foreground`
minvmd->>GuestVsock: configure vsock bridge port
loop 5 concurrent threads
TestBinary->>HostUDS: connect and write payload
HostUDS->>GuestVsock: libkrun forwards payload via vsock
GuestVsock-->>HostUDS: echo payload back
HostUDS-->>TestBinary: return echoed payload
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/minvmd/tests/bridge_e2e.rs (1)
90-92: 💤 Low valueFixed sleep introduces minor race condition.
The 200ms sleep assumes the guest's
socatlistener will be ready within that window. While the subsequent retry logic (lines 103-122) mitigates failures, consider documenting that the retry loop handles race conditions if the sleep is insufficient.📝 Optional: clarify retry safety in comment
- // The guest stub sends READY before exec-ing the vsock 2222 listener; - // give socat a moment to start accepting. + // The guest stub sends READY before exec-ing the vsock 2222 listener. + // Brief sleep to reduce initial connection failures; the per-connection + // retry loop (lines 103-122) handles races if socat is not yet listening. std::thread::sleep(Duration::from_millis(200));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minvmd/tests/bridge_e2e.rs` around lines 90 - 92, The fixed 200ms sleep (std::thread::sleep(Duration::from_millis(200))) can still race with the guest socat listener; update the inline comment above that sleep to explicitly state that the subsequent connect retry loop handles any remaining race (the retry logic that attempts to connect to vsock/2222 and retries on failure), so future readers know the sleep is a best-effort delay and the retry loop provides correctness; optionally mention alternatives (increase sleep or poll for socket) as notes for future change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/minvmd/tests/bridge_e2e.rs`:
- Around line 90-92: The fixed 200ms sleep
(std::thread::sleep(Duration::from_millis(200))) can still race with the guest
socat listener; update the inline comment above that sleep to explicitly state
that the subsequent connect retry loop handles any remaining race (the retry
logic that attempts to connect to vsock/2222 and retries on failure), so future
readers know the sleep is a best-effort delay and the retry loop provides
correctness; optionally mention alternatives (increase sleep or poll for socket)
as notes for future change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1204aea8-852c-471b-9158-9812f9cf8dca
📒 Files selected for processing (2)
crates/minvmd/src/vm.rscrates/minvmd/tests/bridge_e2e.rs
This comment has been minimized.
This comment has been minimized.
bridge_e2e was added by #350 but wired into no CI job, so the self-hosted Apple Silicon runner compiled it (via `cargo test -p minvmd`) yet never executed its `#[ignore]` body. The new test ran nowhere despite the macOS e2e infrastructure existing. Build the bridge_e2e harness alongside boot_e2e before the single codesign, then run its binary directly with the cache-pulled kernel and staged rootfs — mirroring the boot_e2e gating step so the codesigned minvmd keeps its hypervisor entitlement. Refs: #328
This comment has been minimized.
This comment has been minimized.
The 200ms sleep is best-effort; update the comment to explicitly state that the per-connection retry loop handles races if socat is not yet listening, so future readers understand the correctness guarantee. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
|
/revise pull the latest branch and fix the failing test |
Relying on write-side shutdown propagation through the libkrun UDS\u2194vsock bridge caused read_to_string to block until the 5-second IO timeout: the bridge does not reliably translate a UDS half-close into a vsock EOF that socat's cat sees. Switch to read_exact(payload.len()), which reads back exactly the bytes we sent without waiting for EOF. cat echoes data as it arrives, so this is sufficient and never blocks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
|
Commit pushed: Fix: replaced The original approach relied on write-side half-close propagating through libkrun's UDS↔vsock bridge so the guest's
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "index.crates.io"See Network Configuration for more information.
|
There was a problem hiding this comment.
Generated by sdd-review for issue #350 · ● 19.6M
| MINVMD_E2E=1 \ | ||
| MINVMD_KERNEL_PATH="$RUNNER_TEMP/kernel/vmlinuz" \ | ||
| MINVMD_ROOTFS_PATH="$(scripts/build-rootfs.sh --print-path)" \ | ||
| "$testbin" --include-ignored --nocapture |
There was a problem hiding this comment.
LOW · correctness — Missing MINVMD_BOOT_LOG capture.
The adjacent boot E2E step (line 115) sets MINVMD_BOOT_LOG="$RUNNER_TEMP/boot.log" so the hvc0 console output is captured and uploaded on failure. This step boots its own VM but omits the env var, so if this boot fails there is no console log to debug it — the upload step only has the boot_e2e log.
Consider adding e.g. MINVMD_BOOT_LOG="$RUNNER_TEMP/bridge-boot.log" and a matching upload step (or adjusting the existing upload to glob both).
sdd-validate · Implementation boundaryBoundary: Implementation (no spec or architecture files changed). Findings
DetailsFinding 1 — Info · Proof 1 deferred to consumer CI. Finding 2 — Info · Proof 2 deferred to consumer CI. Finding 3 — Pass · TSI cap comment verified. Finding 4 — Blocker · Protected path change outside task scope. Finding 5 — Warning · Finding 6 — Pass · No credentials.
|
|
Proofs have been validated in CI |
|
Unresolved review comment — protected path edit required The unresolved thread from sdd-review (discussion_r3360022935) identifies a missing Addressing this requires editing Required change ( MINVMD_E2E=1 \
MINVMD_KERNEL_PATH="$RUNNER_TEMP/kernel/vmlinuz" \
MINVMD_ROOTFS_PATH="$(scripts/build-rootfs.sh --print-path)" \
+ MINVMD_BOOT_LOG="$RUNNER_TEMP/bridge-boot.log" \
"$testbin" --include-ignored --nocaptureAnd update the upload step (around line 130–138) to capture both log files, e.g. by globbing: with:
name: minvmd-boot-log
- path: ${{ runner.temp }}/boot.log
+ path: ${{ runner.temp }}/*-boot.log
if-no-files-found: ignoreNote on the CodeRabbit nitpick (bridge_e2e.rs lines 90–92): already addressed in the current code — the comment now reads Please make the
|
Closes #328
Summary
Implements the vsock stub and bridge end-to-end tests for the minvmd host daemon (R3.3, R3.4, R3.5, R2.4 READY round-trip).
Changes
crates/minvmd/tests/bridge_e2e.rs(new)Bridge end-to-end test (R3.3, R3.4):
MINVMD_E2E=1,#[ignore], macOS-only.minvmd boot --foreground, waits up to 15 s forvm-up.$XDG_RUNTIME_DIR/minimal/minimald.sock.BRIDGE_PAYLOAD_0..4\n) and reads it back through the libkrun UDS↔vsock bridge.tempfile::TempDirasXDG_RUNTIME_DIR.crates/minvmd/src/vm.rs(modified)Adds a comment near the
krun_add_vsock_port2call documenting the TSI ~62-concurrent-connection cap (R3.5).Proof Artifacts
Proof 1 — Bridge concurrent round-trip (R3.3, R3.4)
Boots a VM, opens 5 concurrent host UDS connections, each writes a distinct payload and reads it back — all 5 succeed.
Proof 2 — Boot→READY round-trip (R2.4)
The pre-existing
tests/boot_e2e.rsasserts the full READY marker round-trip: the guest stub writesREADY\nto vsock port 7350 (VSOCK_MARKER_PORT) and the host parent receivesvm-upon stdout.Proof 3 — TSI cap comment (R3.5)
crates/minvmd/src/vm.rsnow contains a comment nearkrun_add_vsock_port2documenting the TSI ~62-concurrent-connection cap.Guest-side vsock stub (R3.4)
The guest stub source is already embedded in
scripts/build-rootfs.shas/sbin/minvmd-stub-init:READY\n(R2.4 marker).socat VSOCK-LISTEN:2222,fork EXEC:cat— listens on vsock 2222 and echoes per connection (R3.3, R3.4).Merging this pull request closes task sub-issue #328. Once every task sub-issue under tracking issue #311 is closed, the pipeline advances to
sdd:donefor final human review and close.Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
index.crates.ioSee Network Configuration for more information.
Summary by CodeRabbit
Tests
Documentation
Chores