docs(spec-vm-ext4-volume): pkg cache on the in-VM ext4 volume (volume-aware) - #658
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new specification document for a persistent writable ext4 volume in the VM, covering disk layout, provisioning, shutdown quiescing, READY gating, session persistence, and verification requirements. ChangesExt4 volume specification document
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/specs/08-spec-vm-ext4-volume/08-spec-vm-ext4-volume.md`:
- Around line 299-304: The current R2.5 mount-failure check is unreachable
because it depends on inspecting sessions/ after a failed /dev/vdb mount. Update
the spec language to make the guest emit MOUNT_FAILED, or exit, on any mount
error instead of conditionally checking mounted state, and clarify in the R2.5
text that minvmd run should surface the failure as a user-visible error while
the host determines fatality.
- Around line 377-383: Normalize the ProviderIndex schema in the spec text so
the key/value model is consistent: choose a single canonical representation for
session_id and state clearly whether the JSON map is keyed by session UUIDv7,
with session_id stored only in VolumeEntry, or whether session_id is duplicated
in both key and value. Update the ProviderIndex/VolumeEntry description to match
the insert, get_by_session, and remove behavior without conflicting terminology.
- Around line 368-373: The force=true upload flow in the spec’s worktree swap
path assumes rename(2) can atomically replace an existing non-empty worktree,
which it cannot. Update the behavior described around the staging-directory swap
to use a different atomic replacement mechanism or an indirection layer for the
live worktree, and ensure the atomicity requirement is still expressed in terms
of the worktree becoming either the old or new state after upload.
🪄 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: 85ae430a-3cf9-479e-a27f-f459f3746a96
📒 Files selected for processing (3)
.github/workflows/release.ymldocs/specs/08-spec-vm-ext4-volume/08-spec-vm-ext4-volume.mdscripts/stage-release.sh
💤 Files with no reviewable changes (2)
- scripts/stage-release.sh
- .github/workflows/release.yml
This comment has been minimized.
This comment has been minimized.
- R2.5: emit MOUNT_FAILED unconditionally on any /dev/vdb mount failure;
drop the unreachable "inspect sessions/ after a failed mount" branch and
move the fatal-vs-recoverable decision host-side.
- R3.3: replace non-empty worktree via renameat2(RENAME_EXCHANGE) instead
of rename(2), which fails with ENOTEMPTY on a non-empty destination.
- R3.4: normalize the ProviderIndex schema — map keyed by SessionId, with
VolumeEntry holding only { image_path, vm_id } and not repeating the id.
This comment has been minimized.
This comment has been minimized.
| - `/dev/vdb` — per-VM writable RAW ext4, carries `minimal_state_dir = | ||
| /var/lib/minimal` (cache, `tasks/`, `sandboxes/`, `sessions/`). | ||
|
|
||
| Provisioning is guest-driven on first boot: the host creates a sparse raw file; |
There was a problem hiding this comment.
I'm not sure if the sparse raw file thing works, but might be worth testing.
There are two other options:
- qcow2 file, which automagically handles sparsity itself. We keep the split between the rootfs and the mutable file as per this spec.
- We use two files on disk, and rely on the qcow2 "backing file" / "overlay" feature. In this world, the ext4 rootfs is the base qcow2, and the mutable qcow2 file holds the delta from that. In this world, we only need to
krun_add_disk*the mutable one (which has a pointer to the backing file), and the diff goes in there, and the VM is never the wiser that theres actually two files involved.
There was a problem hiding this comment.
You're right it's untested: I've added Proof Artifact 3 as a decision gate.
|
|
||
| **Purpose:** Resolve the `EXDEV` failure by placing the cache and rootfs-staging | ||
| trees on a shared writable ext4 volume. Introduces the host provisioner, the | ||
| `krun_add_disk3` FFI binding, and the guest mount step. |
There was a problem hiding this comment.
Is there a specific reason why we want to use krun_add_disk3 rather than one of the older variants?
There was a problem hiding this comment.
Nvm, answered down the bottom. I'll add that we probably want to experiment with sync mode and direct IO, they will have performance implications.
There was a problem hiding this comment.
R1.9 now makes both MINVMD_DISK_SYNC/MINVMD_DISK_DIRECT_IO tunable instead of hardcoded, and Proof Artifact 4 times a build across the matrix so the default comes from measurement.
Hold the RAW two-disk topology (Phase 1 scope per #583) and answer the review concerns with measurement rather than assertion. - R1.9: MINVMD_DISK_SYNC / MINVMD_DISK_DIRECT_IO tunables; stop hardcoding sync_mode/direct_io in R1.4 so the durability/throughput posture is measurable and per-platform tunable. - Proof Artifact 3 (decision gate): verify allocate-on-write sparsity and characterize reclaim-on-discard on APFS; a failing reclaim result is the documented trigger to reopen qcow2 (#647). - Proof Artifact 4: time a build workload across the sync/direct_io matrix so the default is chosen from data. - Design Considerations: why RAW is held against the qcow2 data-disk and qcow2 backing-file-overlay alternatives (VMM gate, crash surface, Phase-3 topology). - Open Question 4: durability granularity vs. the single shared volume. Refs: #583
This comment has been minimized.
This comment has been minimized.
Measured results — RAW sparse
|
| Measurement | Result |
|---|---|
Allocate-on-write — guest writes 2 GiB to raw /dev/vdb |
host image st_blocks*512: 4 KiB → exactly 2048 MiB (allocates what's written, not the 8 GiB logical size) |
Reclaim-on-discard — BLKDISCARD [0, 2 GiB) |
host image 2048 MiB → 0 MiB (libkrun forwards virtio UNMAP to a host hole-punch on APFS) |
| virtio-blk discard capability | /sys/block/vdb/queue/discard_max_bytes ≈ 2 TiB, discard_granularity 4096 |
Provisioning uses ftruncate (File::set_len), which is sparse on APFS — not fallocate without KEEP_SIZE (which would allocate eagerly and defeat the gate).
@twitchyliquid64 — re "not sure if the sparse raw file thing works": it works, both directions. Lazy allocate-on-write and reclaim-on-TRIM. The RAW space story is sound, so there's no reclamation problem to escape from — no reason to take on qcow2's VMM backing-file gate or added crash surface. Holding the RAW two-disk narrowing.
Proof Artifact 4 — sync/direct_io throughput
Sync-heavy pattern (O_DSYNC per 1 MiB block — exercises VIRTIO_BLK_F_FLUSH on every write, where the sync-mode cost shows most):
MINVMD_DISK_SYNC |
direct_io |
throughput |
|---|---|---|
| none | false | 4.7 GB/s |
| relaxed | false | 3.1 GB/s ← default |
| full | false | 254 MB/s (~12–18× slower) |
| relaxed | true | 1.2 GB/s |
Bulk 2 GiB + single fsync is less differentiated (3.0–4.5 GB/s) since the flush amortizes.
@twitchyliquid64 — re "experiment with sync mode and direct IO, they have performance implications": confirmed.
full(strict drive flush) collapses to 254 MB/s under frequent flushes on APFS;direct_io=truehalves throughput. Default =relaxed+direct_io=false(R1.9): honors flush so ext4 journal ordering holds → replay works on unclean shutdown, withoutfull's drive-flush penalty. Both are env-tunable per platform.
One caveat for the shipped product
Reclaim requires the guest to issue discard — mount -o discard (needs ext4, i.e. R1.7's e2fsprogs) or periodic fstrim (util-linux). The current microvm-rootfs (base + socat) has neither, so reclaim above was driven via an injected static BLKDISCARD helper. This is exactly the gap R1.7 closes.
Spec correction found while executing
R1.1 declares krun_add_disk3(… sync_mode: bool). The libkrun header (libkrun.h:278-284) types it uint32_t — a tri-state (KRUN_SYNC_{NONE,RELAXED,FULL}), not a bool. Implemented correctly as a SyncMode enum; R1.1/R1.9 text should be corrected to match.
Net: the sparsity gate that would have reopened qcow2 passes, so RAW two-disk stands on measurement, not assertion. Continuing into the rest of Unit 1 (guest mount, state-dir relocation, e2fsprogs in the rootfs closure — which also makes reclaim work without the injected helper).
…/KVM leg Fold the executed proof artifacts into the spec and extend them to cover Linux hosts, since two axes are host-dependent. - Proof Artifacts 3 & 4: record the macOS/HVF/APFS results (allocate-on- write 2 GiB exact; BLKDISCARD reclaim to 0; sync sweep none/relaxed/full = 4.7/3.1/0.25 GB/s) and add a required Linux/KVM leg (runs in CI; no /dev/kvm on a macOS host). - New Design Consideration "Platform differences": sync_mode RELAXED is identical to FULL on Linux (macOS drive-flush relaxation does not apply), so the three-point curve collapses to two; reclaim depends on the Linux host FS + KVM virtio-blk backend and must be verified, not inferred. - Non-Goals: Phase-2 reflink needs xfs/btrfs on Linux (ext4 has none), unlike APFS clonefile — a Linux host-FS prerequisite (#647). - Correct R1.1/R1.2/R1.9: krun_add_disk3 sync_mode is a u32 tri-state (KRUN_SYNC_{NONE,RELAXED,FULL}), not a bool; default RELAXED. - Verification matrix + Open Question 4 updated to the per-host split. Refs: #583
This comment has been minimized.
This comment has been minimized.
Reducing the RAM stop-gap when the cache leaves the tmpfs is only safe once a failed volume mount is fatal (no silent tmpfs fallback) and the floor is measured against real in-VM build memory pressure. Neither is settled by this spec, so the reduction moves to a separate memory-pressure spec. Recorded as a Non-Goal; R1.9 is left un-renumbered. Refs: #583
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Very cool. I think its okay that the guest doesnt issue trim, if that becomes a problem we can add a cron to do so. I expect ext4 to at least attempt to reuse its free blocks. SGTM for raw + sparse, sync=relaxed + direct=false. Given it seems relaxed sync means "honor flush", those perf measurements make sense lol. |
…rait Drop the single-use VolumeProvisioner trait / BlankRawProvisioner from the spec to match the implementation (code review: no abstraction for single-use code). R1.3 now specifies `ensure_sparse_raw(path, size)` provisioning at the literal path plus `resolve_data_volume_path()`; R1.4 reflects attach-on-every-boot (not env-gated) and the relaxed sync default. Rewords the Intro overview, the Phase-2 "seam" non-goal, affected areas, and the volume-size open question accordingly. Refs: #583
89c9b7b to
af16a85
Compare
This comment has been minimized.
This comment has been minimized.
Record Tom's review suggestion (#658): the guest does not proactively issue TRIM, so the RAW host image only shrinks on an explicit discard; if host-side growth becomes a problem, a periodic guest fstrim (e.g. a cron) reclaims freed blocks. ext4 reuses its own free blocks in place regardless. Captured as a Non-Goal / follow-up. Refs: #583
sdd-validate — Spec boundary findingsBoundary resolved: Spec ( Tracking issue: #583 (labels: Gate 1 — Acceptance criteria testable: ✅ PASSAll requirement IDs (R1.1–R1.9, R2.1–R2.5, R3.1–R3.5) map to observable pass/fail behaviors. The Verification table at the end of the spec provides explicit test commands and CLI assertions for each requirement cluster. No vague aspirations detected at ≥80% confidence. Gate 2 — No implementation leakage:
|
| Unit | Artifacts | In range? | Notes |
|---|---|---|---|
| Unit 1 | 4 | Artifact 4 explicitly marked "Does not gate merge" — informational throughput measurement for default selection | |
| Unit 2 | 2 | ✅ | — |
| Unit 3 | 2 | ✅ | — |
All artifacts pass the empty-PR rule: each would fail against the current codebase before implementation. Specifically:
- Unit 1 Artifact 2 (boot test): current code emits READY via
/runtmpfs with no/dev/vdbrequirement; the artifact checks/dev/vdbis mounted before READY — fails today. - Unit 2 Artifact 2 (loud-failure test): current code emits READY even without a second disk; the artifact asserts no READY on missing disk — fails today.
- Unit 3 Artifact 1 (self-heal test):
Store::newcurrently hard-fails on corruptindex.jsonafter READY; the artifact asserts READY is reached — fails today.
No Blocker: no unit lacks a proof artifact. The over-count on Unit 1 is advisory. Artifact 4 being explicitly non-gating is acceptable; it quantifies the cost tradeoff that informs R1.9's default values.
Gate 5 — Demoable unit sized to a reviewable PR: ✅ PASS (advisory)
All three units are substantive scope with distinct dependency edges (Unit 2 depends on Unit 1; Unit 3 depends on Unit 1 and Unit 2). No under-sized units or candidates for folding identified.
Summary
| Gate | Result | Severity |
|---|---|---|
| 1 — Acceptance criteria testable | Pass | — |
| 2 — No implementation leakage | Finding | |
| 3 — Assumptions explicit | Pass | — |
| 4 — Proof artifacts present and behavioral | Finding (Unit 1 count) | |
| 5 — Demoable unit sized to reviewable PR | Pass | — |
No Blockers. No needs-human applied. The spec is ready to merge.
The implementation leakage (Gate 2) is pervasive and likely inherited from the detailed tracking-issue plan; the behavioral intent is clear even where API names are specified. The Unit 1 artifact count (Gate 4) exceeds the guideline but all four are behavioral and Artifact 4 is explicitly non-gating.
sdd-validate· spec boundary · PR #658 · workflow run
Generated by sdd-validate for issue #658 · ◷
Spec for #583: per-VM writable ext4
/dev/vdbvolume, resolving theEXDEVhardlink constraint between the package cache and rootfs staging trees inside the VM guest.What this spec covers
This spec replaces the per-boot
/runtmpfs with a persistent per-VM ext4 volume so that:tasks/,sandboxes/), and session workbenches all live on one filesystem (satisfying the hardlink constraint without any cross-device fallback).Three demoable units
Unit 1 — Writable volume attachment (EXDEV fix)
krun_add_disk3FFI binding, second disk attach invm.rs, guestmount_state_volumestep inenter_rootfs,minimal_state_dir→/var/lib/minimal, rootfs ships/var/lib/minimalmountpoint +e2fsprogs.Unit 2 — Shutdown quiesce + crash safety
Shutdown RPC extended with guest
syncfs+ unmount before VMM teardown;minvmd stopinvokes RPC before SIGTERM; loud mount failure (no silent tmpfs fallback); no false READY on volume attach failure.Unit 3 — Session persistence, self-heal, upload-on-resume, provider index
sessions/exempt from boot-time reset; corruptindex.jsonself-heals fromrecord.json; workspace upload to non-empty worktree has defined atomicity semantics; host-sideProviderIndexmaps session id → volume image path.Next step
Please review and merge the spec PR to advance the tracking issue from the spec phase into triage.
Merging this spec PR will close the spec sub-issue (
Closes #<spec-sub-issue>is added to the PR body bysdd-pr-sanitize).Related to #583
Closes #657
Summary by CodeRabbit
/runtmpfs usage for cache, staging, and session workbenches.