From 97d9ceff160fc7b7b5763193a6e581c1f24dd835 Mon Sep 17 00:00:00 2001 From: Wayland Yang Date: Sat, 13 Jun 2026 10:09:00 +0800 Subject: [PATCH] fix(quickstart): make the one-command path actually reach first fork MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit E2E'd `forkd quickstart` end-to-end in an isolated privileged container (host networking untouched). It crashed at six distinct points before a clean run — every one a wall a new user hits in their first minute: 1. **install-guest-kernel.sh hardcodes `sudo`** — absent in minimal environments (containers, slim images) where quickstart already runs as root. Added a `$(id -u)==0 ? "" : "sudo"` shim. 2. **`file`/`strings` assumed present** in the same script's post-install descriptor lines — guarded with `command -v`. 3. **`$USER` unbound under `set -u`** in host-tap.sh + netns-setup.sh (unset in `docker exec` / non-login shells). Fall back through `id -un`. 4. **unpack baked the packing host's absolute paths** into snapshot.json, so the first restore on any other machine failed with FC's "Failed to open snapshot file". `hub::unpack` now rewrites `vmstate`/`memory` to the extraction dir; main.rs re-runs the rewrite AFTER the staging→final `rename(2)` (v1 and v2 chain paths) since the in-unpack pass points at the soon-stale staging dir. Operates on `serde_json::Value` so unknown fields and volume paths survive. Two unit tests. 5. **build-rootfs.sh hardcodes `sudo`** at 18 sites — same shim. 6. **tarball installs can't find `scripts/` or `rootfs-init/`** — the `from-image` bake shells out to build-rootfs.sh, which finds the guest init+agent via `$(dirname $0)/../rootfs-init`. quickstart now stages the embedded scripts as `/scripts/*` with the init files in the sibling `/rootfs-init/` and points FORKD_SCRIPTS_DIR there. Without this the guest booted with no init → `Kernel panic … init /forkd-init.sh failed (error -2)` → FC exits → pause fails. Also: doctor::preflight() softens the three non-gating rows (kernel/tap/ docker) from ✗ to ⚠ — a red ✗ immediately followed by "quickstart sets this up" read as a contradiction. Clean run now: preflight 7✓ → consent heal → docker bake (5.3 s snapshot) → fork 4 children in 59 ms wall-clock, 4/4 alive. Transcript saved. Note: the hub-pull fallback path surfaced a separate, deeper portability bug (snapshots aren't relocatable across hosts — rootfs absolute path in the vmstate + rootfs not shipped in packs); filed as #242. quickstart's preferred local-bake path is unaffected and prints a warning before the hub fallback. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/forkd-cli/src/doctor.rs | 21 +++++- crates/forkd-cli/src/hub.rs | 120 ++++++++++++++++++++++++++++++++ crates/forkd-cli/src/main.rs | 50 +++++++++++++ scripts/build-rootfs.sh | 45 +++++++----- scripts/host-tap.sh | 4 +- scripts/install-guest-kernel.sh | 23 ++++-- scripts/netns-setup.sh | 4 +- 7 files changed, 239 insertions(+), 28 deletions(-) diff --git a/crates/forkd-cli/src/doctor.rs b/crates/forkd-cli/src/doctor.rs index be2f819..ca4cf3e 100644 --- a/crates/forkd-cli/src/doctor.rs +++ b/crates/forkd-cli/src/doctor.rs @@ -141,10 +141,25 @@ pub(crate) fn preflight() -> anyhow::Result { ]; let blocked = gates.iter().any(|c| c.status == Status::Fail); + // Non-gating rows: a red ✗ followed by quickstart proceeding anyway + // reads as a contradiction. Downgrade them to warnings with a hint + // that quickstart itself is the fix. + let soften = |c: Check| -> Check { + if c.status == Status::Fail { + Check::warn( + c.name, + c.detail, + "quickstart sets this up (with your consent)", + ) + } else { + c + } + }; + let mut all = gates; - all.push(kernel); - all.push(tap); - all.push(docker); + all.push(soften(kernel)); + all.push(soften(tap)); + all.push(soften(docker)); print_report(&all); if blocked { diff --git a/crates/forkd-cli/src/hub.rs b/crates/forkd-cli/src/hub.rs index 2f3a0f7..8eaf3e2 100644 --- a/crates/forkd-cli/src/hub.rs +++ b/crates/forkd-cli/src/hub.rs @@ -533,6 +533,7 @@ pub fn unpack(pack_path: &Path, dest_dir: &Path) -> Result { for entry in &manifest.files { verify_one(&dest_dir.join(&entry.path), entry)?; } + rewrite_snapshot_paths(dest_dir)?; } else { // v2 chain layout. for link in &manifest.chain { @@ -540,12 +541,63 @@ pub fn unpack(pack_path: &Path, dest_dir: &Path) -> Result { for entry in &link.files { verify_one(&link_dir.join(&entry.path), entry)?; } + rewrite_snapshot_paths(&link_dir)?; } } Ok(manifest) } +/// Rewrite `snapshot.json`'s absolute `vmstate` / `memory` paths to point +/// into the directory the snapshot was just extracted into. +/// +/// Packs carry the *packing host's* absolute paths (e.g. +/// `/home//.local/share/forkd/snapshots//vmstate`), which are +/// meaningless on the pulling machine — without this fixup the first +/// restore dies with Firecracker's "Failed to open snapshot file: No such +/// file or directory". Runs after sha256 verification so the integrity +/// check still sees the bytes the packer signed. +/// +/// Operates on `serde_json::Value` rather than the typed +/// `forkd_vmm::Snapshot` so fields this forkd version doesn't know about +/// survive the round-trip. Volume host paths are left untouched — they +/// reference paths outside the snapshot dir that we can't relocate. +/// +/// Idempotent, and `pub(crate)` because callers that extract into a +/// staging dir and `rename(2)` to the final location (`unpack_into`) +/// must run it AGAIN post-move — the in-`unpack` pass points paths at +/// the staging dir, which goes stale the moment the rename happens. +pub(crate) fn rewrite_snapshot_paths(dir: &Path) -> Result<()> { + let sj = dir.join("snapshot.json"); + if !sj.exists() { + return Ok(()); + } + let raw = std::fs::read_to_string(&sj) + .with_context(|| format!("read {} for path fixup", sj.display()))?; + let mut v: serde_json::Value = serde_json::from_str(&raw) + .with_context(|| format!("parse {} for path fixup", sj.display()))?; + if let Some(obj) = v.as_object_mut() { + for key in ["vmstate", "memory"] { + let Some(s) = obj.get(key).and_then(|x| x.as_str()) else { + continue; + }; + let Some(name) = Path::new(s).file_name() else { + continue; + }; + let local = dir.join(name); + if local.exists() && local.to_str() != Some(s) { + obj.insert( + key.to_string(), + serde_json::Value::String(local.to_string_lossy().into_owned()), + ); + } + } + } + let out = serde_json::to_string_pretty(&v).context("re-serialize snapshot.json")?; + std::fs::write(&sj, out).with_context(|| format!("write fixed-up {}", sj.display()))?; + Ok(()) +} + /// Verify a single extracted file against its [`FileEntry`] manifest /// declaration. Shared between v1 and v2 unpack paths. fn verify_one(path: &Path, entry: &FileEntry) -> Result<()> { @@ -896,6 +948,74 @@ fn epoch_to_ymd_hms(secs: u64) -> (u32, u32, u32, u32, u32, u32) { mod tests { use super::*; + /// Regression: packs bake the packing host's absolute paths into + /// snapshot.json. `unpack` must rewrite `vmstate` / `memory` to the + /// extraction dir or the first restore on any other machine fails + /// with "Failed to open snapshot file". Unknown fields and volume + /// paths must survive untouched. + #[test] + fn rewrite_snapshot_paths_relocates_vmstate_and_memory() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path(); + std::fs::write(dir.join("vmstate"), b"x").unwrap(); + std::fs::write(dir.join("memory.bin"), b"x").unwrap(); + std::fs::write( + dir.join("snapshot.json"), + r#"{ + "vmstate": "/home/packer/.local/share/forkd/snapshots/t/vmstate", + "memory": "/home/packer/.local/share/forkd/snapshots/t/memory.bin", + "volumes": [{"host_path": "/data/vol.ext4"}], + "some_future_field": 42 +}"#, + ) + .unwrap(); + + rewrite_snapshot_paths(dir).unwrap(); + + let v: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(dir.join("snapshot.json")).unwrap()) + .unwrap(); + assert_eq!( + v["vmstate"].as_str().unwrap(), + dir.join("vmstate").to_str().unwrap() + ); + assert_eq!( + v["memory"].as_str().unwrap(), + dir.join("memory.bin").to_str().unwrap() + ); + // Untouched: volumes + unknown fields. + assert_eq!( + v["volumes"][0]["host_path"].as_str().unwrap(), + "/data/vol.ext4" + ); + assert_eq!(v["some_future_field"].as_i64().unwrap(), 42); + } + + /// When the referenced file isn't in the extraction dir (or there is + /// no snapshot.json at all), rewrite must be a no-op rather than + /// pointing paths at nonexistent files. + #[test] + fn rewrite_snapshot_paths_noop_when_files_absent() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path(); + // No snapshot.json: must not error. + rewrite_snapshot_paths(dir).unwrap(); + + // snapshot.json present but vmstate/memory files absent: keep + // the original paths. + std::fs::write( + dir.join("snapshot.json"), + r#"{"vmstate": "/elsewhere/vmstate", "memory": "/elsewhere/memory.bin"}"#, + ) + .unwrap(); + rewrite_snapshot_paths(dir).unwrap(); + let v: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(dir.join("snapshot.json")).unwrap()) + .unwrap(); + assert_eq!(v["vmstate"].as_str().unwrap(), "/elsewhere/vmstate"); + assert_eq!(v["memory"].as_str().unwrap(), "/elsewhere/memory.bin"); + } + #[test] #[allow(clippy::assertions_on_constants)] fn pack_format_versions_are_in_lockstep() { diff --git a/crates/forkd-cli/src/main.rs b/crates/forkd-cli/src/main.rs index 7ab2931..472a870 100644 --- a/crates/forkd-cli/src/main.rs +++ b/crates/forkd-cli/src/main.rs @@ -1226,6 +1226,9 @@ fn unpack_into( std::fs::create_dir_all(dest.parent().unwrap()).ok(); std::fs::rename(tmp, &dest) .with_context(|| format!("move {} → {}", tmp.display(), dest.display()))?; + // The staging-dir paths hub::unpack wrote into snapshot.json went + // stale with the rename — point them at the final location. + hub::rewrite_snapshot_paths(&dest)?; eprintln!("✓ unpacked tag '{final_tag}' at {}", dest.display()); eprintln!(" next: forkd fork --tag {final_tag} -n "); Ok(()) @@ -1317,6 +1320,9 @@ fn unpack_chain_into( let src = tmp.join(&link.tag); std::fs::rename(&src, dest) .with_context(|| format!("move {} → {} (chain link)", src.display(), dest.display()))?; + // Re-point snapshot.json at the post-rename location (the + // in-unpack pass wrote staging-dir paths). + hub::rewrite_snapshot_paths(dest)?; eprintln!( " ✓ link '{}' → '{final_tag}' at {}{}", link.tag, @@ -1701,6 +1707,49 @@ fn parent_build_cmd( const QS_INSTALL_KERNEL_SH: &str = include_str!("../../../scripts/install-guest-kernel.sh"); const QS_HOST_TAP_SH: &str = include_str!("../../../scripts/host-tap.sh"); const QS_NETNS_SETUP_SH: &str = include_str!("../../../scripts/netns-setup.sh"); +const QS_BUILD_ROOTFS_SH: &str = include_str!("../../../scripts/build-rootfs.sh"); +const QS_FORKD_INIT_SH: &str = include_str!("../../../rootfs-init/forkd-init.sh"); +const QS_FORKD_AGENT_PY: &str = include_str!("../../../rootfs-init/forkd-agent.py"); + +/// Make `find_script` succeed on tarball installs that have no repo +/// checkout: stage the embedded helper scripts on disk and point +/// `FORKD_SCRIPTS_DIR` at them — unless the variable is already set or +/// the script resolves on its own (repo checkouts and packaged layouts +/// keep winning). +/// +/// Layout matters: `build-rootfs.sh` finds the guest init + agent via +/// `$(dirname "$0")/../rootfs-init`, so the scripts go in `/scripts/` +/// and the init files in the sibling `/rootfs-init/`. Get this +/// wrong and the guest boots with no init → kernel panic (error -2) at +/// warmup, long before the first fork. +fn ensure_scripts_dir_for_quickstart() -> Result<()> { + if find_script("build-rootfs.sh").is_ok() { + return Ok(()); + } + let base = std::env::temp_dir().join(format!("forkd-quickstart-{}", std::process::id())); + let scripts = base.join("scripts"); + let rootfs_init = base.join("rootfs-init"); + std::fs::create_dir_all(&scripts).context("create embedded scripts/ dir")?; + std::fs::create_dir_all(&rootfs_init).context("create embedded rootfs-init/ dir")?; + for (name, body) in [ + ("build-rootfs.sh", QS_BUILD_ROOTFS_SH), + ("install-guest-kernel.sh", QS_INSTALL_KERNEL_SH), + ("host-tap.sh", QS_HOST_TAP_SH), + ("netns-setup.sh", QS_NETNS_SETUP_SH), + ] { + std::fs::write(scripts.join(name), body) + .with_context(|| format!("write embedded {name}"))?; + } + for (name, body) in [ + ("forkd-init.sh", QS_FORKD_INIT_SH), + ("forkd-agent.py", QS_FORKD_AGENT_PY), + ] { + std::fs::write(rootfs_init.join(name), body) + .with_context(|| format!("write embedded {name}"))?; + } + std::env::set_var("FORKD_SCRIPTS_DIR", &scripts); + Ok(()) +} fn quickstart_cmd(n: usize, image: String, yes: bool) -> Result<()> { eprintln!("==> forkd quickstart — host preflight"); @@ -1764,6 +1813,7 @@ fn quickstart_cmd(n: usize, image: String, yes: bool) -> Result<()> { eprintln!("==> reusing existing snapshot '{tag}'"); } else if pf.docker_ok { eprintln!("==> baking snapshot '{tag}' from {image} (first run 1-3 min; rootfs is cached)"); + ensure_scripts_dir_for_quickstart()?; from_image_cmd( image, tag.clone(), diff --git a/scripts/build-rootfs.sh b/scripts/build-rootfs.sh index d574a35..eadcd20 100644 --- a/scripts/build-rootfs.sh +++ b/scripts/build-rootfs.sh @@ -17,6 +17,15 @@ set -euo pipefail +# Privilege shim: when already root (e.g. `forkd quickstart` in a +# container, or invoked via `sudo -E forkd parent build`), `sudo` may +# not be installed at all — run the privileged commands directly. +if [ "$(id -u)" -eq 0 ]; then + SUDO="" +else + SUDO="sudo" +fi + IMAGE="${1:-ubuntu:24.04}" OUTPUT="${2:-rootfs.ext4}" SIZE_MB="${3:-2048}" @@ -30,11 +39,11 @@ say() { printf "\033[1;34m==>\033[0m %s\n" "$*"; } die() { printf "\033[1;31merror:\033[0m %s\n" "$*" >&2; cleanup; exit 1; } cleanup() { - sudo umount "$WORK/dev" 2>/dev/null || true - sudo umount "$WORK/sys" 2>/dev/null || true - sudo umount "$WORK/proc" 2>/dev/null || true + $SUDO umount "$WORK/dev" 2>/dev/null || true + $SUDO umount "$WORK/sys" 2>/dev/null || true + $SUDO umount "$WORK/proc" 2>/dev/null || true docker rm -f "$CONTAINER" >/dev/null 2>&1 || true - sudo rm -rf "$WORK" 2>/dev/null || true + $SUDO rm -rf "$WORK" 2>/dev/null || true } trap cleanup EXIT @@ -54,20 +63,20 @@ docker create --name "$CONTAINER" "$IMAGE" /bin/true >/dev/null # ---------------------------------------------------------------------------- say "[2/5] exporting container filesystem to $WORK..." mkdir -p "$WORK" -docker export "$CONTAINER" | sudo tar -xf - -C "$WORK" -sudo du -sh "$WORK" +docker export "$CONTAINER" | $SUDO tar -xf - -C "$WORK" +$SUDO du -sh "$WORK" # ---------------------------------------------------------------------------- if [ "${#EXTRA_PKGS[@]}" -gt 0 ]; then say "[3/5] chroot apt install: ${EXTRA_PKGS[*]}" # bring up host DNS + bind /proc /sys /dev for apt to work - sudo cp /etc/resolv.conf "$WORK/etc/resolv.conf" - sudo mount --bind /proc "$WORK/proc" - sudo mount --bind /sys "$WORK/sys" - sudo mount --bind /dev "$WORK/dev" + $SUDO cp /etc/resolv.conf "$WORK/etc/resolv.conf" + $SUDO mount --bind /proc "$WORK/proc" + $SUDO mount --bind /sys "$WORK/sys" + $SUDO mount --bind /dev "$WORK/dev" - sudo chroot "$WORK" /bin/bash -e <&1 | tail -5 @@ -76,9 +85,9 @@ apt-get clean rm -rf /var/lib/apt/lists/* /var/cache/apt/archives/* EOF - sudo umount "$WORK/dev" || true - sudo umount "$WORK/sys" || true - sudo umount "$WORK/proc" || true + $SUDO umount "$WORK/dev" || true + $SUDO umount "$WORK/sys" || true + $SUDO umount "$WORK/proc" || true else say "[3/5] skipping apt install (no extra pkgs requested)" fi @@ -88,15 +97,15 @@ say "[4/5] installing forkd init + agent..." # Copy the init script and the Python agent into the rootfs. INIT_SRC="$(dirname "$(readlink -f "$0")")/../rootfs-init" if [ -d "$INIT_SRC" ]; then - sudo cp "$INIT_SRC/forkd-init.sh" "$WORK/forkd-init.sh" - sudo cp "$INIT_SRC/forkd-agent.py" "$WORK/forkd-agent.py" - sudo chmod 755 "$WORK/forkd-init.sh" "$WORK/forkd-agent.py" + $SUDO cp "$INIT_SRC/forkd-init.sh" "$WORK/forkd-init.sh" + $SUDO cp "$INIT_SRC/forkd-agent.py" "$WORK/forkd-agent.py" + $SUDO chmod 755 "$WORK/forkd-init.sh" "$WORK/forkd-agent.py" say " installed /forkd-init.sh and /forkd-agent.py" else say " rootfs-init/ not found at $INIT_SRC — guest will boot without forkd agent" fi # Empty root password for development convenience. -sudo chroot "$WORK" /bin/bash -c "passwd -d root 2>/dev/null || true" +$SUDO chroot "$WORK" /bin/bash -c "passwd -d root 2>/dev/null || true" # ---------------------------------------------------------------------------- say "[5/5] building ext4 image ($SIZE_MB MiB)..." diff --git a/scripts/host-tap.sh b/scripts/host-tap.sh index 86f349b..52b2229 100644 --- a/scripts/host-tap.sh +++ b/scripts/host-tap.sh @@ -14,7 +14,9 @@ set -euo pipefail TAP="${TAP:-forkd-tap0}" TAP_IP="${TAP_IP:-10.42.0.1}" -USER_OWNS="${USER_OWNS:-${SUDO_USER:-$USER}}" +# $USER is unset in non-login shells (docker exec, some CI) and this +# script runs under `set -u` — fall back through id(1). +USER_OWNS="${USER_OWNS:-${SUDO_USER:-${USER:-$(id -un)}}}" [ "$(id -u)" -eq 0 ] || { echo "run as root" >&2; exit 1; } command -v ip >/dev/null || { echo "ip(8) required" >&2; exit 1; } diff --git a/scripts/install-guest-kernel.sh b/scripts/install-guest-kernel.sh index 915b576..b1e7d37 100644 --- a/scripts/install-guest-kernel.sh +++ b/scripts/install-guest-kernel.sh @@ -59,8 +59,17 @@ while [ $# -gt 0 ]; do shift done +# Privilege shim: when already root (e.g. `forkd quickstart` running +# the embedded copy of this script in a container or via sudo -E), +# `sudo` may not even be installed — call the commands directly. +if [ "$(id -u)" -eq 0 ]; then + SUDO="" +else + SUDO="sudo" +fi + INSTALL_DIR="$(dirname "$OUT_PATH")" -sudo mkdir -p "$INSTALL_DIR" +$SUDO mkdir -p "$INSTALL_DIR" case "$MODE" in download) @@ -79,8 +88,8 @@ case "$MODE" in exit 1 fi fi - sudo cp "$TMP" "$OUT_PATH" - sudo chmod 644 "$OUT_PATH" + $SUDO cp "$TMP" "$OUT_PATH" + $SUDO chmod 644 "$OUT_PATH" ;; build) echo "==> building from source via scripts/build-guest-kernel.sh" @@ -92,8 +101,12 @@ esac echo echo "==> installed:" ls -la "$OUT_PATH" -file "$OUT_PATH" | head -1 -strings "$OUT_PATH" 2>/dev/null | grep -E "^Linux version" | head -1 +# `file` / `strings` are nice-to-have descriptors, not part of the +# install — minimal hosts (containers, cloud images) often lack them. +command -v file > /dev/null && file "$OUT_PATH" | head -1 +command -v strings > /dev/null \ + && strings "$OUT_PATH" 2>/dev/null | grep -E "^Linux version" | head -1 +true echo echo "==> next: rebuild any v0.5.0-era snapshots so they pick up the" diff --git a/scripts/netns-setup.sh b/scripts/netns-setup.sh index 3ee8f96..cf8836e 100644 --- a/scripts/netns-setup.sh +++ b/scripts/netns-setup.sh @@ -32,7 +32,9 @@ set -euo pipefail N="${1:-10}" -USER_OWNS="${2:-${SUDO_USER:-$USER}}" +# $USER is unset in non-login shells (docker exec, some CI) and this +# script runs under `set -u` — fall back through id(1). +USER_OWNS="${2:-${SUDO_USER:-${USER:-$(id -un)}}}" HOST_IP="${HOST_IP:-10.42.0.1}" say() { printf "\033[1;34m==>\033[0m %s\n" "$*"; }