feat(minvmd,minimald): per-VM writable /dev/vdb ext4 volume (#583 Unit 1) - #671
Closed
norrietaylor wants to merge 7 commits into
Closed
feat(minvmd,minimald): per-VM writable /dev/vdb ext4 volume (#583 Unit 1)#671norrietaylor wants to merge 7 commits into
norrietaylor wants to merge 7 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
…t 1) Place the package cache, rootfs-staging trees, and session state on one per-VM writable ext4 volume (/dev/vdb), resolving the EXDEV hardlink constraint and moving session state off the RAM-backed tmpfs. - R1.1/R1.2: krun_add_disk3 FFI + SyncMode tri-state (u32, not bool) and Context::add_disk_with_sync. - R1.3: crates/minvmd/src/volume.rs — VolumeProvisioner + BlankRawProvisioner (sparse raw via ftruncate); MINVMD_VOLUME_BYTES. - R1.4: attach /dev/vdb in vm.rs; provision in vmm_child (MINVMD_DATA_VOLUME_PATH). - R1.5: guest mount_state_volume — ext4 superblock probe -> mkfs.ext4 on first boot -> mount at /var/lib/minimal. Formats 1 MiB below the device to survive libkrun's backing-file trailer shave across reboots. - R1.6: relocate cache + state onto the volume when it mounts (gated so a no-volume boot still works; Unit 2/R2.4 removes the fallback). - R1.8: reduce guest RAM 4096 -> 2048 MiB when the volume is attached. - R1.9: MINVMD_DISK_SYNC / MINVMD_DISK_DIRECT_IO tunables (default relaxed/false). - R1.7: re-pin gominimal/pkgs to 12f324d8 (microvm-rootfs ships /var/lib/minimal + e2fsprogs/util-linux). Validated end-to-end on macOS/HVF (aarch64, libkrun 1.19.0): EXDEV fix, allocate-on-write + fstrim reclaim, sync-mode throughput, and persistence + mkfs-idempotency across 3 reboots. minvmd clippy/-D + 90 tests; minimald cross-compiles clean for aarch64-musl. Refs: #583 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
norrietaylor
force-pushed
the
feat/vm-ext4-volume-unit1
branch
from
July 8, 2026 05:40
c46eeed to
7e0a55e
Compare
Extend the existing macOS/HVF and Linux/KVM boot-e2e jobs to boot with a writable data volume attached (MINVMD_DATA_VOLUME_PATH) and assert the guest actually formats + mounts /dev/vdb at /var/lib/minimal and relocates cache/state onto it (R1.4/R1.5/R1.6). READY alone is insufficient — Unit 1 falls back to tmpfs on mount failure — so the guest boot log is asserted. A second boot on the same image guards superblock idempotency and the mkfs margin that absorbs libkrun's backing-file trailer shave (no reformat, no ext4 "bad geometry"). The provisioned image is checked to stay sparse (allocate-on-write). This turns the spec's Linux/KVM proof-artifact leg into CI coverage; the macOS leg was previously validated only via a local harness. Refs: #583 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/ci-linux-kvm.yml (1)
54-56: 🧹 Nitpick | 🔵 TrivialTimeout budget: verify headroom for two extra full VM boots.
This job's 60-minute timeout must now accommodate Boot E2E, Session E2E, DM1 relay, Daemon lifecycle, a 10-iteration boot benchmark, and now two additional full VM boots for Volume E2E. Worth confirming this still fits comfortably; a flaky timeout failure here is harder to diagnose than a logic bug.
Also applies to: 172-204
🤖 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 @.github/workflows/ci-linux-kvm.yml around lines 54 - 56, The minvmd-linux-kvm-e2e job’s timeout may be too tight after adding Volume E2E’s two extra full VM boots. Review the full workflow steps in the ci-linux-kvm job, especially the minvmd-linux-kvm-e2e job and the Boot E2E, Session E2E, DM1 relay, Daemon lifecycle, and boot benchmark steps, then increase the timeout-minutes only if needed so the total execution has comfortable headroom..github/workflows/ci-macos.yml (1)
109-109: 🧹 Nitpick | 🔵 TrivialTight 20-minute
boot-e2etimeout now has two more full boots.This job already runs Boot E2E, Session E2E, and a 10-iteration boot benchmark within a 20-minute timeout on a self-hosted runner; adding two more full VM boots for Volume E2E further tightens that budget. Recommend bumping
timeout-minutesfor this job to reduce the risk of spurious timeout failures.Also applies to: 124-124, 196-228
🤖 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 @.github/workflows/ci-macos.yml at line 109, The boot-e2e job in the macOS CI workflow is now doing more full VM boots than its current 20-minute budget can reliably handle. Update the timeout for the boot-e2e job so it has enough headroom for Boot E2E, Session E2E, the boot benchmark, and the added Volume E2E runs. Apply the same timeout change anywhere the boot-e2e job is duplicated in the workflow so the settings stay consistent.crates/minvmd/src/vm.rs (1)
305-313: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider warning on an unrecognized
MINVMD_DISK_SYNCvalue instead of silently defaulting.An unmatched value (e.g. a typo like
ful) falls through toRelaxed, silently dropping a requested stronger durability posture with no signal. Atracing::warn!on the_ => Nonearm would surface misconfiguration without changing the default behavior.🤖 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/src/vm.rs` around lines 305 - 313, The sync mode parsing in vm.rs silently falls back to SyncMode::Relaxed when DISK_SYNC_ENV contains an unrecognized value, so update the match inside the sync_mode initialization to emit a tracing::warn! from the `_ => None` path before defaulting. Keep the current default behavior unchanged, but use the warning to surface bad MINVMD_DISK_SYNC configuration in the code that builds SyncMode from std::env::var(DISK_SYNC_ENV).
🤖 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 @.github/workflows/ci-linux-kvm.yml:
- Around line 188-199: The boot validation step relies on log greps but never
enforces the exit status of the two cargo test runs, so a failed `cargo test -p
minvmd --test boot_e2e` can still pass if expected strings appear. Update this
run block in the workflow to follow the same failure-handling pattern as the
“Daemon lifecycle” step, either by enabling strict shell mode at the start of
the script or by explicitly checking each `cargo test` exit code before running
the `grep` assertions. Use the existing `cargo test` invocations and the
surrounding run block as the place to make this change.
---
Nitpick comments:
In @.github/workflows/ci-linux-kvm.yml:
- Around line 54-56: The minvmd-linux-kvm-e2e job’s timeout may be too tight
after adding Volume E2E’s two extra full VM boots. Review the full workflow
steps in the ci-linux-kvm job, especially the minvmd-linux-kvm-e2e job and the
Boot E2E, Session E2E, DM1 relay, Daemon lifecycle, and boot benchmark steps,
then increase the timeout-minutes only if needed so the total execution has
comfortable headroom.
In @.github/workflows/ci-macos.yml:
- Line 109: The boot-e2e job in the macOS CI workflow is now doing more full VM
boots than its current 20-minute budget can reliably handle. Update the timeout
for the boot-e2e job so it has enough headroom for Boot E2E, Session E2E, the
boot benchmark, and the added Volume E2E runs. Apply the same timeout change
anywhere the boot-e2e job is duplicated in the workflow so the settings stay
consistent.
In `@crates/minvmd/src/vm.rs`:
- Around line 305-313: The sync mode parsing in vm.rs silently falls back to
SyncMode::Relaxed when DISK_SYNC_ENV contains an unrecognized value, so update
the match inside the sync_mode initialization to emit a tracing::warn! from the
`_ => None` path before defaulting. Keep the current default behavior unchanged,
but use the warning to surface bad MINVMD_DISK_SYNC configuration in the code
that builds SyncMode from std::env::var(DISK_SYNC_ENV).
🪄 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: 549df0c0-b16b-4a42-b7dd-4928991af902
📒 Files selected for processing (13)
.github/workflows/ci-linux-kvm.yml.github/workflows/ci-macos.yml.minimal/minimal.tomlcrates/minimald/src/guest.rscrates/minimald/src/main.rscrates/minvmd/src/cmd/mod.rscrates/minvmd/src/cmd/vmm_child.rscrates/minvmd/src/krun/ctx.rscrates/minvmd/src/krun/mod.rscrates/minvmd/src/krun/raw.rscrates/minvmd/src/lib.rscrates/minvmd/src/vm.rscrates/minvmd/src/volume.rs
Comment on lines
+188
to
+199
| run: | | ||
| img="$MINVMD_DATA_VOLUME_PATH"; rm -f "$img" | ||
| echo "=== boot 1 (fresh volume: format + mount) ===" | ||
| MINVMD_BOOT_LOG="$RUNNER_TEMP/vol1.log" cargo test -p minvmd --test boot_e2e -- --include-ignored --nocapture | ||
| grep -q "formatting data volume" "$RUNNER_TEMP/vol1.log" || { echo "::error::boot 1 did not format /dev/vdb"; tail -60 "$RUNNER_TEMP/vol1.log"; exit 1; } | ||
| grep -q "mounted writable state volume" "$RUNNER_TEMP/vol1.log" || { echo "::error::R1.5: /dev/vdb was not mounted"; tail -60 "$RUNNER_TEMP/vol1.log"; exit 1; } | ||
| grep -q "relocated onto /dev/vdb" "$RUNNER_TEMP/vol1.log" || { echo "::error::R1.6: cache/state not relocated"; tail -60 "$RUNNER_TEMP/vol1.log"; exit 1; } | ||
| echo "=== boot 2 (reuse volume: no reformat, no bad geometry) ===" | ||
| MINVMD_BOOT_LOG="$RUNNER_TEMP/vol2.log" cargo test -p minvmd --test boot_e2e -- --include-ignored --nocapture | ||
| grep -q "mounted writable state volume" "$RUNNER_TEMP/vol2.log" || { echo "::error::boot 2 failed to mount the existing volume"; tail -60 "$RUNNER_TEMP/vol2.log"; exit 1; } | ||
| ! grep -q "formatting data volume" "$RUNNER_TEMP/vol2.log" || { echo "::error::boot 2 reformatted (superblock idempotency broken)"; exit 1; } | ||
| ! grep -qi "bad geometry" "$RUNNER_TEMP/vol2.log" || { echo "::error::ext4 bad geometry on reboot (mkfs margin regressed)"; tail -60 "$RUNNER_TEMP/vol2.log"; exit 1; } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing explicit exit-code check on cargo test invocations.
Neither boot invocation (line 191, line 196) nor the run block itself sets set -e/checks $?. If the underlying cargo test process fails (e.g., VM crashes or panics) after emitting the expected log lines ("formatting data volume", "mounted writable state volume") but before completing successfully, the subsequent grep -q checks can still pass and the step reports green despite the actual test failure. The other gating step in this file ("Daemon lifecycle", line 234) explicitly uses set -euo pipefail; this step should follow the same convention.
🔧 Proposed fix
run: |
+ set -uo pipefail
img="$MINVMD_DATA_VOLUME_PATH"; rm -f "$img"
echo "=== boot 1 (fresh volume: format + mount) ==="
- MINVMD_BOOT_LOG="$RUNNER_TEMP/vol1.log" cargo test -p minvmd --test boot_e2e -- --include-ignored --nocapture
+ MINVMD_BOOT_LOG="$RUNNER_TEMP/vol1.log" cargo test -p minvmd --test boot_e2e -- --include-ignored --nocapture \
+ || { echo "::error::boot 1 cargo test failed"; tail -60 "$RUNNER_TEMP/vol1.log"; exit 1; }
grep -q "formatting data volume" "$RUNNER_TEMP/vol1.log" || { echo "::error::boot 1 did not format /dev/vdb"; tail -60 "$RUNNER_TEMP/vol1.log"; exit 1; }
grep -q "mounted writable state volume" "$RUNNER_TEMP/vol1.log" || { echo "::error::R1.5: /dev/vdb was not mounted"; tail -60 "$RUNNER_TEMP/vol1.log"; exit 1; }
grep -q "relocated onto /dev/vdb" "$RUNNER_TEMP/vol1.log" || { echo "::error::R1.6: cache/state not relocated"; tail -60 "$RUNNER_TEMP/vol1.log"; exit 1; }
echo "=== boot 2 (reuse volume: no reformat, no bad geometry) ==="
- MINVMD_BOOT_LOG="$RUNNER_TEMP/vol2.log" cargo test -p minvmd --test boot_e2e -- --include-ignored --nocapture
+ MINVMD_BOOT_LOG="$RUNNER_TEMP/vol2.log" cargo test -p minvmd --test boot_e2e -- --include-ignored --nocapture \
+ || { echo "::error::boot 2 cargo test failed"; tail -60 "$RUNNER_TEMP/vol2.log"; exit 1; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| run: | | |
| img="$MINVMD_DATA_VOLUME_PATH"; rm -f "$img" | |
| echo "=== boot 1 (fresh volume: format + mount) ===" | |
| MINVMD_BOOT_LOG="$RUNNER_TEMP/vol1.log" cargo test -p minvmd --test boot_e2e -- --include-ignored --nocapture | |
| grep -q "formatting data volume" "$RUNNER_TEMP/vol1.log" || { echo "::error::boot 1 did not format /dev/vdb"; tail -60 "$RUNNER_TEMP/vol1.log"; exit 1; } | |
| grep -q "mounted writable state volume" "$RUNNER_TEMP/vol1.log" || { echo "::error::R1.5: /dev/vdb was not mounted"; tail -60 "$RUNNER_TEMP/vol1.log"; exit 1; } | |
| grep -q "relocated onto /dev/vdb" "$RUNNER_TEMP/vol1.log" || { echo "::error::R1.6: cache/state not relocated"; tail -60 "$RUNNER_TEMP/vol1.log"; exit 1; } | |
| echo "=== boot 2 (reuse volume: no reformat, no bad geometry) ===" | |
| MINVMD_BOOT_LOG="$RUNNER_TEMP/vol2.log" cargo test -p minvmd --test boot_e2e -- --include-ignored --nocapture | |
| grep -q "mounted writable state volume" "$RUNNER_TEMP/vol2.log" || { echo "::error::boot 2 failed to mount the existing volume"; tail -60 "$RUNNER_TEMP/vol2.log"; exit 1; } | |
| ! grep -q "formatting data volume" "$RUNNER_TEMP/vol2.log" || { echo "::error::boot 2 reformatted (superblock idempotency broken)"; exit 1; } | |
| ! grep -qi "bad geometry" "$RUNNER_TEMP/vol2.log" || { echo "::error::ext4 bad geometry on reboot (mkfs margin regressed)"; tail -60 "$RUNNER_TEMP/vol2.log"; exit 1; } | |
| run: | | |
| set -uo pipefail | |
| img="$MINVMD_DATA_VOLUME_PATH"; rm -f "$img" | |
| echo "=== boot 1 (fresh volume: format + mount) ===" | |
| MINVMD_BOOT_LOG="$RUNNER_TEMP/vol1.log" cargo test -p minvmd --test boot_e2e -- --include-ignored --nocapture \ | |
| || { echo "::error::boot 1 cargo test failed"; tail -60 "$RUNNER_TEMP/vol1.log"; exit 1; } | |
| grep -q "formatting data volume" "$RUNNER_TEMP/vol1.log" || { echo "::error::boot 1 did not format /dev/vdb"; tail -60 "$RUNNER_TEMP/vol1.log"; exit 1; } | |
| grep -q "mounted writable state volume" "$RUNNER_TEMP/vol1.log" || { echo "::error::R1.5: /dev/vdb was not mounted"; tail -60 "$RUNNER_TEMP/vol1.log"; exit 1; } | |
| grep -q "relocated onto /dev/vdb" "$RUNNER_TEMP/vol1.log" || { echo "::error::R1.6: cache/state not relocated"; tail -60 "$RUNNER_TEMP/vol1.log"; exit 1; } | |
| echo "=== boot 2 (reuse volume: no reformat, no bad geometry) ===" | |
| MINVMD_BOOT_LOG="$RUNNER_TEMP/vol2.log" cargo test -p minvmd --test boot_e2e -- --include-ignored --nocapture \ | |
| || { echo "::error::boot 2 cargo test failed"; tail -60 "$RUNNER_TEMP/vol2.log"; exit 1; } | |
| grep -q "mounted writable state volume" "$RUNNER_TEMP/vol2.log" || { echo "::error::boot 2 failed to mount the existing volume"; tail -60 "$RUNNER_TEMP/vol2.log"; exit 1; } | |
| ! grep -q "formatting data volume" "$RUNNER_TEMP/vol2.log" || { echo "::error::boot 2 reformatted (superblock idempotency broken)"; exit 1; } | |
| ! grep -qi "bad geometry" "$RUNNER_TEMP/vol2.log" || { echo "::error::ext4 bad geometry on reboot (mkfs margin regressed)"; tail -60 "$RUNNER_TEMP/vol2.log"; exit 1; } |
🤖 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 @.github/workflows/ci-linux-kvm.yml around lines 188 - 199, The boot
validation step relies on log greps but never enforces the exit status of the
two cargo test runs, so a failed `cargo test -p minvmd --test boot_e2e` can
still pass if expected strings appear. Update this run block in the workflow to
follow the same failure-handling pattern as the “Daemon lifecycle” step, either
by enabling strict shell mode at the start of the script or by explicitly
checking each `cargo test` exit code before running the `grep` assertions. Use
the existing `cargo test` invocations and the surrounding run block as the place
to make this change.
Both provisioner tests derived their temp dir from only process::id(), so they shared one directory. cargo runs tests in parallel, so one test's remove_dir_all cleanup could race the other's files between its two ensure() calls, recreating the image at the wrong size and failing the "existing image must not be resized" assertion (seen on CI, timing-passed locally). Key the temp dir by a per-test tag as well as the pid. Refs: #583 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
krun_add_disk3 (this PR's /dev/vdb attach) needs libkrun >= 1.19.0, but the self-hosted macOS boot-e2e runner's brew libkrun is stale (1.18.1) and its /opt/homebrew is not writable, so it can't self-upgrade. The symbol resolves lazily, so unit tests and the no-volume boot pass, but attaching /dev/vdb would crash the VMM with a dyld symbol-not-found. - Add a libkrun-macos job that builds a self-contained libkrun v1.19.4 from source on GitHub-hosted macos-latest (mirrors release.yml's build-libkrun-macos-arm64), cached by ref, uploaded as a prefix tarball. - boot-e2e stages it via LIBKRUN_PREFIX (link + rpath) and DYLD_FALLBACK_LIBRARY_PATH (runtime), keeping the runner's libkrunfw as the dlopen fallback. - Explicit gate on both lanes: assert the libkrun in use exports krun_add_disk3 (nm), failing with an actionable "need >= 1.19.0" message instead of a cryptic dyld crash at VM boot. Refs: #583 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
libkrun's krun-input build.rs runs bindgen, which needs libclang. `brew install lld` pulls in the llvm keg that provides it, but bindgen only finds it via LIBCLANG_PATH. Without this the libkrun-macos job fails with "failed to run custom build command for krun-input: libclang.dylib no such file". (release.yml's build-libkrun-macos-arm64 has the same latent gap.) Refs: #583 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… drop R1.8 Apply the xhigh review's correctness batch and wire the volume on by default. - Attach on by default: vmm_child now always provisions + attaches /dev/vdb at the resolved path (MINVMD_DATA_VOLUME_PATH override, else <state>/data-vol.raw honouring XDG_STATE_HOME). The feature is no longer dormant behind an env var. resolve_data_volume_path centralises this. - Provision at the literal path, not a stem-reconstructed <dir>/<stem>.raw: a non-.raw override no longer silently attaches a different blank file. - Drop the single-use VolumeProvisioner trait + BlankRawProvisioner for a plain ensure_sparse_raw(path, size) (CLAUDE.md: no single-use abstractions). - Revert R1.8 (guest RAM reduction): halving RAM off the mere presence of the env var, while the guest mount is best-effort, leaves a mount-failed VM at half RAM with the cache still on tmpfs. Deferred to Unit 2 (R2.4 makes a failed mount fatal) with a measured floor. This also makes `minvmd status` (which reports vm_ram_mib()) correct again. - guest mount_state_volume: use try_exists() (a stat error no longer reads as "absent" and silently drops the volume); create the mountpoint before mkfs so a missing mountpoint fails before a needless format. Refs: #583 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
R1.8 was dropped from the spec entirely (memory pressure moves to a separate spec), so the vm_ram_mib note should not point at Unit 2/R2.4. Refs: #583 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
1 participant
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.
per-VM writable ext4
/dev/vdbvolumeImplements first stage of the spec for #583: a per-VM writable ext4 volume that carries the package cache, rootfs-staging trees, and session state on one filesystem — resolving the
EXDEVhardlink constraint and moving session state off RAM onto durable storage.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests