From b82f42e4b5c1ccb9fbe8e707ccbfc70ccfc04f06 Mon Sep 17 00:00:00 2001 From: yihuaf Date: Tue, 16 May 2023 11:40:48 -0700 Subject: [PATCH] implemented thiserror for containers Signed-off-by: yihuaf --- crates/libcontainer/src/apparmor.rs | 13 +- crates/libcontainer/src/container/builder.rs | 46 ++++--- .../src/container/builder_impl.rs | 77 +++++++----- .../libcontainer/src/container/container.rs | 54 ++++---- .../src/container/container_delete.rs | 41 ++++--- .../src/container/container_events.rs | 4 +- .../src/container/container_kill.rs | 116 ++++++++++-------- .../src/container/container_pause.rs | 4 +- .../src/container/container_resume.rs | 4 +- .../src/container/container_start.rs | 54 ++++---- .../src/container/init_builder.rs | 88 ++++++++----- crates/libcontainer/src/container/state.rs | 4 +- .../src/container/tenant_builder.rs | 6 +- crates/libcontainer/src/error.rs | 76 +++++++++++- .../src/process/container_init_process.rs | 12 +- .../process/container_intermediate_process.rs | 10 +- crates/libcontainer/src/rootfs/rootfs.rs | 5 +- crates/libcontainer/src/rootless.rs | 10 +- crates/youki/src/commands/kill.rs | 6 +- crates/youki/src/commands/mod.rs | 4 +- 20 files changed, 365 insertions(+), 269 deletions(-) diff --git a/crates/libcontainer/src/apparmor.rs b/crates/libcontainer/src/apparmor.rs index 2defe9f74..86480608f 100644 --- a/crates/libcontainer/src/apparmor.rs +++ b/crates/libcontainer/src/apparmor.rs @@ -12,11 +12,6 @@ pub enum AppArmorError { profile: String, source: std::io::Error, }, - #[error("failed to read AppArmor profile: {source} {path}")] - ReadProfile { - path: String, - source: std::io::Error, - }, #[error(transparent)] EnsureProcfs(#[from] utils::EnsureProcfsError), } @@ -26,12 +21,8 @@ type Result = std::result::Result; const ENABLED_PARAMETER_PATH: &str = "/sys/module/apparmor/parameters/enabled"; /// Checks if AppArmor has been enabled on the system. -pub fn is_enabled() -> Result { - let aa_enabled = - fs::read_to_string(ENABLED_PARAMETER_PATH).map_err(|e| AppArmorError::ReadProfile { - path: ENABLED_PARAMETER_PATH.to_string(), - source: e, - })?; +pub fn is_enabled() -> std::result::Result { + let aa_enabled = fs::read_to_string(ENABLED_PARAMETER_PATH)?; Ok(aa_enabled.starts_with('Y')) } diff --git a/crates/libcontainer/src/container/builder.rs b/crates/libcontainer/src/container/builder.rs index 6f83ef5a6..80d0f8b94 100644 --- a/crates/libcontainer/src/container/builder.rs +++ b/crates/libcontainer/src/container/builder.rs @@ -1,7 +1,7 @@ +use crate::error::{ErrInvalidID, LibcontainerError}; use crate::workload::default::DefaultExecutor; use crate::workload::{Executor, ExecutorManager}; use crate::{syscall::Syscall, utils::PathBufExt}; -use anyhow::{anyhow, bail, Context, Result}; use std::path::PathBuf; use super::{init_builder::InitContainerBuilder, tenant_builder::TenantContainerBuilder}; @@ -93,18 +93,20 @@ impl<'a> ContainerBuilder<'a> { /// /// In addition, IDs that can't be used to represent a file name /// (such as . or ..) are rejected. - pub fn validate_id(self) -> Result { + pub fn validate_id(self) -> Result { let container_id = self.container_id.clone(); if container_id.is_empty() { - return Err(anyhow!("invalid container ID format: {:?}", container_id)); + Err(ErrInvalidID::Empty)?; } + if container_id == "." || container_id == ".." { - return Err(anyhow!("invalid container ID format: {:?}", container_id)); + Err(ErrInvalidID::FileName)?; } + for c in container_id.chars() { match c { 'a'..='z' | 'A'..='Z' | '0'..='9' | '_' | '+' | '-' | '.' => (), - _ => return Err(anyhow!("invalid container ID format: {:?}", container_id)), + _ => Err(ErrInvalidID::InvalidChars(c))?, } } Ok(self) @@ -166,11 +168,12 @@ impl<'a> ContainerBuilder<'a> { /// ) /// .with_root_path("/run/containers/youki").expect("invalid root path"); /// ``` - pub fn with_root_path>(mut self, path: P) -> Result { + pub fn with_root_path>(mut self, path: P) -> Result { let path = path.into(); - self.root_path = path - .canonicalize_safely() - .with_context(|| format!("failed to canonicalize root path {path:?}"))?; + self.root_path = path.canonicalize_safely().map_err(|err| { + tracing::error!(?path, ?err, "failed to canonicalize root path"); + LibcontainerError::InvalidInput(format!("invalid root path {path:?}: {err:?}")) + })?; Ok(self) } @@ -190,15 +193,15 @@ impl<'a> ContainerBuilder<'a> { /// ) /// .with_pid_file(Some("/var/run/docker.pid")).expect("invalid pid file"); /// ``` - pub fn with_pid_file>(mut self, path: Option

) -> Result { - self.pid_file = match path { - Some(path) => { - let p = path.into(); - Some( - p.canonicalize_safely() - .with_context(|| format!("failed to canonicalize pid file {p:?}"))?, - ) - } + pub fn with_pid_file>( + mut self, + path: Option

, + ) -> Result { + self.pid_file = match path.map(|p| p.into()) { + Some(path) => Some(path.canonicalize_safely().map_err(|err| { + tracing::error!(?path, ?err, "failed to canonicalize pid file"); + LibcontainerError::InvalidInput(format!("invalid pid file path {path:?}: {err:?}")) + })?), None => None, }; @@ -259,9 +262,12 @@ impl<'a> ContainerBuilder<'a> { /// ) /// .with_executor(vec![Box::::default()]); /// ``` - pub fn with_executor(mut self, executors: Vec>) -> Result { + pub fn with_executor( + mut self, + executors: Vec>, + ) -> Result { if executors.is_empty() { - bail!("executors must not be empty"); + return Err(LibcontainerError::NoExecutors); }; self.executor_manager = ExecutorManager { executors }; Ok(self) diff --git a/crates/libcontainer/src/container/builder_impl.rs b/crates/libcontainer/src/container/builder_impl.rs index 54de3f621..a9645fd61 100644 --- a/crates/libcontainer/src/container/builder_impl.rs +++ b/crates/libcontainer/src/container/builder_impl.rs @@ -1,5 +1,6 @@ use super::{Container, ContainerStatus}; use crate::{ + error::{LibcontainerError, MissingSpecError}, hooks, notify_socket::NotifyListener, process::{ @@ -12,7 +13,6 @@ use crate::{ utils, workload::ExecutorManager, }; -use anyhow::{bail, Context, Result}; use libcgroups::common::CgroupManager; use nix::unistd::Pid; use oci_spec::runtime::Spec; @@ -51,16 +51,14 @@ pub(super) struct ContainerBuilderImpl<'a> { } impl<'a> ContainerBuilderImpl<'a> { - pub(super) fn create(&mut self) -> Result { - match self.run_container().context("failed to create container") { + pub(super) fn create(&mut self) -> Result { + match self.run_container() { Ok(pid) => Ok(pid), Err(outer) => { // Only the init container should be cleaned up in the case of // an error. if matches!(self.container_type, ContainerType::InitContainer) { - if let Err(inner) = self.cleanup_container() { - return Err(outer.context(inner)); - } + self.cleanup_container()?; } Err(outer) @@ -68,8 +66,8 @@ impl<'a> ContainerBuilderImpl<'a> { } } - fn run_container(&mut self) -> Result { - let linux = self.spec.linux().as_ref().context("no linux in spec")?; + fn run_container(&mut self) -> Result { + let linux = self.spec.linux().as_ref().ok_or(MissingSpecError::Linux)?; let cgroups_path = utils::get_cgroup_path( linux.cgroups_path(), &self.container_id, @@ -79,8 +77,13 @@ impl<'a> ContainerBuilderImpl<'a> { cgroups_path, self.use_systemd || self.rootless.is_some(), &self.container_id, - )?; - let process = self.spec.process().as_ref().context("No process in spec")?; + ) + .map_err(|err| LibcontainerError::Cgroups(err.to_string()))?; + let process = self + .spec + .process() + .as_ref() + .ok_or(MissingSpecError::Process)?; if matches!(self.container_type, ContainerType::InitContainer) { if let Some(hooks) = self.spec.hooks() { @@ -105,8 +108,15 @@ impl<'a> ContainerBuilderImpl<'a> { // fork(2) so this will always be propagated properly. if let Some(oom_score_adj) = process.oom_score_adj() { tracing::debug!("Set OOM score to {}", oom_score_adj); - let mut f = fs::File::create("/proc/self/oom_score_adj")?; - f.write_all(oom_score_adj.to_string().as_bytes())?; + let mut f = fs::File::create("/proc/self/oom_score_adj").map_err(|err| { + tracing::error!("failed to open /proc/self/oom_score_adj: {}", err); + LibcontainerError::OtherIO(err) + })?; + f.write_all(oom_score_adj.to_string().as_bytes()) + .map_err(|err| { + tracing::error!("failed to write to /proc/self/oom_score_adj: {}", err); + LibcontainerError::OtherIO(err) + })?; } // Make the process non-dumpable, to avoid various race conditions that @@ -140,11 +150,19 @@ impl<'a> ContainerBuilderImpl<'a> { }; let (init_pid, need_to_clean_up_intel_rdt_dir) = - process::container_main_process::container_main_process(&container_args)?; + process::container_main_process::container_main_process(&container_args).map_err( + |err| { + tracing::error!(?err, "failed to run container process"); + LibcontainerError::MainProcess(err) + }, + )?; // if file to write the pid to is specified, write pid of the child if let Some(pid_file) = &self.pid_file { - fs::write(pid_file, format!("{init_pid}")).context("failed to write pid file")?; + fs::write(pid_file, format!("{init_pid}")).map_err(|err| { + tracing::error!("failed to write pid to file: {}", err); + LibcontainerError::OtherIO(err) + })?; } if let Some(container) = &mut self.container { @@ -154,15 +172,14 @@ impl<'a> ContainerBuilderImpl<'a> { .set_creator(nix::unistd::geteuid().as_raw()) .set_pid(init_pid.as_raw()) .set_clean_up_intel_rdt_directory(need_to_clean_up_intel_rdt_dir) - .save() - .context("Failed to save container state")?; + .save()?; } Ok(init_pid) } - fn cleanup_container(&self) -> Result<()> { - let linux = self.spec.linux().as_ref().context("no linux in spec")?; + fn cleanup_container(&self) -> Result<(), LibcontainerError> { + let linux = self.spec.linux().as_ref().ok_or(MissingSpecError::Linux)?; let cgroups_path = utils::get_cgroup_path( linux.cgroups_path(), &self.container_id, @@ -172,37 +189,37 @@ impl<'a> ContainerBuilderImpl<'a> { cgroups_path, self.use_systemd || self.rootless.is_some(), &self.container_id, - )?; + ) + .map_err(|err| LibcontainerError::Cgroups(err.to_string()))?; let mut errors = Vec::new(); - if let Err(e) = cmanager.remove().context("failed to remove cgroup") { + if let Err(e) = cmanager.remove() { + tracing::error!(error = ?e, "failed to remove cgroup manager"); errors.push(e.to_string()); } if let Some(container) = &self.container { if let Some(true) = container.clean_up_intel_rdt_subdirectory() { - if let Err(e) = delete_resctrl_subdirectory(container.id()).with_context(|| { - format!( - "failed to delete resctrl subdirectory: {:?}", - container.id() - ) - }) { + if let Err(e) = delete_resctrl_subdirectory(container.id()) { + tracing::error!(id = ?container.id(), error = ?e, "failed to delete resctrl subdirectory"); errors.push(e.to_string()); } } if container.root.exists() { - if let Err(e) = fs::remove_dir_all(&container.root) - .with_context(|| format!("could not delete {:?}", container.root)) - { + if let Err(e) = fs::remove_dir_all(&container.root) { + tracing::error!(container_root = ?container.root, error = ?e, "failed to delete container root"); errors.push(e.to_string()); } } } if !errors.is_empty() { - bail!("failed to cleanup container: {}", errors.join(";")); + return Err(LibcontainerError::Other(format!( + "failed to cleanup container: {}", + errors.join(";") + ))); } Ok(()) diff --git a/crates/libcontainer/src/container/container.rs b/crates/libcontainer/src/container/container.rs index 46b157808..241114653 100644 --- a/crates/libcontainer/src/container/container.rs +++ b/crates/libcontainer/src/container/container.rs @@ -1,19 +1,17 @@ -use std::collections::HashMap; -use std::ffi::OsString; -use std::fs; -use std::path::{Path, PathBuf}; +use crate::config::YoukiConfig; +use crate::container::{ContainerStatus, State}; +use crate::error::LibcontainerError; +use crate::syscall::syscall::create_syscall; -use anyhow::Result; use chrono::DateTime; -use nix::unistd::Pid; - use chrono::Utc; +use nix::unistd::Pid; use procfs::process::Process; -use crate::config::YoukiConfig; -use crate::syscall::syscall::create_syscall; - -use crate::container::{ContainerStatus, State}; +use std::collections::HashMap; +use std::ffi::OsString; +use std::fs; +use std::path::{Path, PathBuf}; /// Structure representing the container data #[derive(Debug, Clone)] @@ -40,10 +38,17 @@ impl Container { pid: Option, bundle: &Path, container_root: &Path, - ) -> Result { - let container_root = fs::canonicalize(container_root)?; + ) -> Result { + let container_root = fs::canonicalize(container_root).map_err(|err| { + LibcontainerError::InvalidInput(format!( + "invalid container root {container_root:?}: {err:?}" + )) + })?; + let bundle = fs::canonicalize(bundle).map_err(|err| { + LibcontainerError::InvalidInput(format!("invalid bundle {bundle:?}: {err:?}")) + })?; + let state = State::new(container_id, status, pid, bundle); - let state = State::new(container_id, status, pid, fs::canonicalize(bundle)?); Ok(Self { state, root: container_root, @@ -117,12 +122,12 @@ impl Container { self } - pub fn systemd(&self) -> Option { + pub fn systemd(&self) -> bool { self.state.use_systemd } pub fn set_systemd(&mut self, should_use: bool) -> &mut Self { - self.state.use_systemd = Some(should_use); + self.state.use_systemd = should_use; self } @@ -151,7 +156,7 @@ impl Container { self } - pub fn refresh_status(&mut self) -> Result<()> { + pub fn refresh_status(&mut self) -> Result<(), LibcontainerError> { let new_status = match self.pid() { Some(pid) => { // Note that Process::new does not spawn a new process @@ -180,14 +185,14 @@ impl Container { Ok(()) } - pub fn refresh_state(&mut self) -> Result<&mut Self> { + pub fn refresh_state(&mut self) -> Result<&mut Self, LibcontainerError> { let state = State::load(&self.root)?; self.state = state; Ok(self) } - pub fn load(container_root: PathBuf) -> Result { + pub fn load(container_root: PathBuf) -> Result { let state = State::load(&container_root)?; let mut container = Self { state, @@ -197,14 +202,14 @@ impl Container { Ok(container) } - pub fn save(&self) -> Result<()> { + pub fn save(&self) -> Result<(), LibcontainerError> { tracing::debug!("Save container status: {:?} in {:?}", self, self.root); self.state.save(&self.root)?; Ok(()) } - pub fn spec(&self) -> Result { + pub fn spec(&self) -> Result { let spec = YoukiConfig::load(&self.root)?; Ok(spec) } @@ -225,6 +230,7 @@ pub struct CheckpointOptions { mod tests { use super::*; use anyhow::Context; + use anyhow::Result; use serial_test::serial; #[test] @@ -280,11 +286,11 @@ mod tests { #[test] fn test_get_set_systemd() { let mut container = Container::default(); - assert_eq!(container.systemd(), None); + assert!(!container.systemd()); container.set_systemd(true); - assert_eq!(container.systemd(), Some(true)); + assert!(container.systemd()); container.set_systemd(false); - assert_eq!(container.systemd(), Some(false)); + assert!(!container.systemd()); } #[test] diff --git a/crates/libcontainer/src/container/container_delete.rs b/crates/libcontainer/src/container/container_delete.rs index 2bc9827a8..1cc55b9cf 100644 --- a/crates/libcontainer/src/container/container_delete.rs +++ b/crates/libcontainer/src/container/container_delete.rs @@ -1,8 +1,7 @@ use super::{Container, ContainerStatus}; -use crate::config::YoukiConfig; use crate::hooks; use crate::process::intel_rdt::delete_resctrl_subdirectory; -use anyhow::{bail, Context, Result}; +use crate::{config::YoukiConfig, error::LibcontainerError}; use libcgroups::{self, common::CgroupManager}; use nix::sys::signal; use std::fs; @@ -29,9 +28,8 @@ impl Container { /// # Ok(()) /// # } /// ``` - pub fn delete(&mut self, force: bool) -> Result<()> { - self.refresh_status() - .context("failed to refresh container status")?; + pub fn delete(&mut self, force: bool) -> Result<(), LibcontainerError> { + self.refresh_status()?; tracing::debug!("container status: {:?}", self.status()); @@ -55,11 +53,12 @@ impl Container { self.do_kill(signal::Signal::SIGKILL, true)?; self.set_status(ContainerStatus::Stopped).save()?; } else { - bail!( - "{} could not be deleted because it was {:?}", - self.id(), - self.status() - ) + tracing::error!( + id = ?self.id(), + status = ?self.status(), + "delete requires the container state to be stopped or created", + ); + return Err(LibcontainerError::IncorrectContainerStatus); } } } @@ -83,22 +82,23 @@ impl Container { // remove the cgroup created for the container // check https://man7.org/linux/man-pages/man7/cgroups.7.html // creating and removing cgroups section for more information on cgroups - let use_systemd = self - .systemd() - .context("container state does not contain cgroup manager")?; + let use_systemd = self.systemd(); let cmanager = libcgroups::common::create_cgroup_manager( &config.cgroup_path, use_systemd, self.id(), ) - .context("failed to create cgroup manager")?; - cmanager.remove().with_context(|| { - format!("failed to remove cgroup {}", config.cgroup_path.display()) + .map_err(|err| LibcontainerError::Cgroups(err.to_string()))?; + cmanager.remove().map_err(|err| { + tracing::error!(cgroup_path = ?config.cgroup_path, "failed to remove cgroup due to: {err:?}"); + LibcontainerError::Cgroups(err.to_string()) })?; if let Some(hooks) = config.hooks.as_ref() { - hooks::run_hooks(hooks.poststop().as_ref(), Some(self)) - .with_context(|| "failed to run post stop hooks")?; + hooks::run_hooks(hooks.poststop().as_ref(), Some(self)).map_err(|err| { + tracing::error!(err = ?err, "failed to run post stop hooks"); + err + })?; } } Err(err) => { @@ -114,8 +114,9 @@ impl Container { // remove the directory storing container state tracing::debug!("remove dir {:?}", self.root); - fs::remove_dir_all(&self.root).with_context(|| { - format!("failed to remove container dir {}", self.root.display()) + fs::remove_dir_all(&self.root).map_err(|err| { + tracing::error!(?err, path = ?self.root, "failed to remove container dir"); + LibcontainerError::OtherIO(err) })?; } diff --git a/crates/libcontainer/src/container/container_events.rs b/crates/libcontainer/src/container/container_events.rs index fd90788c5..67e7ac6a8 100644 --- a/crates/libcontainer/src/container/container_events.rs +++ b/crates/libcontainer/src/container/container_events.rs @@ -34,9 +34,7 @@ impl Container { } let cgroups_path = self.spec()?.cgroup_path; - let use_systemd = self - .systemd() - .context("could not determine cgroup manager")?; + let use_systemd = self.systemd(); let cgroup_manager = libcgroups::common::create_cgroup_manager(cgroups_path, use_systemd, self.id())?; diff --git a/crates/libcontainer/src/container/container_kill.rs b/crates/libcontainer/src/container/container_kill.rs index 0de616524..404567b74 100644 --- a/crates/libcontainer/src/container/container_kill.rs +++ b/crates/libcontainer/src/container/container_kill.rs @@ -1,6 +1,5 @@ use super::{Container, ContainerStatus}; -use crate::signal::Signal; -use anyhow::{bail, Context, Result}; +use crate::{error::LibcontainerError, signal::Signal}; use libcgroups::common::{create_cgroup_manager, get_cgroup_setup, CgroupManager}; use nix::sys::signal::{self}; @@ -27,28 +26,29 @@ impl Container { /// # Ok(()) /// # } /// ``` - pub fn kill>(&mut self, signal: S, all: bool) -> Result<()> { - self.refresh_status() - .context("failed to refresh container status")?; - if self.can_kill() { - self.do_kill(signal, all)?; - } else { - // just like runc, allow kill --all even if the container is stopped - if all && self.status() == ContainerStatus::Stopped { + pub fn kill>(&mut self, signal: S, all: bool) -> Result<(), LibcontainerError> { + self.refresh_status()?; + match self.can_kill() { + true => { + self.do_kill(signal, all)?; + } + false if all && self.status() == ContainerStatus::Stopped => { self.do_kill(signal, all)?; - } else { - bail!( - "{} could not be killed because it was {:?}", - self.id(), - self.status() - ) + } + false => { + tracing::error!(id = ?self.id(), status = ?self.status(), "cannot kill container due to incorrect state"); + return Err(LibcontainerError::IncorrectContainerStatus); } } self.set_status(ContainerStatus::Stopped).save()?; Ok(()) } - pub(crate) fn do_kill>(&self, signal: S, all: bool) -> Result<()> { + pub(crate) fn do_kill>( + &self, + signal: S, + all: bool, + ) -> Result<(), LibcontainerError> { if all { self.kill_all_processes(signal) } else { @@ -56,32 +56,36 @@ impl Container { } } - fn kill_one_process>(&self, signal: S) -> Result<()> { + fn kill_one_process>(&self, signal: S) -> Result<(), LibcontainerError> { let signal = signal.into().into_raw(); let pid = self.pid().unwrap(); tracing::debug!("kill signal {} to {}", signal, pid); - let res = signal::kill(pid, signal); - match res { + match signal::kill(pid, signal) { + Ok(_) => {} Err(nix::errno::Errno::ESRCH) => { - /* the process does not exist, which is what we want */ + // the process does not exist, which is what we want + } + Err(err) => { + tracing::error!(id = ?self.id(), err = ?err, ?pid, ?signal, "failed to kill process"); + return Err(LibcontainerError::OtherSyscall(err)); } - _ => res?, } // For cgroup V1, a frozon process cannot respond to signals, // so we need to thaw it. Only thaw the cgroup for SIGKILL. if self.status() == ContainerStatus::Paused && signal == signal::Signal::SIGKILL { - match get_cgroup_setup()? { + match get_cgroup_setup().map_err(|err| LibcontainerError::Cgroups(err.to_string()))? { libcgroups::common::CgroupSetup::Legacy | libcgroups::common::CgroupSetup::Hybrid => { let cgroups_path = self.spec()?.cgroup_path; - let use_systemd = self - .systemd() - .context("container state does not contain cgroup manager")?; - let cmanger = create_cgroup_manager(cgroups_path, use_systemd, self.id())?; - cmanger.freeze(libcgroups::common::FreezerState::Thawed)?; + let use_systemd = self.systemd(); + let cmanger = create_cgroup_manager(cgroups_path, use_systemd, self.id()) + .map_err(|err| LibcontainerError::Cgroups(err.to_string()))?; + cmanger + .freeze(libcgroups::common::FreezerState::Thawed) + .map_err(|err| LibcontainerError::Cgroups(err.to_string()))?; } libcgroups::common::CgroupSetup::Unified => {} } @@ -89,41 +93,45 @@ impl Container { Ok(()) } - fn kill_all_processes>(&self, signal: S) -> Result<()> { + fn kill_all_processes>(&self, signal: S) -> Result<(), LibcontainerError> { let signal = signal.into().into_raw(); let cgroups_path = self.spec()?.cgroup_path; - let use_systemd = self - .systemd() - .context("container state does not contain cgroup manager")?; - let cmanger = create_cgroup_manager(cgroups_path, use_systemd, self.id())?; - let ret = cmanger.freeze(libcgroups::common::FreezerState::Frozen); - if ret.is_err() { + let use_systemd = self.systemd(); + let cmanger = create_cgroup_manager(cgroups_path, use_systemd, self.id()) + .map_err(|err| LibcontainerError::Cgroups(err.to_string()))?; + + if let Err(e) = cmanger.freeze(libcgroups::common::FreezerState::Frozen) { tracing::warn!( - "failed to freeze container {}, error: {}", - self.id(), - ret.unwrap_err() + err = ?e, + id = ?self.id(), + "failed to freeze container", ); } - let pids = cmanger.get_all_pids()?; - pids.iter().try_for_each(|&pid| { - tracing::debug!("kill signal {} to {}", signal, pid); - let res = signal::kill(pid, signal); - match res { - Err(nix::errno::Errno::ESRCH) => { - /* the process does not exist, which is what we want */ - Ok(()) + + let pids = cmanger + .get_all_pids() + .map_err(|err| LibcontainerError::Cgroups(err.to_string()))?; + pids.iter() + .try_for_each(|&pid| { + tracing::debug!("kill signal {} to {}", signal, pid); + let res = signal::kill(pid, signal); + match res { + Err(nix::errno::Errno::ESRCH) => { + // the process does not exist, which is what we want + Ok(()) + } + _ => res, } - _ => res, - } - })?; - let ret = cmanger.freeze(libcgroups::common::FreezerState::Thawed); - if ret.is_err() { + }) + .map_err(LibcontainerError::OtherSyscall)?; + if let Err(err) = cmanger.freeze(libcgroups::common::FreezerState::Thawed) { tracing::warn!( - "failed to thaw container {}, error: {}", - self.id(), - ret.unwrap_err() + err = ?err, + id = ?self.id(), + "failed to thaw container", ); } + Ok(()) } } diff --git a/crates/libcontainer/src/container/container_pause.rs b/crates/libcontainer/src/container/container_pause.rs index 916f8447e..6c916c46c 100644 --- a/crates/libcontainer/src/container/container_pause.rs +++ b/crates/libcontainer/src/container/container_pause.rs @@ -37,9 +37,7 @@ impl Container { } let cgroups_path = self.spec()?.cgroup_path; - let use_systemd = self - .systemd() - .context("container state does not contain cgroup manager")?; + let use_systemd = self.systemd(); let cmanager = libcgroups::common::create_cgroup_manager(cgroups_path, use_systemd, self.id())?; cmanager.freeze(FreezerState::Frozen)?; diff --git a/crates/libcontainer/src/container/container_resume.rs b/crates/libcontainer/src/container/container_resume.rs index cf221d3f9..134b636f8 100644 --- a/crates/libcontainer/src/container/container_resume.rs +++ b/crates/libcontainer/src/container/container_resume.rs @@ -39,9 +39,7 @@ impl Container { } let cgroups_path = self.spec()?.cgroup_path; - let use_systemd = self - .systemd() - .context("container state does not contain cgroup manager")?; + let use_systemd = self.systemd(); let cmanager = libcgroups::common::create_cgroup_manager(cgroups_path, use_systemd, self.id())?; // resume the frozen container diff --git a/crates/libcontainer/src/container/container_start.rs b/crates/libcontainer/src/container/container_start.rs index a348b0ad6..21adbb9f3 100644 --- a/crates/libcontainer/src/container/container_start.rs +++ b/crates/libcontainer/src/container/container_start.rs @@ -1,11 +1,11 @@ use crate::{ config::YoukiConfig, + error::LibcontainerError, hooks, notify_socket::{NotifySocket, NOTIFY_FILE}, }; use super::{Container, ContainerStatus}; -use anyhow::{bail, Context, Result}; use nix::{sys::signal, unistd}; impl Container { @@ -30,49 +30,57 @@ impl Container { /// # Ok(()) /// # } /// ``` - pub fn start(&mut self) -> Result<()> { - self.refresh_status() - .context("failed to refresh container status")?; + pub fn start(&mut self) -> Result<(), LibcontainerError> { + self.refresh_status()?; if !self.can_start() { - let err_msg = format!( - "{} could not be started because it was {:?}", - self.id(), - self.status() - ); - tracing::error!("{}", err_msg); - bail!(err_msg); + tracing::error!(status = ?self.status(), id = ?self.id(), "cannot start container due to incorrect state"); + return Err(LibcontainerError::IncorrectContainerStatus); } - let config = YoukiConfig::load(&self.root) - .with_context(|| format!("failed to load runtime spec for container {}", self.id()))?; + let config = YoukiConfig::load(&self.root).map_err(|err| { + tracing::error!( + "failed to load runtime spec for container {}: {}", + self.id(), + err + ); + err + })?; if let Some(hooks) = config.hooks.as_ref() { // While prestart is marked as deprecated in the OCI spec, the docker and integration test still // uses it. #[allow(deprecated)] - let ret = hooks::run_hooks(hooks.prestart().as_ref(), Some(self)) - .with_context(|| "failed to run pre start hooks"); - if ret.is_err() { + hooks::run_hooks(hooks.prestart().as_ref(), Some(self)).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. - self.kill(signal::Signal::SIGKILL, true)?; - return ret; - } + let _ = self.kill(signal::Signal::SIGKILL, true); + + err + })?; } - unistd::chdir(self.root.as_os_str())?; + 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) .save() - .with_context(|| format!("could not save state for container {}", self.id()))?; + .map_err(|err| { + tracing::error!(id = ?self.id(), ?err, "failed to save state for container"); + err + })?; // 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)) - .with_context(|| "failed to run post start hooks")?; + hooks::run_hooks(hooks.poststart().as_ref(), Some(self)).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 index 8720457ed..8283817ae 100644 --- a/crates/libcontainer/src/container/init_builder.rs +++ b/crates/libcontainer/src/container/init_builder.rs @@ -1,4 +1,3 @@ -use anyhow::{bail, Context, Result}; use nix::unistd; use oci_spec::runtime::Spec; use rootless::Rootless; @@ -8,8 +7,12 @@ use std::{ }; use crate::{ - apparmor, config::YoukiConfig, notify_socket::NOTIFY_FILE, process::args::ContainerType, - rootless, tty, utils, + apparmor, + config::YoukiConfig, + error::{ErrInvalidSpec, LibcontainerError, MissingSpecError}, + notify_socket::NOTIFY_FILE, + process::args::ContainerType, + rootless, tty, }; use super::{ @@ -48,23 +51,27 @@ impl<'a> InitContainerBuilder<'a> { } /// Creates a new container - pub fn build(self) -> Result { - let spec = self.load_spec().context("failed to load spec")?; - let container_dir = self - .create_container_dir() - .context("failed to create container dir")?; - - let mut container = self - .create_container_state(&container_dir) - .context("failed to create container state")?; + pub fn build(self) -> Result { + let spec = self.load_spec()?; + let container_dir = self.create_container_dir()?; + + let mut container = self.create_container_state(&container_dir)?; container .set_systemd(self.use_systemd) .set_annotations(spec.annotations().clone()); - unistd::chdir(&container_dir)?; + 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().context("no root in spec")?.path())?; + let rootfs = fs::canonicalize(spec.root().as_ref().ok_or(MissingSpecError::Root)?.path()) + .map_err(LibcontainerError::OtherIO)?; // if socket file path is given in commandline options, // get file descriptors of console socket @@ -80,9 +87,10 @@ impl<'a> InitContainerBuilder<'a> { let rootless = Rootless::new(&spec)?; let config = YoukiConfig::from_spec(&spec, container.id(), rootless.is_some())?; - config - .save(&container_dir) - .context("failed to save config")?; + config.save(&container_dir).map_err(|err| { + tracing::error!(?container_dir, "failed to save config: {}", err); + err + })?; let mut builder_impl = ContainerBuilderImpl { container_type: ContainerType::InitContainer, @@ -108,46 +116,60 @@ impl<'a> InitContainerBuilder<'a> { Ok(container) } - fn create_container_dir(&self) -> Result { + fn create_container_dir(&self) -> Result { let container_dir = self.base.root_path.join(&self.base.container_id); tracing::debug!("container directory will be {:?}", container_dir); if container_dir.exists() { - bail!("container {} already exists", self.base.container_id); + tracing::error!(id = self.base.container_id, dir = ?container_dir, "container already exists"); + return Err(LibcontainerError::ContainerAlreadyExists); } - utils::create_dir_all(&container_dir).context("failed to create container dir")?; + std::fs::create_dir_all(&container_dir).map_err(|err| { + tracing::error!( + ?container_dir, + "failed to create container directory: {}", + err + ); + LibcontainerError::OtherIO(err) + })?; Ok(container_dir) } - fn load_spec(&self) -> Result { + fn load_spec(&self) -> Result { let source_spec_path = self.bundle.join("config.json"); let mut spec = Spec::load(source_spec_path)?; - Self::validate_spec(&spec).context("failed to validate runtime spec")?; + Self::validate_spec(&spec)?; + + spec.canonicalize_rootfs(&self.bundle).map_err(|err| { + tracing::error!(bundle = ?self.bundle, "failed to canonicalize rootfs: {}", err); + err + })?; - spec.canonicalize_rootfs(&self.bundle) - .context("failed to canonicalize rootfs")?; Ok(spec) } - fn validate_spec(spec: &Spec) -> Result<()> { + fn validate_spec(spec: &Spec) -> Result<(), LibcontainerError> { let version = spec.version(); if !version.starts_with("1.") { - bail!( + tracing::error!( "runtime spec has incompatible version '{}'. Only 1.X.Y is supported", spec.version() ); + Err(ErrInvalidSpec::UnsupportedVersion)?; } if let Some(process) = spec.process() { if let Some(profile) = process.apparmor_profile() { - if !apparmor::is_enabled()? { - bail!( - "apparmor profile {} is specified in runtime spec, \ - but apparmor is not activated on this system", - profile - ); + let apparmor_is_enabled = apparmor::is_enabled().map_err(|err| { + tracing::error!(?err, "failed to check if apparmor is enabled"); + LibcontainerError::OtherIO(err) + })?; + if !apparmor_is_enabled { + tracing::error!(?profile, + "apparmor profile exists in the spec, but apparmor is not activated on this system"); + Err(ErrInvalidSpec::AppArmorNotEnabled)?; } } } @@ -155,7 +177,7 @@ impl<'a> InitContainerBuilder<'a> { Ok(()) } - fn create_container_state(&self, container_dir: &Path) -> Result { + fn create_container_state(&self, container_dir: &Path) -> Result { let container = Container::new( &self.base.container_id, ContainerStatus::Creating, diff --git a/crates/libcontainer/src/container/state.rs b/crates/libcontainer/src/container/state.rs index 4e61866f6..5b6bd6f18 100644 --- a/crates/libcontainer/src/container/state.rs +++ b/crates/libcontainer/src/container/state.rs @@ -118,7 +118,7 @@ pub struct State { #[serde(skip_serializing_if = "Option::is_none")] pub creator: Option, // Specifies if systemd should be used to manage cgroups - pub use_systemd: Option, + pub use_systemd: bool, // Specifies if the Intel RDT subdirectory needs be cleaned up. pub clean_up_intel_rdt_subdirectory: Option, } @@ -141,7 +141,7 @@ impl State { annotations: Some(HashMap::default()), created: None, creator: None, - use_systemd: None, + use_systemd: false, clean_up_intel_rdt_subdirectory: None, } } diff --git a/crates/libcontainer/src/container/tenant_builder.rs b/crates/libcontainer/src/container/tenant_builder.rs index 4dcae7e73..e50b820b1 100644 --- a/crates/libcontainer/src/container/tenant_builder.rs +++ b/crates/libcontainer/src/container/tenant_builder.rs @@ -376,11 +376,7 @@ impl<'a> TenantContainerBuilder<'a> { } fn should_use_systemd(&self, container: &Container) -> bool { - if let Some(use_systemd) = container.systemd() { - return use_systemd; - } - - false + container.systemd() } fn setup_notify_listener(container_dir: &Path) -> Result { diff --git a/crates/libcontainer/src/error.rs b/crates/libcontainer/src/error.rs index 7409f585f..dc9401d1c 100644 --- a/crates/libcontainer/src/error.rs +++ b/crates/libcontainer/src/error.rs @@ -14,9 +14,79 @@ pub enum UnifiedSyscallError { #[derive(Debug, thiserror::Error)] pub enum MissingSpecError { #[error("missing process in spec")] - MissingProcess, + Process, #[error("missing linux in spec")] - MissingLinux, + Linux, #[error("missing args in the process spec")] - MissingArgs, + Args, + #[error("missing root in the spec")] + Root, +} + +#[derive(Debug, thiserror::Error)] +pub enum LibcontainerError { + #[error("failed to perform operation due to incorrect container status")] + IncorrectContainerStatus, + #[error("container already exists")] + ContainerAlreadyExists, + #[error("invalid input")] + InvalidInput(String), + #[error("requires at least one executors")] + NoExecutors, + + // Invalid inputs + #[error(transparent)] + InvalidID(#[from] ErrInvalidID), + #[error(transparent)] + MissingSpec(#[from] MissingSpecError), + #[error("invalid runtime spec")] + InvalidSpec(#[from] ErrInvalidSpec), + + // Errors from submodules and other errors + #[error(transparent)] + Tty(#[from] crate::tty::TTYError), + #[error(transparent)] + Rootless(#[from] crate::rootless::RootlessError), + #[error(transparent)] + NotifyListener(#[from] crate::notify_socket::NotifyListenerError), + #[error(transparent)] + Config(#[from] crate::config::ConfigError), + #[error(transparent)] + Hook(#[from] crate::hooks::HookError), + #[error(transparent)] + State(#[from] crate::container::state::StateError), + #[error("oci spec error")] + Spec(#[from] oci_spec::OciSpecError), + #[error("cgroups error: {0}")] + Cgroups(String), + #[error(transparent)] + MainProcess(#[from] crate::process::container_main_process::ProcessError), + #[error(transparent)] + Procfs(#[from] procfs::ProcError), + + // Catch all errors that are not covered by the above + #[error("syscall error")] + OtherSyscall(#[source] nix::Error), + #[error("IO error")] + OtherIO(#[source] std::io::Error), + #[error("{0}")] + Other(String), +} + +#[derive(Debug, thiserror::Error)] +pub enum ErrInvalidID { + #[error("container id can't be empty")] + Empty, + #[error("container id contains invalid characters: {0}")] + InvalidChars(char), + #[error("container id can't be used to represent a file name (such as . or ..)")] + FileName, +} + +#[derive(Debug, thiserror::Error)] +pub enum ErrInvalidSpec { + #[error("runtime spec has incompatible version. Only 1.X.Y is supported")] + UnsupportedVersion, + #[error("apparmor is specified but not enabled on this system")] + AppArmorNotEnabled, } diff --git a/crates/libcontainer/src/process/container_init_process.rs b/crates/libcontainer/src/process/container_init_process.rs index 23795ad7e..296875513 100644 --- a/crates/libcontainer/src/process/container_init_process.rs +++ b/crates/libcontainer/src/process/container_init_process.rs @@ -339,14 +339,8 @@ pub fn container_init_process( ) -> Result<()> { let syscall = args.syscall; let spec = args.spec; - let linux = spec - .linux() - .as_ref() - .ok_or(MissingSpecError::MissingLinux)?; - let proc = spec - .process() - .as_ref() - .ok_or(MissingSpecError::MissingProcess)?; + let linux = spec.linux().as_ref().ok_or(MissingSpecError::Linux)?; + let proc = spec.process().as_ref().ok_or(MissingSpecError::Process)?; let mut envs: Vec = proc.env().as_ref().unwrap_or(&vec![]).clone(); let rootfs_path = args.rootfs; let hooks = spec.hooks().as_ref(); @@ -693,7 +687,7 @@ pub fn container_init_process( unreachable!("should not be back here"); } else { tracing::error!("on non-Windows, at least one process arg entry is required"); - Err(MissingSpecError::MissingArgs) + Err(MissingSpecError::Args) }? } diff --git a/crates/libcontainer/src/process/container_intermediate_process.rs b/crates/libcontainer/src/process/container_intermediate_process.rs index b6379b764..0d8ac0dee 100644 --- a/crates/libcontainer/src/process/container_intermediate_process.rs +++ b/crates/libcontainer/src/process/container_intermediate_process.rs @@ -42,10 +42,7 @@ pub fn container_intermediate_process( let (init_sender, init_receiver) = init_chan; let command = &args.syscall; let spec = &args.spec; - let linux = spec - .linux() - .as_ref() - .ok_or(MissingSpecError::MissingLinux)?; + let linux = spec.linux().as_ref().ok_or(MissingSpecError::Linux)?; let namespaces = Namespaces::from(linux.namespaces().as_ref()); // this needs to be done before we create the init process, so that the init @@ -96,10 +93,7 @@ pub fn container_intermediate_process( } // set limits and namespaces to the process - let proc = spec - .process() - .as_ref() - .ok_or(MissingSpecError::MissingProcess)?; + let proc = spec.process().as_ref().ok_or(MissingSpecError::Process)?; if let Some(rlimits) = proc.rlimits() { for rlimit in rlimits { command.set_rlimit(rlimit)?; diff --git a/crates/libcontainer/src/rootfs/rootfs.rs b/crates/libcontainer/src/rootfs/rootfs.rs index 916c2eb35..c15aa7257 100644 --- a/crates/libcontainer/src/rootfs/rootfs.rs +++ b/crates/libcontainer/src/rootfs/rootfs.rs @@ -40,10 +40,7 @@ impl RootFS { ) -> Result<()> { tracing::debug!("Prepare rootfs: {:?}", rootfs); let mut flags = MsFlags::MS_REC; - let linux = spec - .linux() - .as_ref() - .ok_or(MissingSpecError::MissingLinux)?; + let linux = spec.linux().as_ref().ok_or(MissingSpecError::Linux)?; match linux.rootfs_propagation().as_deref() { Some("shared") => flags |= MsFlags::MS_SHARED, diff --git a/crates/libcontainer/src/rootless.rs b/crates/libcontainer/src/rootless.rs index 334bea0d8..74c2654bd 100644 --- a/crates/libcontainer/src/rootless.rs +++ b/crates/libcontainer/src/rootless.rs @@ -139,10 +139,7 @@ pub struct Rootless<'a> { impl<'a> Rootless<'a> { pub fn new(spec: &'a Spec) -> Result>> { - let linux = spec - .linux() - .as_ref() - .ok_or(MissingSpecError::MissingLinux)?; + let linux = spec.linux().as_ref().ok_or(MissingSpecError::Linux)?; let namespaces = Namespaces::from(linux.namespaces().as_ref()); let user_namespace = namespaces.get(LinuxNamespaceType::User); @@ -252,10 +249,7 @@ pub fn unprivileged_user_ns_enabled() -> Result { /// running in rootless mode fn validate_spec_for_rootless(spec: &Spec) -> std::result::Result<(), ValidateSpecError> { tracing::debug!(?spec, "validating spec for rootless container"); - let linux = spec - .linux() - .as_ref() - .ok_or(MissingSpecError::MissingLinux)?; + let linux = spec.linux().as_ref().ok_or(MissingSpecError::Linux)?; let namespaces = Namespaces::from(linux.namespaces().as_ref()); if namespaces.get(LinuxNamespaceType::User).is_none() { return Err(ValidateSpecError::NoUserNamespace); diff --git a/crates/youki/src/commands/kill.rs b/crates/youki/src/commands/kill.rs index e87a6266c..1d737ac57 100644 --- a/crates/youki/src/commands/kill.rs +++ b/crates/youki/src/commands/kill.rs @@ -1,7 +1,7 @@ //! Contains functionality of kill container command use std::{convert::TryInto, path::PathBuf}; -use anyhow::Result; +use anyhow::{anyhow, Result}; use crate::commands::load_container; use libcontainer::{container::ContainerStatus, signal::Signal}; @@ -15,9 +15,9 @@ pub fn kill(args: Kill, root_path: PathBuf) -> Result<()> { Err(e) => { // see https://github.com/containers/youki/issues/1314 if container.status() == ContainerStatus::Stopped { - return Err(e.context("container not running")); + return Err(anyhow!(e).context("container not running")); } - Err(e) + Err(anyhow!(e).context("failed to kill container")) } } } diff --git a/crates/youki/src/commands/mod.rs b/crates/youki/src/commands/mod.rs index 4e606fb4a..7cd3669ac 100644 --- a/crates/youki/src/commands/mod.rs +++ b/crates/youki/src/commands/mod.rs @@ -59,9 +59,7 @@ fn create_cgroup_manager>( ) -> Result { let container = load_container(root_path, container_id)?; let cgroups_path = container.spec()?.cgroup_path; - let systemd_cgroup = container - .systemd() - .context("could not determine cgroup manager")?; + let systemd_cgroup = container.systemd(); Ok(libcgroups::common::create_cgroup_manager( cgroups_path,