From ada9cd19eea4ffc9c760f6297e2a280d16cf70b4 Mon Sep 17 00:00:00 2001 From: utam0k Date: Fri, 3 May 2024 22:01:54 +0900 Subject: [PATCH 1/7] Remove unnecessary chdir Co-authored-by: jiaxiao zhou Co-authored-by: Jorge Prendes Signed-off-by: utam0k --- Vagrantfile.containerd2youki | 3 +- .../src/container/container_start.rs | 7 +- .../src/container/init_builder.rs | 9 -- .../src/container/tenant_builder.rs | 3 +- crates/libcontainer/src/tty.rs | 82 ++++++++----------- justfile | 4 + 6 files changed, 42 insertions(+), 66 deletions(-) diff --git a/Vagrantfile.containerd2youki b/Vagrantfile.containerd2youki index d23588dc1..c28822658 100644 --- a/Vagrantfile.containerd2youki +++ b/Vagrantfile.containerd2youki @@ -44,8 +44,7 @@ Vagrant.configure("2") do |config| ./script/setup/install-cni ./script/setup/install-critools rm -rf /bin/runc /sbin/runc /usr/sbin/runc /usr/bin/runc - - cp /vagrant/youki/youki /usr/bin/runc + ln -s /vagrant/youki/youki /usr/bin/runc SHELL end diff --git a/crates/libcontainer/src/container/container_start.rs b/crates/libcontainer/src/container/container_start.rs index c6805511c..9b893b0b4 100644 --- a/crates/libcontainer/src/container/container_start.rs +++ b/crates/libcontainer/src/container/container_start.rs @@ -6,7 +6,7 @@ use crate::{ }; use super::{Container, ContainerStatus}; -use nix::{sys::signal, unistd}; +use nix::sys::signal; impl Container { /// Starts a previously created container @@ -59,11 +59,6 @@ impl Container { })?; } - unistd::chdir(self.root.as_os_str()).map_err(|err| { - tracing::error!("failed to change directory to container root: {}", err); - LibcontainerError::OtherSyscall(err) - })?; - let mut notify_socket = NotifySocket::new(self.root.join(NOTIFY_FILE)); notify_socket.notify_container_start()?; self.set_status(ContainerStatus::Running) diff --git a/crates/libcontainer/src/container/init_builder.rs b/crates/libcontainer/src/container/init_builder.rs index f98781966..3a840098b 100644 --- a/crates/libcontainer/src/container/init_builder.rs +++ b/crates/libcontainer/src/container/init_builder.rs @@ -1,4 +1,3 @@ -use nix::unistd; use oci_spec::runtime::Spec; use std::{ fs, @@ -61,14 +60,6 @@ impl InitContainerBuilder { .set_systemd(self.use_systemd) .set_annotations(spec.annotations().clone()); - unistd::chdir(&container_dir).map_err(|err| { - tracing::error!( - ?container_dir, - ?err, - "failed to chdir into the container directory" - ); - LibcontainerError::OtherSyscall(err) - })?; let notify_path = container_dir.join(NOTIFY_FILE); // convert path of root file system of the container to absolute path let rootfs = fs::canonicalize(spec.root().as_ref().ok_or(MissingSpecError::Root)?.path()) diff --git a/crates/libcontainer/src/container/tenant_builder.rs b/crates/libcontainer/src/container/tenant_builder.rs index adc1cdf11..d06cb508d 100644 --- a/crates/libcontainer/src/container/tenant_builder.rs +++ b/crates/libcontainer/src/container/tenant_builder.rs @@ -1,6 +1,6 @@ use caps::Capability; use nix::fcntl::OFlag; -use nix::unistd::{self, close, pipe2, read, Pid}; +use nix::unistd::{close, pipe2, read, Pid}; use oci_spec::runtime::{ Capabilities as SpecCapabilities, Capability as SpecCapability, LinuxBuilder, LinuxCapabilities, LinuxCapabilitiesBuilder, LinuxNamespace, LinuxNamespaceBuilder, @@ -108,7 +108,6 @@ impl TenantContainerBuilder { tracing::debug!("{:#?}", spec); - unistd::chdir(&container_dir).map_err(LibcontainerError::OtherSyscall)?; let notify_path = Self::setup_notify_listener(&container_dir)?; // convert path of root file system of the container to absolute path let rootfs = fs::canonicalize(spec.root().as_ref().ok_or(MissingSpecError::Root)?.path()) diff --git a/crates/libcontainer/src/tty.rs b/crates/libcontainer/src/tty.rs index b58140af4..c4f2dbd31 100644 --- a/crates/libcontainer/src/tty.rs +++ b/crates/libcontainer/src/tty.rs @@ -93,7 +93,7 @@ pub fn setup_console_socket( ); let csocketfd = match socket::connect( csocketfd.as_raw_fd(), - &socket::UnixAddr::new(socket_name).map_err(|err| TTYError::InvalidSocketName { + &socket::UnixAddr::new(linked.as_path()).map_err(|err| TTYError::InvalidSocketName { source: err, socket_name: socket_name.to_string(), })?, @@ -165,84 +165,72 @@ fn connect_stdio(stdin: &RawFd, stdout: &RawFd, stderr: &RawFd) -> Result<()> { mod tests { use super::*; - use anyhow::Result; + use anyhow::{Ok, Result}; use serial_test::serial; - use std::env; - use std::fs::{self, File}; + use std::fs::File; use std::os::unix::net::UnixListener; - use std::path::PathBuf; const CONSOLE_SOCKET: &str = "console-socket"; - fn setup() -> Result<(tempfile::TempDir, PathBuf, PathBuf)> { - let testdir = tempfile::tempdir()?; - let rundir_path = Path::join(testdir.path(), "run"); - fs::create_dir(&rundir_path)?; - let socket_path = Path::new(&rundir_path).join("socket"); - let _ = File::create(&socket_path); - env::set_current_dir(&testdir)?; - Ok((testdir, rundir_path, socket_path)) - } - #[test] #[serial] - fn test_setup_console_socket() { - let init = setup(); - assert!(init.is_ok()); - let (testdir, rundir_path, socket_path) = init.unwrap(); - let lis = UnixListener::bind(Path::join(testdir.path(), "console-socket")); + fn test_setup_console_socket() -> Result<()> { + let testdir = tempfile::tempdir()?; + let socket_path = Path::join(testdir.path(), "test-socket"); + let lis = UnixListener::bind(&socket_path); assert!(lis.is_ok()); - let fd = setup_console_socket(&rundir_path, &socket_path, CONSOLE_SOCKET); - assert!(fd.is_ok()); - assert_ne!(fd.unwrap().as_raw_fd(), -1); + let fd = setup_console_socket(testdir.path(), &socket_path, CONSOLE_SOCKET)?; + assert_ne!(fd.as_raw_fd(), -1); + Ok(()) } #[test] #[serial] - fn test_setup_console_socket_empty() { - let init = setup(); - assert!(init.is_ok()); - let (_testdir, rundir_path, socket_path) = init.unwrap(); - let fd = setup_console_socket(&rundir_path, &socket_path, CONSOLE_SOCKET); - assert!(fd.is_ok()); - assert_eq!(fd.unwrap().as_raw_fd(), -1); + fn test_setup_console_socket_empty() -> Result<()> { + let testdir = tempfile::tempdir()?; + let socket_path = Path::join(testdir.path(), "test-socket"); + let fd = setup_console_socket(testdir.path(), &socket_path, CONSOLE_SOCKET)?; + assert_eq!(fd.as_raw_fd(), -1); + Ok(()) } #[test] #[serial] - fn test_setup_console_socket_invalid() { - let init = setup(); - assert!(init.is_ok()); - let (testdir, rundir_path, socket_path) = init.unwrap(); + fn test_setup_console_socket_invalid() -> Result<()> { + let testdir = tempfile::tempdir()?; + let socket_path = Path::join(testdir.path(), "test-socket"); let _socket = File::create(Path::join(testdir.path(), "console-socket")); assert!(_socket.is_ok()); - let fd = setup_console_socket(&rundir_path, &socket_path, CONSOLE_SOCKET); + let fd = setup_console_socket(testdir.path(), &socket_path, CONSOLE_SOCKET); assert!(fd.is_err()); + + Ok(()) } #[test] #[serial] - fn test_setup_console() { - let init = setup(); + fn test_setup_console() -> Result<()> { + let testdir = tempfile::tempdir()?; + let socket_path = Path::join(testdir.path(), "test-socket"); // duplicate the existing std* fds // we need to restore them later, and we cannot simply store them // as they themselves get modified in setup_console - let old_stdin: RawFd = nix::unistd::dup(StdIO::Stdin.into()).unwrap(); - let old_stdout: RawFd = nix::unistd::dup(StdIO::Stdout.into()).unwrap(); - let old_stderr: RawFd = nix::unistd::dup(StdIO::Stderr.into()).unwrap(); + let old_stdin: RawFd = nix::unistd::dup(StdIO::Stdin.into())?; + let old_stdout: RawFd = nix::unistd::dup(StdIO::Stdout.into())?; + let old_stderr: RawFd = nix::unistd::dup(StdIO::Stderr.into())?; - assert!(init.is_ok()); - let (testdir, rundir_path, socket_path) = init.unwrap(); - let lis = UnixListener::bind(Path::join(testdir.path(), "console-socket")); + let lis = UnixListener::bind(&socket_path); assert!(lis.is_ok()); - let fd = setup_console_socket(&rundir_path, &socket_path, CONSOLE_SOCKET); + let fd = setup_console_socket(testdir.path(), &socket_path, CONSOLE_SOCKET); let status = setup_console(&fd.unwrap()); // restore the original std* before doing final assert - dup2(old_stdin, StdIO::Stdin.into()).unwrap(); - dup2(old_stdout, StdIO::Stdout.into()).unwrap(); - dup2(old_stderr, StdIO::Stderr.into()).unwrap(); + dup2(old_stdin, StdIO::Stdin.into())?; + dup2(old_stdout, StdIO::Stdout.into())?; + dup2(old_stderr, StdIO::Stderr.into())?; assert!(status.is_ok()); + + Ok(()) } } diff --git a/justfile b/justfile index 1167e6c91..7b6027a25 100644 --- a/justfile +++ b/justfile @@ -74,6 +74,10 @@ containerd-test: youki-dev VAGRANT_VAGRANTFILE=Vagrantfile.containerd2youki vagrant up VAGRANT_VAGRANTFILE=Vagrantfile.containerd2youki vagrant provision --provision-with test +# run containerd integration tests +clean-containerd-test: + VAGRANT_VAGRANTFILE=Vagrantfile.containerd2youki vagrant destroy + [private] kind-cluster: bin-kind #!/usr/bin/env bash From 874414ab9cfc1e0fb0b86e7343c2b76cef8819d1 Mon Sep 17 00:00:00 2001 From: utam0k Date: Wed, 8 May 2024 22:02:23 +0900 Subject: [PATCH 2/7] fixup! Remove unnecessary chdir Signed-off-by: utam0k --- crates/libcontainer/src/container/container_start.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/libcontainer/src/container/container_start.rs b/crates/libcontainer/src/container/container_start.rs index 9b893b0b4..52ed79783 100644 --- a/crates/libcontainer/src/container/container_start.rs +++ b/crates/libcontainer/src/container/container_start.rs @@ -6,7 +6,7 @@ use crate::{ }; use super::{Container, ContainerStatus}; -use nix::sys::signal; +use nix::{sys::signal, unistd}; impl Container { /// Starts a previously created container @@ -46,6 +46,10 @@ impl Container { err })?; if let Some(hooks) = config.hooks.as_ref() { + unistd::chdir(self.root.as_os_str()).map_err(|err| { + tracing::error!("failed to change directory to container root: {}", err); + LibcontainerError::OtherSyscall(err) + })?; // While prestart is marked as deprecated in the OCI spec, the docker and integration test still // uses it. #[allow(deprecated)] From 524ae7a87d8eab7ca08473d8ce1d2a84f7a9b292 Mon Sep 17 00:00:00 2001 From: utam0k Date: Thu, 9 May 2024 21:52:02 +0900 Subject: [PATCH 3/7] fixup! Remove unnecessary chdir Signed-off-by: utam0k --- .../libcontainer/src/container/container_start.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/crates/libcontainer/src/container/container_start.rs b/crates/libcontainer/src/container/container_start.rs index 52ed79783..a9a2f1d00 100644 --- a/crates/libcontainer/src/container/container_start.rs +++ b/crates/libcontainer/src/container/container_start.rs @@ -1,3 +1,5 @@ +use std::env; + use crate::{ config::YoukiConfig, error::LibcontainerError, @@ -46,10 +48,16 @@ impl Container { err })?; if let Some(hooks) = config.hooks.as_ref() { + let original_dir = env::current_dir().map_err(|err| { + tracing::error!("failed to get current directory: {}", err); + LibcontainerError::Other(format!("failed to get current directory: {}", err)) + })?; + unistd::chdir(self.root.as_os_str()).map_err(|err| { tracing::error!("failed to change directory to container root: {}", err); LibcontainerError::OtherSyscall(err) })?; + // While prestart is marked as deprecated in the OCI spec, the docker and integration test still // uses it. #[allow(deprecated)] @@ -61,6 +69,11 @@ impl Container { err })?; + + unistd::chdir(original_dir.as_path()).map_err(|err| { + tracing::error!("failed to change directory to container root: {}", err); + LibcontainerError::OtherSyscall(err) + })?; } let mut notify_socket = NotifySocket::new(self.root.join(NOTIFY_FILE)); From 8de223377e21d5b1e9f1ce3c5d56e6d22f60e601 Mon Sep 17 00:00:00 2001 From: utam0k Date: Fri, 10 May 2024 20:34:55 +0900 Subject: [PATCH 4/7] fixup! Remove unnecessary chdir Signed-off-by: utam0k --- .../src/container/container_start.rs | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/crates/libcontainer/src/container/container_start.rs b/crates/libcontainer/src/container/container_start.rs index a9a2f1d00..0a429cdaf 100644 --- a/crates/libcontainer/src/container/container_start.rs +++ b/crates/libcontainer/src/container/container_start.rs @@ -48,16 +48,6 @@ impl Container { err })?; if let Some(hooks) = config.hooks.as_ref() { - let original_dir = env::current_dir().map_err(|err| { - tracing::error!("failed to get current directory: {}", err); - LibcontainerError::Other(format!("failed to get current directory: {}", err)) - })?; - - unistd::chdir(self.root.as_os_str()).map_err(|err| { - tracing::error!("failed to change directory to container root: {}", err); - LibcontainerError::OtherSyscall(err) - })?; - // While prestart is marked as deprecated in the OCI spec, the docker and integration test still // uses it. #[allow(deprecated)] @@ -69,11 +59,6 @@ impl Container { err })?; - - unistd::chdir(original_dir.as_path()).map_err(|err| { - tracing::error!("failed to change directory to container root: {}", err); - LibcontainerError::OtherSyscall(err) - })?; } let mut notify_socket = NotifySocket::new(self.root.join(NOTIFY_FILE)); @@ -88,10 +73,25 @@ impl Container { // Run post start hooks. It runs after the container process is started. // It is called in the runtime namespace. if let Some(hooks) = config.hooks.as_ref() { + let original_dir = env::current_dir().map_err(|err| { + tracing::error!("failed to get current directory: {}", err); + LibcontainerError::Other(format!("failed to get current directory: {}", err)) + })?; + + unistd::chdir(self.root.as_os_str()).map_err(|err| { + tracing::error!("failed to change directory to container root: {}", err); + LibcontainerError::OtherSyscall(err) + })?; + hooks::run_hooks(hooks.poststart().as_ref(), Some(self)).map_err(|err| { tracing::error!("failed to run post start hooks: {}", err); err })?; + + unistd::chdir(original_dir.as_path()).map_err(|err| { + tracing::error!("failed to change directory to container root: {}", err); + LibcontainerError::OtherSyscall(err) + })?; } Ok(()) From b53659178f8271f08e580c753e90a71187477b69 Mon Sep 17 00:00:00 2001 From: utam0k Date: Sat, 11 May 2024 15:30:11 +0900 Subject: [PATCH 5/7] Enable run_hooks to be passed cwd Signed-off-by: utam0k --- .../src/container/builder_impl.rs | 16 +++-- .../src/container/container_delete.rs | 10 +-- .../src/container/container_start.rs | 33 +++------- crates/libcontainer/src/hooks.rs | 64 +++++++++++++++---- .../src/process/container_init_process.rs | 62 ++++++++++-------- 5 files changed, 114 insertions(+), 71 deletions(-) diff --git a/crates/libcontainer/src/container/builder_impl.rs b/crates/libcontainer/src/container/builder_impl.rs index 92ab4f97f..b235c7261 100644 --- a/crates/libcontainer/src/container/builder_impl.rs +++ b/crates/libcontainer/src/container/builder_impl.rs @@ -1,3 +1,9 @@ +use std::{fs, io::Write, os::unix::prelude::RawFd, path::PathBuf, rc::Rc}; + +use libcgroups::common::CgroupManager; +use nix::unistd::Pid; +use oci_spec::runtime::Spec; + use super::{Container, ContainerStatus}; use crate::{ error::{LibcontainerError, MissingSpecError}, @@ -13,10 +19,6 @@ use crate::{ utils, workload::Executor, }; -use libcgroups::common::CgroupManager; -use nix::unistd::Pid; -use oci_spec::runtime::Spec; -use std::{fs, io::Write, os::unix::prelude::RawFd, path::PathBuf, rc::Rc}; pub(super) struct ContainerBuilderImpl { /// Flag indicating if an init or a tenant container should be created @@ -82,7 +84,11 @@ impl ContainerBuilderImpl { if matches!(self.container_type, ContainerType::InitContainer) { if let Some(hooks) = self.spec.hooks() { - hooks::run_hooks(hooks.create_runtime().as_ref(), self.container.as_ref())? + hooks::run_hooks( + hooks.create_runtime().as_ref(), + self.container.as_ref(), + None, + )? } } diff --git a/crates/libcontainer/src/container/container_delete.rs b/crates/libcontainer/src/container/container_delete.rs index 0322fd392..2a3274bc3 100644 --- a/crates/libcontainer/src/container/container_delete.rs +++ b/crates/libcontainer/src/container/container_delete.rs @@ -94,10 +94,12 @@ impl Container { })?; if let Some(hooks) = config.hooks.as_ref() { - hooks::run_hooks(hooks.poststop().as_ref(), Some(self)).map_err(|err| { - tracing::error!(err = ?err, "failed to run post stop hooks"); - err - })?; + hooks::run_hooks(hooks.poststop().as_ref(), Some(self), None).map_err( + |err| { + tracing::error!(err = ?err, "failed to run post stop hooks"); + err + }, + )?; } } Err(err) => { diff --git a/crates/libcontainer/src/container/container_start.rs b/crates/libcontainer/src/container/container_start.rs index 0a429cdaf..b0c847191 100644 --- a/crates/libcontainer/src/container/container_start.rs +++ b/crates/libcontainer/src/container/container_start.rs @@ -1,5 +1,6 @@ -use std::env; +use nix::sys::signal; +use super::{Container, ContainerStatus}; use crate::{ config::YoukiConfig, error::LibcontainerError, @@ -7,9 +8,6 @@ use crate::{ notify_socket::{NotifySocket, NOTIFY_FILE}, }; -use super::{Container, ContainerStatus}; -use nix::{sys::signal, unistd}; - impl Container { /// Starts a previously created container /// @@ -51,7 +49,7 @@ impl Container { // While prestart is marked as deprecated in the OCI spec, the docker and integration test still // uses it. #[allow(deprecated)] - hooks::run_hooks(hooks.prestart().as_ref(), Some(self)).map_err(|err| { + hooks::run_hooks(hooks.prestart().as_ref(), Some(self), None).map_err(|err| { tracing::error!("failed to run pre start hooks: {}", err); // In the case where prestart hook fails, the runtime must // stop the container before generating an error and exiting. @@ -73,25 +71,12 @@ impl Container { // Run post start hooks. It runs after the container process is started. // It is called in the runtime namespace. if let Some(hooks) = config.hooks.as_ref() { - let original_dir = env::current_dir().map_err(|err| { - tracing::error!("failed to get current directory: {}", err); - LibcontainerError::Other(format!("failed to get current directory: {}", err)) - })?; - - unistd::chdir(self.root.as_os_str()).map_err(|err| { - tracing::error!("failed to change directory to container root: {}", err); - LibcontainerError::OtherSyscall(err) - })?; - - hooks::run_hooks(hooks.poststart().as_ref(), Some(self)).map_err(|err| { - tracing::error!("failed to run post start hooks: {}", err); - err - })?; - - unistd::chdir(original_dir.as_path()).map_err(|err| { - tracing::error!("failed to change directory to container root: {}", err); - LibcontainerError::OtherSyscall(err) - })?; + hooks::run_hooks(hooks.poststart().as_ref(), Some(self), Some(&self.root)).map_err( + |err| { + tracing::error!("failed to run post start hooks: {}", err); + err + }, + )?; } Ok(()) diff --git a/crates/libcontainer/src/hooks.rs b/crates/libcontainer/src/hooks.rs index f12cc6039..05dd4f98b 100644 --- a/crates/libcontainer/src/hooks.rs +++ b/crates/libcontainer/src/hooks.rs @@ -1,14 +1,15 @@ -use nix::{sys::signal, unistd::Pid}; -use oci_spec::runtime::Hook; use std::{ collections::HashMap, - io::ErrorKind, - io::Write, + env, + io::{ErrorKind, Write}, os::unix::prelude::CommandExt, - process::{self}, - thread, time, + path::Path, + process, thread, time, }; +use nix::{sys::signal, unistd::Pid}; +use oci_spec::runtime::Hook; + use crate::{container::Container, utils}; #[derive(Debug, thiserror::Error)] @@ -27,16 +28,35 @@ pub enum HookError { MissingContainerState, #[error("failed to write container state to stdin")] WriteContainerState(#[source] std::io::Error), + #[error("io error")] + OtherIO(#[source] std::io::Error), } type Result = std::result::Result; -pub fn run_hooks(hooks: Option<&Vec>, container: Option<&Container>) -> Result<()> { +pub fn run_hooks( + hooks: Option<&Vec>, + container: Option<&Container>, + cwd: Option<&Path>, +) -> Result<()> { let state = &(container.ok_or(HookError::MissingContainerState)?.state); if let Some(hooks) = hooks { for hook in hooks { - let mut hook_command = process::Command::new(hook.path()); + let mut cmd = process::Command::new(hook.path()); + + let hook_command = if let Some(cwd) = cwd { + cmd.current_dir(cwd) + } else { + cmd.current_dir( + env::current_dir() + .map_err(|err| { + tracing::error!("failed to get current directory: {}", err); + HookError::OtherIO(err) + })? + .as_path(), + ) + }; // Based on OCI spec, the first argument of the args vector is the // arg0, which can be different from the path. For example, path // may be "/usr/bin/true" and arg0 is set to "true". However, rust @@ -165,7 +185,7 @@ mod test { fn test_run_hook() -> Result<()> { { let default_container: Container = Default::default(); - run_hooks(None, Some(&default_container)).context("Failed simple test")?; + run_hooks(None, Some(&default_container), None).context("Failed simple test")?; } { @@ -174,7 +194,7 @@ mod test { let hook = HookBuilder::default().path("true").build()?; let hooks = Some(vec![hook]); - run_hooks(hooks.as_ref(), Some(&default_container)).context("Failed true")?; + run_hooks(hooks.as_ref(), Some(&default_container), None).context("Failed true")?; } { @@ -194,7 +214,27 @@ mod test { .env(vec![String::from("key=value")]) .build()?; let hooks = Some(vec![hook]); - run_hooks(hooks.as_ref(), Some(&default_container)).context("Failed printenv test")?; + run_hooks(hooks.as_ref(), Some(&default_container), None) + .context("Failed printenv test")?; + } + + { + assert!(is_command_in_path("pwd"), "The pwd was not found."); + + let tmp = tempfile::tempdir()?; + + let default_container: Container = Default::default(); + let hook = HookBuilder::default() + .path("bash") + .args(vec![ + String::from("bash"), + String::from("-c"), + format!("test $(pwd) = {:?}", tmp.path()), + ]) + .build()?; + let hooks = Some(vec![hook]); + run_hooks(hooks.as_ref(), Some(&default_container), Some(tmp.path())) + .context("Failed pwd test")?; } Ok(()) @@ -217,7 +257,7 @@ mod test { .timeout(1) .build()?; let hooks = Some(vec![hook]); - match run_hooks(hooks.as_ref(), Some(&default_container)) { + match run_hooks(hooks.as_ref(), Some(&default_container), None) { Ok(_) => { bail!("The test expects the hook to error out with timeout. Should not execute cleanly"); } diff --git a/crates/libcontainer/src/process/container_init_process.rs b/crates/libcontainer/src/process/container_init_process.rs index fb01e0f57..d7f355f24 100644 --- a/crates/libcontainer/src/process/container_init_process.rs +++ b/crates/libcontainer/src/process/container_init_process.rs @@ -1,32 +1,40 @@ +use std::{ + collections::HashMap, + env, fs, mem, + os::unix::io::AsRawFd, + path::{Path, PathBuf}, +}; + use super::args::{ContainerArgs, ContainerType}; -use crate::error::MissingSpecError; -use crate::namespaces::NamespaceError; -use crate::syscall::{Syscall, SyscallError}; -use crate::{apparmor, notify_socket, rootfs, workload}; +#[cfg(feature = "libseccomp")] +use crate::seccomp; use crate::{ - capabilities, hooks, namespaces::Namespaces, process::channel, rootfs::RootFS, tty, - user_ns::UserNamespaceConfig, utils, + apparmor, capabilities, + error::MissingSpecError, + hooks, + namespaces::{NamespaceError, Namespaces}, + notify_socket, + process::channel, + rootfs, + rootfs::RootFS, + syscall::{Syscall, SyscallError}, + tty, + user_ns::UserNamespaceConfig, + utils, workload, }; + use nc; -use nix::mount::MsFlags; -use nix::sched::CloneFlags; -use nix::sys::stat::Mode; -use nix::unistd::setsid; -use nix::unistd::{self, Gid, Uid}; +use nix::{ + mount::MsFlags, + sched::CloneFlags, + sys::stat::Mode, + unistd::setsid, + unistd::{self, Gid, Uid}, +}; use oci_spec::runtime::{ IOPriorityClass, LinuxIOPriority, LinuxNamespaceType, LinuxSchedulerFlag, LinuxSchedulerPolicy, Scheduler, Spec, User, }; -use std::collections::HashMap; -use std::mem; -use std::os::unix::io::AsRawFd; -use std::{ - env, fs, - path::{Path, PathBuf}, -}; - -#[cfg(feature = "libseccomp")] -use crate::seccomp; #[derive(Debug, thiserror::Error)] pub enum InitProcessError { @@ -317,10 +325,12 @@ pub fn container_init_process( // create_container hook needs to be called after the namespace setup, but // before pivot_root is called. This runs in the container namespaces. if let Some(hooks) = hooks { - hooks::run_hooks(hooks.create_container().as_ref(), container).map_err(|err| { - tracing::error!(?err, "failed to run create container hooks"); - InitProcessError::Hooks(err) - })?; + hooks::run_hooks(hooks.create_container().as_ref(), container, None).map_err( + |err| { + tracing::error!(?err, "failed to run create container hooks"); + InitProcessError::Hooks(err) + }, + )?; } let in_user_ns = utils::is_in_new_userns().map_err(InitProcessError::Io)?; @@ -628,7 +638,7 @@ pub fn container_init_process( // before pivot_root is called. This runs in the container namespaces. if matches!(args.container_type, ContainerType::InitContainer) { if let Some(hooks) = hooks { - hooks::run_hooks(hooks.start_container().as_ref(), container).map_err(|err| { + hooks::run_hooks(hooks.start_container().as_ref(), container, None).map_err(|err| { tracing::error!(?err, "failed to run start container hooks"); err })?; From a816712fb320dac0690b2a243b8ef94cb936ee09 Mon Sep 17 00:00:00 2001 From: utam0k Date: Tue, 14 May 2024 21:43:03 +0900 Subject: [PATCH 6/7] fixup! Enable run_hooks to be passed cwd Signed-off-by: utam0k --- crates/libcontainer/src/hooks.rs | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/crates/libcontainer/src/hooks.rs b/crates/libcontainer/src/hooks.rs index 05dd4f98b..432b4990d 100644 --- a/crates/libcontainer/src/hooks.rs +++ b/crates/libcontainer/src/hooks.rs @@ -1,6 +1,5 @@ use std::{ collections::HashMap, - env, io::{ErrorKind, Write}, os::unix::prelude::CommandExt, path::Path, @@ -43,20 +42,12 @@ pub fn run_hooks( if let Some(hooks) = hooks { for hook in hooks { - let mut cmd = process::Command::new(hook.path()); + let mut hook_command = process::Command::new(hook.path()); + + if let Some(cwd) = cwd { + hook_command.current_dir(cwd); + } - let hook_command = if let Some(cwd) = cwd { - cmd.current_dir(cwd) - } else { - cmd.current_dir( - env::current_dir() - .map_err(|err| { - tracing::error!("failed to get current directory: {}", err); - HookError::OtherIO(err) - })? - .as_path(), - ) - }; // Based on OCI spec, the first argument of the args vector is the // arg0, which can be different from the path. For example, path // may be "/usr/bin/true" and arg0 is set to "true". However, rust From 4a4975954e4c8ee1b88c176789225eef37f7a48a Mon Sep 17 00:00:00 2001 From: utam0k Date: Wed, 15 May 2024 20:52:40 +0900 Subject: [PATCH 7/7] fixup! Enable run_hooks to be passed cwd Signed-off-by: utam0k --- crates/libcontainer/src/hooks.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/libcontainer/src/hooks.rs b/crates/libcontainer/src/hooks.rs index 432b4990d..9e5931b5c 100644 --- a/crates/libcontainer/src/hooks.rs +++ b/crates/libcontainer/src/hooks.rs @@ -27,8 +27,6 @@ pub enum HookError { MissingContainerState, #[error("failed to write container state to stdin")] WriteContainerState(#[source] std::io::Error), - #[error("io error")] - OtherIO(#[source] std::io::Error), } type Result = std::result::Result;