From 85b3a851b2da3f9dc7632f184292967056c504a4 Mon Sep 17 00:00:00 2001 From: Aidan Hobson Sayers Date: Sat, 24 Aug 2024 21:47:48 +0100 Subject: [PATCH 1/4] Fix preserve-fds flag Signed-off-by: Aidan Hobson Sayers --- .../libcontainer/src/process/container_init_process.rs | 3 +++ .../libcontainer/src/process/container_main_process.rs | 9 --------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/crates/libcontainer/src/process/container_init_process.rs b/crates/libcontainer/src/process/container_init_process.rs index 64182c1c7..741ff7cf6 100644 --- a/crates/libcontainer/src/process/container_init_process.rs +++ b/crates/libcontainer/src/process/container_init_process.rs @@ -508,6 +508,9 @@ pub fn container_init_process( // will be closed after execve into the container payload. We can't close the // fds immediately since we at least still need it for the pipe used to wait on // starting the container. + // + // Note: this should happen very late, in order to avoid accidentally leaking FDs + // Please refer to https://github.com/opencontainers/runc/security/advisories/GHSA-xr7r-f8xq-vfvv for more details. syscall.close_range(preserve_fds).map_err(|err| { tracing::error!(?err, "failed to cleanup extra fds"); InitProcessError::SyscallOther(err) diff --git a/crates/libcontainer/src/process/container_main_process.rs b/crates/libcontainer/src/process/container_main_process.rs index fd5dfcb1c..7739a89da 100644 --- a/crates/libcontainer/src/process/container_main_process.rs +++ b/crates/libcontainer/src/process/container_main_process.rs @@ -75,15 +75,6 @@ pub fn container_main_process(container_args: &ContainerArgs) -> Result<(Pid, bo }) }; - // Before starting the intermediate process, mark all non-stdio open files as O_CLOEXEC - // to ensure we don't leak any file descriptors to the intermediate process. - // Please refer to https://github.com/opencontainers/runc/security/advisories/GHSA-xr7r-f8xq-vfvv for more details. - let syscall = container_args.syscall.create_syscall(); - syscall.close_range(0).map_err(|err| { - tracing::error!(?err, "failed to cleanup extra fds"); - ProcessError::SyscallOther(err) - })?; - let intermediate_pid = fork::container_clone(cb).map_err(|err| { tracing::error!("failed to fork intermediate process: {}", err); ProcessError::IntermediateProcessFailed(err) From 0a48e106bd8f1e2d97768eef516ba242133f39db Mon Sep 17 00:00:00 2001 From: Aidan Hobson Sayers Date: Sat, 24 Aug 2024 21:48:15 +0100 Subject: [PATCH 2/4] Fix a stray FD leaking in containers when using preserve-fd Signed-off-by: Aidan Hobson Sayers --- crates/libcontainer/src/syscall/linux.rs | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/crates/libcontainer/src/syscall/linux.rs b/crates/libcontainer/src/syscall/linux.rs index 9bc2f13de..b93996504 100644 --- a/crates/libcontainer/src/syscall/linux.rs +++ b/crates/libcontainer/src/syscall/linux.rs @@ -17,7 +17,7 @@ use nix::fcntl::{open, OFlag}; use nix::mount::{mount, umount2, MntFlags, MsFlags}; use nix::sched::{unshare, CloneFlags}; use nix::sys::stat::{mknod, Mode, SFlag}; -use nix::unistd::{chown, chroot, fchdir, pivot_root, sethostname, Gid, Uid}; +use nix::unistd::{chown, chroot, close, fchdir, pivot_root, sethostname, Gid, Uid}; use oci_spec::runtime::PosixRlimit; use super::{Result, Syscall, SyscallError}; @@ -232,11 +232,15 @@ impl Syscall for LinuxSyscall { /// Function to set given path as root path inside process fn pivot_rootfs(&self, path: &Path) -> Result<()> { // open the path as directory and read only - let newroot = - open(path, OFlag::O_DIRECTORY | OFlag::O_RDONLY, Mode::empty()).map_err(|errno| { - tracing::error!(?errno, ?path, "failed to open the new root for pivot root"); - errno - })?; + let newroot = open( + path, + OFlag::O_DIRECTORY | OFlag::O_RDONLY | OFlag::O_CLOEXEC, + Mode::empty(), + ) + .map_err(|errno| { + tracing::error!(?errno, ?path, "failed to open the new root for pivot root"); + errno + })?; // make the given path as the root directory for the container // see https://man7.org/linux/man-pages/man2/pivot_root.2.html, specially the notes @@ -279,6 +283,11 @@ impl Syscall for LinuxSyscall { errno })?; + close(newroot).map_err(|errno| { + tracing::error!(?errno, ?newroot, "failed to close new root directory"); + errno + })?; + Ok(()) } From fc5c131ea1c877ba79cd41b0c9cc9928d56605c3 Mon Sep 17 00:00:00 2001 From: Aidan Hobson Sayers Date: Mon, 2 Sep 2024 02:25:50 +0100 Subject: [PATCH 3/4] Add tests for preserve-fds, extend test harness Test harness additionally needed to support 1. tests that cannot run in parallel 2. tests that need to customise create arguments Signed-off-by: Aidan Hobson Sayers --- tests/contest/contest/src/main.rs | 3 + .../contest/src/tests/fd_control/mod.rs | 107 ++++++++++++++++++ .../contest/contest/src/tests/hooks/invoke.rs | 5 +- .../src/tests/lifecycle/container_create.rs | 4 + .../tests/lifecycle/container_lifecycle.rs | 4 + tests/contest/contest/src/tests/mod.rs | 1 + .../contest/src/tests/pidfile/pidfile_test.rs | 28 ++--- tests/contest/contest/src/utils/mod.rs | 2 +- tests/contest/contest/src/utils/test_utils.rs | 16 ++- tests/contest/runtimetest/src/main.rs | 1 + tests/contest/runtimetest/src/tests.rs | 45 ++++++++ .../contest/test_framework/src/test_group.rs | 90 ++++++++++----- .../test_framework/src/test_manager.rs | 29 +++++ tests/contest/test_framework/src/testable.rs | 1 + 14 files changed, 285 insertions(+), 51 deletions(-) create mode 100644 tests/contest/contest/src/tests/fd_control/mod.rs diff --git a/tests/contest/contest/src/main.rs b/tests/contest/contest/src/main.rs index 8049457c0..9fcbe82be 100644 --- a/tests/contest/contest/src/main.rs +++ b/tests/contest/contest/src/main.rs @@ -12,6 +12,7 @@ use tests::cgroups; use crate::tests::devices::get_devices_test; use crate::tests::domainname::get_domainname_tests; use crate::tests::example::get_example_test; +use crate::tests::fd_control::get_fd_control_test; use crate::tests::hooks::get_hooks_tests; use crate::tests::hostname::get_hostname_test; use crate::tests::intel_rdt::get_intel_rdt_test; @@ -113,6 +114,7 @@ fn main() -> Result<()> { let scheduler = get_scheduler_test(); let io_priority_test = get_io_priority_test(); let devices = get_devices_test(); + let fd_control = get_fd_control_test(); tm.add_test_group(Box::new(cl)); tm.add_test_group(Box::new(cc)); @@ -136,6 +138,7 @@ fn main() -> Result<()> { tm.add_test_group(Box::new(sysctl)); tm.add_test_group(Box::new(scheduler)); tm.add_test_group(Box::new(devices)); + tm.add_test_group(Box::new(fd_control)); tm.add_test_group(Box::new(io_priority_test)); tm.add_cleanup(Box::new(cgroups::cleanup_v1)); diff --git a/tests/contest/contest/src/tests/fd_control/mod.rs b/tests/contest/contest/src/tests/fd_control/mod.rs new file mode 100644 index 000000000..95caa33f1 --- /dev/null +++ b/tests/contest/contest/src/tests/fd_control/mod.rs @@ -0,0 +1,107 @@ +use std::fs; +use std::os::fd::{AsRawFd, RawFd}; + +use anyhow::{anyhow, Context, Result}; +use oci_spec::runtime::{ProcessBuilder, Spec, SpecBuilder}; +use test_framework::{test_result, Test, TestGroup, TestResult}; + +use crate::utils::test_inside_container_create_args; + +fn create_spec() -> Result { + SpecBuilder::default() + .process( + ProcessBuilder::default() + .args( + ["runtimetest", "fd_control"] + .iter() + .map(|s| s.to_string()) + .collect::>(), + ) + .build()?, + ) + .build() + .context("failed to create spec") +} + +fn open_devnull_no_cloexec() -> Result<(fs::File, RawFd)> { + // Rust std by default sets cloexec, so we undo it + let devnull = fs::File::open("/dev/null")?; + let devnull_fd = devnull.as_raw_fd(); + let flags = nix::fcntl::fcntl(devnull_fd, nix::fcntl::FcntlArg::F_GETFD)?; + let mut flags = nix::fcntl::FdFlag::from_bits_retain(flags); + flags.remove(nix::fcntl::FdFlag::FD_CLOEXEC); + nix::fcntl::fcntl(devnull_fd, nix::fcntl::FcntlArg::F_SETFD(flags))?; + Ok((devnull, devnull_fd)) +} + +// If not opening any other FDs, verify youki itself doesnt open anything that gets +// leaked in if passing --preserve-fds with a large number +// NOTE: this will also fail if the test harness itself starts leaking FDs +fn only_stdio_test() -> TestResult { + let spec = test_result!(create_spec()); + test_inside_container_create_args( + spec, + &|bundle_path| { + fs::write(bundle_path.join("num-fds"), "0".as_bytes())?; + Ok(()) + }, + &["--preserve-fds".as_ref(), "100".as_ref()], + ) +} + +// If we know we have an open FD without cloexec, it should be closed if preserve-fds +// is 0 (the default) +fn closes_fd_test() -> TestResult { + // Open this before the setup function so it's kept alive for the container lifetime + let (_devnull, _devnull_fd) = match open_devnull_no_cloexec() { + Ok(v) => v, + Err(e) => return TestResult::Failed(anyhow!("failed to open dev null: {}", e)), + }; + + let spec = test_result!(create_spec()); + test_inside_container_create_args( + spec, + &|bundle_path| { + fs::write(bundle_path.join("num-fds"), "0".as_bytes())?; + Ok(()) + }, + &["--preserve-fds".as_ref(), "0".as_ref()], + ) +} + +// Given an open FD, verify it can be passed down with preserve-fds +fn pass_single_fd_test() -> TestResult { + // Open this before the setup function so it's kept alive for the container lifetime + let (_devnull, devnull_fd) = match open_devnull_no_cloexec() { + Ok(v) => v, + Err(e) => return TestResult::Failed(anyhow!("failed to open dev null: {}", e)), + }; + + let spec = test_result!(create_spec()); + test_inside_container_create_args( + spec, + &|bundle_path| { + fs::write(bundle_path.join("num-fds"), "1".as_bytes())?; + Ok(()) + }, + &[ + "--preserve-fds".as_ref(), + (devnull_fd - 2).to_string().as_ref(), // relative to stdio + ], + ) +} + +pub fn get_fd_control_test() -> TestGroup { + let mut test_group = TestGroup::new("fd_control"); + test_group.set_nonparallel(); // fds are process-wide state + let test_only_stdio = Test::new("only_stdio", Box::new(only_stdio_test)); + let test_closes_fd = Test::new("closes_fd", Box::new(closes_fd_test)); + let test_pass_single_fd = Test::new("pass_single_fd", Box::new(pass_single_fd_test)); + test_group.add(vec![ + Box::new(test_only_stdio), + Box::new(test_closes_fd), + Box::new(test_pass_single_fd), + ]); + + test_group +} diff --git a/tests/contest/contest/src/tests/hooks/invoke.rs b/tests/contest/contest/src/tests/hooks/invoke.rs index 7cd22dac2..f8e0ca701 100644 --- a/tests/contest/contest/src/tests/hooks/invoke.rs +++ b/tests/contest/contest/src/tests/hooks/invoke.rs @@ -71,7 +71,10 @@ fn get_test(test_name: &'static str) -> Test { let id_str = id.to_string(); let bundle = prepare_bundle().unwrap(); set_config(&bundle, &spec).unwrap(); - create_container(&id_str, &bundle).unwrap().wait().unwrap(); + create_container(&id_str, &bundle, &[]) + .unwrap() + .wait() + .unwrap(); start_container(&id_str, &bundle).unwrap().wait().unwrap(); delete_container(&id_str, &bundle).unwrap().wait().unwrap(); let log = { diff --git a/tests/contest/contest/src/tests/lifecycle/container_create.rs b/tests/contest/contest/src/tests/lifecycle/container_create.rs index a2021bff7..2f2050b04 100644 --- a/tests/contest/contest/src/tests/lifecycle/container_create.rs +++ b/tests/contest/contest/src/tests/lifecycle/container_create.rs @@ -82,6 +82,10 @@ impl TestableGroup for ContainerCreate { "create" } + fn parallel(&self) -> bool { + true + } + fn run_all(&self) -> Vec<(&'static str, TestResult)> { vec![ ("empty_id", self.create_empty_id()), diff --git a/tests/contest/contest/src/tests/lifecycle/container_lifecycle.rs b/tests/contest/contest/src/tests/lifecycle/container_lifecycle.rs index 6b925feea..b33f3364c 100644 --- a/tests/contest/contest/src/tests/lifecycle/container_lifecycle.rs +++ b/tests/contest/contest/src/tests/lifecycle/container_lifecycle.rs @@ -93,6 +93,10 @@ impl TestableGroup for ContainerLifecycle { "lifecycle" } + fn parallel(&self) -> bool { + true + } + fn run_all(&self) -> Vec<(&'static str, TestResult)> { vec![ ("create", self.create()), diff --git a/tests/contest/contest/src/tests/mod.rs b/tests/contest/contest/src/tests/mod.rs index 1fee606b1..3e7bb8e11 100644 --- a/tests/contest/contest/src/tests/mod.rs +++ b/tests/contest/contest/src/tests/mod.rs @@ -2,6 +2,7 @@ pub mod cgroups; pub mod devices; pub mod domainname; pub mod example; +pub mod fd_control; pub mod hooks; pub mod hostname; pub mod intel_rdt; diff --git a/tests/contest/contest/src/tests/pidfile/pidfile_test.rs b/tests/contest/contest/src/tests/pidfile/pidfile_test.rs index 3d545bfd4..88bdd5000 100644 --- a/tests/contest/contest/src/tests/pidfile/pidfile_test.rs +++ b/tests/contest/contest/src/tests/pidfile/pidfile_test.rs @@ -1,12 +1,11 @@ use std::fs::File; -use std::process::{Command, Stdio}; use anyhow::anyhow; use test_framework::{Test, TestGroup, TestResult}; use uuid::Uuid; use crate::utils::{ - delete_container, generate_uuid, get_runtime_path, get_state, kill_container, prepare_bundle, + create_container, delete_container, generate_uuid, get_state, kill_container, prepare_bundle, State, }; @@ -19,6 +18,7 @@ fn cleanup(id: &Uuid, bundle: &tempfile::TempDir) { // here we have to manually create and manage the container // as the test_inside container does not provide a way to set the pid file argument +// TODO: this comment is now out of date, the test just needs updating fn test_pidfile() -> TestResult { // create id for the container and pidfile let container_id = generate_uuid(); @@ -30,22 +30,14 @@ fn test_pidfile() -> TestResult { let _ = File::create(&pidfile_path).unwrap(); // start the container - Command::new(get_runtime_path()) - .stdin(Stdio::null()) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .arg("--root") - .arg(bundle.as_ref().join("runtime")) - .arg("create") - .arg(container_id.to_string()) - .arg("--bundle") - .arg(bundle.as_ref().join("bundle")) - .arg("--pid-file") - .arg(pidfile_path) - .spawn() - .unwrap() - .wait() - .unwrap(); + create_container( + &container_id.to_string(), + &bundle, + &["--pid-file".as_ref(), pidfile_path.as_ref()], + ) + .unwrap() + .wait() + .unwrap(); let (out, err) = get_state(&container_id.to_string(), &bundle).unwrap(); diff --git a/tests/contest/contest/src/utils/mod.rs b/tests/contest/contest/src/utils/mod.rs index 9f7c3eccb..eacca6210 100644 --- a/tests/contest/contest/src/utils/mod.rs +++ b/tests/contest/contest/src/utils/mod.rs @@ -7,5 +7,5 @@ pub use support::{ }; pub use test_utils::{ create_container, delete_container, get_state, kill_container, test_inside_container, - test_outside_container, State, + test_inside_container_create_args, test_outside_container, State, }; diff --git a/tests/contest/contest/src/utils/test_utils.rs b/tests/contest/contest/src/utils/test_utils.rs index c72cd8f0d..82b82ed5b 100644 --- a/tests/contest/contest/src/utils/test_utils.rs +++ b/tests/contest/contest/src/utils/test_utils.rs @@ -1,6 +1,7 @@ //! Contains utility functions for testing //! Similar to https://github.com/opencontainers/runtime-tools/blob/master/validation/util/test.go use std::collections::HashMap; +use std::ffi::OsStr; use std::path::{Path, PathBuf}; use std::process::{Child, Command, ExitStatus, Stdio}; use std::thread::sleep; @@ -43,7 +44,7 @@ pub struct ContainerData { } /// Starts the runtime with given directory as root directory -pub fn create_container>(id: &str, dir: P) -> Result { +pub fn create_container>(id: &str, dir: P, extra_args: &[&OsStr]) -> Result { let res = Command::new(get_runtime_path()) // set stdio so that we can get o/p of runtimetest // in test_inside_container function @@ -55,6 +56,7 @@ pub fn create_container>(id: &str, dir: P) -> Result { .arg(id) .arg("--bundle") .arg(dir.as_ref().join("bundle")) + .args(extra_args) .spawn() .context("could not create container")?; Ok(res) @@ -121,7 +123,7 @@ pub fn test_outside_container( let id_str = id.to_string(); let bundle = prepare_bundle().unwrap(); set_config(&bundle, &spec).unwrap(); - let create_result = create_container(&id_str, &bundle).unwrap().wait(); + let create_result = create_container(&id_str, &bundle, &[]).unwrap().wait(); let (out, err) = get_state(&id_str, &bundle).unwrap(); let state: Option = match serde_json::from_str(&out) { Ok(v) => Some(v), @@ -143,6 +145,14 @@ pub fn test_outside_container( pub fn test_inside_container( spec: Spec, setup_for_test: &dyn Fn(&Path) -> Result<()>, +) -> TestResult { + test_inside_container_create_args(spec, setup_for_test, &[]) +} + +pub fn test_inside_container_create_args( + spec: Spec, + setup_for_test: &dyn Fn(&Path) -> Result<()>, + create_args: &[&OsStr], ) -> TestResult { let id = generate_uuid(); let id_str = id.to_string(); @@ -176,7 +186,7 @@ pub fn test_inside_container( .join("runtimetest"), ) .unwrap(); - let create_process = create_container(&id_str, &bundle).unwrap(); + let create_process = create_container(&id_str, &bundle, create_args).unwrap(); // here we do not wait for the process by calling wait() as in the test_outside_container // function because we need the output of the runtimetest. If we call wait, it will return // and we won't have an easy way of getting the stdio of the runtimetest. diff --git a/tests/contest/runtimetest/src/main.rs b/tests/contest/runtimetest/src/main.rs index 95780bd48..d6e667fbb 100644 --- a/tests/contest/runtimetest/src/main.rs +++ b/tests/contest/runtimetest/src/main.rs @@ -44,6 +44,7 @@ fn main() { "io_priority_class_be" => tests::test_io_priority_class(&spec, IoprioClassBe), "io_priority_class_idle" => tests::test_io_priority_class(&spec, IoprioClassIdle), "devices" => tests::validate_devices(&spec), + "fd_control" => tests::validate_fd_control(&spec), _ => eprintln!("error due to unexpected execute test name: {execute_test}"), } } diff --git a/tests/contest/runtimetest/src/tests.rs b/tests/contest/runtimetest/src/tests.rs index 40f5ad29c..33a8cf801 100644 --- a/tests/contest/runtimetest/src/tests.rs +++ b/tests/contest/runtimetest/src/tests.rs @@ -1,3 +1,4 @@ +use std::ffi::OsStr; use std::fs::{self, read_dir}; use std::os::linux::fs::MetadataExt; use std::os::unix::fs::{FileTypeExt, PermissionsExt}; @@ -545,3 +546,47 @@ pub fn test_io_priority_class(spec: &Spec, io_priority_class: IOPriorityClass) { eprintln!("error ioprio_get expected priority {expected_priority:?}, got {priority}") } } + +pub fn validate_fd_control(_spec: &Spec) { + // --preserve-fds does not get passed via the spec so we have to communicate information + // via the root filesystem + let expected_num_fds: usize = fs::read_to_string("/num-fds").unwrap().parse().unwrap(); + + let mut entries = vec![]; + let stdio: &[&OsStr] = &["0".as_ref(), "1".as_ref(), "2".as_ref()]; + for entry in fs::read_dir("/proc/self/fd").unwrap() { + let entry = entry.unwrap(); + let name = entry.file_name(); + if stdio.contains(&name.as_os_str()) { + // Ignore stdio + continue; + } + entries.push((entry.path(), fs::read_link(entry.path()))) + } + + // NOTE: we do this in a separate loop so we can filter out the dirfd used behind + // the scenes in 'fs::read_dir'. It is important to *not* store the full DirEntry + // type, as that keeps the dirfd open. + let mut fd_details = vec![]; + let mut found_dirfd = false; + for (path, linkpath) in &entries { + println!("found fd in container {} {:?}", path.display(), linkpath); + // The difference between metadata.unwrap() and fs::metadata is that the latter + // will now try to follow the symlink + match fs::metadata(path) { + Ok(m) => fd_details.push((path, linkpath, m)), + Err(e) if e.kind() == std::io::ErrorKind::NotFound && !found_dirfd => { + // Expected for the dirfd + println!("(ignoring dirfd)"); + found_dirfd = true + } + Err(e) => { + eprintln!("unexpected error reading metadata: {}", e) + } + } + } + + if fd_details.len() != expected_num_fds { + eprintln!("mismatched fds inside container! {:?}", fd_details); + } +} diff --git a/tests/contest/test_framework/src/test_group.rs b/tests/contest/test_framework/src/test_group.rs index b5a01ec6b..771041165 100644 --- a/tests/contest/test_framework/src/test_group.rs +++ b/tests/contest/test_framework/src/test_group.rs @@ -9,6 +9,9 @@ use crate::testable::{TestResult, Testable, TestableGroup}; pub struct TestGroup { /// name of the test group name: &'static str, + /// can the test group be executed in parallel (both the tests + /// within it, and alongside other test groups) + parallel: bool, /// tests belonging to this group tests: BTreeMap<&'static str, Box>, } @@ -18,10 +21,16 @@ impl TestGroup { pub fn new(name: &'static str) -> Self { TestGroup { name, + parallel: true, tests: BTreeMap::new(), } } + /// mark the test group as unsuitable for parallel execution + pub fn set_nonparallel(&mut self) { + self.parallel = false + } + /// add a test to the group pub fn add(&mut self, tests: Vec>) { tests.into_iter().for_each(|t| { @@ -36,26 +45,41 @@ impl TestableGroup for TestGroup { self.name } + /// can this test group be executed (within itself, and alongside other groups) + fn parallel(&self) -> bool { + self.parallel + } + /// run all the test from the test group fn run_all(&self) -> Vec<(&'static str, TestResult)> { let mut ret = Vec::with_capacity(self.tests.len()); - thread::scope(|s| { - let mut collector = Vec::with_capacity(self.tests.len()); + if self.parallel { + thread::scope(|s| { + let mut collector = Vec::with_capacity(self.tests.len()); + for (_, t) in self.tests.iter() { + let _t = s.spawn(move |_| { + if t.can_run() { + (t.get_name(), t.run()) + } else { + (t.get_name(), TestResult::Skipped) + } + }); + collector.push(_t); + } + for handle in collector { + ret.push(handle.join().unwrap()); + } + }) + .unwrap(); + } else { for (_, t) in self.tests.iter() { - let _t = s.spawn(move |_| { - if t.can_run() { - (t.get_name(), t.run()) - } else { - (t.get_name(), TestResult::Skipped) - } + ret.push(if t.can_run() { + (t.get_name(), t.run()) + } else { + (t.get_name(), TestResult::Skipped) }); - collector.push(_t); } - for handle in collector { - ret.push(handle.join().unwrap()); - } - }) - .unwrap(); + } ret } @@ -66,23 +90,33 @@ impl TestableGroup for TestGroup { .iter() .filter(|(name, _)| selected.contains(name)); let mut ret = Vec::with_capacity(selected.len()); - thread::scope(|s| { - let mut collector = Vec::with_capacity(selected.len()); + if self.parallel { + thread::scope(|s| { + let mut collector = Vec::with_capacity(selected.len()); + for (_, t) in selected_tests { + let _t = s.spawn(move |_| { + if t.can_run() { + (t.get_name(), t.run()) + } else { + (t.get_name(), TestResult::Skipped) + } + }); + collector.push(_t); + } + for handle in collector { + ret.push(handle.join().unwrap()); + } + }) + .unwrap(); + } else { for (_, t) in selected_tests { - let _t = s.spawn(move |_| { - if t.can_run() { - (t.get_name(), t.run()) - } else { - (t.get_name(), TestResult::Skipped) - } + ret.push(if t.can_run() { + (t.get_name(), t.run()) + } else { + (t.get_name(), TestResult::Skipped) }); - collector.push(_t); } - for handle in collector { - ret.push(handle.join().unwrap()); - } - }) - .unwrap(); + } ret } } diff --git a/tests/contest/test_framework/src/test_manager.rs b/tests/contest/test_framework/src/test_manager.rs index 0bccdefd9..2971e24b6 100644 --- a/tests/contest/test_framework/src/test_manager.rs +++ b/tests/contest/test_framework/src/test_manager.rs @@ -64,6 +64,9 @@ impl TestManager { thread::scope(|s| { let mut collector = Vec::with_capacity(self.test_groups.len()); for (name, tg) in &self.test_groups { + if !tg.parallel() { + continue; + } let r = s.spawn(move |_| tg.run_all()); collector.push((name, r)); } @@ -72,6 +75,13 @@ impl TestManager { } }) .unwrap(); + for (name, tg) in &self.test_groups { + if tg.parallel() { + continue; + } + self.print_test_result(name, &tg.run_all()) + } + for cleaner in &self.cleanup { if let Err(e) = cleaner() { print!("Failed to cleanup: {e}"); @@ -85,6 +95,9 @@ impl TestManager { let mut collector = Vec::with_capacity(tests.len()); for (test_group_name, tests) in &tests { if let Some(tg) = self.test_groups.get(test_group_name) { + if !tg.parallel() { + continue; + } let r = match tests { None => s.spawn(move |_| tg.run_all()), Some(tests) => s.spawn(move |_| tg.run_selected(tests)), @@ -99,6 +112,22 @@ impl TestManager { } }) .unwrap(); + for (test_group_name, tests) in &tests { + if let Some(tg) = self.test_groups.get(test_group_name) { + if tg.parallel() { + continue; + } + self.print_test_result( + test_group_name, + &match tests { + None => tg.run_all(), + Some(tests) => tg.run_selected(tests), + }, + ); + } else { + // We've already printed errors for not finding tests + } + } for cleaner in &self.cleanup { if let Err(e) = cleaner() { diff --git a/tests/contest/test_framework/src/testable.rs b/tests/contest/test_framework/src/testable.rs index 2c90986b8..b1e15e311 100644 --- a/tests/contest/test_framework/src/testable.rs +++ b/tests/contest/test_framework/src/testable.rs @@ -39,6 +39,7 @@ pub trait Testable { /// Test groups are used to group tests in sensible manner as well as provide namespacing to tests pub trait TestableGroup { fn get_name(&self) -> &'static str; + fn parallel(&self) -> bool; fn run_all(&self) -> Vec<(&'static str, TestResult)>; fn run_selected(&self, selected: &[&str]) -> Vec<(&'static str, TestResult)>; } From 20d9be4a274a9caab330d42310d3bdb87757fa4b Mon Sep 17 00:00:00 2001 From: Aidan Hobson Sayers Date: Mon, 2 Sep 2024 15:52:59 +0100 Subject: [PATCH 4/4] Opt-out a test from runc that it errors on Signed-off-by: Aidan Hobson Sayers --- tests/contest/contest/src/tests/fd_control/mod.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/tests/contest/contest/src/tests/fd_control/mod.rs b/tests/contest/contest/src/tests/fd_control/mod.rs index 95caa33f1..064bfdf9f 100644 --- a/tests/contest/contest/src/tests/fd_control/mod.rs +++ b/tests/contest/contest/src/tests/fd_control/mod.rs @@ -3,9 +3,9 @@ use std::os::fd::{AsRawFd, RawFd}; use anyhow::{anyhow, Context, Result}; use oci_spec::runtime::{ProcessBuilder, Spec, SpecBuilder}; -use test_framework::{test_result, Test, TestGroup, TestResult}; +use test_framework::{test_result, ConditionalTest, Test, TestGroup, TestResult}; -use crate::utils::test_inside_container_create_args; +use crate::utils::{is_runtime_runc, test_inside_container_create_args}; fn create_spec() -> Result { SpecBuilder::default() @@ -94,11 +94,16 @@ fn pass_single_fd_test() -> TestResult { pub fn get_fd_control_test() -> TestGroup { let mut test_group = TestGroup::new("fd_control"); test_group.set_nonparallel(); // fds are process-wide state - let test_only_stdio = Test::new("only_stdio", Box::new(only_stdio_test)); + let test_only_stdio = ConditionalTest::new( + "only_stdio", + // runc errors if any of the N passed FDs via preserve-fd are not currently open + Box::new(|| !is_runtime_runc()), + Box::new(only_stdio_test), + ); let test_closes_fd = Test::new("closes_fd", Box::new(closes_fd_test)); let test_pass_single_fd = Test::new("pass_single_fd", Box::new(pass_single_fd_test)); + test_group.add(vec![Box::new(test_only_stdio)]); test_group.add(vec![ - Box::new(test_only_stdio), Box::new(test_closes_fd), Box::new(test_pass_single_fd), ]);