Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary chdir #2780

Merged
merged 7 commits into from
May 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions Vagrantfile.containerd2youki
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be more beneficial because we don't need to copy again when we rebuild.

SHELL
end

Expand Down
16 changes: 11 additions & 5 deletions crates/libcontainer/src/container/builder_impl.rs
Original file line number Diff line number Diff line change
@@ -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;

Comment on lines +1 to +6
Copy link
Contributor

@jprendes jprendes May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: We should have a rustfmt.toml to standardize this.
I personally like the one that @Mossaka put together for runwasi.
https://github.com/containerd/runwasi/blob/main/rustfmt.toml

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know that. It is what I want! Thanks for sharing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use super::{Container, ContainerStatus};
use crate::{
error::{LibcontainerError, MissingSpecError},
Expand All @@ -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
Expand Down Expand Up @@ -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,
)?
}
}

Expand Down
10 changes: 6 additions & 4 deletions crates/libcontainer/src/container/container_delete.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down
23 changes: 10 additions & 13 deletions crates/libcontainer/src/container/container_start.rs
Original file line number Diff line number Diff line change
@@ -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
///
Expand Down Expand Up @@ -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.
Expand All @@ -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)
})?;

YJDoc2 marked this conversation as resolved.
Show resolved Hide resolved
let mut notify_socket = NotifySocket::new(self.root.join(NOTIFY_FILE));
notify_socket.notify_container_start()?;
self.set_status(ContainerStatus::Running)
Expand All @@ -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(())
Expand Down
9 changes: 0 additions & 9 deletions crates/libcontainer/src/container/init_builder.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use nix::unistd;
use oci_spec::runtime::Spec;
use std::{
fs,
Expand Down Expand Up @@ -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())
Expand Down
3 changes: 1 addition & 2 deletions crates/libcontainer/src/container/tenant_builder.rs
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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())
Expand Down
51 changes: 40 additions & 11 deletions crates/libcontainer/src/hooks.rs
Original file line number Diff line number Diff line change
@@ -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)]
Expand All @@ -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
Expand Down Expand Up @@ -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")?;
}

{
Expand All @@ -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")?;
}

{
Expand All @@ -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(())
Expand All @@ -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");
}
Expand Down
62 changes: 36 additions & 26 deletions crates/libcontainer/src/process/container_init_process.rs
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -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)?;
Expand Down Expand Up @@ -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
})?;
Expand Down