diff --git a/doc/run.md b/doc/run.md index 4f5c3c4ad..23e5f1de3 100644 --- a/doc/run.md +++ b/doc/run.md @@ -1528,8 +1528,14 @@ of one sandbox take effect one at a time: each waits up to `--timeout` seconds for any other in progress, and of two overlapping starts, exactly one launches OpenVMM while the other then fails because the sandbox is already running. `exec` holds the lock only while it finds the running OpenVMM, so a long -workload does not delay `stop`. A provisioned state directory keeps -`sandbox.lock`, and `deprovision` removes it with NVX's other files. A +workload does not delay `stop`. One `exec` at a time runs in a sandbox, across +processes: an `exec` takes the sandbox's exec lock, `exec.lock` in the state +directory, before it finds the running OpenVMM, and holds it until the workload +ends. An `exec` that finds it held tries again until `--wait` seconds, 60 by +default, have passed, and then fails with `sandbox-busy`. `stop` takes no exec +lock, so it proceeds while a workload runs. A provisioned state directory keeps +`sandbox.lock`, and `exec.lock` from its first `exec` on, and `deprovision` +removes them with NVX's other files. A transition that leaves a state directory without `config.json`, such as one of a sandbox that was never provisioned, removes `sandbox.lock` as it releases the lock. These are the rules of the sandbox store, which other orchestrators of @@ -1633,7 +1639,8 @@ template, so it has the same artifact requirements. It checks that: capability, control socket, report staging file, or OpenVMM process; - every operation fails on a state directory that was never provisioned, a repeated `provision` leaves the configuration unchanged, and `exec` and - `stop` fail before `start`; + `stop` fail before `start`, though the `exec` leaves the `exec.lock` that the + state directory keeps from then on; - a repeated `start` and a `deprovision` of a running sandbox fail without changing its runtime record, capability, or OpenVMM process, and the sandbox keeps serving requests; @@ -1649,7 +1656,7 @@ template, so it has the same artifact requirements. It checks that: `--outcome-report` path exists fails before its workload runs and leaves the file intact; - `stop` ends OpenVMM and its control socket or pipe, leaves only - `config.json`, `sandbox.lock`, `openvmm.log`, and a successful + `config.json`, `sandbox.lock`, `exec.lock`, `openvmm.log`, and a successful `outcome.json`, and leaves a cleanly unmounted scratch file system, after which `exec` and `stop` fail; - two overlapping `start` calls then launch a single OpenVMM process: one @@ -1662,12 +1669,13 @@ template, so it has the same artifact requirements. It checks that: changing the runtime record or OpenVMM process, while the checkout's CLI still serves requests. Its `stop` then terminates OpenVMM without a guest shutdown and warns that it did, and leaves only `config.json`, - `sandbox.lock`, and `openvmm.log`, without a control socket or pipe, after - which its `stop` finds the sandbox not running, and the sandbox starts again; + `sandbox.lock`, `exec.lock`, and `openvmm.log`, without a control socket or + pipe, after which its `stop` finds the sandbox not running, and the sandbox + starts again; - after OpenVMM exits without a `stop`, `exec` reports the stale runtime state once and clears it, after which `stop` finds the sandbox not running, and the - state directory holds only `config.json`, `sandbox.lock`, and `openvmm.log`, - without a control socket or pipe; and + state directory holds only `config.json`, `sandbox.lock`, `exec.lock`, and + `openvmm.log`, without a control socket or pipe; and - `deprovision` removes only NVX's files: it fails and keeps a foreign file in the state directory, and once that file is gone, it removes the directory, which a later `exec` does not recreate. diff --git a/doc/usage.md b/doc/usage.md index 1d811cbb5..79024eb55 100644 --- a/doc/usage.md +++ b/doc/usage.md @@ -99,7 +99,7 @@ each code for callers of the `nvx` library. | `sandbox-not-running` | 1 | `exec` or `stop` found the VM of a provisioned sandbox not running. | | `sandbox-running` | 1 | `start` or `deprovision` found the sandbox's VM, or a launch of it, running. | | `sandbox-exited` | 1 | The sandbox's OpenVMM exited without a `stop`: the transition that finds it reports it and clears its runtime record, and `start` launches the sandbox again. | -| `sandbox-busy` | 1 | Another lifecycle transition held the sandbox's lock until `--timeout` passed. | +| `sandbox-busy` | 1 | Another lifecycle transition held the sandbox's lock until `--timeout` passed, or another `exec` held its exec lock until `--wait` passed. | | `runtime-mismatch` | 1 | The sandbox's VM runs under another runtime identity than the artifacts', whose differing parts the message names, so only `stop`, which terminates it, applies. | | `vmm-indeterminate` | 1 | Whether the sandbox's OpenVMM runs cannot be determined, so its records remain for a later attempt. | | `format-unsupported` | 1 | A record or report has a format that this build does not read, such as the state of a sandbox that another build provisioned or started. | @@ -259,6 +259,7 @@ nvx-rs/target/release/nvx-rs sandbox [--workload-user UID:GID] [--memory-mib MIB] [--timeout SECONDS] + [--wait SECONDS] [--exec-timeout-ms MILLISECONDS] [--cwd GUEST_PATH] [--environment KEY=VALUE]... @@ -313,6 +314,7 @@ do, and their names cannot be abbreviated either. | `--workload-user UID:GID` | `65534:65534` | Select the fixed non-root workload identity for `run` or `provision`. Both IDs must be ASCII decimal integers from 1 to 4294967294, without signs, whitespace, or underscores. | | `--memory-mib MIB` | `256` | Set guest memory in MiB, an ASCII decimal integer from 1 to 17592186044415, without signs, whitespace, or underscores. | | `--timeout SECONDS` | `60` | Set the timeout for each managed control request and its response in `start`, `exec`, and `stop`, and the longest time that `provision`, `start`, `stop`, and `deprovision` wait for another lifecycle transition of the sandbox. It is a decimal number, with an optional fraction and exponent, that must be finite and greater than 0. | +| `--wait SECONDS` | `60` | Set the longest time that a managed `exec` waits for the `exec` that another `nvx-rs` process runs in the sandbox to end, since a sandbox runs one `exec` at a time; it then fails with `sandbox-busy`. `stop` never waits for an `exec`. Like `--timeout`, which still bounds each wait for another lifecycle transition, it is a decimal number that must be finite and greater than 0. | | `--exec-timeout-ms MILLISECONDS` | `0` | Set the managed `exec` guest workload timeout to an ASCII decimal integer from 0 to 4294967295, without signs, whitespace, or underscores; zero disables the workload deadline. This is separate from the finite host `--timeout` control deadline. | | `--cwd GUEST_PATH` | `/` | Set an absolute working directory inside the workload root for managed `exec`. It is resolved after the workload identity and root are applied; missing, inaccessible, or non-directory paths fail the workload launch. Default and layered environments point `PWD` at this directory; an exact replacement environment controls `PWD` itself. | | `--environment KEY=VALUE` | omitted | Set the exact managed `exec` environment. Repeat for multiple entries. Empty values, spaces, additional equals signs, and UTF-8 are preserved. Inline values are visible in the invoking host process arguments; use `--environment-file` for sensitive values. | diff --git a/nvx-rs/README.md b/nvx-rs/README.md index bcdccb7ee..daa22ce03 100644 --- a/nvx-rs/README.md +++ b/nvx-rs/README.md @@ -2869,6 +2869,7 @@ them: | --- | --- | | `config.json` | The provisioned sandbox, in format 9 only: `image` holds its format, path and identity; `upper` is tagged as scratch with its path or RAM with `max_memory_percent`. `distro_binding` binds the image, and `scratch_binding` binds scratch or is `null` for RAM. The export and network are typed, `null` when absent. An atomic pretty-JSON write uses mode 0600, and start reads the same strict typed fields without a legacy reader. | | `sandbox.lock` | The store's lock, which serializes the lifecycle transitions | +| `exec.lock` | The exec lock, which an `exec` holds from before it finds the running VM until its workload ends, so that one `exec` at a time runs in the sandbox; it stays from the first `exec` until `deprovision` | | `launch.json` | The store's journal of a start in progress, or of one that was interrupted | | `runtime.json` | The store's runtime record of the running VM, in its format 4: the OpenVMM process, by its ID, its start time, and the host's boot, its control endpoint, `STATE/control.sock` or `//./pipe/openvmm-microvm-HEX` on Windows, and the [runtime identity](#runtime-identity) of its start | | `control.capability` | The 32-byte capability of the running OpenVMM, with mode 0600 | @@ -2878,7 +2879,10 @@ them: `provision`, `start`, `stop`, and `deprovision` hold the store's lock for their whole transition, and `exec` only while it finds the running VM, so that a long -workload cannot delay a `stop`. The CLI takes the store's +workload cannot delay a `stop`. An `exec` that finds another holding +`exec.lock` releases the store's lock and tries again until `--wait` seconds, +60 by default, have passed, and then fails with `sandbox-busy`; `stop` takes no +exec lock. The CLI takes the store's [lifecycle locks](src/host/store/mod.rs) with `config.json` as its configuration record: a transition that leaves the state directory without it, such as one of a sandbox that was never provisioned, removes the lock file as @@ -3041,7 +3045,9 @@ guest agents of NVX advertise bits 0, 4, and 6 in sandbox mode. - The state directory holds the store's files: `sandbox.lock` replaces the `lifecycle.lock` of `nvx.py`, `runtime.json` has the store's format, and each start replaces `openvmm.log`, which `nvx.py` appended to. `exec` holds the - lock while it finds the running VM, which `nvx.py` did not. A runtime record + lock while it finds the running VM, which `nvx.py` did not, and holds + `exec.lock` until its workload ends, so that one `exec` at a time runs in a + sandbox, where `nvx.py` ran them at once and had no `--wait`. A runtime record of an OpenVMM that exited is cleared by the first transition that finds it, so that `start` launches the sandbox again, where `nvx.py` refused to. - `provision` refuses an existing state directory that is not private to the @@ -3300,7 +3306,7 @@ usage errors, which no library error has. | `sandbox-not-running` | `Other` | The operation needs the sandbox's VM, which is not running | | `sandbox-running` | `Other` | The operation needs a stopped sandbox, but its VM, or a launch of it, may still run | | `sandbox-exited` | `Exited` | The sandbox's VM exited without a stop: the transition that finds it clears its runtime record, and a start launches it again | -| `sandbox-busy` | `TimedOut` | Another lifecycle transition of the sandbox held its lock until the deadline passed | +| `sandbox-busy` | `TimedOut` | Another lifecycle transition, or `exec`, of the sandbox held its lock until the deadline passed | | `runtime-mismatch` | `RuntimeMismatch` | The sandbox's VM runs under another runtime identity than the artifacts', whose differing parts the message names, so only a stop, which terminates it, applies | | `vmm-indeterminate` | `Other` | Whether the OpenVMM process of the sandbox runs cannot be determined, so its records remain for a later attempt | | `format-unsupported` | `Io` | A record or report has a format or schema version that this build does not read, such as the state of a sandbox that another build provisioned or started | diff --git a/nvx-rs/src/error.rs b/nvx-rs/src/error.rs index e98ae8830..5637f62f6 100644 --- a/nvx-rs/src/error.rs +++ b/nvx-rs/src/error.rs @@ -132,8 +132,8 @@ pub enum ErrorCode { SandboxRunning, /// `sandbox-exited`: the sandbox's VM exited without a stop. A start launches it again. SandboxExited, - /// `sandbox-busy`: another lifecycle transition of the sandbox held its lock until the - /// deadline passed. + /// `sandbox-busy`: another lifecycle transition, or exec, of the sandbox held its lock until + /// the deadline passed. SandboxBusy, /// `runtime-mismatch`: the sandbox's VM runs under another runtime identity than the /// artifacts', so it is not driven, and only a stop, which terminates it, applies. diff --git a/nvx-rs/src/host/cli/exec_lock.rs b/nvx-rs/src/host/cli/exec_lock.rs new file mode 100644 index 000000000..b73432e62 --- /dev/null +++ b/nvx-rs/src/host/cli/exec_lock.rs @@ -0,0 +1,57 @@ +//! The exec lock of a sandbox, which runs one managed exec at a time in it, across processes. +//! +//! An exec holds the OS lock of `exec.lock` in the sandbox's state directory from before it finds +//! the running VM until its workload ends. A release leaves the file, which `deprovision` removes +//! with the store's files, so that no release unlinks a file that another exec has open. + +use std::fs::{File, TryLockError}; + +use super::store::{LockedState, open_lock}; +use super::{Exit, Result, check_interrupt}; +use crate::ErrorCode; +use crate::host::platform; + +/// The name of the exec lock in a sandbox's state directory. +pub(crate) const NAME: &str = "exec.lock"; + +/// The held exec lock of a sandbox, which dropping it releases. +#[derive(Debug)] +pub(crate) struct ExecLock { + file: File, +} + +impl ExecLock { + /// Tries the exec lock of the sandbox whose lifecycle lock `state` holds, without waiting, and + /// returns `None` while another exec holds it. The lock file is created and checked as the + /// store's own lock file is, and refused once its entry no longer names the locked file. + pub(crate) fn try_acquire(state: &LockedState) -> Result> { + check_interrupt()?; + let directory = state.directory()?; + let guard = Self { + file: open_lock(&state.dir().join(NAME))?, + }; + match guard.file.try_lock() { + Ok(()) => {} + Err(TryLockError::WouldBlock) => return Ok(None), + Err(TryLockError::Error(error)) => return Err(Exit::from_os_error(error)), + } + let held = platform::opened_file_id(&guard.file).map_err(Exit::from_os_error)?; + if directory.entry_id(NAME).map_err(Exit::from_os_error)? != Some(held) { + return Err(Exit::error( + ErrorCode::Io, + "sandbox exec lock was removed or replaced", + )); + } + Ok(Some(guard)) + } +} + +impl Drop for ExecLock { + fn drop(&mut self) { + // Unlock explicitly so a concurrently forked child's descriptor cannot extend admission. + let _ = self.file.unlock(); + } +} + +#[cfg(test)] +mod tests; diff --git a/nvx-rs/src/host/cli/exec_lock/tests.rs b/nvx-rs/src/host/cli/exec_lock/tests.rs new file mode 100644 index 000000000..5ffe61f77 --- /dev/null +++ b/nvx-rs/src/host/cli/exec_lock/tests.rs @@ -0,0 +1,164 @@ +use super::*; +use crate::host::cli::deadline::Deadline; +use crate::host::cli::store::{StateDir, StateFile}; +use crate::host::platform; +use std::fs; + +fn state() -> (tempfile::TempDir, StateDir) { + let directory = tempfile::tempdir().unwrap(); + let state = StateDir::create(&directory.path().join("state")).unwrap(); + (directory, state) +} + +fn admit(state: &StateDir) -> Result> { + let locked = StateDir::open(state.dir())?.lock(Deadline::after(1.0))?; + ExecLock::try_acquire(&locked) +} + +#[test] +fn admission_is_private_nonblocking_and_keeps_its_file_on_release() { + let (_directory, state) = state(); + let held = admit(&state).unwrap().unwrap(); + let path = state.dir().join(NAME); + let identity = platform::file_id(&path).unwrap(); + assert_eq!(platform::private_file_problem(&held.file).unwrap(), None); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + + assert_eq!( + fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o600 + ); + } + assert!(admit(&state).unwrap().is_none()); + let other = StateDir::create(&_directory.path().join("other")).unwrap(); + drop(admit(&other).unwrap().unwrap()); + drop(held); + let _next = admit(&state).unwrap().unwrap(); + assert_eq!(platform::file_id(&path).unwrap(), identity); +} + +#[test] +fn a_replaced_lifecycle_lock_refuses_admission_before_file_creation() { + let (_directory, state) = state(); + let old = state.lock(Deadline::after(1.0)).unwrap(); + fs::rename(state.path(StateFile::Lock), state.dir().join("old.lock")).unwrap(); + let current = state.lock(Deadline::after(1.0)).unwrap(); + assert_eq!( + ExecLock::try_acquire(&old).unwrap_err().code(), + Some(ErrorCode::Io) + ); + assert!(!state.dir().join(NAME).exists()); + let _held = ExecLock::try_acquire(¤t).unwrap().unwrap(); + drop(old); + assert!(ExecLock::try_acquire(¤t).unwrap().is_none()); +} + +#[cfg(unix)] +#[test] +fn a_link_to_an_already_locked_exec_file_is_refused() { + let (_directory, state) = state(); + let _held = admit(&state).unwrap().unwrap(); + let path = state.dir().join(NAME); + let target = state.dir().join("old.lock"); + fs::rename(&path, &target).unwrap(); + std::os::unix::fs::symlink(&target, &path).unwrap(); + assert_eq!(admit(&state).unwrap_err().code(), Some(ErrorCode::Io)); + assert!(path.is_symlink()); +} + +#[test] +fn non_regular_exec_lock_entries_are_refused() { + let (_directory, state) = state(); + let path = state.dir().join(NAME); + fs::create_dir(&path).unwrap(); + assert_eq!(admit(&state).unwrap_err().code(), Some(ErrorCode::Io)); + assert!(path.is_dir()); + fs::remove_dir(&path).unwrap(); + #[cfg(unix)] + { + let target = state.dir().join("target"); + fs::write(&target, "keep").unwrap(); + std::os::unix::fs::symlink(&target, &path).unwrap(); + assert_eq!(admit(&state).unwrap_err().code(), Some(ErrorCode::Io)); + assert!(path.is_symlink()); + assert_eq!(fs::read_to_string(target).unwrap(), "keep"); + } + #[cfg(windows)] + { + use std::os::windows::process::CommandExt; + use std::process::{Command, Stdio}; + + let target = state.dir().join("target"); + fs::create_dir(&target).unwrap(); + let mut command = Command::new("cmd"); + command + .raw_arg(format!( + "/C mklink /J \"{}\" \"{}\"", + path.display(), + target.display() + )) + .stdout(Stdio::null()); + let child = { + let _spawn = platform::inherit::lock_spawns(); + command.spawn().unwrap() + }; + assert!(child.wait_with_output().unwrap().status.success()); + assert_eq!(admit(&state).unwrap_err().code(), Some(ErrorCode::Io)); + assert_eq!(fs::read_dir(target).unwrap().count(), 0); + } +} + +#[cfg(windows)] +#[test] +fn exec_lock_files_accessible_to_other_users_are_refused_without_restriction() { + use std::process::{Command, Stdio}; + + let (_directory, state) = state(); + drop(admit(&state).unwrap().unwrap()); + let path = state.dir().join(NAME); + let mut command = Command::new("icacls"); + command + .arg(&path) + .args(["/grant", "*S-1-1-0:(F)"]) + .stdout(Stdio::null()); + let child = { + let _spawn = platform::inherit::lock_spawns(); + command.spawn().unwrap() + }; + assert!(child.wait_with_output().unwrap().status.success()); + let problem = platform::private_file_problem(&File::open(&path).unwrap()).unwrap(); + assert!(problem.is_some()); + assert_eq!(admit(&state).unwrap_err().code(), Some(ErrorCode::Io)); + assert_eq!( + platform::private_file_problem(&File::open(&path).unwrap()).unwrap(), + problem + ); +} + +#[cfg(target_os = "linux")] +#[test] +fn private_directories_protect_exec_locks_without_changing_the_store_file_policy() { + use std::os::unix::fs::PermissionsExt; + + let (_directory, state) = state(); + drop(admit(&state).unwrap().unwrap()); + let path = state.dir().join(NAME); + fs::set_permissions(&path, fs::Permissions::from_mode(0o644)).unwrap(); + drop(admit(&state).unwrap().unwrap()); + assert_eq!( + fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o644 + ); + fs::set_permissions(state.dir(), fs::Permissions::from_mode(0o755)).unwrap(); + assert_eq!(admit(&state).unwrap_err().code(), Some(ErrorCode::Io)); + assert_eq!( + fs::metadata(state.dir()).unwrap().permissions().mode() & 0o777, + 0o755 + ); + assert_eq!( + fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o644 + ); +} diff --git a/nvx-rs/src/host/cli/mod.rs b/nvx-rs/src/host/cli/mod.rs index 0ded2ec87..69580d2e9 100644 --- a/nvx-rs/src/host/cli/mod.rs +++ b/nvx-rs/src/host/cli/mod.rs @@ -42,6 +42,7 @@ mod cpu_profile; mod deadline; mod download; mod egress; +mod exec_lock; mod json; mod network; mod output; diff --git a/nvx-rs/src/host/cli/sandbox/lifecycle/start.rs b/nvx-rs/src/host/cli/sandbox/lifecycle/start.rs index 34259d6fd..ad76247b3 100644 --- a/nvx-rs/src/host/cli/sandbox/lifecycle/start.rs +++ b/nvx-rs/src/host/cli/sandbox/lifecycle/start.rs @@ -10,7 +10,8 @@ use super::{ CONFIG_NAME, Found, REQUIRED_FEATURES, connect, find_vm, mismatched, require_config, running_vm, runtime_for, }; -use crate::host::cli::deadline::Deadline; +use crate::host::cli::deadline::{Deadline, RETRY_INTERVAL}; +use crate::host::cli::exec_lock::ExecLock; use crate::host::cli::sandbox::lock::LifecycleLock; use crate::host::cli::sandbox::state::{Config, read_config}; use crate::host::cli::sandbox::{Artifacts, error_with_cause}; @@ -208,18 +209,43 @@ fn check_ready(saved: &SavedVm, runtime: &Runtime, timeout: f64) -> Result, request: &ExecRequest, response_timeout: f64, + wait: f64, ) -> Result { + let busy = || { + Exit::error( + ErrorCode::SandboxBusy, + "another exec in the sandbox is still in progress", + ) + }; let state_dir = prepare_state_directory(state_path, false)?; - let running = { + let deadline = Deadline::after(wait); + let mut retry = false; + let (_exec, running) = loop { let mut lock = LifecycleLock::acquire(&state_dir, response_timeout, false)?; - running_vm(&state_dir, &mut lock, artifacts)? + require_config(&state_dir)?; + if retry && deadline.passed() { + return Err(busy()); + } + if let Some(exec) = ExecLock::try_acquire(lock.locked())? { + let running = running_vm(&state_dir, &mut lock, artifacts)?; + break (exec, running); + } + drop(lock); + retry = true; + if !deadline.pause(RETRY_INTERVAL)? { + return Err(busy()); + } }; let mut session = connect(&running.saved, artifacts, response_timeout)?; session.exec(request, response_timeout) diff --git a/nvx-rs/src/host/cli/sandbox/lock.rs b/nvx-rs/src/host/cli/sandbox/lock.rs index 8ba6e4d85..36716e39f 100644 --- a/nvx-rs/src/host/cli/sandbox/lock.rs +++ b/nvx-rs/src/host/cli/sandbox/lock.rs @@ -4,6 +4,7 @@ use super::error_with_cause; use super::lifecycle::{CONFIG_NAME, LOCK_RETRY_INTERVAL, create_state_directory}; use crate::ErrorCode; use crate::host::cli::deadline::Deadline; +use crate::host::cli::exec_lock; use crate::host::cli::store::{LockedState, StateDir, StateFile}; use crate::host::cli::{Exit, Result, io_error}; use crate::host::platform; @@ -162,6 +163,9 @@ impl LifecycleLock { .filter(|name| !matches!(name, StateFile::Lock | StateFile::Config)) .chain([StateFile::Config]) .collect(); + directory + .remove_file(exec_lock::NAME) + .map_err(|error| io_error(ErrorCode::Io, &error, &state_dir.join(exec_lock::NAME)))?; self.remove_files(&names)?; let foreign = remaining(&state_dir)?; if !foreign.is_empty() { @@ -235,7 +239,11 @@ fn remaining(path: &Path) -> Result> { ))); } }; - (name != StateFile::Lock.name() && !name.starts_with(".nfs")).then_some(Ok(name)) + // On a file system without POSIX deletion, the removed exec lock of an exec that is + // still ending stays under its name until that exec closes it. + let gone = name == exec_lock::NAME && platform::is_deleted(&path.join(&name)); + (name != StateFile::Lock.name() && !gone && !name.starts_with(".nfs")) + .then_some(Ok(name)) }) .collect() } diff --git a/nvx-rs/src/host/cli/sandbox/lock/tests.rs b/nvx-rs/src/host/cli/sandbox/lock/tests.rs index 7c1c6c6db..0ffb1b80e 100644 --- a/nvx-rs/src/host/cli/sandbox/lock/tests.rs +++ b/nvx-rs/src/host/cli/sandbox/lock/tests.rs @@ -115,6 +115,16 @@ fn removals_refuse_live_records_and_keep_foreign_files() { assert!(state.join(CONFIG_NAME).exists()); std::fs::remove_file(path).unwrap(); } + std::fs::write(state.join("exec.lock"), "owned").unwrap(); + let foreign_exec_names = [ + "exec.lock.foreign", + ".exec.lock.42.tmp", + ".exec.lock.x.tmp", + ".exec.lock.42.tmp.more", + ]; + for name in foreign_exec_names { + std::fs::write(state.join(name), "keep").unwrap(); + } std::fs::write(state.join("foreign.txt"), "keep").unwrap(); let lock = LifecycleLock::acquire(&state, 10.0, false).unwrap(); let error = lock.remove(10.0).unwrap_err(); @@ -123,6 +133,10 @@ fn removals_refuse_live_records_and_keep_foreign_files() { std::fs::read_to_string(state.join("foreign.txt")).unwrap(), "keep" ); + for name in foreign_exec_names { + assert_eq!(std::fs::read_to_string(state.join(name)).unwrap(), "keep"); + } + assert!(!state.join("exec.lock").exists()); assert!(!state.join(CONFIG_NAME).exists()); assert!(!state.join(StateFile::Lock.name()).exists()); } @@ -142,6 +156,7 @@ fn removal_cleans_owned_temporaries_without_touching_a_neighbour() { "openvmm.log", "outcome.json", "control.capability", + "exec.lock", ] { std::fs::write(state.join(name), "owned").unwrap(); } diff --git a/nvx-rs/src/host/cli/sandbox/mod.rs b/nvx-rs/src/host/cli/sandbox/mod.rs index 55cc86554..a5415cd89 100644 --- a/nvx-rs/src/host/cli/sandbox/mod.rs +++ b/nvx-rs/src/host/cli/sandbox/mod.rs @@ -284,6 +284,7 @@ pub(super) fn run(root: Option<&str>, options: Options) -> Result { inherit_default_environment: options.inherit_default_environment, }, options.timeout, + options.wait, )?; output::write_stdout(&result.stdout) .and_then(|()| output::write_stderr(&result.stderr)) diff --git a/nvx-rs/src/host/cli/sandbox/options.rs b/nvx-rs/src/host/cli/sandbox/options.rs index 61ba70714..edc637213 100644 --- a/nvx-rs/src/host/cli/sandbox/options.rs +++ b/nvx-rs/src/host/cli/sandbox/options.rs @@ -54,6 +54,8 @@ options: --timeout TIMEOUT control operation timeout, and the longest wait for another lifecycle transition of the sandbox, in seconds (default: 60) + --wait WAIT the longest wait for another exec of the sandbox, in + seconds (default: 60) --exec-timeout-ms EXEC_TIMEOUT_MS guest workload timeout in milliseconds; zero disables it @@ -146,7 +148,7 @@ fn seconds(value: &str) -> std::result::Result { /// The options of `nvx-rs sandbox`, which parse with the defaults of the former /// `nvx.py sandbox`. -pub(in crate::host::cli) const OPTIONS: [Spec; 36] = [ +pub(in crate::host::cli) const OPTIONS: [Spec; 37] = [ Spec::one("--layer"), Spec::one("--layer-format"), Spec::one("--scratch"), @@ -159,6 +161,7 @@ pub(in crate::host::cli) const OPTIONS: [Spec; 36] = [ Spec::one("--workload-user"), Spec::one("--memory-mib"), Spec::one("--timeout"), + Spec::one("--wait"), Spec::one("--exec-timeout-ms"), Spec::one("--cwd"), Spec::one("--environment"), @@ -214,6 +217,8 @@ pub(crate) struct Options { pub(crate) memory_mib: u64, /// `--timeout` in seconds, 60 by default. pub(crate) timeout: f64, + /// `--wait` in seconds, 60 by default. + pub(crate) wait: f64, /// `--exec-timeout-ms`, 0 by default. pub(crate) exec_timeout_ms: u32, /// `--cwd`. @@ -259,6 +264,7 @@ impl Default for Options { workload_user: DEFAULT_WORKLOAD_IDENTITY, memory_mib: 256, timeout: 60.0, + wait: 60.0, exec_timeout_ms: 0, cwd: None, environment: Vec::new(), @@ -321,6 +327,7 @@ impl Receiver for Options { } "--memory-mib" => self.memory_mib = integer(&text(), 1..=MAX_MEMORY_MIB)?, "--timeout" => self.timeout = seconds(&text())?, + "--wait" => self.wait = seconds(&text())?, "--exec-timeout-ms" => self.exec_timeout_ms = integer(&text(), 0..=u32::MAX)?, "--cwd" => self.cwd = Some(text()), "--environment" => self.environment.push(text()), diff --git a/nvx-rs/src/host/cli/sandbox/tests.rs b/nvx-rs/src/host/cli/sandbox/tests.rs index e2c29eb0b..76e293fe0 100644 --- a/nvx-rs/src/host/cli/sandbox/tests.rs +++ b/nvx-rs/src/host/cli/sandbox/tests.rs @@ -11,6 +11,20 @@ use crate::host::sandbox::layer::{LayerRole, SandboxLayer}; use crate::host::sandbox::mount::PolicyKind; use crate::host::sandbox::options::PolicyPathOption; +#[test] +fn exec_wait_defaults_to_sixty_seconds_and_uses_timeout_number_rules() { + assert_eq!(parse(&["exec".to_owned()]).unwrap().0.wait, 60.0); + let (options, unknown) = + parse(&["exec", "--wait", "0.05", "--timeout", "2"].map(str::to_owned)).unwrap(); + assert!(unknown.is_empty()); + assert_eq!((options.wait, options.timeout), (0.05, 2.0)); + for value in ["0", "-1", "nan", "inf", "1e400", "invalid"] { + let error = parse(&["exec".to_owned(), "--wait".to_owned(), value.to_owned()]).unwrap_err(); + assert_eq!(error.code(), Some(ErrorCode::ArgumentInvalid)); + assert!(error.message().unwrap().contains("--wait")); + } +} + #[test] fn the_upper_is_scratch_or_ram() { let upper = |value: &str| { diff --git a/nvx-rs/src/host/cli/store/locked.rs b/nvx-rs/src/host/cli/store/locked.rs index 389eefa36..d9c93d803 100644 --- a/nvx-rs/src/host/cli/store/locked.rs +++ b/nvx-rs/src/host/cli/store/locked.rs @@ -134,7 +134,7 @@ fn not_regular() -> io::Error { /// only for writing, and the open does not wait, as it would for a FIFO without a reader. The /// umask may have cleared even the current user's permission to write it, so a lock file of the /// current user that the user can open for writing no more is given its mode again. -fn open_lock(path: &Path) -> Result { +pub(crate) fn open_lock(path: &Path) -> Result { let open = || { let mut options = OpenOptions::new(); options diff --git a/nvx-rs/src/host/cli/store/mod.rs b/nvx-rs/src/host/cli/store/mod.rs index 17f392a6c..0f4f1da84 100644 --- a/nvx-rs/src/host/cli/store/mod.rs +++ b/nvx-rs/src/host/cli/store/mod.rs @@ -6,6 +6,7 @@ //! STATE/ one sandbox, private to the current user //! config.json the configuration of the provisioned sandbox //! sandbox.lock the lock that serializes its lifecycle transitions +//! exec.lock the admission lock held through each managed exec //! launch.json the journal of a start in progress, or of one that was interrupted //! runtime.json the runtime record of its running VM //! control.capability the control capability of its VM @@ -46,7 +47,7 @@ mod record; use std::io; use std::path::{Path, PathBuf}; -pub(crate) use self::locked::LockedState; +pub(crate) use self::locked::{LockedState, open_lock}; pub(crate) use self::names::StateFile; use crate::ErrorCode; use crate::host::cli::{Exit, Result}; diff --git a/nvx-rs/tests/cli.rs b/nvx-rs/tests/cli.rs index 1e6b64773..b15835b04 100644 --- a/nvx-rs/tests/cli.rs +++ b/nvx-rs/tests/cli.rs @@ -14,7 +14,8 @@ //! `nvx.py sandbox` and `nvx.py run`, and that this test replayed until microsoft/nvx#728 retired //! them. //! -//! The other tests check what `nvx-rs` adds to `nvx.py`. +//! The other tests check what `nvx-rs` adds to `nvx.py`, as those of [`exec_lock`] check that the +//! executions of a managed sandbox run one at a time, across processes. #![cfg(all( feature = "cli", feature = "testing", @@ -29,6 +30,8 @@ mod download; mod dry_run; #[path = "cli/egress.rs"] mod egress; +#[path = "cli/exec_lock.rs"] +mod exec_lock; #[path = "cli/formats.rs"] mod formats; #[path = "cli/harness.rs"] @@ -898,7 +901,7 @@ fn a_restarted_cli_drives_only_a_vm_of_its_runtime_identity() { // The terminated OpenVMM published no outcome report, and its log stays. assert_eq!( state_entries(&root, "state"), - ["config.json", "openvmm.log", "sandbox.lock"] + ["config.json", "exec.lock", "openvmm.log", "sandbox.lock"] ); assert_eq!( transition(&root, "stop", "state", &[]), @@ -1207,7 +1210,7 @@ fn a_sandbox_whose_openvmm_exited_reports_it_once_and_starts_again() { ); assert_eq!( state_entries(&root, "state"), - ["config.json", "openvmm.log", "sandbox.lock"] + ["config.json", "exec.lock", "openvmm.log", "sandbox.lock"] ); assert_eq!( transition(&root, "stop", "state", &[]), diff --git a/nvx-rs/tests/cli/exec_lock.rs b/nvx-rs/tests/cli/exec_lock.rs new file mode 100644 index 000000000..531b1153c --- /dev/null +++ b/nvx-rs/tests/cli/exec_lock.rs @@ -0,0 +1,508 @@ +//! Cross-process exec admission through the actual CLI and fake NVXC server. + +use std::fs::{self, File, TryLockError}; +use std::process::{Child, Stdio}; +use std::thread; +use std::time::{Duration, Instant}; + +use crate::harness::{LAYER, Outcome, Root}; +use crate::lifecycle::{provision, sandbox}; + +struct Pending { + child: Option, + arguments: Vec, +} + +impl Pending { + fn spawn(root: &Root, options: &[String]) -> Self { + let arguments = options + .iter() + .map(|value| root.expand(value)) + .collect::>(); + let child = root + .command(&arguments) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .unwrap(); + Self { + child: Some(child), + arguments, + } + } + + fn finish(mut self) -> Outcome { + let output = self.child.take().unwrap().wait_with_output().unwrap(); + Outcome::new(self.arguments.clone(), output) + } + + fn ended(&mut self) -> bool { + self.child.as_mut().unwrap().try_wait().unwrap().is_some() + } +} + +impl Drop for Pending { + fn drop(&mut self) { + if let Some(mut child) = self.child.take() { + let _ = child.kill(); + let _ = child.wait(); + } + } +} + +fn start(root: &Root, state: &str) { + root.nvx_rs(&sandbox( + "provision", + &format!("{{root/{state}}}"), + &["--layer", LAYER, "--upper", "ram"], + )) + .assert_silent_success(); + operate(root, "start", state, &[]).assert_silent_success(); +} + +fn operate(root: &Root, operation: &str, state: &str, options: &[&str]) -> Outcome { + root.nvx_rs(&sandbox(operation, &format!("{{root/{state}}}"), options)) +} + +fn script(root: &Root, state: &str, script: &str, options: &[&str]) -> Vec { + let mut arguments = sandbox( + "exec", + &format!("{{root/{state}}}"), + &["--entrypoint", "/bin/sh", "--arg=-c", "--arg", script], + ); + arguments.extend(options.iter().map(|value| (*value).to_owned())); + arguments.iter().map(|value| root.expand(value)).collect() +} + +#[test] +fn exec_processes_contend_but_other_sandboxes_and_stop_proceed() { + let root = Root::new(); + start(&root, "first"); + start(&root, "second"); + let first = Pending::spawn( + &root, + &script(&root, "first", "touch {root/entered}; sleep 60000", &[]), + ); + crate::wait_for(&root.below("entered")); + let admission = File::options() + .read(true) + .write(true) + .open(root.below("first/exec.lock")) + .unwrap(); + assert!(matches!( + admission.try_lock(), + Err(TryLockError::WouldBlock) + )); + let sessions = crate::sessions(&root, "first"); + let started = Instant::now(); + let refused = root.nvx_rs(&script( + &root, + "first", + "touch {root/forbidden}", + &["--wait", "0.05"], + )); + refused.assert_error("sandbox-busy", &["another exec"]); + // The refusal came after `--wait`, and long before the 60-second default, however slowly a + // loaded host starts the process. + assert!(started.elapsed() >= Duration::from_millis(50)); + assert!(started.elapsed() < Duration::from_secs(30)); + assert!(!root.below("forbidden").exists()); + assert_eq!(crate::sessions(&root, "first"), sessions); + operate( + &root, + "exec", + "second", + &[ + "--entrypoint", + "/bin/echo", + "--arg", + "independent", + "--wait", + "0.05", + ], + ) + .assert_success(); + assert!(matches!( + admission.try_lock(), + Err(TryLockError::WouldBlock) + )); + let process = crate::recorded_process(&root, "first").unwrap(); + let stop_started = Instant::now(); + let stopped = operate(&root, "stop", "first", &["--timeout", "0.1"]); + assert_eq!(stopped.status, Some(0), "{}", stopped.describe()); + assert!(stopped.stdout().is_empty(), "{}", stopped.describe()); + assert!( + stopped + .stderr() + .lines() + .all(|line| line.starts_with("warning: ")), + "{}", + stopped.describe() + ); + assert!( + stopped.stderr().contains("forcibly terminated"), + "{}", + stopped.describe() + ); + // The stop waited neither for the exec lock nor for the workload, which sleeps for a minute. + assert!(stop_started.elapsed() < Duration::from_secs(30)); + assert!( + process + .wait(Instant::now() + Duration::from_secs(10)) + .unwrap() + .is_some() + ); + let ended = first.finish(); + ended.assert_error("endpoint-closed", &[]); + admission.try_lock().unwrap(); + admission.unlock().unwrap(); + drop(admission); + operate(&root, "deprovision", "first", &[]).assert_silent_success(); + assert!(!root.below("first").exists()); + start(&root, "first"); + operate( + &root, + "exec", + "first", + &[ + "--entrypoint", + "/bin/echo", + "--arg", + "new", + "--wait", + "0.05", + ], + ) + .assert_success(); + for state in ["first", "second"] { + operate(&root, "stop", state, &[]).assert_silent_success(); + operate(&root, "deprovision", state, &[]).assert_silent_success(); + } +} + +#[test] +fn a_retry_that_waits_for_the_lifecycle_lock_past_the_deadline_is_busy() { + // The `--wait` of the waiting exec. + const WAIT: Duration = Duration::from_secs(4); + let root = Root::new(); + start(&root, "state"); + operate( + &root, + "exec", + "state", + &["--entrypoint", "/bin/echo", "--arg", "prepared"], + ) + .assert_success(); + // The test holds the exec lock, as an exec in progress does. + let admission = File::options() + .read(true) + .write(true) + .open(root.below("state/exec.lock")) + .unwrap(); + admission.lock().unwrap(); + let lifecycle = File::options() + .read(true) + .write(true) + .open(root.below("state/sandbox.lock")) + .unwrap(); + let wait = WAIT.as_secs().to_string(); + // The race needs an exec whose retry waits for the lifecycle lock until after its deadline. The + // exec holds the lifecycle lock only while it tries the exec lock, so the test polls that lock + // until it finds a try, and then takes it. When it takes it within the first half of `--wait`, + // the deadline, at least `--wait` after the spawn, is still half of it away, so the exec tries + // again and waits for the lifecycle lock: the precondition holds, and the round must end in + // `sandbox-busy` once the test frees the exec lock and then, past the deadline, the lifecycle + // lock. Only a round that misses every try, or finds one too late, gives way to another, once + // its exec has waited out `--wait` and failed with `sandbox-busy` too, and the test fails if no + // round reaches the precondition. + let reached = (0..5).any(|_| { + let spawned = Instant::now(); + let mut waiting = Pending::spawn( + &root, + &script(&root, "state", "touch {root/forbidden}", &["--wait", &wait]), + ); + let seen = loop { + match lifecycle.try_lock() { + Ok(()) => lifecycle.unlock().unwrap(), + Err(TryLockError::WouldBlock) => break true, + Err(TryLockError::Error(error)) => panic!("cannot try the lock: {error}"), + } + if waiting.ended() { + break false; + } + assert!(spawned.elapsed() < Duration::from_secs(60), "the exec hung"); + thread::yield_now(); + }; + let reached = seen && { + lifecycle.lock().unwrap(); + spawned.elapsed() < WAIT / 2 + }; + if reached { + // The deadline, which the exec took after the spawn and before its try, passes. + thread::sleep(WAIT + Duration::from_millis(500)); + assert!( + !waiting.ended(), + "the exec ended instead of waiting to try again" + ); + admission.unlock().unwrap(); + lifecycle.unlock().unwrap(); + } else if seen { + lifecycle.unlock().unwrap(); + } + waiting + .finish() + .assert_error("sandbox-busy", &["another exec"]); + reached + }); + assert!( + reached, + "no round delayed a retry of the exec past its deadline" + ); + assert!(!root.below("forbidden").exists()); + drop((admission, lifecycle)); + operate( + &root, + "exec", + "state", + &[ + "--entrypoint", + "/bin/echo", + "--arg", + "free", + "--wait", + "0.05", + ], + ) + .assert_success(); + operate(&root, "stop", "state", &[]).assert_silent_success(); + operate(&root, "deprovision", "state", &[]).assert_silent_success(); +} + +#[test] +fn results_errors_and_process_cancellation_release_admission() { + let root = Root::new(); + root.nvx_rs(&provision("{root/state}", &[])) + .assert_silent_success(); + operate(&root, "start", "state", &[]).assert_silent_success(); + for (command, status) in [ + ("echo success", 0), + ("exit 3", 3), + ("fail", 125), + ("launchfail", 1), + ] { + let outcome = root.nvx_rs(&script(&root, "state", command, &["--wait", "0.05"])); + assert_eq!(outcome.status, Some(status), "{}", outcome.describe()); + operate( + &root, + "exec", + "state", + &[ + "--entrypoint", + "/bin/echo", + "--arg", + "released", + "--wait", + "0.05", + ], + ) + .assert_success(); + } + let mut pending = Pending::spawn( + &root, + &script(&root, "state", "touch {root/cancelling}; sleep 60000", &[]), + ); + crate::wait_for(&root.below("cancelling")); + #[cfg(unix)] + nix::sys::signal::kill( + nix::unistd::Pid::from_raw(i32::try_from(pending.child.as_mut().unwrap().id()).unwrap()), + nix::sys::signal::Signal::SIGINT, + ) + .unwrap(); + #[cfg(windows)] + pending.child.as_mut().unwrap().kill().unwrap(); + let cancelled = pending.finish(); + #[cfg(unix)] + assert_eq!(cancelled.status, Some(130), "{}", cancelled.describe()); + #[cfg(windows)] + assert_ne!(cancelled.status, Some(0), "{}", cancelled.describe()); + operate( + &root, + "exec", + "state", + &[ + "--entrypoint", + "/bin/echo", + "--arg", + "after-cancel", + "--wait", + "1", + ], + ) + .assert_success(); + operate(&root, "stop", "state", &[]).assert_silent_success(); + operate(&root, "deprovision", "state", &[]).assert_silent_success(); +} + +#[test] +fn exec_cannot_recreate_admission_after_cleanup_removes_provisioned_state() { + let root = Root::new(); + start(&root, "state"); + operate( + &root, + "exec", + "state", + &["--entrypoint", "/bin/echo", "--arg", "owned"], + ) + .assert_success(); + let process = crate::recorded_process(&root, "state").unwrap(); + operate(&root, "stop", "state", &[]).assert_silent_success(); + assert!( + process + .wait(Instant::now() + Duration::from_secs(10)) + .unwrap() + .is_some() + ); + let lifecycle = File::options() + .read(true) + .write(true) + .open(root.below("state/sandbox.lock")) + .unwrap(); + lifecycle.lock().unwrap(); + root.remove("state/config.json"); + root.remove("state/exec.lock"); + let refused = operate( + &root, + "exec", + "state", + &["--wait", "0.05", "--timeout", "0.05"], + ); + refused.assert_error("sandbox-busy", &["lifecycle"]); + assert!(!root.below("state/exec.lock").exists()); + lifecycle.unlock().unwrap(); + drop(lifecycle); + let refused = operate(&root, "exec", "state", &["--wait", "0.05"]); + refused.assert_error("sandbox-not-provisioned", &[]); + assert!(!root.below("state/exec.lock").exists()); + operate(&root, "deprovision", "state", &[]).assert_silent_success(); + assert!(!root.below("state").exists()); + start(&root, "state"); + operate( + &root, + "exec", + "state", + &["--entrypoint", "/bin/echo", "--arg", "reprovisioned"], + ) + .assert_success(); + operate(&root, "stop", "state", &[]).assert_silent_success(); + operate(&root, "deprovision", "state", &[]).assert_silent_success(); +} + +#[cfg(windows)] +#[test] +fn deprovision_waits_for_its_delete_pending_exec_file_after_verified_exit() { + let root = Root::new(); + start(&root, "state"); + operate( + &root, + "exec", + "state", + &["--entrypoint", "/bin/echo", "--arg", "owned"], + ) + .assert_success(); + let process = crate::recorded_process(&root, "state").unwrap(); + operate(&root, "stop", "state", &[]).assert_silent_success(); + assert!( + process + .wait(Instant::now() + Duration::from_secs(10)) + .unwrap() + .is_some() + ); + let state = root.below("state"); + let pending = crate::lifecycle::windows::delete_pending(&state.join("exec.lock")); + assert!( + fs::read_dir(&state) + .unwrap() + .any(|entry| entry.unwrap().file_name() == "exec.lock") + ); + let mut deprovision = Pending::spawn( + &root, + &sandbox("deprovision", "{root/state}", &["--timeout", "2"]), + ); + let deadline = Instant::now() + Duration::from_secs(10); + while state.join("config.json").exists() { + assert!( + Instant::now() < deadline, + "deprovision did not reach cleanup" + ); + assert!( + deprovision + .child + .as_mut() + .unwrap() + .try_wait() + .unwrap() + .is_none() + ); + thread::sleep(Duration::from_millis(10)); + } + assert!( + deprovision + .child + .as_mut() + .unwrap() + .try_wait() + .unwrap() + .is_none() + ); + assert!( + fs::read_dir(&state) + .unwrap() + .any(|entry| entry.unwrap().file_name() == "exec.lock") + ); + drop(pending); + deprovision.finish().assert_silent_success(); + assert!(!state.exists()); +} + +#[test] +fn a_replaced_exec_lock_entry_is_refused_before_contacting_the_vm() { + let root = Root::new(); + start(&root, "state"); + operate( + &root, + "exec", + "state", + &["--entrypoint", "/bin/echo", "--arg", "prepared"], + ) + .assert_success(); + fs::rename(root.below("state/exec.lock"), root.below("state/old.lock")).unwrap(); + fs::create_dir(root.below("state/exec.lock")).unwrap(); + let sessions = crate::sessions(&root, "state"); + let refused = root.nvx_rs(&script( + &root, + "state", + "touch {root/forbidden}", + &["--wait", "0.05"], + )); + refused.assert_error("io", &["exec.lock"]); + assert!(!root.below("forbidden").exists()); + assert_eq!(crate::sessions(&root, "state"), sessions); + fs::remove_dir(root.below("state/exec.lock")).unwrap(); + fs::rename(root.below("state/old.lock"), root.below("state/exec.lock")).unwrap(); + operate( + &root, + "exec", + "state", + &[ + "--entrypoint", + "/bin/echo", + "--arg", + "current", + "--wait", + "0.05", + ], + ) + .assert_success(); + operate(&root, "stop", "state", &[]).assert_silent_success(); + operate(&root, "deprovision", "state", &[]).assert_silent_success(); +} diff --git a/nvx-rs/tests/cli/lifecycle.rs b/nvx-rs/tests/cli/lifecycle.rs index ac0e45ac3..7f817ae16 100644 --- a/nvx-rs/tests/cli/lifecycle.rs +++ b/nvx-rs/tests/cli/lifecycle.rs @@ -47,6 +47,12 @@ fn running() -> Vec<&'static str> { names } +fn with_exec(mut names: Vec<&'static str>) -> Vec<&'static str> { + names.push("exec.lock"); + names.sort_unstable(); + names +} + /// Returns the arguments of the lifecycle operation `operation` of the sandbox whose state /// directory is `state`, with `options`. pub(crate) fn sandbox>(operation: &str, state: &str, options: &[S]) -> Vec { @@ -576,7 +582,12 @@ fn check_lifecycle(case: &Configuration) { let context = format!("{}, start {round}", case.name); root.nvx_rs(&sandbox::<&str>("start", case.argument, &[])) .assert_silent_success(); - assert_eq!(entries(&state), running(), "{context}"); + let expected = if round == 1 { + running() + } else { + with_exec(running()) + }; + assert_eq!(entries(&state), expected, "{context}"); let (endpoint, process) = assert_runtime_record(&root, case.state, &json(&state.join("runtime.json"))); assert_capability(&state); @@ -596,7 +607,7 @@ fn check_lifecycle(case: &Configuration) { let outcome = root.nvx_rs(&sandbox("exec", case.argument, options)); outcome.assert_success(); assert_eq!(outcome.stdout(), *stdout, "{context}"); - assert_eq!(entries(&state), running(), "{context}"); + assert_eq!(entries(&state), with_exec(running()), "{context}"); root.nvx_rs(&sandbox::<&str>("stop", case.argument, &[])) .assert_silent_success(); assert!( @@ -606,7 +617,7 @@ fn check_lifecycle(case: &Configuration) { .is_some(), "{context}: OpenVMM runs on" ); - assert_eq!(entries(&state), STOPPED, "{context}"); + assert_eq!(entries(&state), with_exec(STOPPED.to_vec()), "{context}"); assert_outcome_report( &json(&state.join("outcome.json")), HYPERVISOR, @@ -669,7 +680,12 @@ fn lifecycle_of_a_sandbox_whose_upper_is_in_ram() { for round in 1..=2 { let context = format!("start {round}"); operate(&root, "start", "state", &[]).assert_silent_success(); - assert_eq!(entries(&state), running(), "{context}"); + let expected = if round == 1 { + running() + } else { + with_exec(running()) + }; + assert_eq!(entries(&state), expected, "{context}"); let (endpoint, process) = assert_runtime_record(&root, "state", &json(&state.join("runtime.json"))); let mut options = Options::parse(&root.openvmm_arguments("build/fake-openvmm-state.json")); @@ -694,7 +710,7 @@ fn lifecycle_of_a_sandbox_whose_upper_is_in_ram() { .is_some(), "{context}: OpenVMM runs on" ); - assert_eq!(entries(&state), STOPPED, "{context}"); + assert_eq!(entries(&state), with_exec(STOPPED.to_vec()), "{context}"); assert_outcome_report( &json(&state.join("outcome.json")), HYPERVISOR, @@ -738,7 +754,7 @@ fn stops_keep_an_outcome_report_that_the_caller_reads_whatever_its_umask() { .is_some(), "{context}: OpenVMM runs on" ); - assert_eq!(entries(&state), STOPPED, "{context}"); + assert_eq!(entries(&state), with_exec(STOPPED.to_vec()), "{context}"); let report = state.join("outcome.json"); let (permissions, expected) = (mode(&report), FAKE_REPORT_MODE & !(umask & 0o077)); assert_eq!( @@ -1071,7 +1087,7 @@ fn managed_execs_exit_with_the_workload_status() { "{}: the workload outlived its timeout", case.name ); - assert_eq!(entries(&state), running(), "{}", case.name); + assert_eq!(entries(&state), with_exec(running()), "{}", case.name); } root.nvx_rs(&sandbox::<&str>("stop", "{root/state}", &[])) .assert_silent_success(); @@ -1143,7 +1159,12 @@ fn refusals_of_a_running_sandbox(root: &Root) { for operation in ["stop", "exec"] { let outcome = operate(root, operation, "state", &[]); assert_refused(root, &outcome, "sandbox-not-running", &["not running"]); - assert_eq!(entries(&state), STOPPED, "{operation}"); + let expected = if operation == "exec" { + with_exec(STOPPED.to_vec()) + } else { + STOPPED.to_vec() + }; + assert_eq!(entries(&state), expected, "{operation}"); } operate(root, "deprovision", "state", &[]).assert_silent_success(); } @@ -1542,7 +1563,7 @@ fn refusals_of_lifecycle_operations() { /// that refuses access for good, and provision waits for a removed one, which a file system /// without POSIX deletion keeps under its name until its last handle closes (microsoft/nvx#464). #[cfg(windows)] -mod windows { +pub(crate) mod windows { use std::fs::{self, File}; use std::os::windows::fs::OpenOptionsExt; use std::os::windows::io::AsRawHandle; @@ -1580,7 +1601,7 @@ mod windows { /// Deletes the file `path` as a file system without POSIX deletion deletes a file that other /// handles hold: its name stays, and opening it fails, until the last handle closes, of which /// the returned one is the last unless others hold the file. - fn delete_pending(path: &Path) -> File { + pub(crate) fn delete_pending(path: &Path) -> File { let file = fs::OpenOptions::new() .access_mode(DELETE) .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE) diff --git a/nvx-rs/tests/cli/usage.rs b/nvx-rs/tests/cli/usage.rs index ee16070b4..9c790a12d 100644 --- a/nvx-rs/tests/cli/usage.rs +++ b/nvx-rs/tests/cli/usage.rs @@ -690,7 +690,11 @@ const HELP: [Help; 6] = [ name: "sandbox after an operation", arguments: &["sandbox", "exec", "--help"], usage: "usage: nvx-rs sandbox [{run,provision,start,exec,stop,deprovision}] [OPTIONS]", - options: &["--environment KEY=VALUE", "--outcome-report OUTCOME_REPORT"], + options: &[ + "--environment KEY=VALUE", + "--outcome-report OUTCOME_REPORT", + "--wait WAIT", + ], }, Help { name: "run", diff --git a/scripts/nvx_tools/sandbox_lifecycle_tests.py b/scripts/nvx_tools/sandbox_lifecycle_tests.py index f0b7fe7b9..45aa7df7b 100644 --- a/scripts/nvx_tools/sandbox_lifecycle_tests.py +++ b/scripts/nvx_tools/sandbox_lifecycle_tests.py @@ -111,6 +111,9 @@ def __str__(self) -> str: SOCKET = "control.sock" OUTCOME = "outcome.json" LOCK = "sandbox.lock" +# The first exec of a provisioned sandbox creates its exec lock, even an exec +# that finds no VM, and the lock stays until deprovision removes the sandbox. +EXEC_LOCK = "exec.lock" # The fields of the store's runtime record. RUNTIME_FIELDS = frozenset({"capability", "endpoint", "process", "runtime", "vmDir"}) PROVISIONED_ENTRIES = frozenset({CONFIG, LOCK}) @@ -428,6 +431,8 @@ def __init__( # An artifact root whose runtime identity differs in the initramfs. self.other_root = root / "other-runtime" self.checks: list[dict[str, object]] = [] + # The state directories that hold the exec lock of an earlier exec. + self.exec_locks: set[Path] = set() def launch_options(self, scratch: Path) -> list[str]: return [ @@ -498,7 +503,14 @@ def invoke( `refusal` on standard error when given. With `root`, the operation runs under that artifact root. """ + provisioned = state is not None and (state / CONFIG).is_file() result = self.run_cli(operation, *arguments, state=state, root=root) + if state is not None: + if operation == "exec" and provisioned: + self.exec_locks.add(state) + elif not (state / CONFIG).exists(): + # Deprovision removes the exec lock with the configuration. + self.exec_locks.discard(state) self.record(operation, result, state=state, expected=expected) if (expected is not None and result.returncode != expected) or ( refusal is not None and not refusal.matches(result.stderr) @@ -531,6 +543,8 @@ def exec( def require_entries( self, state: Path, expected: frozenset[str], description: str ) -> None: + if state in self.exec_locks: + expected |= {EXEC_LOCK} entries = state_entries(state) if entries is None or frozenset(entries) != expected: raise RuntimeError( diff --git a/scripts/test_microvm_tests.py b/scripts/test_microvm_tests.py index 28fc0bea3..21e750912 100644 --- a/scripts/test_microvm_tests.py +++ b/scripts/test_microvm_tests.py @@ -1024,6 +1024,14 @@ def _dispatch(self, command: list[str]) -> subprocess.CompletedProcess[bytes]: values[command[index]] = command[index + 1] index += 2 state = Path(values["--state-dir"]) if "--state-dir" in values else None + if ( + operation == "exec" + and state is not None + and (state / "config.json").is_file() + and "exec-skips-lock" not in self.faults + ): + # Admission creates the exec lock before the exec finds the VM. + (state / "exec.lock").touch() handlers: dict[ str, Callable[[Path | None, dict[str, str], list[str]], _CliResult], @@ -1415,6 +1423,7 @@ def _deprovision( "openvmm.log", "config.json", "sandbox.lock", + "exec.lock", ): (state / name).unlink(missing_ok=True) # The store owns the file in which OpenVMM stages its outcome report. @@ -1727,6 +1736,11 @@ def test_acceptance_detects_lifecycle_regressions(self): "crash-keeps-runtime": ( "after exec of a crashed sandbox: the state directory holds" ), + "exec-skips-lock": ( + "after exec before start: the state directory holds " + "['config.json', 'sandbox.lock'], expected " + "['config.json', 'exec.lock', 'sandbox.lock']" + ), "deprovision-removes-foreign": "deprovision returned 0, expected 1", } for fault, message in regressions.items():