diff --git a/.github/workflows/kvm-test.yaml b/.github/workflows/kvm-test.yaml index 3bfbe21e..7baaa265 100644 --- a/.github/workflows/kvm-test.yaml +++ b/.github/workflows/kvm-test.yaml @@ -375,6 +375,17 @@ jobs: cp target/x86_64-unknown-linux-musl/release/sandbox-agent /tmp/mitos-test/agent ls -lh /tmp/mitos-test/agent + - name: Guest agent RLIMIT_NOFILE tests (issue #786) + run: | + # Run the sys::rlimit suite on this Linux host: the pure parse/resolve + # unit tests plus the Linux-gated integration test that spawns a child + # and asserts its soft RLIMIT_NOFILE is actually raised. Scoped to the + # module so it stays deterministic and does not require the booted-VM + # or Python-kernel fixtures the other guest tests need. + source "$HOME/.cargo/env" 2>/dev/null || true + cd guest/agent-rs + cargo test --lib sys::rlimit + # OCI image -> rootfs pipeline proof (issue #10). This is the gate that # proves Engine.CreateTemplate builds a bootable template FROM A REAL OCI # IMAGE, not a hand-built rootfs: pull busybox:stable, flatten its layers, diff --git a/docs/templates.md b/docs/templates.md index efaf701a..94733a08 100644 --- a/docs/templates.md +++ b/docs/templates.md @@ -157,8 +157,42 @@ E2B-style fast-cached-build behavior. The key computation and the skip decision are pure and unit-tested on any host; the actual layer reuse on a live boot is KVM gated and asserted in the Firecracker suite. +## Guest process resource limits + +The guest agent is PID 1 inside the microVM. A bare Linux init inherits a low +soft `RLIMIT_NOFILE` (commonly 1024 open files), and every process the agent +spawns for `exec`, PTY sessions, `run_code`, and serving workloads would inherit +that same low limit. Data libraries that open many files at import time (pandas, +openpyxl, scikit) hit `EMFILE` ("too many open files") under 1024, so a plain +`import pandas` could die inside a sandbox even when the host-side caps (husk pod +cgroup, per-sandbox stream caps) are nowhere near saturated. Those host caps do +not govern in-guest per-process rlimits; only `setrlimit` inside the guest does. + +The agent therefore raises the soft `RLIMIT_NOFILE` of every process it spawns to +a sane default of 65536, clamped to the inherited hard limit and never lowered +below what the process already has. The raise is installed via a `pre_exec` hook +that runs in the forked child before `execve`, so it applies to the whole child +tree. + +To override the default, set the `MITOS_RLIMIT_NOFILE` environment variable in +the guest agent's environment to a decimal file count of at least 64. It is read +once per spawn from the agent's own environment, so it applies uniformly to every +spawned process. A value below 64, or an unparseable value, is ignored with a +warning and the default applies; the floor stops a `0` or tiny typo from silently +crippling every spawned child. An operator may set a value below the default (but +at or above the floor) deliberately; the agent never raises the hard limit (that +needs `CAP_SYS_RESOURCE` and is out of scope). + +The default and the override resolution are unit-tested on any host; the +end-to-end proof (a spawned guest process observing the raised limit and opening +more than 1024 files) is KVM gated and runs in the Firecracker suite. + ## Open follow-ups +- A first-class `template.spec` field (for example `resources.rlimits.nofile`) + plumbed through CreateTemplate and Fork into the guest agent's + `MITOS_RLIMIT_NOFILE` boot environment, validated at the boundary, so operators + set the limit declaratively instead of via the raw environment variable. - `go:embed` the guest agent into the forkd binary so no external `--agent-bin` path is needed. - OCI layer caching tied to the CAS store so repeated pool builds do not diff --git a/guest/agent-rs/src/kernel/driver.rs b/guest/agent-rs/src/kernel/driver.rs index a58fce7b..0364900f 100644 --- a/guest/agent-rs/src/kernel/driver.rs +++ b/guest/agent-rs/src/kernel/driver.rs @@ -414,20 +414,31 @@ impl KernelManager { )); } - let mut cmd = tokio::process::Command::new(&self.cfg.python); - cmd.arg(driver_path); - cmd.stdin(Stdio::piped()); - cmd.stdout(Stdio::piped()); + // Built as a std::process::Command so the RLIMIT_NOFILE pre_exec hook can + // be registered (all unsafe stays in sys/), then converted to a tokio + // Command for kill_on_drop and async spawn. + let mut std_cmd = std::process::Command::new(&self.cfg.python); + std_cmd.arg(driver_path); + std_cmd.stdin(Stdio::piped()); + std_cmd.stdout(Stdio::piped()); // Route driver stderr to this process's stderr (ipykernel debug noise). - cmd.stderr(Stdio::inherit()); - // Kill the driver when its Child handle is dropped (agent shutdown). - cmd.kill_on_drop(true); + std_cmd.stderr(Stdio::inherit()); // Set working directory to /workspace when it exists (mirrors Go). if std::path::Path::new("/workspace").is_dir() { - cmd.current_dir("/workspace"); + std_cmd.current_dir("/workspace"); } + // Raise the kernel's soft RLIMIT_NOFILE to a sane default (issue #786) so + // `import pandas` / openpyxl / scikit do not die with EMFILE under the + // 1024-fd limit a bare init inherits. See sys/rlimit.rs for the SAFETY + // contract of the pre_exec hook. + crate::sys::rlimit::apply_nofile_limit(&mut std_cmd); + + let mut cmd = tokio::process::Command::from(std_cmd); + // Kill the driver when its Child handle is dropped (agent shutdown). + cmd.kill_on_drop(true); + let mut child = cmd.spawn().map_err(|e| { format!( "kernel unavailable: start kernel driver: {e}; rebuild the base image \ diff --git a/guest/agent-rs/src/service/exec.rs b/guest/agent-rs/src/service/exec.rs index 5bb1440f..f550028e 100644 --- a/guest/agent-rs/src/service/exec.rs +++ b/guest/agent-rs/src/service/exec.rs @@ -34,6 +34,7 @@ use std::collections::HashMap; use std::os::fd::AsRawFd; +use std::os::unix::process::CommandExt as _; use std::sync::Arc; use std::time::{Duration, Instant}; @@ -220,7 +221,11 @@ async fn exec_shell_nostdin( let configured = env.snapshot().await; let merged_env = build_merged_env(&configured, &req_env); - let mut child = tokio::process::Command::new("/bin/sh") + // Built as a std::process::Command so the RLIMIT_NOFILE pre_exec hook can be + // registered (all unsafe stays in sys/), then converted to a tokio Command + // to spawn. process_group(0) is equivalent to Setpgid=true. + let mut std_cmd = std::process::Command::new("/bin/sh"); + std_cmd .arg("-c") .arg(&command) .current_dir(&cwd) @@ -230,7 +235,10 @@ async fn exec_shell_nostdin( .stdin(std::process::Stdio::null()) .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::piped()) - .process_group(0) + .process_group(0); + // Raise the child's soft RLIMIT_NOFILE to a sane default (issue #786). + crate::sys::rlimit::apply_nofile_limit(&mut std_cmd); + let mut child = tokio::process::Command::from(std_cmd) .spawn() .map_err(|e| { let _ = tx.try_send(Ok(exit_frame(1, 0.0, format!("start: {e}")))); @@ -373,7 +381,10 @@ async fn exec_shell( // Spawn /bin/sh -c in its own process group so SIGKILL kills // the whole child tree. process_group(0) is equivalent to Setpgid=true. - let mut child = tokio::process::Command::new("/bin/sh") + // Built as a std::process::Command so the RLIMIT_NOFILE pre_exec hook can be + // registered (all unsafe stays in sys/), then converted to a tokio Command. + let mut std_cmd = std::process::Command::new("/bin/sh"); + std_cmd .arg("-c") .arg(&open.command) .current_dir(&cwd) @@ -383,7 +394,11 @@ async fn exec_shell( .stdin(std::process::Stdio::piped()) .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::piped()) - .process_group(0) + .process_group(0); + // Raise the child's soft RLIMIT_NOFILE to a sane default (issue #786) so a + // command like `python -c 'import pandas'` is not killed by EMFILE. + crate::sys::rlimit::apply_nofile_limit(&mut std_cmd); + let mut child = tokio::process::Command::from(std_cmd) .spawn() .map_err(|e| { // Best-effort: send a spawn-failure exit frame. Ignore if channel full. @@ -659,6 +674,11 @@ async fn exec_pty( pty_sys::apply_pty_session_leader(&mut std_cmd) .map_err(|e| Status::internal(format!("exec: apply_pty_session_leader: {e}")))?; + // Raise the child's soft RLIMIT_NOFILE to a sane default (issue #786) so an + // interactive shell and anything it launches can open more than the 1024 + // files a bare init inherits. See sys/rlimit.rs for the SAFETY contract. + crate::sys::rlimit::apply_nofile_limit(&mut std_cmd); + // Convert the master to an AsyncFd for epoll-driven IO. tokio::fs::File // uses spawn_blocking and does not work correctly with O_NONBLOCK on a PTY // master character device: it would return EAGAIN immediately on reads diff --git a/guest/agent-rs/src/service/workload.rs b/guest/agent-rs/src/service/workload.rs index 472293de..c3218e6f 100644 --- a/guest/agent-rs/src/service/workload.rs +++ b/guest/agent-rs/src/service/workload.rs @@ -63,6 +63,11 @@ pub fn spawn_detached(command: &str, env: &[(String, String)], cwd: &str) -> std for (k, v) in env { cmd.env(k, v); } + // Raise the workload's soft RLIMIT_NOFILE to a sane default (issue #786) so a + // long-running serving app inherits the same file-descriptor headroom as an + // exec child. Registers a second pre_exec hook; hooks stack in registration + // order and this one only touches getrlimit/setrlimit (see sys/rlimit.rs). + crate::sys::rlimit::apply_nofile_limit(&mut cmd); // SAFETY: setsid(2) is async-signal-safe and has no preconditions; the // pre-exec closure runs in the forked child before exec and performs only // setsid plus an error read, no allocation. Detaching the child into a new diff --git a/guest/agent-rs/src/sys/mod.rs b/guest/agent-rs/src/sys/mod.rs index f812fd09..2ea2083a 100644 --- a/guest/agent-rs/src/sys/mod.rs +++ b/guest/agent-rs/src/sys/mod.rs @@ -24,6 +24,7 @@ pub mod kill; pub mod netlink; pub mod pty; pub mod mount; +pub mod rlimit; pub mod signal; pub mod vsock; diff --git a/guest/agent-rs/src/sys/rlimit.rs b/guest/agent-rs/src/sys/rlimit.rs new file mode 100644 index 00000000..5a3f57ba --- /dev/null +++ b/guest/agent-rs/src/sys/rlimit.rs @@ -0,0 +1,266 @@ +// Per-process resource limits for spawned guest processes (issue #786). +// +// The guest agent is PID 1. A bare Linux init inherits a low RLIMIT_NOFILE soft +// limit (commonly 1024), which every exec/run_code child then inherits. Data +// libraries that open many files at import time (pandas, openpyxl, scikit) hit +// EMFILE ("too many open files") under that default, so a plain `import pandas` +// can die inside a sandbox even though the host-side caps (husk pod cgroup, +// per-sandbox stream caps) are nowhere near saturated. Those host caps do NOT +// govern in-guest per-process rlimits; only setrlimit inside the guest does. +// +// This module raises the soft RLIMIT_NOFILE of every process the agent spawns to +// a sane documented default, clamped to the inherited hard limit and never +// lowered below what the process already has. An operator override is read from +// the agent's environment (MITOS_RLIMIT_NOFILE); see apply_nofile_limit. +// +// The raise is installed via a `pre_exec` hook, which runs in the forked child +// after `fork(2)` and before `execve(2)`. That context permits only +// async-signal-safe work: the closure performs a getrlimit syscall, integer +// arithmetic (resolve_nofile_soft, a pure function), and a setrlimit syscall. It +// allocates nothing and touches no locks, mirroring the safety contract of the +// setsid/TIOCSCTTY hook in sys/pty.rs. + +#![deny(unsafe_op_in_unsafe_fn)] + +/// Documented default soft limit for RLIMIT_NOFILE inside the guest. +/// +/// 65536 is high enough for the data-science import path that motivated this +/// change and well under the kernel's default fs.nr_open ceiling (1048576), so +/// setrlimit never fails when the value is clamped to the inherited hard limit. +pub const DEFAULT_NOFILE_SOFT: u64 = 65536; + +/// Environment variable an operator (or the future template knob) sets to +/// override the default soft RLIMIT_NOFILE. The value is a decimal file count. +/// It is read from the agent's own environment, so it applies uniformly to +/// every spawned process (exec, PTY, run_code kernel, serving workload). +pub const RLIMIT_NOFILE_ENV: &str = "MITOS_RLIMIT_NOFILE"; + +/// Minimum accepted soft NOFILE override. Below this a spawned child could not +/// open enough descriptors for even basic operation (stdio plus a handful), so a +/// degenerate value like `0` would recreate the exact EMFILE-on-everything +/// failure this module exists to prevent. An override below the floor (or empty, +/// or unparseable) is ignored and `DEFAULT_NOFILE_SOFT` applies instead. +pub const MIN_NOFILE_SOFT: u64 = 64; + +/// Parse the operator override value for the soft NOFILE limit. +/// +/// Accepts a decimal file count of at least `MIN_NOFILE_SOFT`. Returns `None` for +/// empty, unparseable, or below-floor input so the caller falls back to +/// `DEFAULT_NOFILE_SOFT`. Only finite decimal values are accepted deliberately: +/// an "unlimited" soft NOFILE is rejected by the kernel above fs.nr_open and +/// would turn a benign misconfiguration into a spawn failure, which violates the +/// boring-failure rule. The floor prevents the opposite footgun: a `0` or tiny +/// value silently crippling every spawned child. +pub fn parse_nofile_override(raw: &str) -> Option { + let trimmed = raw.trim(); + if trimmed.is_empty() { + return None; + } + trimmed.parse::().ok().filter(|&v| v >= MIN_NOFILE_SOFT) +} + +/// Compute the soft RLIMIT_NOFILE to install given the process's current +/// `(soft, hard)` pair and an optional operator override. +/// +/// Pure and side-effect free so it is both unit-testable and safe to call from +/// the async-signal-safe pre_exec closure (integer arithmetic only). +/// +/// Rules: +/// - With an explicit override, honor it exactly, clamped to the hard limit +/// (raising the hard limit needs CAP_SYS_RESOURCE and is out of scope). +/// An operator may deliberately choose a lower value than the current soft. +/// - Without an override, raise toward `DEFAULT_NOFILE_SOFT` but never above +/// the hard limit and never below the current soft limit. Clamping to the +/// hard limit guarantees the subsequent setrlimit (soft <= hard) succeeds. +pub fn resolve_nofile_soft(current_soft: u64, hard: u64, override_target: Option) -> u64 { + match override_target { + Some(target) => target.min(hard), + None => DEFAULT_NOFILE_SOFT.min(hard).max(current_soft), + } +} + +/// Read the operator override for the soft NOFILE limit from the agent's +/// environment. Logs a warning (never the raw secret-free value silently) when +/// the variable is set but cannot be parsed, then returns `None` so the default +/// applies. The value is a plain file count, never a secret. +fn configured_nofile_override() -> Option { + match std::env::var(RLIMIT_NOFILE_ENV) { + Ok(raw) => { + let parsed = parse_nofile_override(&raw); + if parsed.is_none() { + tracing::warn!( + var = RLIMIT_NOFILE_ENV, + value = %raw, + "ignoring invalid RLIMIT_NOFILE override (want an integer >= {}); using default {}", + MIN_NOFILE_SOFT, + DEFAULT_NOFILE_SOFT + ); + } + parsed + } + Err(_) => None, + } +} + +/// Register a `pre_exec` hook on `cmd` that raises the child's soft +/// RLIMIT_NOFILE to the resolved default (or operator override) before exec. +/// +/// Linux only: on other platforms this is a no-op (the guest runs Linux; the +/// hook is compiled out on the darwin developer build, matching sys/pty.rs). +/// +/// The override is read once here, in the parent, so the closure captures a +/// plain `Option` and performs no allocation or environment lookup in the +/// post-fork child. +pub fn apply_nofile_limit(cmd: &mut std::process::Command) { + let override_target = configured_nofile_override(); + apply_nofile_limit_inner(cmd, override_target); +} + +#[cfg(target_os = "linux")] +fn apply_nofile_limit_inner(cmd: &mut std::process::Command, override_target: Option) { + use std::os::unix::process::CommandExt; + + // SAFETY: this closure runs in the child after fork(2) and before execve(2), + // where only async-signal-safe work is permitted. It performs exactly: + // 1. getrlimit(RLIMIT_NOFILE): reads the inherited (soft, hard) pair. A + // raw syscall, async-signal-safe. + // 2. resolve_nofile_soft(...): pure integer arithmetic, no allocation. + // 3. setrlimit(RLIMIT_NOFILE): installs the new soft limit (<= hard, so it + // cannot fail on the value). A raw syscall, async-signal-safe. + // No heap allocation, no locks, no Rust runtime calls. io::Error::last_os_error + // reads errno only (same pattern as sys/pty.rs). rlim_t is u64 on Linux. + unsafe { + cmd.pre_exec(move || { + let mut lim = libc::rlimit { + rlim_cur: 0, + rlim_max: 0, + }; + if libc::getrlimit(libc::RLIMIT_NOFILE, &mut lim) != 0 { + return Err(std::io::Error::last_os_error()); + } + let new_soft = + resolve_nofile_soft(lim.rlim_cur as u64, lim.rlim_max as u64, override_target); + lim.rlim_cur = new_soft as libc::rlim_t; + if libc::setrlimit(libc::RLIMIT_NOFILE, &lim) != 0 { + return Err(std::io::Error::last_os_error()); + } + Ok(()) + }); + } +} + +#[cfg(not(target_os = "linux"))] +fn apply_nofile_limit_inner(cmd: &mut std::process::Command, override_target: Option) { + let _ = (cmd, override_target); +} + +#[cfg(test)] +#[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] +mod tests { + use super::*; + + #[test] + fn parse_accepts_plain_decimal_at_or_above_floor() { + assert_eq!(parse_nofile_override("65536"), Some(65536)); + assert_eq!(parse_nofile_override(" 4096 "), Some(4096)); + assert_eq!(parse_nofile_override("64"), Some(MIN_NOFILE_SOFT)); + } + + #[test] + fn parse_rejects_zero_and_sub_floor_values() { + // A degenerate override must never reach setrlimit; it falls back to the + // default instead of crippling every spawned child with EMFILE. + assert_eq!(parse_nofile_override("0"), None); + assert_eq!(parse_nofile_override("1"), None); + assert_eq!(parse_nofile_override("63"), None); + } + + #[test] + fn parse_rejects_empty_and_garbage() { + assert_eq!(parse_nofile_override(""), None); + assert_eq!(parse_nofile_override(" "), None); + assert_eq!(parse_nofile_override("lots"), None); + assert_eq!(parse_nofile_override("-1"), None); + assert_eq!(parse_nofile_override("6.5"), None); + assert_eq!(parse_nofile_override("unlimited"), None); + } + + #[test] + fn default_raises_soft_toward_documented_value() { + // A bare init: soft 1024, generous hard. We raise to the default. + assert_eq!( + resolve_nofile_soft(1024, 1_048_576, None), + DEFAULT_NOFILE_SOFT + ); + } + + #[test] + fn default_never_exceeds_hard() { + // Hard below the default: clamp to hard so setrlimit cannot fail. + assert_eq!(resolve_nofile_soft(1024, 4096, None), 4096); + } + + #[test] + fn default_never_lowers_an_already_high_soft() { + // A process that already has a soft above the default keeps it. + assert_eq!(resolve_nofile_soft(100_000, 1_048_576, None), 100_000); + } + + #[test] + fn override_is_honored_and_clamped_to_hard() { + // Operator raises higher than the default. + assert_eq!(resolve_nofile_soft(1024, 1_048_576, Some(200_000)), 200_000); + // Operator override above hard is clamped down to hard. + assert_eq!(resolve_nofile_soft(1024, 8192, Some(200_000)), 8192); + } + + #[test] + fn override_may_lower_deliberately() { + // An operator that explicitly asks for a lower value gets it. + assert_eq!(resolve_nofile_soft(4096, 1_048_576, Some(512)), 512); + } + + // The real proof (a spawned child actually sees the resolved limit) needs a + // running Linux process. This spawns a child with the hook installed and + // asserts its soft NOFILE equals what resolve_nofile_soft computes from this + // process's own limits. It reads the limits READ-ONLY (no setrlimit on the + // test process), so it touches no process-wide state and cannot race with + // other parallel tests (CodeRabbit). When the host starts below the default, + // as CI Linux runners do (soft 1024), this asserts the raise to + // DEFAULT_NOFILE_SOFT. It is Linux-gated (matches the guest target) and so is + // skipped on the darwin developer build, like the spawn tests in + // service/workload.rs. + #[cfg(target_os = "linux")] + #[test] + fn spawned_child_sees_resolved_soft_nofile() { + use std::process::Command; + let mut cur = libc::rlimit { + rlim_cur: 0, + rlim_max: 0, + }; + // SAFETY: getrlimit only reads into a local; no process state is changed. + unsafe { + assert_eq!(libc::getrlimit(libc::RLIMIT_NOFILE, &mut cur), 0); + } + let soft = cur.rlim_cur as u64; + let hard = cur.rlim_max as u64; + + let mut cmd = Command::new("/bin/sh"); + cmd.arg("-c").arg("ulimit -Sn"); + apply_nofile_limit(&mut cmd); + let out = cmd.output().expect("spawn ulimit"); + let reported = String::from_utf8_lossy(&out.stdout); + let reported = reported.trim(); + // A host whose soft limit is already unlimited has nothing to prove and + // ulimit prints "unlimited" rather than a number; skip that case. + if reported == "unlimited" { + return; + } + let child_soft: u64 = reported.parse().expect("numeric ulimit -Sn"); + assert_eq!( + child_soft, + resolve_nofile_soft(soft, hard, None), + "child soft NOFILE should equal the resolved default" + ); + } +}