From eaca36f4b6840948dbe361d18bba10df4623d04b Mon Sep 17 00:00:00 2001 From: Alexander Petric Date: Tue, 6 Jan 2026 13:08:47 -0500 Subject: [PATCH] fix: use tini with unshare to preserve signals (e.g oom) (#7471) * use tini with unshare to preserve signals (e.g oom) * fix ci * ci as nsjail * simplify * fix flaky go integration test --------- Co-authored-by: Ruben Fiszel --- .github/workflows/docker-image.yml | 3 +- Dockerfile | 2 +- backend/windmill-worker/src/common.rs | 14 ++++- backend/windmill-worker/src/worker.rs | 73 +++++++++++++++++++++++---- 4 files changed, 80 insertions(+), 12 deletions(-) diff --git a/.github/workflows/docker-image.yml b/.github/workflows/docker-image.yml index d6b55aaa78..a8d6ea2f44 100644 --- a/.github/workflows/docker-image.yml +++ b/.github/workflows/docker-image.yml @@ -593,9 +593,10 @@ jobs: ${{ steps.meta-ee-public.outputs.labels }} build_ee_slim: - if: ${{ startsWith(github.ref, 'refs/tags/v') }} || ((github.event_name != 'workflow_dispatch') || (github.event.inputs.slim)) needs: [build_ee] runs-on: ubicloud + if: (github.event_name != 'pull_request') && ((github.event_name != 'workflow_dispatch') || (github.event.inputs.ee || github.event.inputs.slim)) + steps: - uses: actions/checkout@v4 with: diff --git a/Dockerfile b/Dockerfile index 3ad32f9593..d57176dd4b 100644 --- a/Dockerfile +++ b/Dockerfile @@ -129,7 +129,7 @@ ENV PATH /usr/local/bin:/root/.local/bin:/tmp/.local/bin:$PATH RUN apt-get update \ - && apt-get install -y --no-install-recommends netbase tzdata ca-certificates wget curl jq unzip build-essential unixodbc xmlsec1 software-properties-common \ + && apt-get install -y --no-install-recommends netbase tzdata ca-certificates wget curl jq unzip build-essential unixodbc xmlsec1 software-properties-common tini \ && apt-get clean \ && rm -rf /var/lib/apt/lists/* diff --git a/backend/windmill-worker/src/common.rs b/backend/windmill-worker/src/common.rs index ea1f6a763d..95d6d7060e 100644 --- a/backend/windmill-worker/src/common.rs +++ b/backend/windmill-worker/src/common.rs @@ -642,7 +642,19 @@ pub fn build_command_with_isolation(program: &str, args: &[&str]) -> Command { cmd.arg(flag); } - cmd.arg("--"); + // If tini is available, use it for proper PID 1 signal handling + // (especially OOM exit codes which return 137 instead of sigprocmask errors). + // Note: --fork should already be in the flags for proper namespace setup. + if let Some(tini_path) = crate::TINI_AVAILABLE.as_ref() { + cmd.arg("--"); + cmd.arg(tini_path); + cmd.arg("-s"); + cmd.arg("--"); + } else { + // Without tini, just run the command directly (--fork is in flags) + cmd.arg("--"); + } + cmd.arg(program); cmd.args(args); cmd diff --git a/backend/windmill-worker/src/worker.rs b/backend/windmill-worker/src/worker.rs index 36ec01b2fb..04e65d91c4 100644 --- a/backend/windmill-worker/src/worker.rs +++ b/backend/windmill-worker/src/worker.rs @@ -315,16 +315,73 @@ lazy_static::lazy_static! { .and_then(|x| x.parse::().ok()) .unwrap_or(false); + pub static ref UNSHARE_TINI_PATH: String = { + std::env::var("UNSHARE_TINI_PATH").unwrap_or_else(|_| "tini".to_string()) + }; + + // --fork is required for unshare to work with --pid --mount-proc. + // When tini is available, it runs as PID 1 inside the forked namespace for proper signal handling. pub static ref UNSHARE_ISOLATION_FLAGS: String = { std::env::var("UNSHARE_ISOLATION_FLAGS") .unwrap_or_else(|_| "--user --map-root-user --pid --fork --mount-proc".to_string()) }; + // Check if tini is available for proper PID 1 handling in unshare namespaces. + // tini handles OOM signals correctly, returning exit code 137 instead of sigprocmask errors. + pub static ref TINI_AVAILABLE: Option = { + let tini_path = UNSHARE_TINI_PATH.as_str(); + let test_result = std::process::Command::new(tini_path) + .args(["-s", "--", "true"]) + .output(); + + match test_result { + Ok(output) if output.status.success() => { + tracing::info!("tini available at: {}", tini_path); + Some(tini_path.to_string()) + } + Ok(output) => { + let stderr = String::from_utf8_lossy(&output.stderr); + tracing::warn!( + "tini test failed: {}. Proceeding without tini (OOM exit codes may be incorrect).", + stderr.trim() + ); + None + } + Err(e) => { + if e.kind() == std::io::ErrorKind::NotFound { + tracing::warn!( + "tini not found at '{}'. Install tini for correct OOM exit codes, or set UNSHARE_TINI_PATH.", + tini_path + ); + } else { + tracing::warn!( + "Failed to test tini: {}. Proceeding without tini.", + e + ); + } + None + } + } + }; + pub static ref UNSHARE_PATH: Option = { let flags = UNSHARE_ISOLATION_FLAGS.as_str(); let mut test_cmd_args: Vec<&str> = flags.split_whitespace().collect(); - test_cmd_args.push("--"); - test_cmd_args.push("true"); + + // Build the test command based on whether tini is available + // Note: --fork should already be in the flags for proper namespace setup + if let Some(tini_path) = TINI_AVAILABLE.as_ref() { + // Test with tini: unshare -- tini -s -- true + test_cmd_args.push("--"); + test_cmd_args.push(tini_path.as_str()); + test_cmd_args.push("-s"); + test_cmd_args.push("--"); + test_cmd_args.push("true"); + } else { + // Fallback without tini: unshare -- true + test_cmd_args.push("--"); + test_cmd_args.push("true"); + } let test_result = std::process::Command::new("unshare") .args(&test_cmd_args) @@ -332,7 +389,11 @@ lazy_static::lazy_static! { match test_result { Ok(output) if output.status.success() => { - tracing::info!("PID namespace isolation enabled. Flags: {}", flags); + if TINI_AVAILABLE.is_some() { + tracing::info!("PID namespace isolation enabled with tini. Flags: {}", flags); + } else { + tracing::info!("PID namespace isolation enabled. Flags: {}", flags); + } Some("unshare".to_string()) }, Ok(output) => { @@ -1140,12 +1201,6 @@ pub async fn run_worker( if *ENABLE_UNSHARE_PID { // Access UNSHARE_PATH to trigger lazy_static initialization and test let _ = &*UNSHARE_PATH; - - tracing::info!( - worker = %worker_name, hostname = %hostname, - "PID namespace isolation enabled via unshare with flags: {}", - UNSHARE_ISOLATION_FLAGS.as_str() - ); } let start_time = Instant::now();