From 8e0cf401917de95875297b01b36d8a7194f257b1 Mon Sep 17 00:00:00 2001 From: Justin Schneck Date: Thu, 13 Aug 2026 13:27:02 -0400 Subject: [PATCH 1/3] fix(vm): sync the new runtime into var instead of destroying it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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- 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. --- src/commands/vm/update.rs | 312 ++++++++++++++------------------------ src/main.rs | 15 +- src/utils/vm/config.rs | 18 ++- src/utils/vm/lifecycle.rs | 205 +++++++++++++++++++++++++ src/utils/vm/qemu.rs | 120 +++++++++++++++ 5 files changed, 457 insertions(+), 213 deletions(-) diff --git a/src/commands/vm/update.rs b/src/commands/vm/update.rs index e350977f..d533fbca 100644 --- a/src/commands/vm/update.rs +++ b/src/commands/vm/update.rs @@ -9,28 +9,33 @@ //! - `seed_only` — the `var` image. Fetched on first install and, on a //! version bump, re-fetched and re-seeded. See below. //! -//! ## Why a version bump resets `var` +//! ## Why a version bump needs more than the boot artifacts //! -//! `seed_only` was written to preserve user state across image bumps, -//! on the assumption that the in-VM agent would migrate the existing -//! btrfs at boot. That -//! migration does not exist yet, and `var` holds more than user state: -//! the VM's own system extensions live in `/var/lib/avocado/images` and -//! are merged onto `/usr` and `/etc` at boot. Carrying `var` across a -//! bump therefore boots a new kernel and rootfs against the *old* -//! userspace. +//! `var` holds more than user state: the VM's own system extensions +//! live in `/var/lib/avocado` and are merged onto `/usr` and `/etc` at +//! boot. Replacing only kernel/initramfs/rootfs therefore boots a new +//! kernel against the *old* userspace, and nothing catches it — each +//! extension's `extension-release` carries `ID=_any`, systemd-sysext's +//! "matches any OS" wildcard, so mismatched extensions merge silently. //! -//! Nothing catches that. Each extension's `extension-release` is -//! synthesized at merge time carrying `ID=_any`, which is -//! systemd-sysext's explicit "matches any OS" wildcard — so the -//! mismatched extensions merge silently and the VM looks healthy. +//! It cannot be fixed by carrying the new seed's `var` wholesale +//! either: the same filesystem holds the user's Docker volumes and +//! installed SDKs (`$AVOCADO_PREFIX` is a Docker named volume inside +//! the VM), which are expensive to rebuild and entirely theirs. //! -//! Until the guest can migrate in place, a version bump takes the new -//! seed and discards the live disk. That costs the user their Docker -//! volumes, SDK images and `/data` work, so they must re-run `avocado -//! install` and `avocado build` — a tradeoff deliberately accepted over -//! shipping a VM whose halves don't match. An unchanged `var` sha means -//! no reset. +//! So the two lifetimes are separated at the granularity that already +//! distinguishes them. The avocado-owned half — `runtimes/`, `images/` +//! and the `active` pointer — is content-addressed and versioned, so +//! the guest can install the new runtime *alongside* the old one out of +//! the new seed and switch `active` only once it is fully staged. +//! `vm update` records the seed sha here; `vm start` attaches that seed +//! read-only; the guest does the work in early boot, before extensions +//! merge. Nothing the user owns is touched, and every failure leaves +//! the VM on its previous runtime rather than on none. +//! +//! 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. //! //! Behaviour with a running VM: query lifecycle, stop it cleanly, //! perform the swap, restart with the same `start` options. The @@ -60,10 +65,6 @@ pub struct UpdateCommand { pub channel: Option, pub check_only: bool, pub assume_yes: bool, - /// Authorizes the var reset when combined with `--yes`. Interactive - /// runs consent at the typed prompt instead, so this is only - /// consulted when `assume_yes` is set. - pub reset_var: bool, pub output: OutputFormat, } @@ -197,39 +198,20 @@ impl UpdateCommand { return Ok(()); } - let reseed_var = - should_reseed_var(installed.as_ref(), &new_manifest, paths.var_disk().exists()); - - // `--yes` alone consents to a non-destructive update only; the - // var reset needs `--reset-var` (interactive runs consent at - // the typed prompt instead). - if reseed_var && self.assume_yes && !self.reset_var { - bail!( - "this update ships a new /var image and resets the VM's internal \ - state (installed SDKs, Docker volumes, /data). Re-run with \ - `--yes --reset-var` to authorize, or without `--yes` to be \ - prompted." - ); - } + let sync_var_seed = + should_sync_var_seed(installed.as_ref(), &new_manifest, paths.var_disk().exists()); - // Confirm with the user (unless --yes). Deliberately after - // planning: whether this destroys the VM's state is the material - // fact about the operation, and we can't know it before reading - // the remote manifest. + // Confirm with the user (unless --yes). This no longer needs a + // destructive-consent path: the update replaces boot artifacts + // and schedules a state sync, and keeps the VM's Docker volumes, + // installed SDKs and /data either way. if !self.assume_yes { // The prompt writes prose into the NDJSON stream and blocks // on stdin; machine mode never prompts. if self.output.is_json() { - bail!( - "refusing to prompt in --output json mode; re-run with --yes{}", - if reseed_var { - " --reset-var (this update resets the VM's /var)" - } else { - "" - } - ); + bail!("refusing to prompt in --output json mode; re-run with --yes"); } - confirm(&avail.pointer, installed_version.as_deref(), reseed_var)?; + confirm(&avail.pointer, installed_version.as_deref(), sync_var_seed)?; } // Was the VM running before we tear it down? @@ -316,37 +298,43 @@ impl UpdateCommand { // the boot path stays the single owner of both, and an update // that dies here leaves no half-copied disk to mistake for // state — the next start just seeds it. - if reseed_var { - let var = paths.var_disk(); - if var.exists() { - // The live disk's length is the only record of how far - // the user grew /var (growing never wrote config). - // Persist it before the delete so the re-seeded disk - // comes back at the same capacity. - let live_len = std::fs::metadata(&var) - .with_context(|| format!("stat {}", var.display()))? - .len(); - let mut cfg = crate::utils::vm::config::VmConfig::load(&paths) - .context("loading config.yaml to record the var size")?; - if record_var_size_floor(&mut cfg, live_len)? { - cfg.save(&paths) - .context("recording runtime.var_size in config.yaml")?; - } - std::fs::remove_file(&var) - .with_context(|| format!("removing stale var disk {}", var.display()))?; - } + if sync_var_seed { + // Record the seed the guest owes a state sync from, rather + // than acting on the live disk here. `vm start` attaches + // that seed read-only and the guest lifts the new runtime + // out of it in early boot, before extensions merge — the + // only place the work can happen, since the host is macOS + // or Windows and has no btrfs. + // + // Nothing is destroyed: the new runtime is installed + // alongside the old and `active` only moves once it is + // fully staged, so every failure leaves the VM on its + // previous runtime rather than on none. + let new_sha = new_manifest + .artifact("var") + .map(|a| a.sha256.clone()) + .expect("should_sync_var_seed only fires on a var artifact"); + let mut cfg = crate::utils::vm::config::VmConfig::load(&paths) + .context("loading config.yaml to record the pending var seed")?; + cfg.runtime + .get_or_insert_with(Default::default) + .pending_var_seed_sha = Some(new_sha); + cfg.save(&paths) + .context("recording runtime.pending_var_seed_sha in config.yaml")?; + if json_mode { - // Host applications need this to know the VM's SDK and - // Docker state is gone, so they can drop cached install - // state and tell the user to re-run install/build. + // Distinct from the old `var_reset`: nothing is lost, so + // host applications keep their cached install state. + // Extensions change on the next boot, not on this call. crate::utils::output_format::emit_json_object(&json!({ - "event": "var_reset", + "event": "var_seed_sync_pending", "reason": "vm_image_updated", })); } else { println!( - "avocado vm update: reset /var to the new seed; \ - re-run `avocado install` and `avocado build`." + "avocado vm update: the VM will pick up the new Avocado \ + extensions on its next start; installed SDKs, Docker \ + volumes and /data are kept." ); } } @@ -444,17 +432,23 @@ fn plan_downloads( out } -/// Whether this run replaces the live var disk: +/// Whether the guest owes a state sync from the new seed: /// -/// - an installed manifest exists (a first install has no state to -/// destroy), -/// - the live disk exists (a never-started VM has nothing to reset, -/// and the prompt + `var_reset` event must not fire for one), and +/// - an installed manifest exists (a first install has nothing to carry +/// forward), +/// - the live disk exists — a never-started VM gets the new seed copied +/// wholesale by `seed_var_disk`, so it is already current. This case +/// also *must* stay false: the copy is byte-identical to the seed, so +/// attaching that seed alongside it would put two devices with one +/// btrfs fsid in front of the kernel. /// - a `seed_only` artifact's sha changed. Keyed on the manifests, not /// the planned downloads: `plan_downloads` also re-fetches a seed -/// that merely went missing (same sha), which must not reset the -/// live disk. -fn should_reseed_var(installed: Option<&Manifest>, new: &Manifest, var_disk_exists: bool) -> bool { +/// that merely went missing (same sha), which needs no sync. +fn should_sync_var_seed( + installed: Option<&Manifest>, + new: &Manifest, + var_disk_exists: bool, +) -> bool { let Some(installed) = installed else { return false; }; @@ -467,32 +461,6 @@ fn should_reseed_var(installed: Option<&Manifest>, new: &Manifest, var_disk_exis }) } -/// Raise `runtime.var_size` to cover a live disk of `live_len` bytes, -/// rounded up to whole GiB. Returns whether the config changed. The -/// effective size never shrinks (`grow_var_file` refuses to), so the -/// live disk is always >= any previously configured target; recording -/// its length preserves the largest size the user ever grew to. -fn record_var_size_floor( - cfg: &mut crate::utils::vm::config::VmConfig, - live_len: u64, -) -> Result { - use crate::utils::vm::lifecycle::{parse_size, DEFAULT_VAR_SIZE}; - - let configured = cfg - .runtime - .as_ref() - .and_then(|r| r.var_size.as_deref()) - .unwrap_or(DEFAULT_VAR_SIZE); - let configured_bytes = parse_size(configured) - .with_context(|| format!("invalid runtime.var_size {configured:?} in config.yaml"))?; - if live_len <= configured_bytes { - return Ok(false); - } - let gib = live_len.div_ceil(1 << 30); - cfg.runtime.get_or_insert_with(Default::default).var_size = Some(format!("{gib}G")); - Ok(true) -} - /// Best guess at the host's platform string. Matches what the avocado-vm /// stone generator emits. fn default_platform_for_host() -> String { @@ -511,36 +479,18 @@ async fn is_vm_running() -> bool { .unwrap_or(false) } -fn confirm(p: &ChannelPointer, installed: Option<&str>, reseed_var: bool) -> Result<()> { +fn confirm(p: &ChannelPointer, installed: Option<&str>, sync_var_seed: bool) -> Result<()> { let from = installed.unwrap_or("(not installed)"); println!("avocado vm update: {} -> {}", from, p.version); - if reseed_var { - // Spelled out, and gated behind typing a word rather than `y`, - // because this is not the non-destructive update the command name - // implies. Matches `avocado vm reset`'s prompt for the same reason. - println!(); - println!("This release ships a new /var image, which carries the VM's own"); - println!("system extensions, so /var is replaced rather than migrated. The"); - println!("update resets the VM's internal state:"); + if sync_var_seed { + // Informational, not a consent gate: the sync installs the new + // runtime alongside the old and only then switches, so nothing + // the user owns is at risk. Worth saying anyway, because the + // extensions change on the next boot rather than on this call. println!(); - println!(" - installed SDKs and their container images"); - println!(" - Docker volumes and build caches"); - println!(" - anything resident in /data inside the VM"); + println!("This release ships new system extensions. They are applied on the"); + println!("VM's next start; installed SDKs, Docker volumes and /data are kept."); println!(); - println!("Your projects on the host are untouched, but each will need"); - println!("`avocado install` and `avocado build` run again afterwards."); - println!(); - print!("Type 'update' to confirm: "); - use std::io::Write; - std::io::stdout().flush().ok(); - let mut line = String::new(); - std::io::stdin() - .read_line(&mut line) - .context("reading confirmation")?; - if line.trim() != "update" { - bail!("aborted by user"); - } - return Ok(()); } print!("Proceed? [y/N] "); use std::io::Write; @@ -787,24 +737,34 @@ mod tests { assert!(plan.is_empty()); } - // ── should_reseed_var — the one decision that destroys user state ── + // ── should_sync_var_seed — when the guest owes a state sync ── #[test] - fn reseed_requires_a_var_sha_change() { + fn sync_requires_a_var_sha_change() { let old = manifest("aa", "bb"); - assert!(should_reseed_var(Some(&old), &manifest("aa2", "bb2"), true)); + assert!(should_sync_var_seed( + Some(&old), + &manifest("aa2", "bb2"), + true + )); // Boot-artifact-only release: var untouched. - assert!(!should_reseed_var(Some(&old), &manifest("aa2", "bb"), true)); + assert!(!should_sync_var_seed( + Some(&old), + &manifest("aa2", "bb"), + true + )); } #[test] - fn reseed_never_fires_on_first_install_or_without_a_live_disk() { + fn sync_never_fires_on_first_install_or_without_a_live_disk() { let old = manifest("aa", "bb"); - // No installed manifest → no user state to destroy. - assert!(!should_reseed_var(None, &manifest("aa", "bb"), true)); - // VM never started → nothing to reset; claiming otherwise makes - // host applications drop cached install state for no reason. - assert!(!should_reseed_var( + // No installed manifest → nothing to carry forward. + assert!(!should_sync_var_seed(None, &manifest("aa", "bb"), true)); + // VM never started → `seed_var_disk` copies the new seed + // wholesale, so it is already current. Syncing anyway would + // attach a seed byte-identical to the live disk, colliding on + // btrfs fsid. + assert!(!should_sync_var_seed( Some(&old), &manifest("aa2", "bb2"), false @@ -812,60 +772,16 @@ mod tests { } #[test] - fn repairing_a_missing_seed_does_not_reseed_the_live_disk() { + fn repairing_a_missing_seed_does_not_schedule_a_sync() { // `plan_downloads` re-fetches a merely-missing seed at the same - // sha; that repair must never be read as "the release changed - // var" and cost the user their live disk. + // sha. That repair must not be read as "the release changed + // var" — the seed it would attach is the one the live disk was + // copied from, i.e. the fsid-collision case. let old = manifest("aa", "bb"); - assert!(!should_reseed_var(Some(&old), &manifest("aa", "bb"), true)); - } - - // ── record_var_size_floor — capacity survives the re-seed ── - - use crate::utils::vm::config::VmConfig; - - const GIB: u64 = 1 << 30; - - fn var_size_of(cfg: &VmConfig) -> Option<&str> { - cfg.runtime.as_ref().and_then(|r| r.var_size.as_deref()) - } - - #[test] - fn a_grown_disk_records_its_size() { - let mut cfg = VmConfig::default(); - assert!(record_var_size_floor(&mut cfg, 200 * GIB).unwrap()); - assert_eq!(var_size_of(&cfg), Some("200G")); - // Partial GiB rounds up — recording less than the live disk - // would shrink it on re-seed. - let mut cfg = VmConfig::default(); - assert!(record_var_size_floor(&mut cfg, 200 * GIB + 1).unwrap()); - assert_eq!(var_size_of(&cfg), Some("201G")); - } - - #[test] - fn a_default_sized_disk_records_nothing() { - // 50G file with no config: nothing to preserve, and the config - // file must not grow a runtime section as a side effect. - let mut cfg = VmConfig::default(); - assert!(!record_var_size_floor(&mut cfg, 50 * GIB).unwrap()); - assert!(cfg.runtime.is_none()); - } - - #[test] - fn an_explicit_config_already_covering_the_disk_is_untouched() { - let mut cfg = VmConfig::default(); - cfg.runtime.get_or_insert_with(Default::default).var_size = Some("300G".into()); - assert!(!record_var_size_floor(&mut cfg, 200 * GIB).unwrap()); - assert_eq!(var_size_of(&cfg), Some("300G")); - } - - #[test] - fn a_disk_grown_past_its_config_raises_the_config() { - // Growing (e.g. the desktop's disk-grow) never wrote config, so - // the file can legitimately exceed it; the file is the truth. - let mut cfg = VmConfig::default(); - cfg.runtime.get_or_insert_with(Default::default).var_size = Some("100G".into()); - assert!(record_var_size_floor(&mut cfg, 200 * GIB).unwrap()); - assert_eq!(var_size_of(&cfg), Some("200G")); + assert!(!should_sync_var_seed( + Some(&old), + &manifest("aa", "bb"), + true + )); } } diff --git a/src/main.rs b/src/main.rs index 60037251..6fceb834 100644 --- a/src/main.rs +++ b/src/main.rs @@ -3291,14 +3291,12 @@ async fn main() -> Result<()> { channel, check, yes, - reset_var, output, } => { commands::vm::update::UpdateCommand { channel, check_only: check, assume_yes: yes, - reset_var, output, } .execute() @@ -4813,8 +4811,8 @@ enum VmCommands { }, /// Check for and apply VM image updates from the release channel. /// Stops + restarts the VM if it was running. A release that ships - /// a new `var` image resets the VM's internal state (see - /// --reset-var); otherwise `var` is preserved. + /// a new `var` image schedules a state sync applied on the next + /// start; the VM's Docker volumes, SDKs and /data are preserved. Update { /// Channel name (default: `~/.avocado/config.yaml [vm].channel`, /// or `stable` if unset). @@ -4823,16 +4821,9 @@ enum VmCommands { /// Print availability + exit without downloading. #[arg(long)] check: bool, - /// Skip the interactive confirmation prompt. When the release - /// ships a new /var image, --reset-var is also required. + /// Skip the interactive confirmation prompt. #[arg(short = 'y', long)] yes: bool, - /// With --yes: authorize resetting the VM's internal state - /// (installed SDKs, Docker volumes, /data) when the release - /// ships a new /var image. Interactive runs confirm at the - /// prompt instead. - #[arg(long)] - reset_var: bool, /// Output format (human prose or single JSON object). #[arg(long, value_enum, default_value_t = crate::utils::output_format::OutputFormat::Human)] output: crate::utils::output_format::OutputFormat, diff --git a/src/utils/vm/config.rs b/src/utils/vm/config.rs index f5d4f88f..891949a1 100644 --- a/src/utils/vm/config.rs +++ b/src/utils/vm/config.rs @@ -48,12 +48,23 @@ pub struct RuntimeConfig { pub memory_mib: Option, /// Minimum var disk size, e.g. `"200G"`. `vm start` grows the live - /// disk to at least this (never shrinks), and a var reset re-seeds - /// at it. `vm update` records the live disk's size here before a - /// reset, so a grown disk keeps its capacity across the re-seed. + /// disk to at least this (never shrinks). #[serde(default, skip_serializing_if = "Option::is_none")] pub var_size: Option, + /// sha256 of a var seed the live disk has not yet synced its Avocado + /// state from. Set by `vm update` when the seed changes; `vm start` + /// attaches that seed read-only so the guest can lift the new + /// runtime out of it, and clears the key once the guest confirms. + /// + /// Left set on any failure, which is what makes the sync retry on + /// the next start rather than needing a repair path. It must also + /// never point at the seed the live disk was *copied* from: the copy + /// is byte-identical, so attaching it would put two devices with one + /// btrfs fsid in front of the kernel. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub pending_var_seed_sha: Option, + #[serde(flatten)] pub extra: BTreeMap, } @@ -334,6 +345,7 @@ mod tests { cpus: Some(6), memory_mib: Some(8192), var_size: Some("200G".into()), + pending_var_seed_sha: Some("abc123".into()), extra: Default::default(), }), ..Default::default() diff --git a/src/utils/vm/lifecycle.rs b/src/utils/vm/lifecycle.rs index 5086de0e..71eba60b 100644 --- a/src/utils/vm/lifecycle.rs +++ b/src/utils/vm/lifecycle.rs @@ -223,6 +223,13 @@ pub async fn start(opts: StartOptions) -> Result { // and Avocado.app's settings UI both see the same value. let (cpus, memory_mib) = resolve_and_persist_runtime(&paths, opts.cpus, opts.memory_mib)?; + // A seed the guest still has to sync its Avocado state out of, if + // `vm update` left one pending. Resolved before launch so the drive + // is present from the first instruction — the guest does the sync + // in early boot, before extensions merge. + let pending_seed = resolve_pending_var_seed(&paths, &manifest, &artifact_dir); + let pending_seed_sha = pending_seed.as_ref().map(|(sha, _)| sha.clone()); + let cfg = QemuConfig { memory_mib, cpus, @@ -230,6 +237,7 @@ pub async fn start(opts: StartOptions) -> Result { cmdline_extra: opts.cmdline_extra, artifact_dir: artifact_dir.clone(), workspace: workspace.clone(), + var_seed: pending_seed.map(|(sha256, path)| super::qemu::VarSeed { sha256, path }), }; // The CLI is authoritative for the qemu lifecycle on every platform. @@ -309,6 +317,15 @@ pub async fn start(opts: StartOptions) -> Result { ); } + // Clear the pending-sync marker only once the guest confirms it + // applied this exact seed. Every other outcome — sync failed, unit + // absent from an older rootfs, SSH unreachable — leaves the marker + // set, so the next `vm start` re-attaches and retries. That retry is + // the entire recovery story; there is no repair path to get wrong. + if let Some(sha) = pending_seed_sha.as_deref() { + clear_pending_var_seed_if_applied(&paths, &target, sha).await; + } + // Apply persisted network config (+ any one-shot --dns override). The // most common reason this matters: macOS host on a VPN that pushes DNS // via scoped resolvers, which QEMU's slirp DNS proxy (10.0.2.3) can't @@ -685,6 +702,79 @@ fn seed_var_disk(paths: &VmPaths, manifest: &Manifest, artifact_dir: &Path) -> R Ok(()) } +/// Path inside the guest where the sync unit records the seed it applied. +/// Deliberately outside `/var/lib/avocado`, so replacing that tree can't +/// take the record with it. +const GUEST_SEED_STAMP: &str = "/var/.avocado-seed-applied"; + +/// Resolve a var seed the guest still owes a state sync for, as +/// `(sha, path)`. +/// +/// Returns `None` — leaving the live disk untouched — unless all of: +/// +/// - `runtime.pending_var_seed_sha` is set (only `vm update` sets it, and +/// only when the seed actually changed), +/// - it still matches the installed manifest's seed sha. A mismatch means +/// the config outlived the install it referred to; attaching that seed +/// would sync state from a release the VM isn't running. +/// - the live var disk exists. A first boot copies the new seed wholesale +/// via `seed_var_disk`, so it is already current — and attaching the +/// source of that copy would present two devices with one btrfs fsid. +/// - the seed file is actually on disk. +fn resolve_pending_var_seed( + paths: &VmPaths, + manifest: &Manifest, + artifact_dir: &Path, +) -> Option<(String, PathBuf)> { + let pending = super::config::VmConfig::load(paths) + .ok()? + .runtime? + .pending_var_seed_sha?; + + if !paths.var_disk().exists() { + return None; + } + let art = manifest.artifact("var")?; + if !art.sha256.eq_ignore_ascii_case(&pending) { + return None; + } + let path = artifact_dir.join(&art.file); + path.exists().then_some((pending, path)) +} + +/// Drop `runtime.pending_var_seed_sha` once the guest reports it applied +/// this seed. Silent and best-effort in every failure direction: an +/// unreported sync just retries on the next start, which is cheap +/// (content-addressed images make a redundant sync close to a no-op) and +/// strictly safer than clearing a marker for work that didn't happen. +async fn clear_pending_var_seed_if_applied( + paths: &VmPaths, + target: &super::ssh::SshTarget, + expected_sha: &str, +) { + let Ok((applied, _)) = target + .exec(&format!("cat {GUEST_SEED_STAMP} 2>/dev/null || true")) + .await + else { + return; + }; + if !applied.trim().eq_ignore_ascii_case(expected_sha) { + crate::utils::output::print_warning( + "the VM did not apply the new Avocado state this boot; \ + it will retry on the next `avocado vm start`.", + crate::utils::output::OutputLevel::Normal, + ); + return; + } + let Ok(mut cfg) = super::config::VmConfig::load(paths) else { + return; + }; + if let Some(rt) = cfg.runtime.as_mut() { + rt.pending_var_seed_sha = None; + } + let _ = cfg.save(paths); +} + fn send_signal(pid: u32, sig: libc::c_int) { #[cfg(unix)] unsafe { @@ -1019,3 +1109,118 @@ async fn spawn_supervisor( tokio::time::sleep(Duration::from_millis(50)).await; } } + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + fn manifest_with_var(var_file: &str, var_sha: &str) -> Manifest { + serde_json::from_value(json!({ + "format": "avocado-direct", + "format_version": 1, + "platform": "avocado-qemuarm64", + "architecture": "arm64", + "artifacts": { + "kernel": { "file": "Image", "sha256": "00", "type": "kernel" }, + "var": { + "file": var_file, + "sha256": var_sha, + "type": "btrfs", + "update_policy": "seed_only" + } + }, + "cmdline_default": "console=ttyAMA0" + })) + .unwrap() + } + + /// Builds a VM state dir + artifact dir. `pending` seeds + /// `runtime.pending_var_seed_sha`; `live_disk` controls whether the + /// VM has ever started. + fn fixture( + pending: Option<&str>, + live_disk: bool, + seed_on_disk: bool, + ) -> (tempfile::TempDir, VmPaths, PathBuf) { + let tmp = tempfile::tempdir().unwrap(); + let paths = VmPaths::at(tmp.path().join("state")); + paths.ensure().unwrap(); + + if live_disk { + std::fs::write(paths.var_disk(), b"live").unwrap(); + } + if let Some(sha) = pending { + let mut cfg = super::super::config::VmConfig::default(); + cfg.runtime + .get_or_insert_with(Default::default) + .pending_var_seed_sha = Some(sha.to_string()); + cfg.save(&paths).unwrap(); + } + + let artifact_dir = tmp.path().join("artifacts"); + std::fs::create_dir_all(&artifact_dir).unwrap(); + if seed_on_disk { + std::fs::write(artifact_dir.join("var-new.btrfs"), b"seed").unwrap(); + } + (tmp, paths, artifact_dir) + } + + #[test] + fn pending_seed_resolves_when_everything_lines_up() { + let (_t, paths, art) = fixture(Some("newsha"), true, true); + let got = + resolve_pending_var_seed(&paths, &manifest_with_var("var-new.btrfs", "newsha"), &art); + assert_eq!(got, Some(("newsha".to_string(), art.join("var-new.btrfs")))); + } + + #[test] + fn no_marker_means_no_seed_drive() { + let (_t, paths, art) = fixture(None, true, true); + assert!(resolve_pending_var_seed( + &paths, + &manifest_with_var("var-new.btrfs", "newsha"), + &art + ) + .is_none()); + } + + /// The fsid-collision guard. Without a live disk, `seed_var_disk` + /// byte-copies this very seed into place; attaching the source + /// alongside the copy shows the kernel two devices with one btrfs + /// fsid, which it reads as a single multi-device filesystem. + #[test] + fn a_never_started_vm_never_attaches_the_seed_it_was_copied_from() { + let (_t, paths, art) = fixture(Some("newsha"), false, true); + assert!(resolve_pending_var_seed( + &paths, + &manifest_with_var("var-new.btrfs", "newsha"), + &art + ) + .is_none()); + } + + /// A marker left over from an install that has since been replaced + /// would sync state from a release the VM isn't running. + #[test] + fn a_marker_that_outlived_its_install_is_ignored() { + let (_t, paths, art) = fixture(Some("oldsha"), true, true); + assert!(resolve_pending_var_seed( + &paths, + &manifest_with_var("var-new.btrfs", "newsha"), + &art + ) + .is_none()); + } + + #[test] + fn a_missing_seed_file_is_not_attached() { + let (_t, paths, art) = fixture(Some("newsha"), true, false); + assert!(resolve_pending_var_seed( + &paths, + &manifest_with_var("var-new.btrfs", "newsha"), + &art + ) + .is_none()); + } +} diff --git a/src/utils/vm/qemu.rs b/src/utils/vm/qemu.rs index cd2bc847..251718a4 100644 --- a/src/utils/vm/qemu.rs +++ b/src/utils/vm/qemu.rs @@ -13,6 +13,11 @@ use super::fdt; use super::manifest::Manifest; use super::state::VmPaths; +/// virtio-blk serial for the read-only var seed. The guest resolves the +/// device as `/dev/disk/by-id/virtio-`; changing it breaks the +/// guest-side sync unit, which hardcodes the same string. +pub const VAR_SEED_SERIAL: &str = "avocado-varseed"; + /// Knobs the CLI passes to QEMU at launch. #[derive(Debug, Clone)] pub struct QemuConfig { @@ -28,6 +33,28 @@ pub struct QemuConfig { pub artifact_dir: PathBuf, /// Host path exposed to the guest as a 9p `workspace` share. pub workspace: PathBuf, + /// A var seed to attach read-only so the guest can sync its Avocado + /// state out of it. `None` on every boot that isn't the first one + /// after a seed change. + /// + /// Must never be the seed the live var disk was copied from — that + /// copy is byte-identical, so both would present the same btrfs + /// fsid and the kernel would treat them as one multi-device + /// filesystem. `vm update` only sets this when the sha differs, + /// which is exactly the case where the seeds were built separately. + pub var_seed: Option, +} + +/// A pending seed and the sha identifying it. +/// +/// The sha rides the kernel cmdline because the guest has no other way +/// to learn it — it names the seed on the *host*, and the guest must +/// echo it back for `vm start` to know this exact sync landed rather +/// than some earlier one. +#[derive(Debug, Clone)] +pub struct VarSeed { + pub sha256: String, + pub path: PathBuf, } /// Resolve the right qemu-system binary for the manifest's architecture. @@ -120,6 +147,14 @@ pub fn build_qemu_args( cmdline.push_str(" systemd.tpm2_wait=false"); } + // Name the pending seed to the guest. The sync unit echoes this back + // into a stamp file, which is how `vm start` distinguishes "this + // sync landed" from "a stamp is lying around from an earlier one" + // before it clears the pending marker. + if let Some(seed) = &cfg.var_seed { + cmdline.push_str(&format!(" avocado.seed_sha={}", seed.sha256)); + } + let mut args: Vec = Vec::new(); args.push("-machine".into()); @@ -170,6 +205,25 @@ pub fn build_qemu_args( args.push(format!("file={},if=virtio,format=qcow2", data.display())); } + // Var seed for a pending state sync, read-only. Attached last and + // addressed by serial rather than `/dev/vdN`: both drives above are + // conditional, so enumeration order isn't stable across boots or + // arches, and the guest resolves this one through + // /dev/disk/by-id/virtio-… instead. `if=none` plus an explicit + // -device is required because modern QEMU dropped `serial=` from + // -drive. + if let Some(seed) = &cfg.var_seed { + args.push("-drive".into()); + args.push(format!( + "file={},if=none,id=varseed,format=raw,readonly=on", + seed.path.display() + )); + args.push("-device".into()); + args.push(format!( + "virtio-blk-pci,drive=varseed,serial={VAR_SEED_SERIAL}" + )); + } + // Usermode networking; one port-forward to guest sshd args.push("-netdev".into()); args.push(format!( @@ -502,6 +556,7 @@ mod tests { cmdline_extra: Some("init=/sbin/init".into()), artifact_dir: tmp.path().to_path_buf(), workspace: tmp.path().to_path_buf(), + var_seed: None, }; let args = build_qemu_args(&m, &paths, &cfg).unwrap(); let rendered = args.join(" "); @@ -518,6 +573,71 @@ mod tests { assert!(rendered.contains("mount_tag=workspace")); } + fn cfg_for(tmp: &Path, var_seed: Option) -> QemuConfig { + QemuConfig { + memory_mib: 2048, + cpus: 2, + ssh_port: 51234, + cmdline_extra: None, + artifact_dir: tmp.to_path_buf(), + workspace: tmp.to_path_buf(), + var_seed, + } + } + + /// The overwhelmingly common boot. A seed drive here would be an + /// fsid collision with the live var disk it was copied from. + #[test] + fn no_seed_drive_without_a_pending_sync() { + let tmp = tempfile::tempdir().unwrap(); + let args = build_qemu_args( + &fake_manifest("arm64"), + &VmPaths::at(tmp.path()), + &cfg_for(tmp.path(), None), + ) + .unwrap(); + let rendered = args.join(" "); + assert!(!rendered.contains("varseed"), "unexpected seed: {rendered}"); + assert!(!rendered.contains(VAR_SEED_SERIAL)); + assert!(!rendered.contains("avocado.seed_sha")); + } + + #[test] + fn seed_drive_is_read_only_and_addressed_by_serial() { + let tmp = tempfile::tempdir().unwrap(); + let seed = tmp.path().join("var-new.btrfs"); + std::fs::write(&seed, b"seed").unwrap(); + + let args = build_qemu_args( + &fake_manifest("arm64"), + &VmPaths::at(tmp.path()), + &cfg_for( + tmp.path(), + Some(VarSeed { + sha256: "deadbeef".into(), + path: seed.clone(), + }), + ), + ) + .unwrap(); + let rendered = args.join(" "); + + // The guest can only learn which seed it is applying from here. + assert!(rendered.contains("avocado.seed_sha=deadbeef")); + + assert!(rendered.contains(&format!( + "file={},if=none,id=varseed,format=raw,readonly=on", + seed.display() + ))); + assert!(rendered.contains(&format!( + "virtio-blk-pci,drive=varseed,serial={VAR_SEED_SERIAL}" + ))); + // The guest finds it by serial, so it must not be wedged in + // ahead of the drives whose /dev/vdN the guest fstab pins. + let seed_at = rendered.find("id=varseed").unwrap(); + assert!(rendered.find("format=qcow2").is_none_or(|d| d < seed_at)); + } + #[test] fn pick_free_port_returns_high_port() { let p = pick_free_port().unwrap(); From b56f943ceed39ff486f9ab7d8c0123dc4b8ffc62 Mon Sep 17 00:00:00 2001 From: Justin Schneck Date: Thu, 13 Aug 2026 15:02:32 -0400 Subject: [PATCH 2/3] fix(ext): read [Install] from drop-ins, not just the unit body MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- src/commands/ext/build.rs | 106 +++++++++++++++++++++++++++++++++++--- 1 file changed, 100 insertions(+), 6 deletions(-) diff --git a/src/commands/ext/build.rs b/src/commands/ext/build.rs index d13e3ef4..3f4cfbdf 100644 --- a/src/commands/ext/build.rs +++ b/src/commands/ext/build.rs @@ -1291,8 +1291,32 @@ done if [ -n "$unit_file" ]; then echo "Enabling service: $service (found at $unit_target) in sysroot $sysroot" - # Parse WantedBy= from [Install] section and create .wants symlinks - wanted_by=$(sed -n '/^\[Install\]/,/^\[/{{/^WantedBy=/s/^WantedBy=//p}}' "$unit_file" | tr ',' ' ') + # [Install] can come from a drop-in as well as the unit body, and + # `systemctl enable` honours a WantedBy= declared there. Reading only + # the unit made the standard way to enable a vendor unit that ships + # without an [Install] — a drop-in supplying one — invisible here, so + # the build failed with the fix already sitting in the overlay. + install_sources="$unit_file" + for dropin_dir in /etc/systemd/system /usr/lib/systemd/system; do + for dropin in "$sysroot$dropin_dir/$service.d/"*.conf; do + [ -f "$dropin" ] && install_sources="$install_sources $dropin" + done + done + + # One sed per file: a single invocation over several files shares one + # input stream, so the /^\[Install\]/,/^\[/ range would run past the + # end of one file and match keys in the next. + wanted_by="" + required_by="" + for install_src in $install_sources; do + wanted_by="$wanted_by $(sed -n '/^\[Install\]/,/^\[/{{/^WantedBy=/s/^WantedBy=//p}}' "$install_src" | tr ',' ' ')" + required_by="$required_by $(sed -n '/^\[Install\]/,/^\[/{{/^RequiredBy=/s/^RequiredBy=//p}}' "$install_src" | tr ',' ' ')" + done + # Unquoted: collapses the separators left by accumulation so the + # emptiness checks below see "" rather than a run of spaces. + wanted_by=$(echo $wanted_by) + required_by=$(echo $required_by) + for target in $wanted_by; do target_dir="$sysroot/etc/systemd/system/$target.wants" mkdir -p "$target_dir" @@ -1300,8 +1324,6 @@ if [ -n "$unit_file" ]; then echo "Created symlink: $target_dir/$service -> $unit_target" done - # Parse RequiredBy= from [Install] section and create .requires symlinks - required_by=$(sed -n '/^\[Install\]/,/^\[/{{/^RequiredBy=/s/^RequiredBy=//p}}' "$unit_file" | tr ',' ' ') for target in $required_by; do target_dir="$sysroot/etc/systemd/system/$target.requires" mkdir -p "$target_dir" @@ -1917,6 +1939,78 @@ mod tests { assert!(script.contains("AVOCADO_EXT_SYSROOTS/multi-scope-ext/etc/extension-release.d")); } + // A drop-in supplying [Install] must actually enable the unit. + // String assertions can't catch a shell bug in generated code, so + // this runs the emitted fragment against a real sysroot. + #[test] + fn install_section_from_a_dropin_enables_the_unit() { + let tmp = tempfile::tempdir().unwrap(); + let sysroot = tmp.path().join("sysroot"); + let unit_dir = sysroot.join("usr/lib/systemd/system"); + let dropin_dir = unit_dir.join("qga.service.d"); + std::fs::create_dir_all(&dropin_dir).unwrap(); + + // Vendor unit with no [Install] — the case that used to fail. + std::fs::write( + unit_dir.join("qga.service"), + "[Unit]\nDescription=q\n[Service]\nExecStart=/bin/true\n", + ) + .unwrap(); + std::fs::write( + dropin_dir.join("install.conf"), + "[Install]\nWantedBy=multi-user.target\n", + ) + .unwrap(); + + let fragment = fragment_for("qga.service"); + let out = std::process::Command::new("sh") + .arg("-c") + .arg(&fragment) + .env("AVOCADO_EXT_SYSROOTS", tmp.path()) + .output() + .expect("run fragment"); + + assert!( + out.status.success(), + "fragment failed: {}", + String::from_utf8_lossy(&out.stdout) + ); + assert!( + sysroot + .join("etc/systemd/system/multi-user.target.wants/qga.service") + .is_symlink(), + "no .wants symlink created: {}", + String::from_utf8_lossy(&out.stdout) + ); + } + + /// Slice the generated enable-services shell for one unit out of the + /// full build script. The extension name doubles as the sysroot dir. + fn fragment_for(service: &str) -> String { + let cmd = ExtBuildCommand { + extension: "sysroot".to_string(), + config_path: "avocado.yaml".to_string(), + ..Default::default() + }; + let script = cmd.create_confext_build_script( + "1.0", + &["system".to_string()], + None, + &[service.to_string()], + &[], + &[], + None, + None, + false, + "/opt/src", + ); + let start = script + .find("# Enable service file for") + .expect("no enable block"); + let end = script[start..].find("\nfi").expect("no block end") + start + 3; + script[start..end].to_string() + } + #[test] fn test_create_confext_build_script_with_services() { let cmd = ExtBuildCommand { @@ -1953,12 +2047,12 @@ mod tests { assert!(script.contains("for unit_dir in /etc/systemd/system /usr/lib/systemd/system; do")); assert!(script.contains("if [ -f \"$sysroot$unit_dir/$service\" ]; then")); // Check for WantedBy= parsing - assert!(script.contains("wanted_by=$(sed -n")); assert!(script.contains("/^WantedBy=/")); assert!(script.contains("$sysroot/etc/systemd/system/$target.wants")); // Check for RequiredBy= parsing - assert!(script.contains("required_by=$(sed -n")); assert!(script.contains("/^RequiredBy=/")); + // [Install] is collected from drop-ins too, matching systemd. + assert!(script.contains("$service.d/\"*.conf")); assert!(script.contains("$sysroot/etc/systemd/system/$target.requires")); // The symlink points at wherever the unit was actually found, so a unit // under /etc is not linked to a /usr/lib path that holds nothing. From 5675107dc8b13a2e6ca7b806eaf2f29c37c4872e Mon Sep 17 00:00:00 2001 From: Justin Schneck Date: Thu, 13 Aug 2026 15:03:07 -0400 Subject: [PATCH 3/3] fix(vm): attach the seed as a plain virtio drive, found by filesystem UUID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/utils/vm/lifecycle.rs | 31 ++++++++++++++- src/utils/vm/qemu.rs | 84 +++++++++++++++++++++++---------------- 2 files changed, 80 insertions(+), 35 deletions(-) diff --git a/src/utils/vm/lifecycle.rs b/src/utils/vm/lifecycle.rs index 71eba60b..a77e3c48 100644 --- a/src/utils/vm/lifecycle.rs +++ b/src/utils/vm/lifecycle.rs @@ -237,7 +237,11 @@ pub async fn start(opts: StartOptions) -> Result { cmdline_extra: opts.cmdline_extra, artifact_dir: artifact_dir.clone(), workspace: workspace.clone(), - var_seed: pending_seed.map(|(sha256, path)| super::qemu::VarSeed { sha256, path }), + var_seed: pending_seed.and_then(|(sha256, path)| { + // No fsid, no way for the guest to find the disk — so skip + // attaching rather than hand it one it cannot identify. + btrfs_fsid(&path).map(|fsid| super::qemu::VarSeed { sha256, fsid, path }) + }), }; // The CLI is authoritative for the qemu lifecycle on every platform. @@ -702,6 +706,31 @@ fn seed_var_disk(paths: &VmPaths, manifest: &Manifest, artifact_dir: &Path) -> R Ok(()) } +/// Read a btrfs filesystem UUID straight out of an image. +/// +/// The superblock sits at a fixed 64 KiB offset with the fsid 32 bytes +/// into it, so this is a 16-byte read. Done by hand rather than shelling +/// out to `blkid` because the hosts that run this VM are macOS and +/// Windows, where there is no blkid and no btrfs at all. +fn btrfs_fsid(path: &Path) -> Option { + use std::io::{Read, Seek, SeekFrom}; + const SUPERBLOCK_FSID_OFFSET: u64 = 0x1_0020; + + let mut f = std::fs::File::open(path).ok()?; + f.seek(SeekFrom::Start(SUPERBLOCK_FSID_OFFSET)).ok()?; + let mut b = [0u8; 16]; + f.read_exact(&mut b).ok()?; + let hex = |r: &[u8]| r.iter().map(|x| format!("{x:02x}")).collect::(); + Some(format!( + "{}-{}-{}-{}-{}", + hex(&b[0..4]), + hex(&b[4..6]), + hex(&b[6..8]), + hex(&b[8..10]), + hex(&b[10..16]) + )) +} + /// Path inside the guest where the sync unit records the seed it applied. /// Deliberately outside `/var/lib/avocado`, so replacing that tree can't /// take the record with it. diff --git a/src/utils/vm/qemu.rs b/src/utils/vm/qemu.rs index 251718a4..f6f8e0f5 100644 --- a/src/utils/vm/qemu.rs +++ b/src/utils/vm/qemu.rs @@ -13,11 +13,6 @@ use super::fdt; use super::manifest::Manifest; use super::state::VmPaths; -/// virtio-blk serial for the read-only var seed. The guest resolves the -/// device as `/dev/disk/by-id/virtio-`; changing it breaks the -/// guest-side sync unit, which hardcodes the same string. -pub const VAR_SEED_SERIAL: &str = "avocado-varseed"; - /// Knobs the CLI passes to QEMU at launch. #[derive(Debug, Clone)] pub struct QemuConfig { @@ -45,15 +40,24 @@ pub struct QemuConfig { pub var_seed: Option, } -/// A pending seed and the sha identifying it. +/// A pending seed, plus the two identifiers the guest needs for it. +/// +/// `sha256` rides the kernel cmdline because the guest has no other way +/// to learn it — it names the seed on the *host*, and the guest echoes +/// it back so `vm start` knows this exact sync landed rather than an +/// earlier one. /// -/// The sha rides the kernel cmdline because the guest has no other way -/// to learn it — it names the seed on the *host*, and the guest must -/// echo it back for `vm start` to know this exact sync landed rather -/// than some earlier one. +/// `fsid` is the seed's btrfs filesystem UUID, so the guest can find the +/// disk by `/dev/disk/by-uuid/` instead of guessing a `/dev/vdN`. It is +/// deliberately *not* a virtio serial: giving the seed an explicit +/// `-device` moved it ahead of the `if=virtio` drives in PCI order, so +/// the seed became `/dev/vda` and the rootfs and var disks shifted out +/// from under `root=/dev/vda` and the guest's fstab. Keeping every drive +/// on `if=virtio` makes attaching one purely additive. #[derive(Debug, Clone)] pub struct VarSeed { pub sha256: String, + pub fsid: String, pub path: PathBuf, } @@ -152,7 +156,10 @@ pub fn build_qemu_args( // sync landed" from "a stamp is lying around from an earlier one" // before it clears the pending marker. if let Some(seed) = &cfg.var_seed { - cmdline.push_str(&format!(" avocado.seed_sha={}", seed.sha256)); + cmdline.push_str(&format!( + " avocado.seed_sha={} avocado.seed_fsid={}", + seed.sha256, seed.fsid + )); } let mut args: Vec = Vec::new(); @@ -205,23 +212,22 @@ pub fn build_qemu_args( args.push(format!("file={},if=virtio,format=qcow2", data.display())); } - // Var seed for a pending state sync, read-only. Attached last and - // addressed by serial rather than `/dev/vdN`: both drives above are - // conditional, so enumeration order isn't stable across boots or - // arches, and the guest resolves this one through - // /dev/disk/by-id/virtio-… instead. `if=none` plus an explicit - // -device is required because modern QEMU dropped `serial=` from - // -drive. + // Var seed for a pending state sync, read-only, appended last. + // + // Must stay `if=virtio` like the drives above. 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, which drops the VM into emergency mode. Appending an + // `if=virtio` drive is purely additive; the guest finds this one by + // filesystem UUID rather than by position. if let Some(seed) = &cfg.var_seed { args.push("-drive".into()); args.push(format!( - "file={},if=none,id=varseed,format=raw,readonly=on", + "file={},if=virtio,format=raw,readonly=on", seed.path.display() )); - args.push("-device".into()); - args.push(format!( - "virtio-blk-pci,drive=varseed,serial={VAR_SEED_SERIAL}" - )); } // Usermode networking; one port-forward to guest sshd @@ -597,9 +603,13 @@ mod tests { ) .unwrap(); let rendered = args.join(" "); - assert!(!rendered.contains("varseed"), "unexpected seed: {rendered}"); - assert!(!rendered.contains(VAR_SEED_SERIAL)); assert!(!rendered.contains("avocado.seed_sha")); + assert!(!rendered.contains("avocado.seed_fsid")); + // Only the rootfs in this fixture (var/data are conditional on + // the files existing). The pair with the seed test below is the + // point: attaching a seed adds exactly one drive and never + // renumbers the ones the cmdline and guest fstab pin. + assert_eq!(rendered.matches("if=virtio,format=raw").count(), 1); } #[test] @@ -615,6 +625,7 @@ mod tests { tmp.path(), Some(VarSeed { sha256: "deadbeef".into(), + fsid: "fed386a1-f288-4748-8c91-f614242e0410".into(), path: seed.clone(), }), ), @@ -622,20 +633,25 @@ mod tests { .unwrap(); let rendered = args.join(" "); - // The guest can only learn which seed it is applying from here. + // The guest can only learn these from here. assert!(rendered.contains("avocado.seed_sha=deadbeef")); + assert!(rendered.contains("avocado.seed_fsid=fed386a1-f288-4748-8c91-f614242e0410")); assert!(rendered.contains(&format!( - "file={},if=none,id=varseed,format=raw,readonly=on", + "file={},if=virtio,format=raw,readonly=on", seed.display() ))); - assert!(rendered.contains(&format!( - "virtio-blk-pci,drive=varseed,serial={VAR_SEED_SERIAL}" - ))); - // The guest finds it by serial, so it must not be wedged in - // ahead of the drives whose /dev/vdN the guest fstab pins. - let seed_at = rendered.find("id=varseed").unwrap(); - assert!(rendered.find("format=qcow2").is_none_or(|d| d < seed_at)); + // No explicit -device: one takes a lower PCI slot than the + // if=virtio drives, which made the seed /dev/vda and shifted the + // rootfs out from under `root=/dev/vda`. + assert!(!rendered.contains("virtio-blk-pci,drive=")); + // The seed must come after the rootfs, so attaching it cannot + // renumber the disks the cmdline and fstab pin by position. + let rootfs_at = rendered.find("rootfs").expect("rootfs drive"); + let seed_at = rendered.find(&seed.display().to_string()).unwrap(); + assert!(rootfs_at < seed_at, "seed must not precede the rootfs"); + // One more drive than the no-seed case, and no more. + assert_eq!(rendered.matches("if=virtio,format=raw").count(), 2); } #[test]