Remove unnecessary chdir (#2780)

* Remove unnecessary chdir

Co-authored-by: jiaxiao zhou <jiazho@microsoft.com>
Co-authored-by: Jorge Prendes <jorge.prendes@gmail.com>
Signed-off-by: utam0k <k0ma@utam0k.jp>

* fixup! Remove unnecessary chdir

Signed-off-by: utam0k <k0ma@utam0k.jp>

* fixup! Remove unnecessary chdir

Signed-off-by: utam0k <k0ma@utam0k.jp>

* fixup! Remove unnecessary chdir

Signed-off-by: utam0k <k0ma@utam0k.jp>

* Enable run_hooks to be passed cwd

Signed-off-by: utam0k <k0ma@utam0k.jp>

* fixup! Enable run_hooks to be passed cwd

Signed-off-by: utam0k <k0ma@utam0k.jp>

* fixup! Enable run_hooks to be passed cwd

Signed-off-by: utam0k <k0ma@utam0k.jp>

---------

Signed-off-by: utam0k <k0ma@utam0k.jp>
Co-authored-by: jiaxiao zhou <jiazho@microsoft.com>
Co-authored-by: Jorge Prendes <jorge.prendes@gmail.com>

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
-
+        ln -s /vagrant/youki/youki /usr/bin/runc
-        cp /vagrant/youki/youki /usr/bin/runc
       SHELL
     end
 

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,
+                )?
             }
         }
 

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| {
+                        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) => {

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| {
+            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(())

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())

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())

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::{ErrorKind, Write},
-    io::Write,
     os::unix::prelude::CommandExt,
-    process::{self},
+    path::Path,
-    thread, time,
+    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<T> = std::result::Result<T, HookError>;
 
-pub fn run_hooks(hooks: Option<&Vec<Hook>>, container: Option<&Container>) -> Result<()> {
+pub fn run_hooks(
+    hooks: Option<&Vec<Hook>>,
+    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");
             }

crates/libcontainer/src/process/container_init_process.rs

@@ -1,32 +1,40 @@
-use super::args::{ContainerArgs, ContainerType};
+use std::{
-use crate::error::MissingSpecError;
+    collections::HashMap,
-use crate::namespaces::NamespaceError;
+    env, fs, mem,
-use crate::syscall::{Syscall, SyscallError};
+    os::unix::io::AsRawFd,
-use crate::{apparmor, notify_socket, rootfs, workload};
+    path::{Path, PathBuf},
-use crate::{
-    capabilities, hooks, namespaces::Namespaces, process::channel, rootfs::RootFS, tty,
-    user_ns::UserNamespaceConfig, utils,
 };
+
+use super::args::{ContainerArgs, ContainerType};
+#[cfg(feature = "libseccomp")]
+use crate::seccomp;
+use crate::{
+    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::{
-use nix::sched::CloneFlags;
+    mount::MsFlags,
-use nix::sys::stat::Mode;
+    sched::CloneFlags,
-use nix::unistd::setsid;
+    sys::stat::Mode,
-use nix::unistd::{self, Gid, Uid};
+    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| {
+            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
             })?;

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::File;
-    use std::fs::{self, File};
     use std::os::unix::net::UnixListener;
-    use std::path::PathBuf;
 
     const CONSOLE_SOCKET: &str = "console-socket";
 
-    fn setup() -> Result<(tempfile::TempDir, PathBuf, PathBuf)> {
+    #[test]
+    #[serial]
+    fn test_setup_console_socket() -> Result<()> {
         let testdir = tempfile::tempdir()?;
-        let rundir_path = Path::join(testdir.path(), "run");
+        let socket_path = Path::join(testdir.path(), "test-socket");
-        fs::create_dir(&rundir_path)?;
+        let lis = UnixListener::bind(&socket_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"));
         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)?;
-        assert!(fd.is_ok());
+        assert_ne!(fd.as_raw_fd(), -1);
-        assert_ne!(fd.unwrap().as_raw_fd(), -1);
+        Ok(())
     }
 
     #[test]
     #[serial]
-    fn test_setup_console_socket_empty() {
+    fn test_setup_console_socket_empty() -> Result<()> {
-        let init = setup();
+        let testdir = tempfile::tempdir()?;
-        assert!(init.is_ok());
+        let socket_path = Path::join(testdir.path(), "test-socket");
-        let (_testdir, rundir_path, socket_path) = init.unwrap();
+        let fd = setup_console_socket(testdir.path(), &socket_path, CONSOLE_SOCKET)?;
-        let fd = setup_console_socket(&rundir_path, &socket_path, CONSOLE_SOCKET);
+        assert_eq!(fd.as_raw_fd(), -1);
-        assert!(fd.is_ok());
+        Ok(())
-        assert_eq!(fd.unwrap().as_raw_fd(), -1);
     }
 
     #[test]
     #[serial]
-    fn test_setup_console_socket_invalid() {
+    fn test_setup_console_socket_invalid() -> Result<()> {
-        let init = setup();
+        let testdir = tempfile::tempdir()?;
-        assert!(init.is_ok());
+        let socket_path = Path::join(testdir.path(), "test-socket");
-        let (testdir, rundir_path, socket_path) = init.unwrap();
         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() {
+    fn test_setup_console() -> Result<()> {
-        let init = setup();
+        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_stdin: RawFd = nix::unistd::dup(StdIO::Stdin.into())?;
-        let old_stdout: RawFd = nix::unistd::dup(StdIO::Stdout.into()).unwrap();
+        let old_stdout: RawFd = nix::unistd::dup(StdIO::Stdout.into())?;
-        let old_stderr: RawFd = nix::unistd::dup(StdIO::Stderr.into()).unwrap();
+        let old_stderr: RawFd = nix::unistd::dup(StdIO::Stderr.into())?;
 
-        assert!(init.is_ok());
+        let lis = UnixListener::bind(&socket_path);
-        let (testdir, rundir_path, socket_path) = init.unwrap();
-        let lis = UnixListener::bind(Path::join(testdir.path(), "console-socket"));
         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_stdin, StdIO::Stdin.into())?;
-        dup2(old_stdout, StdIO::Stdout.into()).unwrap();
+        dup2(old_stdout, StdIO::Stdout.into())?;
-        dup2(old_stderr, StdIO::Stderr.into()).unwrap();
+        dup2(old_stderr, StdIO::Stderr.into())?;
 
         assert!(status.is_ok());
+
+        Ok(())
     }
 }

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