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. 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..a77e3c48 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,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.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. @@ -309,6 +321,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 +706,104 @@ 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. +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 +1138,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..f6f8e0f5 100644 --- a/src/utils/vm/qemu.rs +++ b/src/utils/vm/qemu.rs @@ -28,6 +28,37 @@ 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, 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. +/// +/// `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, } /// Resolve the right qemu-system binary for the manifest's architecture. @@ -120,6 +151,17 @@ 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={} avocado.seed_fsid={}", + seed.sha256, seed.fsid + )); + } + let mut args: Vec = Vec::new(); args.push("-machine".into()); @@ -170,6 +212,24 @@ pub fn build_qemu_args( args.push(format!("file={},if=virtio,format=qcow2", data.display())); } + // 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=virtio,format=raw,readonly=on", + seed.path.display() + )); + } + // Usermode networking; one port-forward to guest sshd args.push("-netdev".into()); args.push(format!( @@ -502,6 +562,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 +579,81 @@ 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("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] + 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(), + fsid: "fed386a1-f288-4748-8c91-f614242e0410".into(), + path: seed.clone(), + }), + ), + ) + .unwrap(); + let rendered = args.join(" "); + + // 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=virtio,format=raw,readonly=on", + seed.display() + ))); + // 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] fn pick_free_port_returns_high_port() { let p = pick_free_port().unwrap();