feat(minvmd,minimald): quiesce data volume on stop and gate READY on mount - #705
Merged
Conversation
|
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 (12)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChangesState volume lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant minvmd
participant rpc_client
participant minimald
participant state_volume
minvmd->>rpc_client: request guest shutdown
rpc_client->>minimald: send Shutdown RPC
minimald->>state_volume: release tracker and quiesce mount
minvmd->>minvmd: terminate VMM
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
…mount Unit 2 of the per-VM writable ext4 volume spec (R2.1-R2.5): a clean `minvmd stop` now leaves the volume's ext4 journal closed, and a volume that cannot be mounted fails the boot loudly instead of ghost-READY. Guest (minimald): - quiesce_state_volume: syncfs + plain umount, falling back to remount-ro (clears INCOMPAT_RECOVER despite open fds) + MNT_DETACH; called from the Shutdown RPC handler after session drain, before the ack, with a 10s ceiling (R2.1, R2.2). - mount_state_volume fails closed: device absence is an error (minvmd attaches vdb on every boot), and a formatted volume that fails to mount gets one e2fsck -p repair + retry, never a reformat (R2.4). - MOUNT_FAILED beacon: a mount failure emits MOUNT_FAILED\n<reason>\n instead of READY and parks; no tmpfs fallback path remains (R2.4, R2.5). - The sessions manager releases the lcache read tracker on shutdown: its locked alog fd on the volume pinned the mount busy (EBUSY) and left the journal dirty on every stop. lcache/mctx grow a release_read_tracker() for this; the EBUSY warn now lists fd holders. Host (minvmd): - `stop` invokes the guest Shutdown RPC (force) over the bridge UDS before SIGTERM, via a new blocking rpc_client; failures warn and proceed with the unchanged SIGTERM->SIGKILL escalation (R2.3). The 15s client timeout contains the guest's 10s quiesce budget. - read_ready_beacon returns a typed BootBeacon; on MountFailed, boot/ run fail with a pre-existence-aware message: a pre-existing image may hold session data and is kept, a freshly provisioned blank is removed so the next boot is not misclassified (R2.5). e2e (gated MINVMD_E2E=1, verified on macOS/HVF): a clean stop leaves EXT4_VALID_FS set and INCOMPAT_RECOVER clear on the image; an unformattable fresh volume and an unmountable pre-existing volume both fail boot loudly with the right image disposition. boot_e2e (and the new tests) now also isolate HOME: dirs::state_dir() ignores XDG_STATE_HOME on macOS, so the tests otherwise clobber the real state dir. Refs: #583 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
norrietaylor
force-pushed
the
feat/vdb-unit2-quiesce
branch
from
July 10, 2026 17:04
8f969a6 to
bd23a5e
Compare
Member
Author
|
Rebased onto latest main (over #690's socket/path rework). Two things beyond mechanical conflict resolution:
All proofs re-verified on macOS/HVF after the rebase: clean stop → 🤖 Generated with Claude Code |
norrietaylor
marked this pull request as ready for review
July 10, 2026 18:21
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@crates/minvmd/src/cmd/mod.rs`:
- Around line 289-311: Track provisioning ownership from the volume
creation/provisioning function through the mount-failure path instead of relying
on the pre-spawn volume_preexisted check. Update the provisioning result and
callers to carry an atomic created_by_this_boot flag, then have
mount_failed_error remove the image only when that flag is true; preserve the
existing warning and pre-existing-image behavior otherwise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d56ead12-e9ac-41be-a112-ad8d48472d0c
📒 Files selected for processing (15)
crates/lcache/src/lib.rscrates/mctx/src/lib.rscrates/minimald/src/guest.rscrates/minimald/src/main.rscrates/minimald/src/rpc.rscrates/minimald/src/server.rscrates/minimald/src/sessions.rscrates/minvmd/src/cmd/boot.rscrates/minvmd/src/cmd/mod.rscrates/minvmd/src/cmd/run.rscrates/minvmd/src/cmd/stop.rscrates/minvmd/src/lib.rscrates/minvmd/src/rpc_client.rscrates/minvmd/tests/boot_e2e.rscrates/minvmd/tests/volume_quiesce_e2e.rs
Review findings on the quiesce/loud-mount-failure work: - stop: split the Shutdown RPC deadline into a 5 s connect/handshake budget (a wedged guest must not stall the recovery command — libkrun accepts the bridge connect regardless of guest health, so only a completed handshake proves a live daemon) and a 120 s RPC budget (the ack lands only after the unbounded session drain plus the guest's 10 s quiesce; giving up mid-drain would SIGTERM with a dirty journal). - boot/run: the image pre-existence probe treats a stat error as "pre-existing" so a transient failure can never route a data-bearing image to the delete branch; the fresh blank image is discarded on every failed-boot arm (beacon error, READY timeout) — not just MOUNT_FAILED — so a stranded blank can't masquerade as data-at-risk on the next boot; mount_failed_error is now message-only. - the shutdown quiesce gates on Config.state_volume_mounted (set only when the guest actually mounted the volume) instead of the vsock transport, so a native --vsock daemon or a volume-less microVM never has its state dir synced-and-unmounted out from under it. - the guest instance-lock relocation to /run keys on being the VM init (is_minimal_microvm) rather than --vsock, keeping non-root native --vsock daemons working; a non-vsock daemon whose state-volume mount fails exits with an error instead of parking behind a beacon nobody reads; a failed MOUNT_FAILED emit is now logged rather than silently degrading to a READY timeout. - the data volume mounts via a strict (EBUSY-propagating) mount — the EBUSY-tolerant raw_mount is for idempotent pseudo-fs mounts only — and every mount_state_volume failure names its operation, so the MOUNT_FAILED reason can't misdirect (a pre-R1.7 rootfs EROFS no longer reads as a corrupt image needing fsck). - run --detach: the supervisor's stderr lands in <state>/run.log and both failure messages point at it, instead of /dev/null swallowing the mount-failure diagnosis. - volume_quiesce_e2e stops the VM on drop, so a mid-test panic no longer leaks a detached VM whose state dir the TempDir then unlinks. Refs: #583 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
norrietaylor
added a commit
that referenced
this pull request
Jul 13, 2026
VM e2e harnesses are now selected by binary-name convention instead of a hardcoded per-test list, so adding a test never requires a CI edit: - Harnesses in crates/minvmd/tests/ end in `_e2e` (non-root) or `_root_e2e` (root, run under sudo for CAP_NET_ADMIN). - The KVM lane's three enumerated steps collapse to two convention filtersets: `binary(/_e2e$/) and not binary(/_root_e2e$/)` and `binary(/_root_e2e$/)`. A new `*_e2e.rs` is picked up with no YAML. - Rename krun_smoke -> krun_smoke_e2e and vsock_relay_e2e -> vsock_relay_root_e2e; delete the retired bridge_e2e stub. - A unit test in minvmd (runs in core-tests, no libkrun) guards the suffix so a typo fails CI instead of silently dropping a harness. This newly runs volume_quiesce_e2e (added in #705 but never wired into a lane) in the KVM lane. The macOS lane tracks the rename only; its filterset conversion is coupled to the nextest-archive-on-mac work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbXL6jxHTY2YuLPaMxc1yU
norrietaylor
added a commit
that referenced
this pull request
Jul 14, 2026
…s every lane (#732) * test(minvmd,ci): auto-discover VM e2e harnesses by name suffix VM e2e harnesses are now selected by binary-name convention instead of a hardcoded per-test list, so adding a test never requires a CI edit: - Harnesses in crates/minvmd/tests/ end in `_e2e` (non-root) or `_root_e2e` (root, run under sudo for CAP_NET_ADMIN). - The KVM lane's three enumerated steps collapse to two convention filtersets: `binary(/_e2e$/) and not binary(/_root_e2e$/)` and `binary(/_root_e2e$/)`. A new `*_e2e.rs` is picked up with no YAML. - Rename krun_smoke -> krun_smoke_e2e and vsock_relay_e2e -> vsock_relay_root_e2e; delete the retired bridge_e2e stub. - A unit test in minvmd (runs in core-tests, no libkrun) guards the suffix so a typo fails CI instead of silently dropping a harness. This newly runs volume_quiesce_e2e (added in #705 but never wired into a lane) in the KVM lane. The macOS lane tracks the rename only; its filterset conversion is coupled to the nextest-archive-on-mac work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbXL6jxHTY2YuLPaMxc1yU * test(minimald,linux-native): fold the netns track into root e2e auto-discovery The netns-integration job hardcoded `cargo test -p minimald --test netns`. Rename netns.rs -> netns_root_e2e.rs and select it by the same convention the KVM lane uses: the renamed `minimald-root-e2e` job runs `cargo nextest run -p minimald -E 'binary(/_root_e2e$/)'`, so a new crates/minimald/tests/*_root_e2e.rs runs with no workflow edit. A minimald unit test guards the suffix; mesh_uc7 is allowlisted because its networking-wg feature is off by default and no lane runs it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbXL6jxHTY2YuLPaMxc1yU * ci(macos): auto-discover VM e2e harnesses from a nextest archive The mac e2e job hardcoded two test names and ran them as direct binaries. Build a nextest archive before the codesign (`nextest run --archive-file` is offline, so the signature survives) and select harnesses by the `_e2e` convention, so a new crates/minvmd/tests/*_e2e.rs runs on mac with no workflow edit. This newly runs boot_e2e and volume_quiesce_e2e on mac. The full-boot harnesses spawn the codesigned minvmd as a subprocess (MINVMD_BIN). krun_smoke_e2e stays a separate kernel-less FFI-smoke step, excluded from the archive run: it boots in-process through krun_smoke_child, which cannot take the hypervisor entitlement from within the archive. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbXL6jxHTY2YuLPaMxc1yU * docs: update specs for the e2e test renames and bridge_e2e removal Reflect the auto-discovery migration in the frozen unit specs: krun_smoke -> krun_smoke_e2e in R1.5, and note that bridge_e2e was removed (superseded by minimald_session_e2e). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbXL6jxHTY2YuLPaMxc1yU * refactor(minvmd,ci): drop the dead host-side tap relay + its KVM wiring minvmd's net::relay (open_tap/attach_to_switch/SwitchRelay) has had no production caller since #581 moved the per-PTask tap+relay into the guest minimald over the vsock shuttle; it was a frozen byte-for-byte fork of minimald's net::switch that never received the ingress gate the live copy grew. Its only caller was vsock_relay_root_e2e, which booted no VM and re-proved the relay against a hand-made netns — coverage already provided, with real PTask-to-PTask traffic, by minimald's netns_root_e2e UC6. Delete net/relay.rs, its module wiring, and the test; correct the net.rs module doc (the tap + relay run in the guest, not here). The KVM lane's root step existed only for that test, so it is removed along with the gvproxy materialize step and the vendor/gvproxy pin filter it needed; the lane keeps a single auto-discovered harness step (a future minvmd *_root_e2e would need its own sudo-wrapped step). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbXL6jxHTY2YuLPaMxc1yU * refactor(minimald,ci): name and describe the root e2e proofs functionally Drop the internal use-case numbers (UC1/UC4/UC6/UC7) from the netns test function names, its module doc, the native-lane comments, and the minimald naming guard. Describe each proof by what it does — a no-network task cannot reach the internet, a static ingress mapping exposes then removes a task's port, two own-IP tasks reach each other over the gvproxy switch — and state why they need root: they create tap devices and configure network namespaces, which requires CAP_NET_ADMIN, so the unprivileged CI runner grants it via sudo. No behavior change; the harness is selected by binary name, not function name, so the renames are self-contained. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbXL6jxHTY2YuLPaMxc1yU * refactor(minvmd,minimald,ci): rename harness e2e tests to _integration The harness-driven nextest proofs are component/subsystem INTEGRATION tests, not full-system e2e: each stands up one subsystem against real resources (boot a microVM, round-trip the vsock bridge, drive the libkrun FFI, check a real ext4 volume, exercise the tap/netns stack) rather than driving the `minimal` CLI through the whole system. The only true end-to-end proof is the CLI script (scripts/session-e2e.sh). Rename the auto-discovery suffix _e2e / _root_e2e -> _integration / _root_integration across the five harnesses, the KVM/native/mac lane filtersets, and both naming guards; the guard docs now state when to reach for an integration harness vs. a full-system e2e script. Rename the minvmd-scoped lifecycle script (lifecycle-e2e.sh -> minvmd-lifecycle.sh) and the native root job (minimald-root-e2e -> minimald-root-integration). Also scrub the workflow YAMLs of internal plan jargon (UC/N/R references) in favour of functional descriptions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HbXL6jxHTY2YuLPaMxc1yU --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Since #672, session worktrees and the package cache persist on the per-VM
/dev/vdbvolume — user data with no host copy. This PR fixes the failure modes that made that persistence unsafe: stopping a VM corrupted-by-default, and a broken volume failed silently.Failure modes fixed
1. Every
minvmd stopwas an unclean unmount — dirty journal, possible data loss.stopsent SIGTERM straight to the VMM with nothing syncing or unmounting the volume. Every boot after a stop replayed the ext4 journal ("recovering journal" on every startup), and withsync_mode=relaxedany writes from the final seconds before the kill — agit commitin a session, a build artifact — could vanish entirely.Now:
stopasks the in-VM minimald to shut down over the bridge UDS before signalling: sessions drain, the volume getssyncfs+ unmount (falling back to remount-ro, which closes the journal even with open fds), and only then does SIGTERM fire. A clean stop leaves a cleanly-unmounted filesystem; SIGTERM/SIGKILL escalation is unchanged when the guest doesn't answer.2. A VM whose volume failed to mount reported READY anyway — a ghost VM that eats data.
A mount failure was only logged; the guest silently kept its state on the boot tmpfs and signalled READY. Sessions appeared to work, wrote everything to RAM, and lost it all at the next stop — with zero indication anything was wrong. An undersized or corrupt volume image produced a "healthy" VM indistinguishable from a real one.
Now: the guest never substitutes tmpfs. On any mount failure it emits a distinct
MOUNT_FAILED\n<reason>\nbeacon instead of READY, andminvmd boot/runfail with a non-zero exit and the guest's actual failure reason.3. A failing volume offered no repair path and no safe disposition.
There was no distinction between "this image holds your sessions and won't mount" and "this blank image was just provisioned wrong" — and no repair attempt at all.
Now: a formatted volume that fails to mount gets one
e2fsck -prepair + mount retry before failing closed (never a reformat once a superblock exists). On failure, the host keys off whether the image pre-existed the boot: a pre-existing image may hold session data, so it is left untouched and the error points at repair orMINVMD_DATA_VOLUME_PATH; a blank image freshly created this boot is removed so the next boot isn't misdiagnosed as data-at-risk.Found along the way
Two lifetime-held write fds on the volume made every unmount/remount fail
EBUSY, keeping the journal dirty even after a quiesce — the shutdown sequence only became verifiable once both were gone:alog/<n>.v1open for the daemon's lifetime; the sessions manager now releases it during shutdown (Cache::release_read_tracker).minimald.locklives in the provider dir, which in-guest sits on the volume; the vsock-mode instance lock now lives on the/runtmpfs (host-side placement unchanged — nothing outside the boot reads the guest's lock).The quiesce EBUSY warning now lists the offending fd holders via a
/procscan, so the next accidental pin is diagnosable from the console log.Verification
New gated e2e suite (
MINVMD_E2E=1, verified on macOS/HVF), asserting against the raw image bytes — no host ext4 tooling needed:EXT4_VALID_FSset,INCOMPAT_RECOVERclear);vm-up, non-zero exit, blank image removed;vm-up, non-zero exit, image preserved.VM-booting tests (including the pre-existing
boot_e2e) now also isolateHOME, sodirs-based path fallbacks can never escape onto a developer's real state dir.Refs: #583, builds on #613's Shutdown RPC
🤖 Generated with Claude Code
Summary by CodeRabbit
MOUNT_FAILEDreasons, and better run diagnostics viarun.log.