From 7f8422f2ef1babbe4b65d86152ade37f74f605bd Mon Sep 17 00:00:00 2001 From: Yashodhan Joshi Date: Mon, 19 Feb 2024 12:41:26 +0530 Subject: [PATCH] Add channel message when exec fails Signed-off-by: Yashodhan Joshi --- crates/libcontainer/src/process/channel.rs | 4 +++ .../src/process/container_init_process.rs | 6 ++++ .../process/container_intermediate_process.rs | 8 ++--- crates/libcontainer/src/workload/default.rs | 31 +++++++++++++++---- crates/libcontainer/src/workload/mod.rs | 4 +-- crates/youki/src/main.rs | 3 ++ 6 files changed, 44 insertions(+), 12 deletions(-) diff --git a/crates/libcontainer/src/process/channel.rs b/crates/libcontainer/src/process/channel.rs index 5a03c484..9683e5ed 100644 --- a/crates/libcontainer/src/process/channel.rs +++ b/crates/libcontainer/src/process/channel.rs @@ -173,6 +173,10 @@ impl MainReceiver { })?; match msg { Message::InitReady => Ok(()), + // this case in unique and known enough to have a special error format + Message::ExecFailed(err) => Err(ChannelError::ExecError(format!( + "error in executing process : {err}" + ))), msg => Err(ChannelError::UnexpectedMessage { expected: Message::InitReady, received: msg, diff --git a/crates/libcontainer/src/process/container_init_process.rs b/crates/libcontainer/src/process/container_init_process.rs index 46327f78..cd71e532 100644 --- a/crates/libcontainer/src/process/container_init_process.rs +++ b/crates/libcontainer/src/process/container_init_process.rs @@ -355,6 +355,12 @@ pub fn container_init_process( })?; } + // As we have chagned the root mount, from here on + // logs are no longer visible in journalctl + // so make sure that you bubble up any errors + // and do not call unwrap as any panics would not be correctly logged + + rootfs.adjust_root_mount_propagation(linux).map_err(|err| { tracing::error!(?err, "failed to adjust root mount propagation"); InitProcessError::RootFS(err) diff --git a/crates/libcontainer/src/process/container_intermediate_process.rs b/crates/libcontainer/src/process/container_intermediate_process.rs index 8feb71ee..53ff3d49 100644 --- a/crates/libcontainer/src/process/container_intermediate_process.rs +++ b/crates/libcontainer/src/process/container_intermediate_process.rs @@ -122,19 +122,19 @@ pub fn container_intermediate_process( match container_init_process(&args, &mut main_sender, &mut init_receiver) { Ok(_) => 0, Err(e) => { + tracing::error!("failed to initialize container process: {e}"); + if let Err(err) = main_sender.exec_failed(e.to_string()) { + tracing::error!(?err, "failed sending error to main sender"); + } if let ContainerType::TenantContainer { exec_notify_fd } = args.container_type { let buf = format!("{e}"); if let Err(err) = write(exec_notify_fd, buf.as_bytes()) { tracing::error!(?err, "failed to write to exec notify fd"); - return -1; } if let Err(err) = close(exec_notify_fd) { tracing::error!(?err, "failed to close exec notify fd"); - return -1; } } - - tracing::error!("failed to initialize container process: {e}"); -1 } } diff --git a/crates/libcontainer/src/workload/default.rs b/crates/libcontainer/src/workload/default.rs index 28f5ef97..f57d9a6a 100644 --- a/crates/libcontainer/src/workload/default.rs +++ b/crates/libcontainer/src/workload/default.rs @@ -34,7 +34,13 @@ impl Executor for DefaultExecutor { .collect(); unistd::execvp(&cstring_path, &a).map_err(|err| { tracing::error!(?err, filename = ?cstring_path, args = ?a, "failed to execvp"); - ExecutorError::Execution(err.into()) + ExecutorError::Execution( + format!( + "error '{}' executing '{:?}' with args '{:?}'", + err, cstring_path, a + ) + .into(), + ) })?; // After execvp is called, the process is replaced with the container @@ -46,14 +52,18 @@ impl Executor for DefaultExecutor { let proc = spec .process() .as_ref() - .ok_or(ExecutorValidationError::InvalidArg)?; + .ok_or(ExecutorValidationError::ArgValidationError( + "spec did not contain process".into(), + ))?; if let Some(args) = proc.args() { let envs: Vec = proc.env().as_ref().unwrap_or(&vec![]).clone(); let path_vars: Vec<&String> = envs.iter().filter(|&e| e.starts_with("PATH=")).collect(); if path_vars.is_empty() { tracing::error!("PATH environment variable is not set"); - Err(ExecutorValidationError::InvalidArg)?; + Err(ExecutorValidationError::ArgValidationError( + "PATH environment variable is not set".into(), + ))?; } let path_var = path_vars[0].trim_start_matches("PATH="); match get_executable_path(&args[0], path_var) { @@ -62,7 +72,10 @@ impl Executor for DefaultExecutor { executable = ?args[0], "executable for container process not found in PATH", ); - Err(ExecutorValidationError::InvalidArg)?; + Err(ExecutorValidationError::ArgValidationError(format!( + "executable '{}' not found in $PATH", + args[0] + )))?; } Some(path) => match is_executable(&path) { Ok(true) => { @@ -73,7 +86,10 @@ impl Executor for DefaultExecutor { executable = ?path, "executable does not have the correct permission set", ); - Err(ExecutorValidationError::InvalidArg)?; + Err(ExecutorValidationError::ArgValidationError(format!( + "executable '{}' at path '{:?}' does not have correct permissions", + args[0], path + )))?; } Err(err) => { tracing::error!( @@ -81,7 +97,10 @@ impl Executor for DefaultExecutor { ?err, "failed to check permissions for executable", ); - Err(ExecutorValidationError::InvalidArg)?; + Err(ExecutorValidationError::ArgValidationError(format!( + "failed to check permissions for executable '{}' at path '{:?}' : {}", + args[0], path, err + )))?; } }, } diff --git a/crates/libcontainer/src/workload/mod.rs b/crates/libcontainer/src/workload/mod.rs index 722a3cf7..78a3980f 100644 --- a/crates/libcontainer/src/workload/mod.rs +++ b/crates/libcontainer/src/workload/mod.rs @@ -20,8 +20,8 @@ pub enum ExecutorError { pub enum ExecutorValidationError { #[error("{0} executor can't handle spec")] CantHandle(&'static str), - #[error("invalid argument")] - InvalidArg, + #[error("{0}")] + ArgValidationError(String), } // Here is an explanation about the complexity below regarding to diff --git a/crates/youki/src/main.rs b/crates/youki/src/main.rs index 6a92be8d..668d2ca6 100644 --- a/crates/youki/src/main.rs +++ b/crates/youki/src/main.rs @@ -123,6 +123,7 @@ fn main() -> Result<()> { CommonCmd::Exec(exec) => match commands::exec::exec(exec, root_path) { Ok(exit_code) => std::process::exit(exit_code), Err(e) => { + tracing::error!("error in executing command: {:?}", e); eprintln!("exec failed : {e}"); std::process::exit(-1); } @@ -135,6 +136,7 @@ fn main() -> Result<()> { CommonCmd::Run(run) => match commands::run::run(run, root_path, systemd_cgroup) { Ok(exit_code) => std::process::exit(exit_code), Err(e) => { + tracing::error!("error in executing command: {:?}", e); eprintln!("run failed : {e}"); std::process::exit(-1); } @@ -151,6 +153,7 @@ fn main() -> Result<()> { if let Err(ref e) = cmd_result { tracing::error!("error in executing command: {:?}", e); + eprintln!("error in executing command: {:?}", e); } cmd_result }