From f42e400eba57b55dbe0987b1f36055dec8928eb5 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 4 Mar 2026 21:49:21 +0000 Subject: [PATCH 1/2] credentials: Fix local-fs.target references missed by remote-fs.target migration Commit be88173 ("credentials: Use remote-fs.target for virtiofs mount ordering") changed the mount unit generator to use Before=remote-fs.target and updated the libvirt code path dropin, but missed three other locations: - run_ephemeral.rs SMBIOS dropin: still created a dropin targeting local-fs.target~bcvk-mounts instead of remote-fs.target~bcvk-mounts - run_ephemeral.rs inject_systemd_units(): still created local-fs.target.wants/ directory and symlinks there - bcvk-qemu smbios_creds_for_mount_unit(): still referenced local-fs.target in the dropin credential This inconsistency meant that the ephemeral code path would create mount units ordered Before=remote-fs.target but activated via a local-fs.target dropin, which is contradictory: remote-fs.target orders After=local-fs.target, so a unit that says Before=remote-fs.target should not be pulled in by local-fs.target. Assisted-by: OpenCode (Claude claude-opus-4-6) Signed-off-by: Colin Walters --- crates/bcvk-qemu/src/credentials.rs | 6 +++--- crates/kit/src/run_ephemeral.rs | 16 +++++++++------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/crates/bcvk-qemu/src/credentials.rs b/crates/bcvk-qemu/src/credentials.rs index 66b0a2c..385f7bb 100644 --- a/crates/bcvk-qemu/src/credentials.rs +++ b/crates/bcvk-qemu/src/credentials.rs @@ -80,7 +80,7 @@ pub fn generate_virtiofs_mount_unit( /// /// Creates systemd credentials for: /// 1. The mount unit itself (via systemd.extra-unit) -/// 2. A dropin for local-fs.target that wants this mount unit +/// 2. A dropin for remote-fs.target that wants this mount unit /// /// Returns a vector of SMBIOS credential strings. pub fn smbios_creds_for_mount_unit( @@ -95,14 +95,14 @@ pub fn smbios_creds_for_mount_unit( let mount_cred = format!("io.systemd.credential.binary:systemd.extra-unit.{unit_name}={encoded_mount}"); - // Create a dropin for local-fs.target that wants this mount + // Create a dropin for remote-fs.target that wants this mount let dropin_content = format!( "[Unit]\n\ Wants={unit_name}\n" ); let encoded_dropin = data_encoding::BASE64.encode(dropin_content.as_bytes()); let dropin_cred = format!( - "io.systemd.credential.binary:systemd.unit-dropin.local-fs.target~bcvk-mounts={encoded_dropin}" + "io.systemd.credential.binary:systemd.unit-dropin.remote-fs.target~bcvk-mounts={encoded_dropin}" ); Ok(vec![mount_cred, dropin_cred]) diff --git a/crates/kit/src/run_ephemeral.rs b/crates/kit/src/run_ephemeral.rs index 0bef58c..3957261 100644 --- a/crates/kit/src/run_ephemeral.rs +++ b/crates/kit/src/run_ephemeral.rs @@ -728,7 +728,7 @@ pub(crate) fn process_disk_files( } /// Copy systemd units from /run/systemd-units/system/ to container image /etc/systemd/system/. -/// Auto-enables .mount units in local-fs.target.wants/, preserves default.target.wants/ symlinks. +/// Auto-enables .mount units in remote-fs.target.wants/, preserves default.target.wants/ symlinks. fn inject_systemd_units() -> Result<()> { use std::fs; @@ -744,7 +744,7 @@ fn inject_systemd_units() -> Result<()> { // Create target directories fs::create_dir_all(target_units)?; fs::create_dir_all(&format!("{}/default.target.wants", target_units))?; - fs::create_dir_all(&format!("{}/local-fs.target.wants", target_units))?; + fs::create_dir_all(&format!("{}/remote-fs.target.wants", target_units))?; // Copy all .service and .mount files for entry in fs::read_dir(source_units)? { @@ -759,7 +759,7 @@ fn inject_systemd_units() -> Result<()> { // Create symlinks for mount units to enable them if extension.as_deref() == Some("mount") { - let wants_dir = format!("{}/local-fs.target.wants", target_units); + let wants_dir = format!("{}/remote-fs.target.wants", target_units); let symlink_path = format!("{}/{}", wants_dir, filename); let relative_target = format!("../{}", filename); std::os::unix::fs::symlink(&relative_target, &symlink_path).ok(); @@ -1062,7 +1062,7 @@ pub(crate) async fn run_impl(opts: RunEphemeralOpts) -> Result<()> { ); mount_unit_smbios_creds.push(mount_cred); - // Collect unit name for the local-fs.target dropin + // Collect unit name for the remote-fs.target dropin mount_unit_names.push(unit_name.clone()); debug!( @@ -1072,17 +1072,19 @@ pub(crate) async fn run_impl(opts: RunEphemeralOpts) -> Result<()> { } } - // If we have mount units, create a single dropin for local-fs.target + // If we have mount units, create a single dropin for remote-fs.target. + // We use remote-fs.target because virtiofs is conceptually similar to a remote + // filesystem - it requires virtio transport infrastructure, like NFS needs network. if !mount_unit_names.is_empty() { let wants_list = mount_unit_names.join(" "); let dropin_content = format!("[Unit]\nWants={}\n", wants_list); let encoded_dropin = data_encoding::BASE64.encode(dropin_content.as_bytes()); let dropin_cred = format!( - "io.systemd.credential.binary:systemd.unit-dropin.local-fs.target~bcvk-mounts={encoded_dropin}" + "io.systemd.credential.binary:systemd.unit-dropin.remote-fs.target~bcvk-mounts={encoded_dropin}" ); mount_unit_smbios_creds.push(dropin_cred); debug!( - "Created local-fs.target dropin for {} mount units", + "Created remote-fs.target dropin for {} mount units", mount_unit_names.len() ); } From 6f9a90f0ef2718a3af240147ec29839b44b2e468 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 4 Mar 2026 23:54:24 +0000 Subject: [PATCH 2/2] tests: Add systemd health checks using structured journal queries Add two new integration tests for systemd health: test_systemd_health_cross_distro (parameterized, runs per image): Queries the guest journal via SSH for "Failed with result" and "ordering cycle" messages using journalctl -o json, parsed with serde. This catches regressions where mount unit ordering changes (like the local-fs/remote-fs migration) cause units to fail or create dependency cycles. test_run_ephemeral_detect_ordering_cycle (negative test): Injects two units with a deliberate mutual Before= cycle via --systemd-units, boots the VM, and verifies the "ordering cycle" diagnostic appears in the journal. This ensures the health check is not passing vacuously. Assisted-by: OpenCode (Claude claude-opus-4-6) Signed-off-by: Colin Walters --- .../src/tests/run_ephemeral.rs | 84 ++++++++++++++++++ .../src/tests/run_ephemeral_ssh.rs | 88 +++++++++++++++++++ 2 files changed, 172 insertions(+) diff --git a/crates/integration-tests/src/tests/run_ephemeral.rs b/crates/integration-tests/src/tests/run_ephemeral.rs index 997a37e..221ec5a 100644 --- a/crates/integration-tests/src/tests/run_ephemeral.rs +++ b/crates/integration-tests/src/tests/run_ephemeral.rs @@ -18,6 +18,10 @@ use color_eyre::Result; use integration_tests::integration_test; use xshell::cmd; +use std::fs; +use tempfile::TempDir; + +use camino::Utf8Path; use tracing::debug; use crate::{get_bck_command, get_test_image, shell, INTEGRATION_TEST_LABEL}; @@ -458,3 +462,83 @@ fn test_run_ephemeral_mount_layout() -> Result<()> { Ok(()) } integration_test!(test_run_ephemeral_mount_layout); + +/// Verify that systemd ordering cycle detection actually works by injecting +/// a deliberate cycle: unit A Before=B, unit B Before=A. +/// +/// We inject the units via --systemd-units with default.target.wants/ +/// (which inject_systemd_units() knows how to copy), let the system boot +/// normally, then use --execute to check the journal for the expected +/// "ordering cycle" diagnostic. +fn test_run_ephemeral_detect_ordering_cycle() -> Result<()> { + let sh = shell()?; + let bck = get_bck_command()?; + let label = INTEGRATION_TEST_LABEL; + let image = get_test_image(); + + let units_dir = TempDir::new()?; + let units_dir_path = Utf8Path::from_path(units_dir.path()).expect("temp dir is not utf8"); + let system_dir = units_dir_path.join("system"); + fs::create_dir(&system_dir)?; + + // Create a cycle: A wants and orders before B, B wants and orders before A. + // Both Wants= ensure both units are in the transaction; the mutual + // Before= constraints form the actual ordering cycle. + fs::write( + system_dir.join("cycle-a.service"), + "[Unit]\n\ + Description=Cycle test unit A\n\ + Wants=cycle-b.service\n\ + Before=cycle-b.service\n\ + [Service]\n\ + Type=oneshot\n\ + ExecStart=/bin/true\n\ + RemainAfterExit=yes\n", + )?; + fs::write( + system_dir.join("cycle-b.service"), + "[Unit]\n\ + Description=Cycle test unit B\n\ + Wants=cycle-a.service\n\ + Before=cycle-a.service\n\ + [Service]\n\ + Type=oneshot\n\ + ExecStart=/bin/true\n\ + RemainAfterExit=yes\n", + )?; + + // Enable via default.target.wants/ which inject_systemd_units() copies + let wants_dir = system_dir.join("default.target.wants"); + fs::create_dir(&wants_dir)?; + std::os::unix::fs::symlink("../cycle-a.service", wants_dir.join("cycle-a.service"))?; + + // Use --execute to query the journal in JSON format for ordering cycle + // messages after the system boots. journalctl -g exits non-zero when no + // matches are found, so we ignore the exit status. + let check_script = "journalctl -b --no-pager -o json -g 'ordering cycle'"; + + let stdout = cmd!( + sh, + "{bck} ephemeral run --rm --label {label} --execute {check_script} --systemd-units {units_dir_path} {image}" + ) + .ignore_status() + .read()?; + + // Parse JSON lines and look for cycle messages + let has_cycle = stdout.lines().any(|line| { + serde_json::from_str::(line) + .ok() + .and_then(|v| v.get("MESSAGE")?.as_str().map(String::from)) + .is_some_and(|msg| msg.contains("ordering cycle")) + }); + + assert!( + has_cycle, + "Expected ordering cycle to be detected for deliberately cyclic units. \ + Output: {}", + stdout + ); + + Ok(()) +} +integration_test!(test_run_ephemeral_detect_ordering_cycle); diff --git a/crates/integration-tests/src/tests/run_ephemeral_ssh.rs b/crates/integration-tests/src/tests/run_ephemeral_ssh.rs index eeebdd7..635417b 100644 --- a/crates/integration-tests/src/tests/run_ephemeral_ssh.rs +++ b/crates/integration-tests/src/tests/run_ephemeral_ssh.rs @@ -414,3 +414,91 @@ fn test_run_ephemeral_ssh_timeout() -> Result<()> { Ok(()) } integration_test!(test_run_ephemeral_ssh_timeout); + +/// A journal entry from `journalctl -o json`. +/// +/// We only deserialize the fields we care about; serde ignores the rest. +#[derive(serde::Deserialize)] +struct JournalEntry { + #[serde(rename = "MESSAGE", default)] + message: String, + #[serde(rename = "UNIT", default)] + unit: Option, +} + +/// Parse `journalctl -o json` output (one JSON object per line). +fn parse_journal_entries(output: &str) -> Vec { + output + .lines() + .filter_map(|line| serde_json::from_str::(line).ok()) + .collect() +} + +/// Test systemd health across all configured test images +/// +/// Queries the guest journal directly (via SSH) for well-known systemd +/// messages that indicate problems: +/// - "Failed with result" — a unit entered failed state +/// - "ordering cycle" — conflicting Before=/After= dependencies +/// +/// Uses `journalctl -o json` for structured output parsed with serde, +/// avoiding brittle text parsing of human-readable journal formats. +fn test_systemd_health_cross_distro(image: &str) -> Result<()> { + let sh = shell()?; + let bck = get_bck_command()?; + let label = INTEGRATION_TEST_LABEL; + + // Query journal for unit failures and ordering cycles in a single SSH call. + // Using -o json gives us one JSON object per line, which we parse with serde. + // journalctl -g exits non-zero when no matches are found, so we ignore the + // exit status and just parse whatever stdout we get (empty = no matches = pass). + let check_script = "journalctl -b --no-pager -o json -g 'Failed with result|ordering cycle'"; + + let stdout = cmd!( + sh, + "{bck} ephemeral run-ssh --label {label} {image} -- {check_script}" + ) + .ignore_status() + .read()?; + + let entries = parse_journal_entries(&stdout); + + let failures: Vec<&JournalEntry> = entries + .iter() + .filter(|e| e.message.contains("Failed with result")) + .collect(); + + let cycles: Vec<&JournalEntry> = entries + .iter() + .filter(|e| e.message.contains("ordering cycle")) + .collect(); + + assert!( + failures.is_empty(), + "Found failed systemd unit(s) on image {}:\n{}", + image, + failures + .iter() + .map(|e| format!( + " {}: {}", + e.unit.as_deref().unwrap_or(""), + e.message + )) + .collect::>() + .join("\n") + ); + + assert!( + cycles.is_empty(), + "Found systemd ordering cycle(s) on image {}:\n{}", + image, + cycles + .iter() + .map(|e| format!(" {}", e.message)) + .collect::>() + .join("\n") + ); + + Ok(()) +} +parameterized_integration_test!(test_systemd_health_cross_distro);