From 93dce2b8405cd1e634a456619e944fc749a869b8 Mon Sep 17 00:00:00 2001 From: ejc3 Date: Tue, 8 Sep 2026 01:55:03 +0000 Subject: [PATCH 1/5] fix: relaunch Cloud Hypervisor at guest reset Observe CH reset events through an owned per-child socketpair and shut down the VMM after the guest reaches reset. The existing host relaunch then consumes reboot intent, so a later container exit terminates fcvm normally. Retain monitor join ownership across cancellation and reap the child before reporting monitor failure. Place the mandatory reboot regression in Host-Root CI, provision CH there, and remove the CH builder's shallow-clone flag. Verified VM and lifecycle regressions red/green/revert-red. Final four-test batch and make lint pass. Default-feature test selection excludes CH only with the privileged-tests gate. --- .github/workflows/ci.yml | 3 + Makefile | 5 +- src/hypervisor/cloud_hypervisor/mod.rs | 204 ++++++++++++++++++++++++- src/setup/kernel.rs | 4 +- tests/test_reboot.rs | 52 ++++++- 5 files changed, 256 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c0e4646e1..7f3fc44f0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -795,6 +795,9 @@ jobs: run: | echo 512 | sudo tee /proc/sys/vm/nr_hugepages echo "Allocated $(cat /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages) hugepages" + - name: Build Cloud Hypervisor backend for reboot regression + working-directory: fcvm + run: make setup-cloud-hypervisor - name: test-root working-directory: fcvm run: | diff --git a/Makefile b/Makefile index b72ab2030..3c9cc0b0f 100644 --- a/Makefile +++ b/Makefile @@ -245,7 +245,7 @@ CONTAINER_RUN := $(CONTAINER_RUN_BASE) --ulimit nproc=65536:65536 --pids-limit=6 _test-unit _test-agent-unit _test-fast _test-all _test-root _setup-fcvm _bench \ container-build container-test container-test-unit container-test-fast container-test-all container-test-fc-mock \ container-setup-fcvm container-shell container-clean container-bench \ - cargo-target-link build-host-tools setup-btrfs setup-default release-default-kernel setup-fcvm setup-passt setup-pjdfstest setup-hugepages bench bench-vm bench-hugepages bench-hugepages-test \ + cargo-target-link build-host-tools setup-btrfs setup-default release-default-kernel setup-fcvm setup-cloud-hypervisor setup-passt setup-pjdfstest setup-hugepages bench bench-vm bench-hugepages bench-hugepages-test \ bench-container-import bench-chromium analyze-chromium-request analyze-chromium-campaign bench-clone-latency test-chromium-request \ bench-chromium-request-build bench-webkit-request-build bench-webkit-request-golden bench-webkit-request-verify bench-webkit-request-run test-chromium bench-chromium-request-golden bench-chromium-request-verify \ bench-chromium-corpus bench-chromium-corpus-extra bench-stop \ @@ -679,6 +679,9 @@ container-clean: podman rmi $(CONTAINER_TAG) 2>/dev/null || true # Setup targets +setup-cloud-hypervisor: build + ./target/release/fcvm setup --cloud-hypervisor + setup-passt: ./scripts/build-passt.sh diff --git a/src/hypervisor/cloud_hypervisor/mod.rs b/src/hypervisor/cloud_hypervisor/mod.rs index be10bba29..24153fa91 100644 --- a/src/hypervisor/cloud_hypervisor/mod.rs +++ b/src/hypervisor/cloud_hypervisor/mod.rs @@ -17,10 +17,13 @@ pub mod api; use anyhow::{anyhow, bail, Context, Result}; use std::collections::VecDeque; +use std::os::fd::AsRawFd; +use std::os::unix::net::UnixStream; use std::path::{Path, PathBuf}; use std::process::ExitStatus; use std::sync::{Arc, Mutex}; use std::time::Duration; +use tokio::io::{AsyncBufRead, AsyncBufReadExt, BufReader}; use tokio::process::{Child, Command}; use tokio::sync::mpsc; use tokio::task::JoinHandle; @@ -87,6 +90,8 @@ pub struct CloudHypervisorBackend { /// Guest console (hvc0) lines observed by the console tail since spawn /// (see [`Hypervisor::console_line_counter`]). console_lines: Arc, + /// One event reader per VMM child. It must finish before the API path is reused. + reboot_monitor: Option>>, } impl CloudHypervisorBackend { @@ -110,6 +115,7 @@ impl CloudHypervisorBackend { vsock_path: None, console_tail: None, console_lines: Arc::new(std::sync::atomic::AtomicU64::new(0)), + reboot_monitor: None, } } @@ -117,6 +123,15 @@ impl CloudHypervisorBackend { self.client.as_ref().context("Cloud Hypervisor not started") } + async fn stop_reboot_monitor(&mut self) { + // Keep ownership across await: wait() can be cancelled by the VM loop. + if let Some(monitor) = self.reboot_monitor.as_mut() { + monitor.abort(); + let _ = monitor.await; + } + self.reboot_monitor = None; + } + /// Merge a spawn spec into the retained namespace isolation: only fields the spec /// actually provides overwrite the retained values. A guest reboot relaunches via /// [`Hypervisor::spawn`] with a minimal spec (binary + args only), so this preserves @@ -299,6 +314,7 @@ impl Hypervisor for CloudHypervisorBackend { } async fn spawn(&mut self, spec: &ProcessSpec) -> Result<()> { + self.stop_reboot_monitor().await; // A reboot relaunch re-enters spawn() with a minimal spec (binary + args). Update // retained state only when the spec provides a value, so the namespace isolation // and name captured on the first spawn persist across reboots (mirrors the @@ -320,6 +336,11 @@ impl Hypervisor for CloudHypervisorBackend { let mut cmd = Command::new(&spec.binary); cmd.arg("--api-socket").arg(&self.api_socket); + let (events, child_events) = UnixStream::pair().context("creating CH event socketpair")?; + events.set_nonblocking(true)?; + let events = tokio::net::UnixStream::from_std(events)?; + let event_fd = child_events.as_raw_fd(); + cmd.arg("--event-monitor").arg(format!("fd={event_fd}")); if let Some(log_path) = &self.log_path { cmd.arg("--log-file").arg(log_path); cmd.arg("-v"); @@ -330,6 +351,18 @@ impl Hypervisor for CloudHypervisorBackend { } } + // Clear CLOEXEC only in this child. Changing it in the parent would let + // unrelated concurrent spawns inherit the event socket. Namespace setup + // remains the last pre_exec so its post-setns PDEATHSIG is preserved. + // SAFETY: fcntl is async-signal-safe, and child_events owns the fd until spawn returns. + unsafe { + cmd.pre_exec(move || { + if libc::fcntl(event_fd, libc::F_SETFD, 0) == -1 { + return Err(std::io::Error::last_os_error()); + } + Ok(()) + }); + } install_namespace_pre_exec(&mut cmd, &self.namespace)?; let stderr_tail = Arc::clone(&self.stderr_tail); @@ -353,11 +386,23 @@ impl Hypervisor for CloudHypervisorBackend { } }) .context("spawning Cloud Hypervisor process")?; + drop(child_events); self.process = Some(spawned.child); self.stderr_reader = Some(spawned.stderr_reader); self.wait_for_api().await?; self.client = Some(ChClient::new(self.api_socket.clone())); + let client = self.client()?.clone(); + self.reboot_monitor = Some(tokio::spawn(async move { + if read_reboot_event(BufReader::new(events)).await? { + // The guest's vsock notification is early intent, before shutdown + // writeback. CH emits this event only when it handles the actual + // reset. Exit the VMM here so fcvm consumes intent and cold-relaunches. + info!("Cloud Hypervisor guest reset, shutting down VMM for host relaunch"); + client.shutdown_vmm().await?; + } + Ok(()) + })); // Tail the guest console (hvc0 → file) into fcvm's tracing logs, mirroring the // Firecracker serial-to-stdout capture. Abort any prior tail first: a reboot @@ -387,6 +432,9 @@ impl Hypervisor for CloudHypervisorBackend { match self.process.as_mut() { Some(p) => match p.try_wait().context("checking Cloud Hypervisor status")? { Some(status) => { + if let Some(monitor) = &self.reboot_monitor { + monitor.abort(); + } self.process = None; Ok(Some(status)) } @@ -397,17 +445,43 @@ impl Hypervisor for CloudHypervisorBackend { } async fn wait(&mut self) -> Result { - match self.process.as_mut() { - Some(p) => { - let status = p.wait().await.context("waiting for Cloud Hypervisor")?; - self.process = None; - Ok(status) + let process = self + .process + .as_mut() + .context("Cloud Hypervisor process not running")?; + let mut monitor_failure = None; + let status = if let Some(monitor) = self.reboot_monitor.as_mut() { + tokio::select! { + biased; + status = process.wait() => status, + result = monitor => { + self.reboot_monitor = None; + if let Err(error) = result.context("CH reboot monitor task failed").and_then(|r| r) { + warn!(%error, "CH reboot monitor failed, terminating VMM"); + // The caller treats any wait() return as child termination. + // Never report this failure while the VMM can still be alive. + process.start_kill().context("terminating CH after event monitor failure")?; + monitor_failure = Some(error); + } + process.wait().await + } } - None => bail!("Cloud Hypervisor process not running"), + } else { + process.wait().await + } + .context("waiting for Cloud Hypervisor")?; + self.stop_reboot_monitor().await; + self.process = None; + if let Some(error) = monitor_failure { + return Err(error); } + Ok(status) } fn start_kill(&mut self) -> Result<()> { + if let Some(monitor) = &self.reboot_monitor { + monitor.abort(); + } if let Some(tail) = self.console_tail.take() { tail.abort(); } @@ -420,6 +494,7 @@ impl Hypervisor for CloudHypervisorBackend { } async fn reap(&mut self) { + self.stop_reboot_monitor().await; if let Some(mut p) = self.process.take() { let _ = p.wait().await; } @@ -536,6 +611,43 @@ impl Hypervisor for CloudHypervisorBackend { } } +impl Drop for CloudHypervisorBackend { + fn drop(&mut self) { + if let Some(monitor) = &self.reboot_monitor { + monitor.abort(); + } + } +} + +/// CH writes pretty-printed JSON objects separated by a blank line, not JSONL. +async fn read_reboot_event(reader: impl AsyncBufRead + Unpin) -> Result { + #[derive(serde::Deserialize)] + struct Event { + source: String, + event: String, + } + + let mut lines = reader.lines(); + let mut frame = String::new(); + while let Some(line) = lines.next_line().await.context("reading CH events")? { + if line.is_empty() { + if frame.is_empty() { + continue; + } + let event: Event = serde_json::from_str(&frame).context("decoding CH event")?; + if event.source == "vm" && event.event == "rebooting" { + return Ok(true); + } + frame.clear(); + } else { + frame.push_str(&line); + frame.push('\n'); + } + } + anyhow::ensure!(frame.is_empty(), "CH event stream ended within an event"); + Ok(false) +} + /// The default guest CID Cloud Hypervisor uses for the host↔guest vsock device. pub const fn default_guest_cid() -> u32 { GUEST_CID @@ -602,6 +714,86 @@ async fn tail_console_to_tracing(path: PathBuf, console_lines: Arc(); + // A started blocking task models a reader still in its current poll: + // abort requests cancellation but cannot finish the join until it yields. + be.reboot_monitor = Some(tokio::task::spawn_blocking(move || { + let _ = started_tx.send(()); + let _ = release_rx.recv(); + Ok(()) + })); + started_rx.await.unwrap(); + let pending = { + let mut cleanup = std::pin::pin!(be.stop_reboot_monitor()); + let mut context = std::task::Context::from_waker(std::task::Waker::noop()); + cleanup.as_mut().poll(&mut context).is_pending() + }; + let retained = be.reboot_monitor.is_some(); + // Release even on a failed assertion, so the fixture cannot strand a worker. + drop(release_tx); + be.stop_reboot_monitor().await; + assert!(pending, "cleanup must wait for the reader to finish"); + assert!( + retained, + "cancelled cleanup must retain the reader's join handle" + ); + assert!(be.reboot_monitor.is_none()); + } + + #[tokio::test] + async fn reboot_monitor_failure_reaps_child_before_returning() { + let mut be = backend(); + be.process = Some( + Command::new("sleep") + .arg("60") + .kill_on_drop(true) + .spawn() + .unwrap(), + ); + be.reboot_monitor = Some(tokio::spawn(async { bail!("injected CH event failure") })); + let result = tokio::time::timeout(Duration::from_secs(2), be.wait()).await; + let reaped = be.process.is_none(); + be.start_kill().unwrap(); + be.reap().await; + assert!(result + .unwrap() + .unwrap_err() + .to_string() + .contains("injected CH event failure")); + assert!( + reaped, + "wait must not report a monitor failure with a live VMM child" + ); + } + + #[tokio::test] + async fn reboot_monitor_requires_complete_vm_reset_event() { + let other_events = concat!( + "{\n \"source\": \"vmm\",\n \"event\": \"rebooting\"\n}\n\n", + "{\n \"source\": \"vm\",\n \"event\": \"rebooted\"\n}\n\n", + ); + assert!(!read_reboot_event(other_events.as_bytes()).await.unwrap()); + let events = format!( + "{other_events}{{\n \"source\": \"vm\",\n \"event\": \"rebooting\",\n \"properties\": null\n}}\n\n" + ); + // One-byte reads split both the JSON and its delimiter across reads. + assert!( + read_reboot_event(BufReader::with_capacity(1, events.as_bytes())) + .await + .unwrap() + ); + assert!(read_reboot_event(b"{\n \"source\": \"vm\"".as_slice()) + .await + .is_err()); + assert!(read_reboot_event(b"not-json\n\n".as_slice()).await.is_err()); + } + fn backend() -> CloudHypervisorBackend { CloudHypervisorBackend::new( "vm-test".to_string(), diff --git a/src/setup/kernel.rs b/src/setup/kernel.rs index 4a84ab925..ea6abf471 100644 --- a/src/setup/kernel.rs +++ b/src/setup/kernel.rs @@ -2642,13 +2642,11 @@ pub async fn ensure_cloud_hypervisor(repo: &str, branch: &str) -> Result Result<()> { // `reboot` goes through systemd, which starts fcvm-reboot-notify.service // (WantedBy=reboot.target) -> fc-agent --notify-reboot -> host relaunches - // Firecracker in place. The exec dies mid-command as the VM resets. + // the VMM in place. The exec dies mid-command as the VM resets. let _ = common::exec_in_vm(pid, &["reboot"]).await; let deadline = Instant::now() + Duration::from_secs(150); @@ -124,6 +124,54 @@ async fn test_vm_reboot_comes_back_healthy_and_preserves_work() -> Result<()> { Ok(()) } +/// A normal Cloud Hypervisor VM must consume reboot intent before its final exit. +// Host-Root CI provisions Cloud Hypervisor; container-test-all does not. +#[cfg(feature = "privileged-tests")] +#[tokio::test] +async fn test_cloud_hypervisor_reboot_recovers_and_then_exits() -> Result<()> { + fcvm::commands::common::find_cloud_hypervisor() + .context("CH reboot test requires the backend")?; + let (name, _, _, _) = common::unique_names("ch-reboot"); + let (mut child, pid) = common::spawn_fcvm_with_logs( + &[ + "podman", + "run", + "--name", + &name, + "--hypervisor", + "cloud-hypervisor", + "--no-snapshot", + "nginx:alpine", + "sh", + "-c", + "while [ ! -e /stop ]; do sleep 1; done", + ], + "ch-reboot-base", + ) + .await?; + let result = async { + common::poll_health_by_pid(pid, 120).await?; + let token = format!("ch-reboot-token-{pid}"); + write_work_marker(pid, &token).await?; + reboot_and_assert_relaunch(pid, &token).await?; + + // The exec response may disappear when its container exits; the process + // exit below proves the stop marker was consumed and the final boot ended. + let _ = common::exec_in_container(pid, &["touch", "/stop"]).await; + let status = tokio::time::timeout(Duration::from_secs(60), child.wait()) + .await + .context("CH VM did not terminate after its container exited")??; + anyhow::ensure!(status.success(), "CH VM final exit must be zero: {status}"); + Ok::<_, anyhow::Error>(()) + } + .await; + if child.try_wait()?.is_none() { + common::kill_process(pid).await; + let _ = child.kill().await; + } + result +} + /// Data-disk preservation: an in-place reboot must NOT rebuild --disk-dir /// images from the host directory — guest writes to the data disk live only in /// the per-VM image and would be silently destroyed (confirmed review finding). From 6e1945af4a76dc2a7f30b563d1931dd068a7845b Mon Sep 17 00:00:00 2001 From: ejc3 Date: Sun, 13 Sep 2026 14:09:48 +0000 Subject: [PATCH 2/5] Lease setup-cloud-hypervisor and make test-root provision it setup-cloud-hypervisor ran ./target/release/fcvm without holding the target generation lease, so a concurrent make could repoint the target link under it. makefile_leases_every_raw_target_access failed on it in every Host, Host-Root and Container job. The target now declares `private SHELL := $(TARGET_LEASE_SHELL)`, as setup-default and setup-fcvm do. test_cloud_hypervisor_reboot_recovers_and_then_exits is compiled under privileged-tests, the feature _test-root enables, and fails at find_cloud_hypervisor() when the backend is absent. fcvm setup does not build Cloud Hypervisor, so make test-root failed on a clean box, and CI passed only because it ran setup-cloud-hypervisor as a separate step. test-root now depends on setup-cloud-hypervisor and that CI step is removed. CH setup is content-addressed, so the second test-root pass in SnapshotEnabled mode skips the build. test_root_provisions_the_cloud_hypervisor_its_reboot_test_requires pins the prerequisite. Red on the unfixed tree: 2 tests run: 0 passed, 2 failed Green with the fix: 2 tests run: 2 passed Red again with the Makefile fix reverted: 2 tests run: 0 passed, 2 failed test_documented_make_targets, test_cargo_target_link, test_ci_workflow_coverage: 102 tests run: 102 passed (test_cargo_target_link 62, test_ci_workflow_coverage 23, test_documented_make_targets 17) make lint: exit 0 (cargo-deny: advisories ok, bans ok, licenses ok, sources ok) --- .github/workflows/ci.yml | 3 -- Makefile | 3 +- tests/test_documented_make_targets.rs | 40 +++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7f3fc44f0..c0e4646e1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -795,9 +795,6 @@ jobs: run: | echo 512 | sudo tee /proc/sys/vm/nr_hugepages echo "Allocated $(cat /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages) hugepages" - - name: Build Cloud Hypervisor backend for reboot regression - working-directory: fcvm - run: make setup-cloud-hypervisor - name: test-root working-directory: fcvm run: | diff --git a/Makefile b/Makefile index 3c9cc0b0f..6a667d74b 100644 --- a/Makefile +++ b/Makefile @@ -594,7 +594,7 @@ test-unit: show-notes check-disk build _test-unit test-agent-unit: show-notes check-disk cargo-target-link _test-agent-unit test-fast: show-notes check-disk setup-fcvm _test-fast test-all: show-notes check-disk setup-fcvm _test-all -test-root: show-notes check-disk setup-fcvm setup-pjdfstest setup-hugepages _test-root +test-root: show-notes check-disk setup-fcvm setup-pjdfstest setup-hugepages setup-cloud-hypervisor _test-root test: test-root # Seeded lifecycle chaos fuzz (tests/test_fuzz_chaos.rs): one rootless VM per @@ -679,6 +679,7 @@ container-clean: podman rmi $(CONTAINER_TAG) 2>/dev/null || true # Setup targets +setup-cloud-hypervisor: private SHELL := $(TARGET_LEASE_SHELL) setup-cloud-hypervisor: build ./target/release/fcvm setup --cloud-hypervisor diff --git a/tests/test_documented_make_targets.rs b/tests/test_documented_make_targets.rs index 811a7b356..1cd5a3324 100644 --- a/tests/test_documented_make_targets.rs +++ b/tests/test_documented_make_targets.rs @@ -1263,3 +1263,43 @@ fn hugepage_lock_recipes_are_valid_shell_as_make_runs_them() { String::from_utf8_lossy(&out.stderr) ); } + +/// `make test-root` must provision the Cloud Hypervisor backend its reboot test needs. +/// +/// `test_cloud_hypervisor_reboot_recovers_and_then_exits` in tests/test_reboot.rs is compiled +/// under `privileged-tests`, the feature `_test-root` enables, and it fails at +/// `find_cloud_hypervisor()` when the backend binary is absent. `fcvm setup` does not build CH, +/// so a `test-root` that does not depend on `setup-cloud-hypervisor` fails on a clean +/// developer box while CI, which called that target as its own step, passes. +#[test] +fn test_root_provisions_the_cloud_hypervisor_its_reboot_test_requires() { + let makefile = repo_file("Makefile"); + let prerequisites: Vec<&str> = makefile + .lines() + .find_map(|line| line.strip_prefix("test-root:")) + .expect( + "the Makefile defines no `test-root:` rule, so this cannot fail for the right reason", + ) + .split_whitespace() + .collect(); + assert!( + prerequisites.contains(&"setup-fcvm"), + "`test-root:` names no `setup-fcvm` prerequisite ({prerequisites:?}), so the parse is not \ + reading the rule it thinks it is" + ); + + let reboot = repo_file("tests/test_reboot.rs"); + assert!( + reboot.contains("fn test_cloud_hypervisor_reboot_recovers_and_then_exits") + && reboot.contains("find_cloud_hypervisor()"), + "the Cloud Hypervisor reboot test is gone or no longer requires the backend; remove this \ + pin with it" + ); + + assert!( + prerequisites.contains(&"setup-cloud-hypervisor"), + "`make test-root` runs test_cloud_hypervisor_reboot_recovers_and_then_exits, which needs \ + the Cloud Hypervisor binary, but does not depend on `setup-cloud-hypervisor`: \ + {prerequisites:?}" + ); +} From e84164299e492a541c76d82c94833530b4ded6e6 Mon Sep 17 00:00:00 2001 From: ejc3 Date: Sun, 13 Sep 2026 15:53:07 +0000 Subject: [PATCH 3/5] Opt Cloud Hypervisor guest memory out of transparent hugepages fcvm's vm.create memory config carried no `thp` field, and CH defaults it to true, which madvises guest RAM MADV_HUGEPAGE. Hosts run `defrag=madvise`, so each guest fault on that memory may compact synchronously in the VMM's own thread. On 6e1945af, Host-Root-arm64-SnapshotEnabled failed on its second test-root pass only. Between 14:45 and 14:53 (pidstat): - every cloud-hypervisor process ran at 100-180% of a core, all %system - kcompactd0 sat at 94-100%; khugepaged ran, which it does only for madvised memory - firecracker processes averaged 4.4% system, the same as pass 1 - the host was 86% idle (sar) Effects: test_cloud_hypervisor_cold_boot hit the 30 s vm.boot timeout, then reached fc-agent at 150 s of guest uptime (1.4 s on main); test_cloud_hypervisor_reboot_recovers_and_then_exits reached fc-agent at 94 s; the snapshot_roundtrip clone's file restore took 183 s against 11 s on pass 1. Same CH binary as main (cloud-hypervisor-6bb88929652f.bin). The memory config now sends `thp: false`. Firecracker guest RAM is not madvised either. vm_config_opts_guest_memory_out_of_transparent_hugepages serializes the vm.create payload and requires memory.thp == false. Red on the unfixed tree: left: Null, right: Bool(false) Green with the fix: 1 test run: 1 passed Red again, fix reverted: left: Null, right: Bool(false) One 1 GiB CH VM per build on an unfragmented arm64 host, guest RAM mapping from /proc//smaps: pre-fix: VmFlags has hg, THPeligible 1, AnonHugePages 530432 kB, thp_fault_alloc +259 fixed: no hg, THPeligible 0, AnonHugePages 0 kB, thp_fault_alloc +0 That host has free 2 MB blocks, so this shows the madvise, not the stall; the stall needs CI's second pass. make test-root FILTER="-E 'test(/test_cloud_hypervisor/)'": 3 tests run: 3 passed (cold_boot 3.8 s, reboot 9.4 s, snapshot_roundtrip 16.4 s) make test-unit FILTER="-E 'test(/cloud_hypervisor/)'": 11 tests run: 11 passed make lint: exit 0 --- src/hypervisor/cloud_hypervisor/api.rs | 6 ++++++ src/hypervisor/cloud_hypervisor/mod.rs | 16 ++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/src/hypervisor/cloud_hypervisor/api.rs b/src/hypervisor/cloud_hypervisor/api.rs index 68587c3c0..4c8eef01b 100644 --- a/src/hypervisor/cloud_hypervisor/api.rs +++ b/src/hypervisor/cloud_hypervisor/api.rs @@ -176,6 +176,12 @@ pub struct MemoryConfig { pub size: u64, /// Back guest RAM with a shared mmap (required for vhost-user / some restore modes). pub shared: bool, + /// When true (CH's default) CH madvises guest RAM MADV_HUGEPAGE. Under the host's + /// `defrag=madvise` each guest fault on that memory can then compact synchronously. + /// On a fragmented CI host that kept kcompactd near 100%, put every CH VMM at + /// 100-180% of a core in the kernel, and stretched a file restore to 183 s from + /// 11 s. Firecracker guest RAM is not madvised either. + pub thp: bool, } #[derive(Debug, Serialize)] diff --git a/src/hypervisor/cloud_hypervisor/mod.rs b/src/hypervisor/cloud_hypervisor/mod.rs index 24153fa91..76e7668b8 100644 --- a/src/hypervisor/cloud_hypervisor/mod.rs +++ b/src/hypervisor/cloud_hypervisor/mod.rs @@ -250,6 +250,7 @@ impl CloudHypervisorBackend { memory: MemoryConfig { size: self.pending.mem_mib as u64 * 1024 * 1024, shared: false, + thp: false, }, payload: PayloadConfig { kernel: kernel.display().to_string(), @@ -802,6 +803,21 @@ mod tests { ) } + /// CH madvises guest RAM MADV_HUGEPAGE unless the request says `thp: false`, so an + /// absent field opts in. See `MemoryConfig::thp` for what that cost on CI. + #[test] + fn vm_config_opts_guest_memory_out_of_transparent_hugepages() { + let mut be = backend(); + be.pending.kernel = Some(PathBuf::from("/boot/Image")); + let config = serde_json::to_value(be.build_vm_config().unwrap()).unwrap(); + assert_eq!( + config["memory"]["thp"], + serde_json::Value::Bool(false), + "vm.create memory config: {}", + config["memory"] + ); + } + /// Codex #632 P1 #1: a reboot relaunches with a minimal spec (binary + args only). /// The namespace isolation captured on the FIRST spawn must persist, or the /// relaunched VMM would run outside its namespaces. Before the fix, `spawn` read the From fb907481fb48b31b807ae6c81c48cc150d43cfc3 Mon Sep 17 00:00:00 2001 From: ejc3 Date: Sun, 13 Sep 2026 16:25:50 +0000 Subject: [PATCH 4/5] Order setup-cloud-hypervisor after the assets-store mount setup-cloud-hypervisor runs `fcvm setup --cloud-hypervisor`, which writes under /mnt/fcvm-btrfs, but depended only on `build`. Under `make -j test-root` it is a sibling of setup-fcvm, which reaches setup-btrfs only through setup-default, so on a host that is not btrfs it could write into the bare directory before setup-btrfs mounts the loopback over it (CodeRabbit on #921). Every other target that runs `fcvm setup` already reaches setup-btrfs. setup-cloud-hypervisor now depends on it directly. targets_that_run_fcvm_setup_mount_the_assets_store_first parses the Makefile's rules and recipes and requires setup-btrfs among the transitive prerequisites of every target whose recipe runs `fcvm setup` (excluding --generate-config, which writes the user's config, and `_` targets, which run inside the container their wrapper starts). Red on the unfixed tree: 1 test run: 0 passed, 1 failed Green with the fix: 1 test run: 1 passed Red again, fix reverted: 1 test run: 0 passed, 1 failed test_documented_make_targets: 18 tests run: 18 passed make lint: exit 0 --- Makefile | 2 +- tests/test_documented_make_targets.rs | 77 +++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 6a667d74b..1bd2234ce 100644 --- a/Makefile +++ b/Makefile @@ -680,7 +680,7 @@ container-clean: # Setup targets setup-cloud-hypervisor: private SHELL := $(TARGET_LEASE_SHELL) -setup-cloud-hypervisor: build +setup-cloud-hypervisor: build setup-btrfs ./target/release/fcvm setup --cloud-hypervisor setup-passt: diff --git a/tests/test_documented_make_targets.rs b/tests/test_documented_make_targets.rs index 1cd5a3324..1d018d92d 100644 --- a/tests/test_documented_make_targets.rs +++ b/tests/test_documented_make_targets.rs @@ -1303,3 +1303,80 @@ fn test_root_provisions_the_cloud_hypervisor_its_reboot_test_requires() { {prerequisites:?}" ); } + +/// A target that runs `fcvm setup` must mount the assets store before its recipe runs. +/// +/// `fcvm setup` writes under /mnt/fcvm-btrfs, which `setup-btrfs` mounts as a loopback on a host +/// that is not btrfs. Make runs a recipe after that target's own prerequisites only, and sibling +/// prerequisites run in any order under `make -j`, so a target that reaches `setup-btrfs` only +/// through a sibling can write into the bare directory and have the mount hide what it wrote. +/// `setup-cloud-hypervisor` did that under `test-root` (CodeRabbit on #921). Excluded: +/// `--generate-config`, which writes the user's config and not the store, and `_`-prefixed +/// targets, which run inside the container their host-side wrapper starts after that wrapper's +/// own prerequisites (`container-setup-fcvm` runs `_setup-fcvm`). +#[test] +fn targets_that_run_fcvm_setup_mount_the_assets_store_first() { + let makefile = repo_file("Makefile"); + let mut prerequisites: std::collections::HashMap<&str, Vec<&str>> = Default::default(); + let mut runs_setup: Vec<&str> = Vec::new(); + let mut current: Option<&str> = None; + for line in makefile.lines() { + if let Some(command) = line.strip_prefix('\t') { + if let Some(target) = current { + if command.contains("target/release/fcvm setup") + && !command.contains("--generate-config") + && !runs_setup.contains(&target) + { + runs_setup.push(target); + } + } + continue; + } + let rule = line.split_once(':').filter(|(name, _)| { + !name.is_empty() + && name + .chars() + .all(|c| c.is_ascii_alphanumeric() || "_.-".contains(c)) + }); + match rule { + Some((name, rest)) => { + current = Some(name); + // `target: private SHELL := ...` sets a variable; it names no prerequisites. + if !rest.contains('=') { + prerequisites + .entry(name) + .or_default() + .extend(rest.split_whitespace()); + } + } + None if !line.trim().is_empty() => current = None, + None => {} + } + } + + for expected in ["setup-default", "setup-fcvm", "setup-cloud-hypervisor"] { + assert!( + runs_setup.contains(&expected), + "`{expected}` no longer runs `fcvm setup` as far as this parse can tell \ + ({runs_setup:?}), so it is not reading the recipes it thinks it is" + ); + } + + for target in runs_setup.iter().filter(|t| !t.starts_with('_')) { + let mut seen: Vec<&str> = Vec::new(); + let mut stack = vec![*target]; + while let Some(t) = stack.pop() { + if seen.contains(&t) { + continue; + } + seen.push(t); + stack.extend(prerequisites.get(t).into_iter().flatten().copied()); + } + assert!( + seen.contains(&"setup-btrfs"), + "`{target}` runs `fcvm setup`, which writes under /mnt/fcvm-btrfs, but `setup-btrfs` \ + is not among its prerequisites. Under `make -j` it can run before the mount and \ + write into a directory the mount then hides. Prerequisites reached: {seen:?}" + ); + } +} From 23dce0469875071d1a7e152ea20c6309b3f651d2 Mon Sep 17 00:00:00 2001 From: ejc3 Date: Sun, 13 Sep 2026 17:28:34 +0000 Subject: [PATCH 5/5] Require a real pre-reboot machine-id before counting a reboot as witnessed reboot_and_assert_relaunch read /etc/machine-id before the reboot with unwrap_or_default(), so a failed or timed-out read left an empty baseline. The recovery loop then accepted any non-empty machine-id as regenerated, including one read from a VM that never rebooted, and the Cloud Hypervisor reboot test's identity check could pass on nothing (CodeRabbit on #921). test_vm_reboot_preserves_disk_dir_writes carried the same read and the same comparison, so both sites now share one fix: - Each pre-reboot read propagates its error and must return a non-empty id. - Both recovery loops call machine_id_regenerated(before, after), which is false for an empty or blank baseline, an empty result, or an unchanged id. a_regenerated_machine_id_needs_a_real_baseline pins the comparison. Red, comparison as it was: 1 test run: 0 passed, 1 failed ("an empty pre-reboot machine-id witnesses nothing") Green, with the fix: 1 test run: 1 passed Red again, helper reverted: 1 test run: 0 passed, 1 failed make test-root FILTER="-E 'binary(test_reboot)'": 5 tests run: 5 passed (test_cloud_hypervisor_reboot_recovers_and_then_exits 9.6 s, test_vm_reboot_preserves_disk_dir_writes 13.5 s, test_vm_reboot_comes_back_healthy_and_preserves_work 14.1 s, test_restored_clone_reboot_comes_back_healthy 16.7 s) make lint: exit 0 --- tests/test_reboot.rs | 43 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/tests/test_reboot.rs b/tests/test_reboot.rs index 3c67b3472..2dde176d8 100644 --- a/tests/test_reboot.rs +++ b/tests/test_reboot.rs @@ -24,13 +24,43 @@ fn process_alive(pid: u32) -> bool { .unwrap_or(false) } +/// True when `after` is a machine-id that differs from the one read before the reboot. +fn machine_id_regenerated(before: &str, after: &str) -> bool { + let (before, after) = (before.trim(), after.trim()); + !before.is_empty() && !after.is_empty() && after != before +} + +/// The regeneration witness must compare against a real pre-reboot machine-id. An empty baseline, +/// which a failed read produced through `unwrap_or_default()`, made any machine-id read after the +/// reboot count as regenerated, including one from a VM that never rebooted (CodeRabbit on #921). +#[test] +fn a_regenerated_machine_id_needs_a_real_baseline() { + let before = "0123456789abcdef0123456789abcdef"; + let after = "fedcba9876543210fedcba9876543210\n"; + assert!(machine_id_regenerated(before, after)); + assert!(!machine_id_regenerated(before, before)); + assert!(!machine_id_regenerated(before, "")); + assert!( + !machine_id_regenerated("", after), + "an empty pre-reboot machine-id witnesses nothing" + ); + assert!( + !machine_id_regenerated("\n", after), + "a blank pre-reboot machine-id witnesses nothing" + ); +} + /// Reboot the guest and assert the SAME fcvm process relaunches it in place: /// machine-id regenerates (positive witness of the re-boot), health recovers, /// and the container's writable layer survives. async fn reboot_and_assert_relaunch(pid: u32, token: &str) -> Result<()> { let mid_before = common::exec_in_vm(pid, &["cat", "/etc/machine-id"]) .await - .unwrap_or_default(); + .context("reading the machine-id before the reboot")?; + anyhow::ensure!( + !mid_before.trim().is_empty(), + "the machine-id read before the reboot is empty, so a regenerated one cannot be told apart" + ); // `reboot` goes through systemd, which starts fcvm-reboot-notify.service // (WantedBy=reboot.target) -> fc-agent --notify-reboot -> host relaunches @@ -45,8 +75,7 @@ async fn reboot_and_assert_relaunch(pid: u32, token: &str) -> Result<()> { "fcvm process (pid {pid}) must stay alive across an in-place reboot" ); if let Ok(mid) = common::exec_in_vm(pid, &["cat", "/etc/machine-id"]).await { - let mid = mid.trim().to_string(); - if !mid.is_empty() && mid != mid_before.trim() { + if machine_id_regenerated(&mid_before, &mid) { recovered = true; break; } @@ -216,7 +245,11 @@ async fn test_vm_reboot_preserves_disk_dir_writes() -> Result<()> { // Reboot; wait for the relaunch (machine-id change is the positive witness). let mid_before = common::exec_in_vm(pid, &["cat", "/etc/machine-id"]) .await - .unwrap_or_default(); + .context("reading the machine-id before the reboot")?; + anyhow::ensure!( + !mid_before.trim().is_empty(), + "the machine-id read before the reboot is empty, so a regenerated one cannot be told apart" + ); let _ = common::exec_in_vm(pid, &["reboot"]).await; let deadline = Instant::now() + Duration::from_secs(150); loop { @@ -225,7 +258,7 @@ async fn test_vm_reboot_preserves_disk_dir_writes() -> Result<()> { "fcvm process must stay alive across the reboot" ); if let Ok(mid) = common::exec_in_vm(pid, &["cat", "/etc/machine-id"]).await { - if !mid.trim().is_empty() && mid.trim() != mid_before.trim() { + if machine_id_regenerated(&mid_before, &mid) { break; } }