From 25edc956485c26bdf4eb188c17e2f8c4c7c7ffe8 Mon Sep 17 00:00:00 2001 From: Toru Komatsu Date: Wed, 15 May 2024 21:20:27 +0900 Subject: [PATCH] Remove unnecessary chdir (#2780) * Remove unnecessary chdir Co-authored-by: jiaxiao zhou Co-authored-by: Jorge Prendes Signed-off-by: utam0k * fixup! Remove unnecessary chdir Signed-off-by: utam0k * fixup! Remove unnecessary chdir Signed-off-by: utam0k * fixup! Remove unnecessary chdir Signed-off-by: utam0k * Enable run_hooks to be passed cwd Signed-off-by: utam0k * fixup! Enable run_hooks to be passed cwd Signed-off-by: utam0k * fixup! Enable run_hooks to be passed cwd Signed-off-by: utam0k --------- Signed-off-by: utam0k Co-authored-by: jiaxiao zhou Co-authored-by: Jorge Prendes --- Vagrantfile.containerd2youki | 3 +- .../src/container/builder_impl.rs | 16 ++-- .../src/container/container_delete.rs | 10 ++- .../src/container/container_start.rs | 23 +++--- .../src/container/init_builder.rs | 9 -- .../src/container/tenant_builder.rs | 3 +- crates/libcontainer/src/hooks.rs | 51 +++++++++--- .../src/process/container_init_process.rs | 62 ++++++++------ crates/libcontainer/src/tty.rs | 82 ++++++++----------- justfile | 4 + 10 files changed, 144 insertions(+), 119 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/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 c6805511c..b0c847191 100644 --- a/crates/libcontainer/src/container/container_start.rs +++ b/crates/libcontainer/src/container/container_start.rs @@ -1,3 +1,6 @@ +use nix::sys::signal; + +use super::{Container, ContainerStatus}; use crate::{ config::YoukiConfig, error::LibcontainerError, @@ -5,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 /// @@ -49,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. @@ -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) @@ -76,10 +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() { - hooks::run_hooks(hooks.poststart().as_ref(), Some(self)).map_err(|err| { - tracing::error!("failed to run post start hooks: {}", err); - 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/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/hooks.rs b/crates/libcontainer/src/hooks.rs index f12cc6039..9e5931b5c 100644 --- a/crates/libcontainer/src/hooks.rs +++ b/crates/libcontainer/src/hooks.rs @@ -1,14 +1,14 @@ -use nix::{sys::signal, unistd::Pid}; -use oci_spec::runtime::Hook; use std::{ collections::HashMap, - io::ErrorKind, - io::Write, + 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)] @@ -31,12 +31,21 @@ pub enum HookError { 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()); + + if let Some(cwd) = cwd { + hook_command.current_dir(cwd); + } + // 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 +174,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 +183,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 +203,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 +246,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 })?; 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