fix(vm): sync the new runtime into var instead of destroying it - #206
fix(vm): sync the new runtime into var instead of destroying it#206mobileoverlord wants to merge 3 commits into
Conversation
Builds on the var-reset branch and replaces the reset. Deleting var.btrfs on a seed change also deletes /var/lib/docker, which holds the SDK image layers and the avo-<uuid> volume that *is* $AVOCADO_PREFIX — so every update cost a full SDK re-pull and sysroot rebuild, and the release notes had to tell people to re-run `avocado install` and `avocado build`. That was never necessary. The avocado-owned half of var is already content-addressed and versioned (images/, runtimes/, active), and avocadoctl already installs a runtime alongside the current one and switches `active` only once it is staged and verified. So the guest can adopt the release's runtime out of the new seed and leave everything else untouched. The btrfs work has to happen guest-side regardless: this VM exists because the host is macOS or Windows, neither of which can mount a btrfs image. - `vm update` records runtime.pending_var_seed_sha rather than unlinking the disk. - `vm start` attaches that seed read-only and names its sha on the cmdline; the guest echoes it into a stamp, which is what lets the marker be cleared for this sync specifically. Every failure leaves it set, so retry is the whole recovery story. - The seed is addressed by serial rather than /dev/vdN: the var and data drives are both conditional, so enumeration order is not stable across boots or arches. - Never attached when the live disk was byte-copied from that same seed — identical btrfs fsid, which the kernel reads as one multi-device filesystem. Nothing is destroyed, so --reset-var, the typed-'update' prompt and the var_reset event go away; the JSON event becomes var_seed_sync_pending, which host applications can ignore rather than acting on. record_var_size_floor goes with them — it existed only so a grown disk survived the delete. Requires the guest-side unit in avocado-vm; without it the drive is attached and nothing reads it, which is exactly today's behaviour.
`enable_services` parsed WantedBy=/RequiredBy= out of the unit file alone. systemd also reads [Install] from drop-ins, and `systemctl enable` honours a WantedBy= declared there — so the standard way to enable a vendor unit that ships without an [Install] was invisible here, and the build failed with the fix already sitting in the overlay. Hit while building avocado-vm against distro 2026, where qemu-guest-agent.service no longer carries an [Install] section. Parsed one file at a time: a single sed over several files shares an input stream, so the /^\[Install\]/,/^\[/ range would run past the end of one file and match keys in the next. Test runs the generated fragment with sh against a real sysroot holding a unit with no [Install] plus a drop-in supplying one, and asserts the .wants symlink appears — string assertions can't catch a shell bug in generated code. Fails against the previous parser.
… UUID Found by running the upgrade on a real VM: it dropped into emergency mode, with btrfs on /dev/vda where the erofs rootfs belongs. `serial=` needs `if=none` plus an explicit `-device`, and an explicit `-device virtio-blk-pci` takes a lower PCI slot than the devices qemu creates for `if=virtio`. So the seed became /dev/vda and pushed the rootfs and var disks along, breaking both `root=/dev/vda` on the cmdline and `/dev/vdb /var` in the guest fstab. Addressing the seed by serial fixed the guest's lookup and broke the two consumers that address disks by position. Keep every drive on `if=virtio` so attaching one is purely additive, and identify the seed by its btrfs filesystem UUID instead, passed as avocado.seed_fsid= alongside the sha. Reading that UUID is a 16-byte read at a fixed superblock offset rather than a shell out to blkid: the hosts this VM exists for are macOS and Windows, which have neither blkid nor btrfs. Nothing here is cfg-gated, so it behaves the same on all three. Tests pin the ordering property directly — no seed emits one if=virtio drive, a seed emits exactly two, the seed never precedes the rootfs, and no explicit -device appears.
|
End-to-end tested — stock avocado-vm 0.3.0 migrated to a locally built release with the seed attached by this change, 8/8. Results in the PR body under Testing: Nothing needed re-running Guest half: avocado-vm#3 + #4, with avocadoctl#18 + #19 in the guest binary. |
| .runtime? | ||
| .pending_var_seed_sha?; | ||
|
|
||
| if !paths.var_disk().exists() { |
There was a problem hiding this comment.
This exists() guard is dead in the start() path: seed_var_disk runs earlier (line 155) and recreates a missing disk from this same seed, so the disk always exists by the time this runs. And vm reset / rebuild --reset-data delete the disk without clearing pending_var_seed_sha, so the next start byte-copies the seed via seed_var_disk and then attaches the seed beside its copy — the two-devices-one-fsid state the doc comment forbids, reachable via a documented command. Suggest guarding by comparing btrfs_fsid of the live disk against the seed instead of existence, and clearing the marker wherever the var disk is deleted.
| let new_sha = new_manifest | ||
| .artifact("var") | ||
| .map(|a| a.sha256.clone()) | ||
| .expect("should_sync_var_seed only fires on a var artifact"); |
There was a problem hiding this comment.
should_sync_var_seed matches any seed_only artifact, but this resolves artifact("var") specifically — a manifest whose seed_only artifact has a different name panics here mid-update, after the VM teardown. Derive the sha from the same artifact the gate matched, or make the gate require artifact("var").
| else { | ||
| return; | ||
| }; | ||
| if !applied.trim().eq_ignore_ascii_case(expected_sha) { |
There was a problem hiding this comment.
The stamp is a bare sha that persists in /var across boots, so it can vouch for a sync that did not happen this boot. Example: update to sha X, guest syncs and stamps X; later the channel rolls back and forward so pending_var_seed_sha is X again, but this boot the guest sync fails (runtime GC'd, copy failed) — the old stamp still reads X, the marker clears, and the retry-on-next-start story silently ends while the VM runs the wrong runtime. A per-update nonce on the cmdline, echoed into the stamp, would make only this boot's sync able to clear this update's marker.
| return; | ||
| }; | ||
| if !applied.trim().eq_ignore_ascii_case(expected_sha) { | ||
| crate::utils::output::print_warning( |
There was a problem hiding this comment.
This warning is suppressed under --output json, and there is no applied/failed counterpart to var_seed_sync_pending — so a host app driving vm start in JSON mode cannot tell "extensions updated" from "sync failed, still on the old runtime" and will report success on stale extensions. Worth emitting a var_seed_sync_applied / var_seed_sync_failed event on this path.
2 participants
Stacked on #196 — targets
fix-vm-update-reset-varso the diff reads as a delta. Squashing into #196 is fine; the two are one change in intent.Why this replaces the reset
#196 deletes
var.btrfson a seed change. That also deletes/var/lib/docker, which holds the SDK image layers and theavo-<uuid>volume that is$AVOCADO_PREFIX— hence the release note telling people to re-runavocado installandavocado build.That was never necessary. The avocado-owned half of var is already content-addressed and versioned (
images/,runtimes/,active), andavocadoctlalready installs a runtime alongside the current one and switchesactiveonly once it is staged and verified. The guest can adopt the release's runtime out of the new seed and leave everything else alone.The btrfs work has to happen guest-side regardless: this VM exists because the host is macOS or Windows, neither of which can mount a btrfs image.
Change
vm updaterecordsruntime.pending_var_seed_shainstead of unlinking the disk.vm startattaches that seed read-only and names its sha and filesystem UUID on the cmdline. The guest echoes the sha into a stamp, which is what lets the marker be cleared for this sync. Every failure leaves it set, so the next start retries — that is the whole recovery story.seed_var_diskis astd::fs::copy, so the two carry an identical btrfs fsid and the kernel would read them as one multi-device filesystem. A correctness gate, not an optimisation.The ordering bug this found
The first version addressed the seed by
serial=, which needsif=noneplus an explicit-device. An explicit-device virtio-blk-pcitakes a lower PCI slot than the devices qemu creates forif=virtio— so the seed became/dev/vda, the rootfs and var disks shifted, and the VM dropped into emergency mode withroot=/dev/vdapointing at a btrfs seed. Fixing the guest's lookup broke the two consumers that address disks positionally: the kernel cmdline and the guest fstab.Every drive now stays on
if=virtioso attaching one is purely additive, and the guest finds the seed by filesystem UUID. Reading that UUID is a 16-byte read at a fixed superblock offset rather than shelling out toblkid— the hosts this VM exists for have neither blkid nor btrfs. Nothing is cfg-gated; it behaves the same on macOS, Linux and Windows.Tests pin the ordering property directly: no seed emits one
if=virtiodrive, a seed emits exactly two, the seed never precedes the rootfs, and no explicit-deviceappears.Also included
ext/build.rsnow reads[Install]from drop-ins, not just the unit body. systemd honours aWantedBy=declared in a drop-in, so the standard way to enable a vendor unit shipping without an[Install]was invisible to the build. Parsed one file at a time — a singlesedover several files shares an input stream, so the[Install]range would run past a file boundary. Test runs the generated fragment withshagainst a real sysroot and asserts the.wantssymlink appears; it fails against the previous parser.What this removes from #196
Nothing is destroyed, so
--reset-var, the typed-updateprompt and thevar_resetevent all go. The JSON event becomesvar_seed_sync_pending, which host applications can ignore rather than act on.record_var_size_floorgoes too — it existed only so a grown disk survived the delete.min_cli_version— the most valuable part of #196 — is untouched.Testing
1412 unit tests, clippy clean.
End to end on a real VM: stock 0.3.0 migrated to a locally built release, 8/8 assertions — var.btrfs the same inode throughout, active runtime changed, all extensions merged, and the user's Docker volume, image layers and
/varmarker intact. Kernel 6.6.123 → 6.18.35, extensions 2024.1.0 → 0.1.0.Depends on
avocado-vm#3 (guest unit) and avocado-vm#4. Without #3 the drive is attached and nothing reads it — exactly today's behaviour, so they can land in either order.