From 76c3bd8e6681aad19259a8f4d09cd45805e5fc19 Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Wed, 27 Sep 2023 16:27:39 +0100 Subject: [PATCH] build libcontainer's container on Instance::new to be spec compliant Signed-off-by: Jorge Prendes --- crates/containerd-shim-wasm-test/src/lib.rs | 2 +- .../src/sandbox/instance.rs | 23 ++++---- .../containerd-shim-wasm/src/sandbox/shim.rs | 20 ++----- .../src/sys/unix/container/instance.rs | 59 ++++++++----------- .../src/sys/windows/container/instance.rs | 2 +- 5 files changed, 45 insertions(+), 61 deletions(-) diff --git a/crates/containerd-shim-wasm-test/src/lib.rs b/crates/containerd-shim-wasm-test/src/lib.rs index 53b693a95..169924e05 100644 --- a/crates/containerd-shim-wasm-test/src/lib.rs +++ b/crates/containerd-shim-wasm-test/src/lib.rs @@ -137,7 +137,7 @@ where .set_stderr(dir.join("stderr").to_string_lossy().to_string()) .set_stdin(dir.join("stdin").to_string_lossy().to_string()); - let instance = WasiInstance::new("test".to_string(), Some(&cfg)); + let instance = WasiInstance::new("test".to_string(), Some(&cfg))?; Ok(WasiTest { instance, tempdir }) } } diff --git a/crates/containerd-shim-wasm/src/sandbox/instance.rs b/crates/containerd-shim-wasm/src/sandbox/instance.rs index d4d71ac5b..d702ceb6a 100644 --- a/crates/containerd-shim-wasm/src/sandbox/instance.rs +++ b/crates/containerd-shim-wasm/src/sandbox/instance.rs @@ -112,7 +112,9 @@ pub trait Instance { type Engine: Send + Sync + Clone; /// Create a new instance - fn new(id: String, cfg: Option<&InstanceConfig>) -> Self; + fn new(id: String, cfg: Option<&InstanceConfig>) -> Result + where + Self: Sized; /// Start the instance /// The returned value should be a unique ID (such as a PID) for the instance. @@ -178,10 +180,10 @@ pub struct Nop { impl Instance for Nop { type Engine = (); - fn new(_id: String, _cfg: Option<&InstanceConfig>) -> Self { - Nop { + fn new(_id: String, _cfg: Option<&InstanceConfig>) -> Result { + Ok(Nop { exit_code: Arc::new((Mutex::new(None), Condvar::new())), - } + }) } fn start(&self) -> Result { Ok(std::process::id()) @@ -220,7 +222,7 @@ mod noptests { #[test] fn test_nop_kill_sigkill() -> Result<(), Error> { - let nop = Nop::new("".to_string(), None); + let nop = Nop::new("".to_string(), None)?; let (tx, rx) = channel(); let waiter = Wait::new(tx); @@ -233,7 +235,7 @@ mod noptests { #[test] fn test_nop_kill_sigterm() -> Result<(), Error> { - let nop = Nop::new("".to_string(), None); + let nop = Nop::new("".to_string(), None)?; let (tx, rx) = channel(); let waiter = Wait::new(tx); @@ -246,7 +248,7 @@ mod noptests { #[test] fn test_nop_kill_sigint() -> Result<(), Error> { - let nop = Nop::new("".to_string(), None); + let nop = Nop::new("".to_string(), None)?; let (tx, rx) = channel(); let waiter = Wait::new(tx); @@ -258,8 +260,9 @@ mod noptests { } #[test] - fn test_nop_delete_after_create() { - let nop = Nop::new("".to_string(), None); - nop.delete().unwrap(); + fn test_nop_delete_after_create() -> Result<(), Error> { + let nop = Nop::new("".to_string(), None)?; + nop.delete()?; + Ok(()) } } diff --git a/crates/containerd-shim-wasm/src/sandbox/shim.rs b/crates/containerd-shim-wasm/src/sandbox/shim.rs index 77be5703c..9b5533a41 100644 --- a/crates/containerd-shim-wasm/src/sandbox/shim.rs +++ b/crates/containerd-shim-wasm/src/sandbox/shim.rs @@ -746,10 +746,10 @@ impl Local { ); InstanceData { instance: None, - base: Some(Nop::new(id, None)), + base: Some(Nop::new(id, None).unwrap()), cfg, pid: RwLock::new(None), - status: Arc::new((Mutex::new(None), Condvar::new())), + status: Arc::default(), state: Arc::new(RwLock::new(TaskStateWrapper::Created( TaskState:: { s: std::marker::PhantomData, @@ -954,11 +954,11 @@ impl Local { self.instances.write().unwrap().insert( req.id().to_string(), Arc::new(InstanceData { - instance: Some(T::new(req.id().to_string(), Some(&builder))), + instance: Some(T::new(req.id().to_string(), Some(&builder))?), base: None, cfg: builder, pid: RwLock::new(None), - status: Arc::new((Mutex::new(None), Condvar::new())), + status: Arc::default(), state: Arc::new(RwLock::new(TaskStateWrapper::Created( TaskState:: { s: std::marker::PhantomData, @@ -1097,9 +1097,7 @@ impl Local { ..Default::default() }; - let status = i.status.0.lock().unwrap(); - if status.is_some() { - let ec = status.unwrap(); + if let Some(ec) = *i.status.0.lock().unwrap() { event.exit_status = ec.0; resp.exit_status = ec.0; @@ -1111,7 +1109,6 @@ impl Local { event.set_exited_at(timestamp.clone()); resp.set_exited_at(timestamp); } - drop(status); self.instances.write().unwrap().remove(req.id()); @@ -1175,12 +1172,7 @@ impl Local { state.set_pid(pid.unwrap()); - let status = i.status.0.lock().unwrap(); - - let code = *status; - drop(status); - - if let Some(c) = code { + if let Some(c) = *i.status.0.lock().unwrap() { state.set_status(Status::STOPPED); let ec = c; state.exit_status = ec.0; diff --git a/crates/containerd-shim-wasm/src/sys/unix/container/instance.rs b/crates/containerd-shim-wasm/src/sys/unix/container/instance.rs index c6b1d51a0..e3f298faa 100644 --- a/crates/containerd-shim-wasm/src/sys/unix/container/instance.rs +++ b/crates/containerd-shim-wasm/src/sys/unix/container/instance.rs @@ -1,3 +1,4 @@ +use std::marker::PhantomData; use std::path::{Path, PathBuf}; use std::thread; @@ -19,33 +20,37 @@ use crate::sys::container::executor::Executor; static DEFAULT_CONTAINER_ROOT_DIR: &str = "/run/containerd"; pub struct Instance { - engine: E, exit_code: ExitCode, - stdio: Stdio, - bundle: PathBuf, rootdir: PathBuf, id: String, + _phantom: PhantomData, } impl SandboxInstance for Instance { type Engine = E; - fn new(id: String, cfg: Option<&InstanceConfig>) -> Self { + fn new(id: String, cfg: Option<&InstanceConfig>) -> Result { let cfg = cfg.unwrap(); let engine = cfg.get_engine(); - let bundle = cfg.get_bundle().unwrap_or_default().into(); + let bundle = cfg.get_bundle().unwrap_or_default(); let namespace = cfg.get_namespace(); let rootdir = Path::new(DEFAULT_CONTAINER_ROOT_DIR).join(E::name()); let rootdir = determine_rootdir(&bundle, &namespace, rootdir).unwrap(); let stdio = Stdio::init_from_cfg(cfg).expect("failed to open stdio"); - Self { + + ContainerBuilder::new(id.clone(), SyscallType::Linux) + .with_executor(Executor::new(engine, stdio)) + .with_root_path(rootdir.clone())? + .as_init(&bundle) + .with_systemd(false) + .build()?; + + Ok(Self { id, exit_code: ExitCode::default(), - engine, - stdio, - bundle, rootdir, - } + _phantom: Default::default(), + }) } /// Start the instance @@ -53,18 +58,12 @@ impl SandboxInstance for Instance { /// Nothing internally should be using this ID, but it is returned to containerd where a user may want to use it. fn start(&self) -> Result { log::info!("starting instance: {}", self.id); - let mut container = ContainerBuilder::new(self.id.clone(), SyscallType::Linux) - .with_executor(Executor::new(self.engine.clone(), self.stdio.take())) - .with_root_path(self.rootdir.clone())? - .as_init(&self.bundle) - .with_systemd(false) - .build()?; + let container_root = get_instance_root(&self.rootdir, &self.id)?; + let mut container = Container::load(container_root)?; let pid = container.pid().context("failed to get pid")?; - container.start().map_err(|err| { - SandboxError::Any(anyhow::anyhow!("failed to start container: {}", err)) - })?; + container.start()?; let exit_code = self.exit_code.clone(); thread::spawn(move || { @@ -109,30 +108,20 @@ impl SandboxInstance for Instance { fn delete(&self) -> Result<(), SandboxError> { log::info!("deleting instance: {}", self.id); match instance_exists(&self.rootdir, &self.id) { - Ok(exists) => { - if !exists { - return Ok(()); - } - } + Ok(true) => {} + Ok(false) => return Ok(()), Err(err) => { log::error!("could not find the container, skipping cleanup: {}", err); return Ok(()); } } let container_root = get_instance_root(&self.rootdir, &self.id)?; - let container = Container::load(container_root) - .with_context(|| format!("could not load state for container {}", self.id)); - match container { - Ok(mut container) => container.delete(true).map_err(|err| { - SandboxError::Any(anyhow::anyhow!( - "failed to delete container {}: {}", - &self.id, - err - )) - })?, + match Container::load(container_root) { + Ok(mut container) => { + let _ = container.delete(true); + } Err(err) => { log::error!("could not find the container, skipping cleanup: {}", err); - return Ok(()); } } Ok(()) diff --git a/crates/containerd-shim-wasm/src/sys/windows/container/instance.rs b/crates/containerd-shim-wasm/src/sys/windows/container/instance.rs index 0cd561e4e..340b91aee 100644 --- a/crates/containerd-shim-wasm/src/sys/windows/container/instance.rs +++ b/crates/containerd-shim-wasm/src/sys/windows/container/instance.rs @@ -9,7 +9,7 @@ pub struct Instance(PhantomData); impl SandboxInstance for Instance { type Engine = E; - fn new(_id: String, _cfg: Option<&InstanceConfig>) -> Self { + fn new(_id: String, _cfg: Option<&InstanceConfig>) -> Result { todo!(); }