From 4bafaabb426116d0e8fbae173231304f51f467b5 Mon Sep 17 00:00:00 2001 From: utam0k Date: Wed, 29 Jun 2022 21:22:46 +0900 Subject: [PATCH 1/8] Wait for the terminated of the command ran by exec command. Signed-off-by: utam0k --- .../src/container/builder_impl.rs | 21 ++++++------- .../src/container/tenant_builder.rs | 9 +++--- crates/youki/src/commands/exec.rs | 30 +++++++++++++++---- crates/youki/src/main.rs | 5 +++- 4 files changed, 45 insertions(+), 20 deletions(-) diff --git a/crates/libcontainer/src/container/builder_impl.rs b/crates/libcontainer/src/container/builder_impl.rs index 40f99cf0..fc8f0d9a 100644 --- a/crates/libcontainer/src/container/builder_impl.rs +++ b/crates/libcontainer/src/container/builder_impl.rs @@ -8,6 +8,7 @@ use crate::{ utils, }; use anyhow::{bail, Context, Result}; +use nix::unistd::Pid; use oci_spec::runtime::Spec; use std::{fs, io::Write, os::unix::prelude::RawFd, path::PathBuf}; @@ -40,19 +41,19 @@ pub(super) struct ContainerBuilderImpl<'a> { } impl<'a> ContainerBuilderImpl<'a> { - pub(super) fn create(&mut self) -> Result<()> { - if let Err(outer) = self.run_container().context("failed to create container") { - if let Err(inner) = self.cleanup_container() { - return Err(outer.context(inner)); + pub(super) fn create(&mut self) -> Result { + match self.run_container().context("failed to create container") { + Ok(pid) => Ok(pid), + Err(outer) => { + if let Err(inner) = self.cleanup_container() { + return Err(outer.context(inner)); + } + Err(outer) } - - return Err(outer); } - - Ok(()) } - fn run_container(&mut self) -> Result<()> { + fn run_container(&mut self) -> Result { let linux = self.spec.linux().as_ref().context("no linux in spec")?; let cgroups_path = utils::get_cgroup_path( linux.cgroups_path(), @@ -138,7 +139,7 @@ impl<'a> ContainerBuilderImpl<'a> { .context("Failed to save container state")?; } - Ok(()) + Ok(init_pid) } fn cleanup_container(&self) -> Result<()> { diff --git a/crates/libcontainer/src/container/tenant_builder.rs b/crates/libcontainer/src/container/tenant_builder.rs index 2fd7a3df..5de9b02a 100644 --- a/crates/libcontainer/src/container/tenant_builder.rs +++ b/crates/libcontainer/src/container/tenant_builder.rs @@ -1,6 +1,6 @@ use anyhow::{bail, Context, Result}; use caps::Capability; -use nix::unistd; +use nix::unistd::{self, Pid}; use oci_spec::runtime::{ Capabilities as SpecCapabilities, Capability as SpecCapability, LinuxBuilder, LinuxCapabilities, LinuxCapabilitiesBuilder, LinuxNamespace, LinuxNamespaceBuilder, @@ -89,7 +89,7 @@ impl<'a> TenantContainerBuilder<'a> { } /// Joins an existing container - pub fn build(self) -> Result<()> { + pub fn build(self) -> Result { let container_dir = self .lookup_container_dir() .context("failed to look up container dir")?; @@ -131,11 +131,12 @@ impl<'a> TenantContainerBuilder<'a> { preserve_fds: self.base.preserve_fds, }; - builder_impl.create()?; + let pid = builder_impl.create()?; let mut notify_socket = NotifySocket::new(notify_path); notify_socket.notify_container_start()?; - Ok(()) + + Ok(pid) } fn lookup_container_dir(&self) -> Result { diff --git a/crates/youki/src/commands/exec.rs b/crates/youki/src/commands/exec.rs index 562d4fee..826b1b9e 100644 --- a/crates/youki/src/commands/exec.rs +++ b/crates/youki/src/commands/exec.rs @@ -1,12 +1,16 @@ -use anyhow::Result; -use std::path::PathBuf; +use anyhow::{bail, Context, Result}; +use nix::{ + libc, + poll::{PollFd, PollFlags}, +}; +use std::{os::unix::prelude::RawFd, path::PathBuf}; use libcontainer::{container::builder::ContainerBuilder, syscall::syscall::create_syscall}; use liboci_cli::Exec; -pub fn exec(args: Exec, root_path: PathBuf) -> Result<()> { +pub fn exec(args: Exec, root_path: PathBuf) -> Result { let syscall = create_syscall(); - ContainerBuilder::new(args.container_id.clone(), syscall.as_ref()) + let pid = ContainerBuilder::new(args.container_id.clone(), syscall.as_ref()) .with_root_path(root_path)? .with_console_socket(args.console_socket.as_ref()) .with_pid_file(args.pid_file.as_ref())? @@ -16,5 +20,21 @@ pub fn exec(args: Exec, root_path: PathBuf) -> Result<()> { .with_process(args.process.as_ref()) .with_no_new_privs(args.no_new_privs) .with_container_args(args.command.clone()) - .build() + .build()?; + + let pidfd = pidfd_open(pid.as_raw(), 0)?; + let poll_fd = PollFd::new(pidfd, PollFlags::POLLIN); + nix::poll::poll(&mut [poll_fd], -1).context("failed to wait for the container id")?; + + // TODO + Ok(0) +} + +fn pidfd_open(pid: libc::pid_t, flags: libc::c_uint) -> Result { + let fd = unsafe { libc::syscall(libc::SYS_pidfd_open, pid, flags) }; + if fd == -1 { + bail!("faild to pifd_open syscall") + } else { + Ok(fd as RawFd) + } } diff --git a/crates/youki/src/main.rs b/crates/youki/src/main.rs index cd1e32ae..853467fe 100644 --- a/crates/youki/src/main.rs +++ b/crates/youki/src/main.rs @@ -112,7 +112,10 @@ fn main() -> Result<()> { commands::checkpoint::checkpoint(checkpoint, root_path) } CommonCmd::Events(events) => commands::events::events(events, root_path), - CommonCmd::Exec(exec) => commands::exec::exec(exec, root_path), + CommonCmd::Exec(exec) => { + let exit_code = commands::exec::exec(exec, root_path)?; + std::process::exit(exit_code) + } CommonCmd::List(list) => commands::list::list(list, root_path), CommonCmd::Pause(pause) => commands::pause::pause(pause, root_path), CommonCmd::Ps(ps) => commands::ps::ps(ps, root_path), From dacc73773f33ddff86834d05976289bafd93de2f Mon Sep 17 00:00:00 2001 From: utam0k Date: Tue, 19 Jul 2022 21:06:25 +0900 Subject: [PATCH 2/8] wip. Signed-off-by: utam0k --- .../src/process/container_init_process.rs | 12 ++++++++- .../process/container_intermediate_process.rs | 3 ++- .../src/process/container_main_process.rs | 27 +++++++++++++++++-- crates/youki/src/commands/exec.rs | 12 ++++++++- 4 files changed, 49 insertions(+), 5 deletions(-) diff --git a/crates/libcontainer/src/process/container_init_process.rs b/crates/libcontainer/src/process/container_init_process.rs index 6ce80c4e..2a0ff4d9 100644 --- a/crates/libcontainer/src/process/container_init_process.rs +++ b/crates/libcontainer/src/process/container_init_process.rs @@ -15,7 +15,10 @@ use nix::unistd::setsid; use nix::unistd::{self, Gid, Uid}; use oci_spec::runtime::{LinuxNamespaceType, Spec, User}; use std::collections::HashMap; +use std::fs::File; +use std::io::Write; use std::os::unix::io::AsRawFd; +use std::os::unix::prelude::FromRawFd; use std::{ env, fs, path::{Path, PathBuf}, @@ -159,6 +162,7 @@ pub fn container_init_process( args: &ContainerArgs, main_sender: &mut channel::MainSender, init_receiver: &mut channel::InitReceiver, + fifo_fd: i32, ) -> Result<()> { let syscall = args.syscall; let spec = args.spec; @@ -412,7 +416,13 @@ pub fn container_init_process( } if proc.args().is_some() { - ExecutorManager::exec(spec) + ExecutorManager::exec(spec)?; + if fifo_fd != 0 { + let f = &mut unsafe { File::from_raw_fd(fifo_fd) }; + // TODO: impl + write!(f, "1")?; + } + Ok(()) } else { bail!("on non-Windows, at least one process arg entry is required") } diff --git a/crates/libcontainer/src/process/container_intermediate_process.rs b/crates/libcontainer/src/process/container_intermediate_process.rs index 8681afa2..02c349cf 100644 --- a/crates/libcontainer/src/process/container_intermediate_process.rs +++ b/crates/libcontainer/src/process/container_intermediate_process.rs @@ -14,6 +14,7 @@ pub fn container_intermediate_process( intermediate_chan: &mut (channel::IntermediateSender, channel::IntermediateReceiver), init_chan: &mut (channel::InitSender, channel::InitReceiver), main_sender: &mut channel::MainSender, + fifo_fd: i32, ) -> Result<()> { let (inter_sender, inter_receiver) = intermediate_chan; let (init_sender, init_receiver) = init_chan; @@ -95,7 +96,7 @@ pub fn container_intermediate_process( inter_sender .close() .context("failed to close sender in the intermediate process")?; - container_init_process(args, main_sender, init_receiver) + container_init_process(args, main_sender, init_receiver, fifo_fd) })?; // Once we fork the container init process, the job for intermediate process // is done. We notify the container main process about the pid we just diff --git a/crates/libcontainer/src/process/container_main_process.rs b/crates/libcontainer/src/process/container_main_process.rs index 64975bdb..294ebed3 100644 --- a/crates/libcontainer/src/process/container_main_process.rs +++ b/crates/libcontainer/src/process/container_main_process.rs @@ -6,8 +6,11 @@ use crate::{ }; use anyhow::{Context, Result}; use nix::{ - sys::socket::{self, UnixAddr}, - unistd::{self, Pid}, + sys::{ + socket::{self, UnixAddr}, + stat, + }, + unistd::{self, mkfifo, Pid}, }; use oci_spec::runtime; use std::{io::IoSlice, path::Path}; @@ -22,12 +25,32 @@ pub fn container_main_process(container_args: &ContainerArgs) -> Result { let inter_chan = &mut channel::intermediate_channel()?; let init_chan = &mut channel::init_channel()?; + // TODO: implement Option version + let mut fifo_fd = 0; + // let container_root = &container_args + // .container + // .as_ref() + // .context("container state is required")? + // .root; + let container_root = &std::path::Path::new("/run/youki/tutorial_container/"); + let fifo_path = container_root.join("state.fifo"); + if container_args.init { + mkfifo(&fifo_path, stat::Mode::S_IRWXU).context("failed to create the fifo file.")?; + } + + let mut open_flags = nix::fcntl::OFlag::empty(); + open_flags.insert(nix::fcntl::OFlag::O_PATH); + open_flags.insert(nix::fcntl::OFlag::O_CLOEXEC); + fifo_fd = nix::fcntl::open(&fifo_path, open_flags, stat::Mode::S_IRWXU)?; + log::debug!("fifo_fd: {}", fifo_fd); + let intermediate_pid = fork::container_fork(|| { container_intermediate_process::container_intermediate_process( container_args, inter_chan, init_chan, main_sender, + fifo_fd, ) })?; // Close down unused fds. The corresponding fds are duplicated to the diff --git a/crates/youki/src/commands/exec.rs b/crates/youki/src/commands/exec.rs index 826b1b9e..1e55bd76 100644 --- a/crates/youki/src/commands/exec.rs +++ b/crates/youki/src/commands/exec.rs @@ -3,12 +3,15 @@ use nix::{ libc, poll::{PollFd, PollFlags}, }; -use std::{os::unix::prelude::RawFd, path::PathBuf}; +use std::{fs::OpenOptions, io::Read, os::unix::prelude::RawFd, path::PathBuf}; use libcontainer::{container::builder::ContainerBuilder, syscall::syscall::create_syscall}; use liboci_cli::Exec; +use super::load_container; + pub fn exec(args: Exec, root_path: PathBuf) -> Result { + let container = load_container(&root_path, &args.container_id)?; let syscall = create_syscall(); let pid = ContainerBuilder::new(args.container_id.clone(), syscall.as_ref()) .with_root_path(root_path)? @@ -26,6 +29,13 @@ pub fn exec(args: Exec, root_path: PathBuf) -> Result { let poll_fd = PollFd::new(pidfd, PollFlags::POLLIN); nix::poll::poll(&mut [poll_fd], -1).context("failed to wait for the container id")?; + let fifo_path = &container.root.join("state.fifo"); + println!("fifo_path: {:?}", fifo_path); + let mut f = OpenOptions::new().read(true).open(fifo_path)?; + let mut contents = String::new(); + f.read_to_string(&mut contents)?; + println!("get the value: {:?}", contents); + // TODO Ok(0) } From 3de7458dc1bcdbed20b48db06d96d40d7ef56828 Mon Sep 17 00:00:00 2001 From: utam0k Date: Thu, 1 Sep 2022 21:11:07 +0900 Subject: [PATCH 3/8] fix conflicts. Signed-off-by: utam0k --- .../src/container/builder_impl.rs | 5 ++- .../src/process/container_init_process.rs | 11 +----- .../process/container_intermediate_process.rs | 9 +++-- .../src/process/container_main_process.rs | 37 ++++++------------- crates/libcontainer/src/process/fork.rs | 15 ++++---- crates/youki/src/commands/exec.rs | 37 ++++--------------- 6 files changed, 35 insertions(+), 79 deletions(-) diff --git a/crates/libcontainer/src/container/builder_impl.rs b/crates/libcontainer/src/container/builder_impl.rs index fc8f0d9a..ed8f5de3 100644 --- a/crates/libcontainer/src/container/builder_impl.rs +++ b/crates/libcontainer/src/container/builder_impl.rs @@ -122,7 +122,8 @@ impl<'a> ContainerBuilderImpl<'a> { cgroup_manager: cmanager, }; - let init_pid = process::container_main_process::container_main_process(&container_args)?; + let (intermediate, init_pid) = + process::container_main_process::container_main_process(&container_args)?; // if file to write the pid to is specified, write pid of the child if let Some(pid_file) = &self.pid_file { @@ -139,7 +140,7 @@ impl<'a> ContainerBuilderImpl<'a> { .context("Failed to save container state")?; } - Ok(init_pid) + Ok(intermediate) } fn cleanup_container(&self) -> Result<()> { diff --git a/crates/libcontainer/src/process/container_init_process.rs b/crates/libcontainer/src/process/container_init_process.rs index 2a0ff4d9..ade3cf66 100644 --- a/crates/libcontainer/src/process/container_init_process.rs +++ b/crates/libcontainer/src/process/container_init_process.rs @@ -15,10 +15,7 @@ use nix::unistd::setsid; use nix::unistd::{self, Gid, Uid}; use oci_spec::runtime::{LinuxNamespaceType, Spec, User}; use std::collections::HashMap; -use std::fs::File; -use std::io::Write; use std::os::unix::io::AsRawFd; -use std::os::unix::prelude::FromRawFd; use std::{ env, fs, path::{Path, PathBuf}, @@ -162,7 +159,6 @@ pub fn container_init_process( args: &ContainerArgs, main_sender: &mut channel::MainSender, init_receiver: &mut channel::InitReceiver, - fifo_fd: i32, ) -> Result<()> { let syscall = args.syscall; let spec = args.spec; @@ -417,12 +413,7 @@ pub fn container_init_process( if proc.args().is_some() { ExecutorManager::exec(spec)?; - if fifo_fd != 0 { - let f = &mut unsafe { File::from_raw_fd(fifo_fd) }; - // TODO: impl - write!(f, "1")?; - } - Ok(()) + unreachable!("process image should have been replaced after exec"); } else { bail!("on non-Windows, at least one process arg entry is required") } diff --git a/crates/libcontainer/src/process/container_intermediate_process.rs b/crates/libcontainer/src/process/container_intermediate_process.rs index 02c349cf..be263d60 100644 --- a/crates/libcontainer/src/process/container_intermediate_process.rs +++ b/crates/libcontainer/src/process/container_intermediate_process.rs @@ -1,6 +1,7 @@ use crate::{namespaces::Namespaces, process::channel, process::fork}; use anyhow::{Context, Error, Result}; use libcgroups::common::CgroupManager; +use nix::sys::wait::{waitpid, WaitStatus}; use nix::unistd::{Gid, Pid, Uid}; use oci_spec::runtime::{LinuxNamespaceType, LinuxResources}; use procfs::process::Process; @@ -14,8 +15,7 @@ pub fn container_intermediate_process( intermediate_chan: &mut (channel::IntermediateSender, channel::IntermediateReceiver), init_chan: &mut (channel::InitSender, channel::InitReceiver), main_sender: &mut channel::MainSender, - fifo_fd: i32, -) -> Result<()> { +) -> Result { let (inter_sender, inter_receiver) = intermediate_chan; let (init_sender, init_receiver) = init_chan; let command = &args.syscall; @@ -96,7 +96,8 @@ pub fn container_intermediate_process( inter_sender .close() .context("failed to close sender in the intermediate process")?; - container_init_process(args, main_sender, init_receiver, fifo_fd) + container_init_process(args, main_sender, init_receiver)?; + Ok(0) })?; // Once we fork the container init process, the job for intermediate process // is done. We notify the container main process about the pid we just @@ -116,7 +117,7 @@ pub fn container_intermediate_process( .close() .context("failed to close unused init sender")?; - Ok(()) + Ok(waitpid(pid, None)?) } fn apply_cgroups( diff --git a/crates/libcontainer/src/process/container_main_process.rs b/crates/libcontainer/src/process/container_main_process.rs index 294ebed3..c8d34562 100644 --- a/crates/libcontainer/src/process/container_main_process.rs +++ b/crates/libcontainer/src/process/container_main_process.rs @@ -8,14 +8,14 @@ use anyhow::{Context, Result}; use nix::{ sys::{ socket::{self, UnixAddr}, - stat, + wait::WaitStatus, }, - unistd::{self, mkfifo, Pid}, + unistd::{self,Pid}, }; use oci_spec::runtime; use std::{io::IoSlice, path::Path}; -pub fn container_main_process(container_args: &ContainerArgs) -> Result { +pub fn container_main_process(container_args: &ContainerArgs) -> Result<(Pid, Pid)> { // We use a set of channels to communicate between parent and child process. // Each channel is uni-directional. Because we will pass these channel to // forked process, we have to be deligent about closing any unused channel. @@ -25,33 +25,18 @@ pub fn container_main_process(container_args: &ContainerArgs) -> Result { let inter_chan = &mut channel::intermediate_channel()?; let init_chan = &mut channel::init_channel()?; - // TODO: implement Option version - let mut fifo_fd = 0; - // let container_root = &container_args - // .container - // .as_ref() - // .context("container state is required")? - // .root; - let container_root = &std::path::Path::new("/run/youki/tutorial_container/"); - let fifo_path = container_root.join("state.fifo"); - if container_args.init { - mkfifo(&fifo_path, stat::Mode::S_IRWXU).context("failed to create the fifo file.")?; - } - - let mut open_flags = nix::fcntl::OFlag::empty(); - open_flags.insert(nix::fcntl::OFlag::O_PATH); - open_flags.insert(nix::fcntl::OFlag::O_CLOEXEC); - fifo_fd = nix::fcntl::open(&fifo_path, open_flags, stat::Mode::S_IRWXU)?; - log::debug!("fifo_fd: {}", fifo_fd); - let intermediate_pid = fork::container_fork(|| { - container_intermediate_process::container_intermediate_process( + let t = container_intermediate_process::container_intermediate_process( container_args, inter_chan, init_chan, main_sender, - fifo_fd, - ) + )?; + match t { + WaitStatus::Exited(_, s) => Ok(s), + WaitStatus::Signaled(_, sig, _) => Ok(sig as i32), + _ => Ok(0), + } })?; // Close down unused fds. The corresponding fds are duplicated to the // child process during fork. @@ -113,7 +98,7 @@ pub fn container_main_process(container_args: &ContainerArgs) -> Result { log::debug!("init pid is {:?}", init_pid); - Ok(init_pid) + Ok((intermediate_pid, init_pid)) } fn sync_seccomp( diff --git a/crates/libcontainer/src/process/fork.rs b/crates/libcontainer/src/process/fork.rs index 5ac1f32c..ac37ef44 100644 --- a/crates/libcontainer/src/process/fork.rs +++ b/crates/libcontainer/src/process/fork.rs @@ -8,15 +8,16 @@ use nix::unistd::Pid; // using clone, we would have to manually make sure all the variables are // correctly send to the new process, especially Rust borrow checker will be a // lot of hassel to deal with every details. -pub fn container_fork Result<()>>(cb: F) -> Result { +pub fn container_fork Result>(cb: F) -> Result { match unsafe { unistd::fork()? } { unistd::ForkResult::Parent { child } => Ok(child), unistd::ForkResult::Child => { - let ret = if let Err(error) = cb() { - log::debug!("failed to run fork: {:?}", error); - -1 - } else { - 0 + let ret = match cb() { + Err(error) => { + log::debug!("failed to run fork: {:?}", error); + -1 + } + Ok(ec) => ec, }; std::process::exit(ret); } @@ -31,7 +32,7 @@ mod test { #[test] fn test_container_fork() -> Result<()> { - let pid = container_fork(|| Ok(()))?; + let pid = container_fork(|| Ok(0))?; match waitpid(pid, None).expect("wait pid failed.") { WaitStatus::Exited(p, status) => { assert_eq!(pid, p); diff --git a/crates/youki/src/commands/exec.rs b/crates/youki/src/commands/exec.rs index 1e55bd76..b24becb9 100644 --- a/crates/youki/src/commands/exec.rs +++ b/crates/youki/src/commands/exec.rs @@ -1,17 +1,11 @@ -use anyhow::{bail, Context, Result}; -use nix::{ - libc, - poll::{PollFd, PollFlags}, -}; -use std::{fs::OpenOptions, io::Read, os::unix::prelude::RawFd, path::PathBuf}; +use anyhow::Result; +use nix::sys::wait::{waitpid, WaitStatus}; +use std::path::PathBuf; use libcontainer::{container::builder::ContainerBuilder, syscall::syscall::create_syscall}; use liboci_cli::Exec; -use super::load_container; - pub fn exec(args: Exec, root_path: PathBuf) -> Result { - let container = load_container(&root_path, &args.container_id)?; let syscall = create_syscall(); let pid = ContainerBuilder::new(args.container_id.clone(), syscall.as_ref()) .with_root_path(root_path)? @@ -25,26 +19,9 @@ pub fn exec(args: Exec, root_path: PathBuf) -> Result { .with_container_args(args.command.clone()) .build()?; - let pidfd = pidfd_open(pid.as_raw(), 0)?; - let poll_fd = PollFd::new(pidfd, PollFlags::POLLIN); - nix::poll::poll(&mut [poll_fd], -1).context("failed to wait for the container id")?; - - let fifo_path = &container.root.join("state.fifo"); - println!("fifo_path: {:?}", fifo_path); - let mut f = OpenOptions::new().read(true).open(fifo_path)?; - let mut contents = String::new(); - f.read_to_string(&mut contents)?; - println!("get the value: {:?}", contents); - - // TODO - Ok(0) -} - -fn pidfd_open(pid: libc::pid_t, flags: libc::c_uint) -> Result { - let fd = unsafe { libc::syscall(libc::SYS_pidfd_open, pid, flags) }; - if fd == -1 { - bail!("faild to pifd_open syscall") - } else { - Ok(fd as RawFd) + match waitpid(pid, None)? { + WaitStatus::Exited(_, status) => Ok(status), + WaitStatus::Signaled(_, sig, _) => Ok(sig as i32), + _ => Ok(0), } } From aa5d9c3c9b681fd0c7a476807a34493735ba1f6f Mon Sep 17 00:00:00 2001 From: Yashodhan Joshi Date: Tue, 23 Aug 2022 14:47:31 +0530 Subject: [PATCH 4/8] Make the intermediate process wait conditionally, only for exec Now the intermediate process waits for the main process only when exec is called, not when the container is created. This prevents it from waiting for created contianers, for which exec might not be called. This also fixes some formatting issues --- crates/libcontainer/src/container/builder_impl.rs | 8 ++++++-- crates/libcontainer/src/container/init_builder.rs | 1 + crates/libcontainer/src/container/tenant_builder.rs | 1 + .../src/process/container_intermediate_process.rs | 11 +++++++++-- .../src/process/container_main_process.rs | 5 +++-- 5 files changed, 20 insertions(+), 6 deletions(-) diff --git a/crates/libcontainer/src/container/builder_impl.rs b/crates/libcontainer/src/container/builder_impl.rs index ed8f5de3..31d7548c 100644 --- a/crates/libcontainer/src/container/builder_impl.rs +++ b/crates/libcontainer/src/container/builder_impl.rs @@ -38,6 +38,8 @@ pub(super) struct ContainerBuilderImpl<'a> { pub container: Option, /// File descriptos preserved/passed to the container init process. pub preserve_fds: i32, + /// Denotes if the builder is for the exec call or not + pub is_exec_builder: bool, } impl<'a> ContainerBuilderImpl<'a> { @@ -122,8 +124,10 @@ impl<'a> ContainerBuilderImpl<'a> { cgroup_manager: cmanager, }; - let (intermediate, init_pid) = - process::container_main_process::container_main_process(&container_args)?; + let (intermediate, init_pid) = process::container_main_process::container_main_process( + &container_args, + self.is_exec_builder, + )?; // if file to write the pid to is specified, write pid of the child if let Some(pid_file) = &self.pid_file { diff --git a/crates/libcontainer/src/container/init_builder.rs b/crates/libcontainer/src/container/init_builder.rs index e9ef18bc..e2bd1997 100644 --- a/crates/libcontainer/src/container/init_builder.rs +++ b/crates/libcontainer/src/container/init_builder.rs @@ -87,6 +87,7 @@ impl<'a> InitContainerBuilder<'a> { notify_path, container: Some(container.clone()), preserve_fds: self.base.preserve_fds, + is_exec_builder: false, }; builder_impl.create()?; diff --git a/crates/libcontainer/src/container/tenant_builder.rs b/crates/libcontainer/src/container/tenant_builder.rs index 5de9b02a..c5fb5a01 100644 --- a/crates/libcontainer/src/container/tenant_builder.rs +++ b/crates/libcontainer/src/container/tenant_builder.rs @@ -129,6 +129,7 @@ impl<'a> TenantContainerBuilder<'a> { notify_path: notify_path.clone(), container: None, preserve_fds: self.base.preserve_fds, + is_exec_builder: true, }; let pid = builder_impl.create()?; diff --git a/crates/libcontainer/src/process/container_intermediate_process.rs b/crates/libcontainer/src/process/container_intermediate_process.rs index be263d60..26c1304a 100644 --- a/crates/libcontainer/src/process/container_intermediate_process.rs +++ b/crates/libcontainer/src/process/container_intermediate_process.rs @@ -1,7 +1,7 @@ use crate::{namespaces::Namespaces, process::channel, process::fork}; use anyhow::{Context, Error, Result}; use libcgroups::common::CgroupManager; -use nix::sys::wait::{waitpid, WaitStatus}; +use nix::sys::wait::{waitpid, WaitStatus}; use nix::unistd::{Gid, Pid, Uid}; use oci_spec::runtime::{LinuxNamespaceType, LinuxResources}; use procfs::process::Process; @@ -15,6 +15,7 @@ pub fn container_intermediate_process( intermediate_chan: &mut (channel::IntermediateSender, channel::IntermediateReceiver), init_chan: &mut (channel::InitSender, channel::InitReceiver), main_sender: &mut channel::MainSender, + wait: bool, ) -> Result { let (inter_sender, inter_receiver) = intermediate_chan; let (init_sender, init_receiver) = init_chan; @@ -117,7 +118,13 @@ pub fn container_intermediate_process( .close() .context("failed to close unused init sender")?; - Ok(waitpid(pid, None)?) + if wait { + Ok(waitpid(pid, None)?) + } else { + // we don't actually want to wait, so + // pid and status doesn't really matter + Ok(WaitStatus::Exited(Pid::from_raw(0), 0)) + } } fn apply_cgroups( diff --git a/crates/libcontainer/src/process/container_main_process.rs b/crates/libcontainer/src/process/container_main_process.rs index c8d34562..11ee210b 100644 --- a/crates/libcontainer/src/process/container_main_process.rs +++ b/crates/libcontainer/src/process/container_main_process.rs @@ -10,12 +10,12 @@ use nix::{ socket::{self, UnixAddr}, wait::WaitStatus, }, - unistd::{self,Pid}, + unistd::{self, Pid}, }; use oci_spec::runtime; use std::{io::IoSlice, path::Path}; -pub fn container_main_process(container_args: &ContainerArgs) -> Result<(Pid, Pid)> { +pub fn container_main_process(container_args: &ContainerArgs, wait: bool) -> Result<(Pid, Pid)> { // We use a set of channels to communicate between parent and child process. // Each channel is uni-directional. Because we will pass these channel to // forked process, we have to be deligent about closing any unused channel. @@ -31,6 +31,7 @@ pub fn container_main_process(container_args: &ContainerArgs) -> Result<(Pid, Pi inter_chan, init_chan, main_sender, + wait, )?; match t { WaitStatus::Exited(_, s) => Ok(s), From 4f2e27d5887aaeee8f381ca162e9f2987a0bad2b Mon Sep 17 00:00:00 2001 From: Yashodhan Joshi Date: Mon, 29 Aug 2022 20:39:44 +0530 Subject: [PATCH 5/8] Remove the is_exec_builder as it is redundant, replace with !init --- crates/libcontainer/src/container/builder_impl.rs | 8 ++------ crates/libcontainer/src/container/init_builder.rs | 1 - crates/libcontainer/src/container/tenant_builder.rs | 1 - 3 files changed, 2 insertions(+), 8 deletions(-) diff --git a/crates/libcontainer/src/container/builder_impl.rs b/crates/libcontainer/src/container/builder_impl.rs index 31d7548c..2bd0ebd3 100644 --- a/crates/libcontainer/src/container/builder_impl.rs +++ b/crates/libcontainer/src/container/builder_impl.rs @@ -38,8 +38,6 @@ pub(super) struct ContainerBuilderImpl<'a> { pub container: Option, /// File descriptos preserved/passed to the container init process. pub preserve_fds: i32, - /// Denotes if the builder is for the exec call or not - pub is_exec_builder: bool, } impl<'a> ContainerBuilderImpl<'a> { @@ -124,10 +122,8 @@ impl<'a> ContainerBuilderImpl<'a> { cgroup_manager: cmanager, }; - let (intermediate, init_pid) = process::container_main_process::container_main_process( - &container_args, - self.is_exec_builder, - )?; + let (intermediate, init_pid) = + process::container_main_process::container_main_process(&container_args, !self.init)?; // if file to write the pid to is specified, write pid of the child if let Some(pid_file) = &self.pid_file { diff --git a/crates/libcontainer/src/container/init_builder.rs b/crates/libcontainer/src/container/init_builder.rs index e2bd1997..e9ef18bc 100644 --- a/crates/libcontainer/src/container/init_builder.rs +++ b/crates/libcontainer/src/container/init_builder.rs @@ -87,7 +87,6 @@ impl<'a> InitContainerBuilder<'a> { notify_path, container: Some(container.clone()), preserve_fds: self.base.preserve_fds, - is_exec_builder: false, }; builder_impl.create()?; diff --git a/crates/libcontainer/src/container/tenant_builder.rs b/crates/libcontainer/src/container/tenant_builder.rs index c5fb5a01..5de9b02a 100644 --- a/crates/libcontainer/src/container/tenant_builder.rs +++ b/crates/libcontainer/src/container/tenant_builder.rs @@ -129,7 +129,6 @@ impl<'a> TenantContainerBuilder<'a> { notify_path: notify_path.clone(), container: None, preserve_fds: self.base.preserve_fds, - is_exec_builder: true, }; let pid = builder_impl.create()?; From e526fe3c71069baaccc5e88798d0a5328a8d0205 Mon Sep 17 00:00:00 2001 From: Yashodhan Joshi Date: Tue, 6 Sep 2022 12:34:46 +0530 Subject: [PATCH 6/8] Make changes suggested in review --- .../src/process/container_main_process.rs | 11 ++++++++--- crates/libcontainer/src/process/fork.rs | 5 ++++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/crates/libcontainer/src/process/container_main_process.rs b/crates/libcontainer/src/process/container_main_process.rs index 11ee210b..c4336641 100644 --- a/crates/libcontainer/src/process/container_main_process.rs +++ b/crates/libcontainer/src/process/container_main_process.rs @@ -26,14 +26,13 @@ pub fn container_main_process(container_args: &ContainerArgs, wait: bool) -> Res let init_chan = &mut channel::init_channel()?; let intermediate_pid = fork::container_fork(|| { - let t = container_intermediate_process::container_intermediate_process( + match container_intermediate_process::container_intermediate_process( container_args, inter_chan, init_chan, main_sender, wait, - )?; - match t { + )? { WaitStatus::Exited(_, s) => Ok(s), WaitStatus::Signaled(_, sig, _) => Ok(sig as i32), _ => Ok(0), @@ -99,6 +98,12 @@ pub fn container_main_process(container_args: &ContainerArgs, wait: bool) -> Res log::debug!("init pid is {:?}", init_pid); + // here we send both intermediate and init pid, because : + // init pid is required for writing it to pid_file (if) given by the high-level runtime + // intermediate pid is required in the case when we call exec, as we nned to wait for the + // intermediate process to exit, which itself waits for child process (the exec process) to exit + // in order to get the proper exit code. We cannot simply wait for the init_pid , that is the actual container + // process, as it is not (direect) child of our process Ok((intermediate_pid, init_pid)) } diff --git a/crates/libcontainer/src/process/fork.rs b/crates/libcontainer/src/process/fork.rs index ac37ef44..12ba2dfc 100644 --- a/crates/libcontainer/src/process/fork.rs +++ b/crates/libcontainer/src/process/fork.rs @@ -9,6 +9,9 @@ use nix::unistd::Pid; // correctly send to the new process, especially Rust borrow checker will be a // lot of hassel to deal with every details. pub fn container_fork Result>(cb: F) -> Result { + // here we return the child's pid in case of parent, the i32 in return signature, + // and for child, we run the callback function, and exit with the same exit code + // given by it. If there was any error when trying to run callback, exit with -1 match unsafe { unistd::fork()? } { unistd::ForkResult::Parent { child } => Ok(child), unistd::ForkResult::Child => { @@ -17,7 +20,7 @@ pub fn container_fork Result>(cb: F) -> Result { log::debug!("failed to run fork: {:?}", error); -1 } - Ok(ec) => ec, + Ok(exit_code) => exit_code, }; std::process::exit(ret); } From c1d0f10b9c78a76576cede0e2065ab990733e5d1 Mon Sep 17 00:00:00 2001 From: Yashodhan Joshi Date: Mon, 12 Sep 2022 15:56:23 +0530 Subject: [PATCH 7/8] Move wait logic from intermediate process to main process --- .../process/container_intermediate_process.rs | 12 ++---------- .../src/process/container_main_process.rs | 19 ++++++++++++------- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/crates/libcontainer/src/process/container_intermediate_process.rs b/crates/libcontainer/src/process/container_intermediate_process.rs index 26c1304a..2a120310 100644 --- a/crates/libcontainer/src/process/container_intermediate_process.rs +++ b/crates/libcontainer/src/process/container_intermediate_process.rs @@ -1,7 +1,6 @@ use crate::{namespaces::Namespaces, process::channel, process::fork}; use anyhow::{Context, Error, Result}; use libcgroups::common::CgroupManager; -use nix::sys::wait::{waitpid, WaitStatus}; use nix::unistd::{Gid, Pid, Uid}; use oci_spec::runtime::{LinuxNamespaceType, LinuxResources}; use procfs::process::Process; @@ -15,8 +14,7 @@ pub fn container_intermediate_process( intermediate_chan: &mut (channel::IntermediateSender, channel::IntermediateReceiver), init_chan: &mut (channel::InitSender, channel::InitReceiver), main_sender: &mut channel::MainSender, - wait: bool, -) -> Result { +) -> Result { let (inter_sender, inter_receiver) = intermediate_chan; let (init_sender, init_receiver) = init_chan; let command = &args.syscall; @@ -118,13 +116,7 @@ pub fn container_intermediate_process( .close() .context("failed to close unused init sender")?; - if wait { - Ok(waitpid(pid, None)?) - } else { - // we don't actually want to wait, so - // pid and status doesn't really matter - Ok(WaitStatus::Exited(Pid::from_raw(0), 0)) - } + return Ok(pid); } fn apply_cgroups( diff --git a/crates/libcontainer/src/process/container_main_process.rs b/crates/libcontainer/src/process/container_main_process.rs index c4336641..c1b9504a 100644 --- a/crates/libcontainer/src/process/container_main_process.rs +++ b/crates/libcontainer/src/process/container_main_process.rs @@ -8,7 +8,7 @@ use anyhow::{Context, Result}; use nix::{ sys::{ socket::{self, UnixAddr}, - wait::WaitStatus, + wait::{waitpid, WaitStatus}, }, unistd::{self, Pid}, }; @@ -26,16 +26,21 @@ pub fn container_main_process(container_args: &ContainerArgs, wait: bool) -> Res let init_chan = &mut channel::init_channel()?; let intermediate_pid = fork::container_fork(|| { - match container_intermediate_process::container_intermediate_process( + let container_pid = container_intermediate_process::container_intermediate_process( container_args, inter_chan, init_chan, main_sender, - wait, - )? { - WaitStatus::Exited(_, s) => Ok(s), - WaitStatus::Signaled(_, sig, _) => Ok(sig as i32), - _ => Ok(0), + )?; + + if wait { + match waitpid(container_pid, None)? { + WaitStatus::Exited(_, s) => Ok(s), + WaitStatus::Signaled(_, sig, _) => Ok(sig as i32), + _ => Ok(0), + } + } else { + Ok(0) } })?; // Close down unused fds. The corresponding fds are duplicated to the From f8dc27f144ab8b63cb9c8bdceacf9a0faf849743 Mon Sep 17 00:00:00 2001 From: Yashodhan Joshi Date: Fri, 16 Sep 2022 15:24:04 +0530 Subject: [PATCH 8/8] Fix clippy lint --- .../libcontainer/src/process/container_intermediate_process.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/libcontainer/src/process/container_intermediate_process.rs b/crates/libcontainer/src/process/container_intermediate_process.rs index 2a120310..565de7ee 100644 --- a/crates/libcontainer/src/process/container_intermediate_process.rs +++ b/crates/libcontainer/src/process/container_intermediate_process.rs @@ -116,7 +116,7 @@ pub fn container_intermediate_process( .close() .context("failed to close unused init sender")?; - return Ok(pid); + Ok(pid) } fn apply_cgroups(