From 1090c51251b29d9cd73fc2202aa872337ca32040 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Tue, 10 Nov 2026 16:10:16 -0800 Subject: [PATCH] cli: serialize sandbox exec admission across CLI processes Concurrent nvx-rs processes could run two `sandbox exec` workloads in one sandbox at once (#819). An exec now takes a per-sandbox OS file lock, `exec.lock` in the state directory, which the new `host::cli::exec_lock` module owns, and holds it until its workload ends. Under the lifecycle lock, after it checks the configuration, it opens the lock file as the store opens its own, without following links and refusing one that is not a private regular file, tries the lock without waiting, and checks that the directory entry still names the locked file. An exec that finds the lock held releases the lifecycle lock, pauses for the store's retry interval and tries again: once at least, and only until `--wait` seconds, 60 by default, have passed, even when a retry waited past them for the lifecycle lock. It then fails with `sandbox-busy`. `--timeout` still bounds each wait for the lifecycle lock, and `stop` takes no exec lock, so it proceeds while a workload runs. The lock file stays, as `sandbox.lock` does, so that no release unlinks a file that another exec may hold open. `deprovision` removes it with the store's files, also when a file system without POSIX deletion keeps it delete-pending after the VM exited. It is the exec lock module's own file rather than one of the store's, so the store's temporary names and snapshot capture's owned files do not change. The docs describe `--wait`, the exec lock and the new cause of `sandbox-busy`, and the sandbox acceptance expects the lock in the state directory from the first exec on. Tests: tests/cli/exec_lock.rs runs CLI processes against the fake OpenVMM: a held lock makes a second exec fail with `sandbox-busy` after `--wait`, while other sandboxes and `stop` proceed; results, errors and cancellation release it; a retry that waits for the lifecycle lock past the deadline is busy; a removed configuration or a replaced lock file refuses admission; and deprovision waits for a delete-pending lock on Windows. Unit tests cover the lock's privacy and identity, the `--wait` default and number rules, and deprovision's removal of the file, and the exact state listings of existing tests include the lock. Part of #1111 (SL3.9). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- doc/run.md | 24 +- doc/usage.md | 4 +- nvx-rs/README.md | 12 +- nvx-rs/src/error.rs | 4 +- nvx-rs/src/host/cli/exec_lock.rs | 57 ++ nvx-rs/src/host/cli/exec_lock/tests.rs | 164 ++++++ nvx-rs/src/host/cli/mod.rs | 1 + .../src/host/cli/sandbox/lifecycle/start.rs | 36 +- nvx-rs/src/host/cli/sandbox/lock.rs | 10 +- nvx-rs/src/host/cli/sandbox/lock/tests.rs | 15 + nvx-rs/src/host/cli/sandbox/mod.rs | 1 + nvx-rs/src/host/cli/sandbox/options.rs | 9 +- nvx-rs/src/host/cli/sandbox/tests.rs | 14 + nvx-rs/src/host/cli/store/locked.rs | 2 +- nvx-rs/src/host/cli/store/mod.rs | 3 +- nvx-rs/tests/cli.rs | 9 +- nvx-rs/tests/cli/exec_lock.rs | 508 ++++++++++++++++++ nvx-rs/tests/cli/lifecycle.rs | 41 +- nvx-rs/tests/cli/usage.rs | 6 +- scripts/nvx_tools/sandbox_lifecycle_tests.py | 14 + scripts/test_microvm_tests.py | 14 + 21 files changed, 911 insertions(+), 37 deletions(-) create mode 100644 nvx-rs/src/host/cli/exec_lock.rs create mode 100644 nvx-rs/src/host/cli/exec_lock/tests.rs create mode 100644 nvx-rs/tests/cli/exec_lock.rs 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():