Remove unnecessary chdir (#2780)

* Remove unnecessary chdir Co-authored-by: jiaxiao zhou <[email protected]> Co-authored-by: Jorge Prendes <[email protected]> Signed-off-by: utam0k <[email protected]> * fixup! Remove unnecessary chdir Signed-off-by: utam0k <[email protected]> * fixup! Remove unnecessary chdir Signed-off-by: utam0k <[email protected]> * fixup! Remove unnecessary chdir Signed-off-by: utam0k <[email protected]> * Enable run_hooks to be passed cwd Signed-off-by: utam0k <[email protected]> * fixup! Enable run_hooks to be passed cwd Signed-off-by: utam0k <[email protected]> * fixup! Enable run_hooks to be passed cwd Signed-off-by: utam0k <[email protected]> --------- Signed-off-by: utam0k <[email protected]> Co-authored-by: jiaxiao zhou <[email protected]> Co-authored-by: Jorge Prendes <[email protected]>
youki-dev · May 15, 2024 · 25edc95 · 25edc95
1 parent c6fffb2
commit 25edc95
Show file tree

Hide file tree

Showing 10 changed files with 144 additions and 119 deletions.
diff --git 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
@@ -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
@@ -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
@@ -1,13 +1,13 @@
+use nix::sys::signal;
+
+use super::{Container, ContainerStatus};
 use crate::{
     config::YoukiConfig,
     error::LibcontainerError,
     hooks,
     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
@@ -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
@@ -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
@@ -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<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");
             }

diff --git 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
             })?;