diff --git a/src/executor/wall_time/executor.rs b/src/executor/wall_time/executor.rs index fc956e6d..d9d236b9 100644 --- a/src/executor/wall_time/executor.rs +++ b/src/executor/wall_time/executor.rs @@ -1,5 +1,5 @@ use super::helpers::validate_walltime_results; -use super::isolation::{requires_isolation, wrap_isolation_scope}; +use super::isolation::Isolation; use super::profiler::Profiler; use super::profiler::perf::PerfProfiler; use super::profiler::samply::SamplyProfiler; @@ -29,58 +29,8 @@ use std::cell::OnceCell; use std::fs::canonicalize; use std::io::Write; use std::path::Path; -use std::path::PathBuf; -use std::process::Command; use tempfile::NamedTempFile; -struct HookScriptsGuard { - post_bench_script: PathBuf, -} - -impl HookScriptsGuard { - fn execute_script_from_path>(path: P) -> anyhow::Result<()> { - let path = path.as_ref(); - if !path.exists() || !path.is_file() { - debug!("Script not found or not a file: {}", path.display()); - return Ok(()); - } - - let output = Command::new("bash").args([&path]).output()?; - if !output.status.success() { - debug!("stdout: {}", String::from_utf8_lossy(&output.stdout)); - debug!("stderr: {}", String::from_utf8_lossy(&output.stderr)); - bail!("Failed to execute script: {}", path.display()); - } - - Ok(()) - } - - pub fn setup_with_scripts>(pre_bench_script: P, post_bench_script: P) -> Self { - if let Err(e) = Self::execute_script_from_path(pre_bench_script.as_ref()) { - debug!("Failed to execute pre-bench script: {e}"); - } - - Self { - post_bench_script: post_bench_script.as_ref().to_path_buf(), - } - } - - pub fn setup() -> Self { - Self::setup_with_scripts( - "/usr/local/bin/codspeed-pre-bench", - "/usr/local/bin/codspeed-post-bench", - ) - } -} - -impl Drop for HookScriptsGuard { - fn drop(&mut self) { - if let Err(e) = Self::execute_script_from_path(&self.post_bench_script) { - debug!("Failed to execute post-bench script: {e}"); - } - } -} - pub struct WallTimeExecutor { profiler: Option>, @@ -176,19 +126,16 @@ impl Executor for WallTimeExecutor { execution_context: &ExecutionContext, _mongo_tracer: &Option, ) -> Result<()> { - let _guard = HookScriptsGuard::setup(); + // Resolve isolation once: this runs the pre-bench setup, wraps the bench + // leaf, and (for hook mode) runs post-bench on drop. Held for the whole run + // so its teardown fires after the benchmark completes. `requires_sudo` is + // read off it for the privilege wrapping below (or in the profiler). + let isolation = Isolation::resolve(); + let requires_sudo = isolation.requires_sudo(); let (_env_file, _script_file, cmd_builder) = WallTimeExecutor::walltime_bench_cmd(&execution_context.config, execution_context)?; - - // Resolve the isolation decision once and reuse it for both the scope - // wrapping here and the privilege wrapping below (or in the profiler). - let isolate = requires_isolation(); - let cmd_builder = if isolate { - wrap_isolation_scope(cmd_builder)? - } else { - cmd_builder - }; + let cmd_builder = isolation.wrap_bench(cmd_builder)?; // Split-borrow `self` so the closure inside `run_with_profiler` can // capture `benchmark_state` while we hold `&mut profiler`. @@ -204,16 +151,13 @@ impl Executor for WallTimeExecutor { cmd_builder, &execution_context.config, &execution_context.profile_folder, - isolate, + requires_sudo, benchmark_state, ) .await } _ => { - // No profiler: still honor the isolation decision, since the - // scope (and the sudo it requires) is a property of the run, not - // of the profiler. - let cmd_builder = if isolate { + let cmd_builder = if requires_sudo { wrap_with_sudo(cmd_builder)? } else { cmd_builder @@ -262,11 +206,11 @@ async fn run_with_profiler( cmd_builder: CommandBuilder, config: &ExecutorConfig, profile_folder: &Path, - isolate: bool, + requires_sudo: bool, benchmark_state: &OnceCell<(FifoBenchmarkData, ExecutionTimestamps)>, ) -> Result { let wrapped = profiler - .wrap_command(cmd_builder, config, profile_folder, isolate) + .wrap_command(cmd_builder, config, profile_folder, requires_sudo) .await?; let cmd = wrapped.build(); debug!("cmd: {cmd:?}"); @@ -304,51 +248,3 @@ async fn run_with_profiler( }) .await } - -#[cfg(test)] -mod tests { - use tempfile::NamedTempFile; - - use super::*; - use std::{ - io::{Read, Write}, - os::unix::fs::PermissionsExt, - }; - - #[test] - fn test_env_guard_no_crash() { - fn create_run_script(content: &str) -> anyhow::Result { - let rwx = std::fs::Permissions::from_mode(0o777); - let mut script_file = tempfile::Builder::new() - .suffix(".sh") - .permissions(rwx) - .disable_cleanup(true) - .tempfile()?; - script_file.write_all(content.as_bytes())?; - - Ok(script_file) - } - - let mut tmp_dst = tempfile::NamedTempFile::new().unwrap(); - - let pre_script = create_run_script(&format!( - "#!/usr/bin/env bash\necho \"pre\" >> {}", - tmp_dst.path().display() - )) - .unwrap(); - let post_script = create_run_script(&format!( - "#!/usr/bin/env bash\necho \"post\" >> {}", - tmp_dst.path().display() - )) - .unwrap(); - - { - let _guard = - HookScriptsGuard::setup_with_scripts(pre_script.path(), post_script.path()); - } - - let mut result = String::new(); - tmp_dst.read_to_string(&mut result).unwrap(); - assert_eq!(result, "pre\npost\n"); - } -} diff --git a/src/executor/wall_time/isolation.rs b/src/executor/wall_time/isolation.rs index cc691190..fbffb654 100644 --- a/src/executor/wall_time/isolation.rs +++ b/src/executor/wall_time/isolation.rs @@ -1,60 +1,168 @@ +use std::process::Command; + use crate::executor::helpers::command::CommandBuilder; use crate::prelude::*; -/// Whether the benchmark must run inside the privileged systemd scope, which in -/// turn requires the profiler itself to record under elevated privileges. -/// -/// When isolated, the scope reparents the benchmark out of the profiler's -/// process subtree, so the profiler can only observe it by recording -/// system-wide — which needs `sudo`. When not isolated, the profiler records its -/// own descendant tree and runs unprivileged (relying on a permissive -/// `perf_event_paranoid`). -/// -/// `CODSPEED_ISOLATION` overrides the decision; otherwise we isolate only when we -/// can elevate without prompting (root or passwordless sudo, as on CI), so a -/// local run never blocks on a password. +/// Paths of the machine's CPU-isolation hooks. The machine image installs them +/// and owns all cpuset logic behind them; the runner only invokes them. +const PRE_BENCH_HOOK: &str = "/usr/local/bin/codspeed-pre-bench"; +const WRAP_BENCH_HOOK: &str = "/usr/local/bin/codspeed-wrap-bench"; +const POST_BENCH_HOOK: &str = "/usr/local/bin/codspeed-post-bench"; + +/// Environment variable selecting how walltime runs isolate the benchmark. +const ISOLATION_ENV: &str = "CODSPEED_ISOLATION"; + +/// How the benchmark is pinned to dedicated cores, away from the rest of the +/// system, for the lifetime of one walltime run. /// -/// Isolation relies on `systemd-run`, so it is Linux-only; other platforms always -/// record their own descendant tree unprivileged. -pub fn requires_isolation() -> bool { - if !cfg!(target_os = "linux") { - return false; +/// This owns the whole isolation lifecycle: [`resolve`](Self::resolve) runs the +/// machine's pre-bench setup, [`wrap_bench`](Self::wrap_bench) pins the benchmark +/// leaf, and [`Drop`] runs the post-bench teardown. The runner holds one of these +/// for the duration of the run and is otherwise oblivious to *how* cores are +/// attributed — that lives entirely on the machine (its hooks) or in the systemd +/// fallback below. +#[derive(Debug)] +pub enum Isolation { + /// The benchmark runs unpinned, in the runner's own process subtree. + None, + /// The machine's hooks own core attribution. The image built a static cgroup + /// skeleton and ships `codspeed-{pre,wrap,post}-bench`. `pre-bench` (already + /// run, with the runner's PID) placed the runner — and the profiler it later + /// spawns as a child — onto the system cores; `wrap-bench` moves the benchmark + /// onto the bench cores; `post-bench` runs on drop. The benchmark stays a + /// descendant of the profiler, so the profiler records it unprivileged. + Hooks, + /// Gen-1 fallback, used when the machine ships no `codspeed-wrap-bench` hook. + /// `systemd-run --scope --slice=codspeed.slice` reparents the benchmark out of + /// the profiler's subtree, so the profiler needs elevated privileges (sudo) to + /// observe it. + Systemd, +} + +impl Isolation { + /// Resolve how this run isolates the benchmark, running any required setup + /// (the pre-bench hook) as a side effect. Decision, from [`ISOLATION_ENV`]: + /// + /// - non-Linux: [`None`](Self::None), no mechanism is available; + /// - `false`: [`None`](Self::None); + /// - otherwise, on Linux: + /// - if the machine ships an executable `codspeed-wrap-bench`: [`Hooks`](Self::Hooks) + /// (the forward path — the machine owns core attribution); + /// - else if we can elevate without a prompt (root or passwordless sudo, as + /// on CI / gen-1 images): [`Systemd`](Self::Systemd); + /// - else [`None`](Self::None), so a local run never blocks on a password prompt. + pub fn resolve() -> Self { + if !cfg!(target_os = "linux") { + return Isolation::None; + } + + if std::env::var(ISOLATION_ENV).is_ok_and(|v| v == "false") { + return Isolation::None; + } + + if hook_is_executable(WRAP_BENCH_HOOK) { + run_pre_bench(); + return Isolation::Hooks; + } + + if crate::executor::helpers::run_with_sudo::can_elevate_without_prompt() { + return Isolation::Systemd; + } + + if std::env::var(ISOLATION_ENV).is_ok_and(|v| !v.is_empty()) { + info!( + "Running without process isolation: no {WRAP_BENCH_HOOK} hook, and elevating \ + would require a password prompt." + ); + } + Isolation::None + } + + /// Whether the profiler must run under sudo. True only for [`Systemd`](Self::Systemd), + /// where the benchmark is reparented out of the profiler's subtree. + pub fn requires_sudo(&self) -> bool { + matches!(self, Isolation::Systemd) } - match std::env::var("CODSPEED_ISOLATION").as_deref() { - Ok("true") => true, - Ok("false") => false, - _ => { - let can_isolate = crate::executor::helpers::run_with_sudo::can_elevate_without_prompt(); - if !can_isolate { - info!( - "Running without process isolation: elevating privileges would require a \ - password prompt. Set CODSPEED_ISOLATION=true to force it." - ); - } - can_isolate + + /// Wrap the benchmark leaf so it runs pinned according to this mode. The + /// profiler wraps its recorder around the result, so this must only touch the + /// leaf and never move the profiler onto the bench cores. + pub fn wrap_bench(&self, bench_cmd: CommandBuilder) -> Result { + match self { + Isolation::None => Ok(bench_cmd), + Isolation::Hooks => wrap_with_hook(bench_cmd), + Isolation::Systemd => wrap_isolation_scope(bench_cmd), } } } -/// Run the benchmark command in an isolated process scope. -/// -/// On Linux, the command is wrapped with `systemd-run --scope` so it runs inside the -/// `codspeed.slice` cgroup (predefined on CodSpeed CI runners to pin and isolate the -/// benchmark). Only applied when [`requires_isolation`] is true. +impl Drop for Isolation { + fn drop(&mut self) { + if matches!(self, Isolation::Hooks) { + run_hook(POST_BENCH_HOOK, &[]); + } + } +} + +/// Whether `path` is an executable file. Used to detect machine hooks. +fn hook_is_executable(path: &str) -> bool { + #[cfg(target_os = "linux")] + { + use std::os::unix::fs::PermissionsExt; + std::fs::metadata(path) + .map(|m| m.is_file() && m.permissions().mode() & 0o111 != 0) + .unwrap_or(false) + } + #[cfg(not(target_os = "linux"))] + { + let _ = path; + false + } +} + +/// Run a machine hook, logging (never failing the run) on error. Isolation is +/// best-effort: a missing or failing hook leaves the benchmark less isolated, but +/// must not abort the run. +fn run_hook(hook: &str, args: &[String]) { + let output = Command::new(hook).args(args).output(); + match output { + Ok(o) if o.status.success() => {} + Ok(o) => debug!( + "{hook} exited {}: {}", + o.status, + String::from_utf8_lossy(&o.stderr) + ), + Err(e) => debug!("failed to run {hook}: {e}"), + } +} + +/// Run the pre-bench hook with the runner's own PID, so the hook places the runner +/// (and the profiler it later spawns) onto the system cores. +fn run_pre_bench() { + run_hook(PRE_BENCH_HOOK, &[std::process::id().to_string()]); +} + +/// Wrap the benchmark with `codspeed-wrap-bench`, which moves itself onto the +/// bench cores and `exec`s the benchmark — so the benchmark inherits the pinning +/// while staying the same PID and a descendant of the profiler. The hook is a +/// command wrapper: it takes the program and its arguments and `exec`s them. +fn wrap_with_hook(mut bench_cmd: CommandBuilder) -> Result { + bench_cmd.wrap(WRAP_BENCH_HOOK, [] as [&str; 0]); + Ok(bench_cmd) +} + +/// Wrap the benchmark leaf so it runs inside the systemd `codspeed.slice` scope. /// -/// Remarks: -/// - We're using `--scope` so that the profiler is able to capture the events of the benchmark -/// process. -/// - We can't use `--user` here because we need to run in `codspeed.slice`, otherwise we'd run in -/// `user.slice` (which is isolated). We use `--uid` and `--gid` to keep running as the current -/// user. -/// - `--scope` only inherits the system environment, so the caller is expected to have already -/// forwarded the relevant variables (via `wrap_with_env`). -/// - The caller is expected to have already set the working directory on `bench_cmd`; it will be -/// propagated to `systemd-run` via [`CommandBuilder::wrap_with`], and `--same-dir` makes the -/// spawned scope inherit it. +/// Notes on the `systemd-run` flags: +/// - `--scope` (rather than a transient service) keeps the benchmark a child of +/// the runner, so the profiler can capture its events; see [`Isolation::Systemd`]. +/// - `--user` would land us in `user.slice`, not `codspeed.slice`; `--uid`/`--gid` +/// instead keep the scope running as the current user. +/// - `--scope` only inherits the system environment, so the caller must have +/// already forwarded the benchmark's variables (via `wrap_with_env`) and set +/// its working directory, which `--same-dir` propagates into the scope. #[cfg(target_os = "linux")] -pub fn wrap_isolation_scope(mut bench_cmd: CommandBuilder) -> Result { +fn wrap_isolation_scope(mut bench_cmd: CommandBuilder) -> Result { use crate::executor::helpers::env::is_codspeed_debug_enabled; let mut cmd_builder = CommandBuilder::new("systemd-run"); @@ -75,6 +183,45 @@ pub fn wrap_isolation_scope(mut bench_cmd: CommandBuilder) -> Result Result { +fn wrap_isolation_scope(bench_cmd: CommandBuilder) -> Result { Ok(bench_cmd) } + +#[cfg(test)] +mod tests { + use super::*; + use temp_env::with_vars; + + #[test] + fn isolation_false_disables() { + with_vars([(ISOLATION_ENV, Some("false"))], || { + assert!(matches!(Isolation::resolve(), Isolation::None)); + }); + } + + #[test] + fn systemd_requires_sudo_but_hooks_do_not() { + assert!(Isolation::Systemd.requires_sudo()); + assert!(!Isolation::Hooks.requires_sudo()); + assert!(!Isolation::None.requires_sudo()); + } + + #[test] + fn hook_wrap_prepends_wrap_bench() { + let mut cmd = CommandBuilder::new("bash"); + cmd.arg("/tmp/bench.sh"); + let wrapped = Isolation::Hooks.wrap_bench(cmd).unwrap(); + assert_eq!( + wrapped.as_command_line(), + format!("{WRAP_BENCH_HOOK} bash /tmp/bench.sh") + ); + } + + #[test] + fn none_wrap_is_passthrough() { + let mut cmd = CommandBuilder::new("bash"); + cmd.arg("/tmp/bench.sh"); + let wrapped = Isolation::None.wrap_bench(cmd).unwrap(); + assert_eq!(wrapped.as_command_line(), "bash /tmp/bench.sh"); + } +} diff --git a/src/executor/wall_time/profiler/mod.rs b/src/executor/wall_time/profiler/mod.rs index 0ee6becc..ab7f62cb 100644 --- a/src/executor/wall_time/profiler/mod.rs +++ b/src/executor/wall_time/profiler/mod.rs @@ -48,15 +48,16 @@ pub trait Profiler { /// Profilers stash any live state they need for the duration of the run /// (control fifos, output paths) on `self`. /// - /// `isolate` mirrors the decision the executor used to wrap the benchmark in - /// the systemd scope: when set, the profiler records system-wide under sudo; - /// otherwise it records its own descendant tree unprivileged. + /// When `requires_sudo` is set, the profiler must run elevated: the + /// isolation mechanism reparented the benchmark out of its subtree, so it can + /// only observe it with elevated privileges. Otherwise it records its own + /// descendant tree unprivileged. async fn wrap_command( &mut self, cmd: CommandBuilder, config: &ExecutorConfig, profile_folder: &Path, - isolate: bool, + requires_sudo: bool, ) -> anyhow::Result; /// The benchmarked process signaled the start of a measured region. diff --git a/src/executor/wall_time/profiler/perf/mod.rs b/src/executor/wall_time/profiler/perf/mod.rs index 5536af12..2c5514d2 100644 --- a/src/executor/wall_time/profiler/perf/mod.rs +++ b/src/executor/wall_time/profiler/perf/mod.rs @@ -91,7 +91,7 @@ impl Profiler for PerfProfiler { mut cmd_builder: CommandBuilder, config: &ExecutorConfig, profile_folder: &Path, - isolate: bool, + requires_sudo: bool, ) -> anyhow::Result { let perf_fifo = PerfFifo::new()?; let perf_file_path = profile_folder.join(PERF_PIPEDATA_FILE_NAME); @@ -183,10 +183,9 @@ impl Profiler for PerfProfiler { self.perf_fifo = Some(perf_fifo); self.perf_file_path = Some(perf_file_path); - // Isolated runs reparent the benchmark out of perf's subtree, so perf - // must record system-wide under sudo. Unisolated runs record perf's own - // descendant tree unprivileged. - let wrapped_builder = if isolate { + // When the benchmark was reparented out of perf's subtree, perf needs + // sudo to observe it; otherwise it records its own descendants unprivileged. + let wrapped_builder = if requires_sudo { wrap_with_sudo(wrapped_builder)? } else { wrapped_builder diff --git a/src/executor/wall_time/profiler/samply/mod.rs b/src/executor/wall_time/profiler/samply/mod.rs index 7a1a6f04..97b5bffd 100644 --- a/src/executor/wall_time/profiler/samply/mod.rs +++ b/src/executor/wall_time/profiler/samply/mod.rs @@ -86,7 +86,7 @@ impl Profiler for SamplyProfiler { mut cmd_builder: CommandBuilder, _config: &ExecutorConfig, profile_folder: &Path, - isolate: bool, + requires_sudo: bool, ) -> anyhow::Result { let output_path = profile_folder.join(SAMPLY_OUTPUT_FILE_NAME); @@ -157,10 +157,9 @@ impl Profiler for SamplyProfiler { self.output_path = Some(output_path); self.v8_log_dir = Some(v8_log_dir); - // Isolated runs reparent the benchmark out of samply's subtree, so samply - // must record system-wide under sudo. Unisolated runs record samply's own - // descendant tree unprivileged. - let cmd_builder = if isolate { + // When the benchmark was reparented out of samply's subtree, samply needs + // sudo to observe it; otherwise it records its own descendants unprivileged. + let cmd_builder = if requires_sudo { wrap_with_sudo(cmd_builder)? } else { cmd_builder