build libcontainer's container on Instance::new to be spec compliant

Signed-off-by: Jorge Prendes <[email protected]>

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 })
     }
 }

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::Engine>>) -> Self;
+    fn new(id: String, cfg: Option<&InstanceConfig<Self::Engine>>) -> Result<Self, Error>
+    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::Engine>>) -> Self {
-        Nop {
+    fn new(_id: String, _cfg: Option<&InstanceConfig<Self::Engine>>) -> Result<Self, Error> {
+        Ok(Nop {
             exit_code: Arc::new((Mutex::new(None), Condvar::new())),
-        }
+        })
     }
     fn start(&self) -> Result<u32, Error> {
         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(())
     }
 }

crates/containerd-shim-wasm/src/sandbox/shim.rs

@@ -746,10 +746,10 @@ impl<T: Instance + Send + Sync> Local<T> {
         );
         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::<Created> {
                     s: std::marker::PhantomData,
@@ -954,11 +954,11 @@ impl<T: Instance + Send + Sync> Local<T> {
         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::<Created> {
                         s: std::marker::PhantomData,
@@ -1097,9 +1097,7 @@ impl<T: Instance + Send + Sync> Local<T> {
             ..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<T: Instance + Send + Sync> Local<T> {
             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<T: Instance + Send + Sync> Local<T> {
 
         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;

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,52 +20,50 @@ use crate::sys::container::executor::Executor;
 static DEFAULT_CONTAINER_ROOT_DIR: &str = "/run/containerd";
 
 pub struct Instance<E: Engine> {
-    engine: E,
     exit_code: ExitCode,
-    stdio: Stdio,
-    bundle: PathBuf,
     rootdir: PathBuf,
     id: String,
+    _phantom: PhantomData<E>,
 }
 
 impl<E: Engine> SandboxInstance for Instance<E> {
     type Engine = E;
 
-    fn new(id: String, cfg: Option<&InstanceConfig<Self::Engine>>) -> Self {
+    fn new(id: String, cfg: Option<&InstanceConfig<Self::Engine>>) -> Result<Self, SandboxError> {
         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
     /// The returned value should be a unique ID (such as a PID) for the 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<u32, SandboxError> {
         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<E: Engine> SandboxInstance for Instance<E> {
     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(())

crates/containerd-shim-wasm/src/sys/windows/container/instance.rs

@@ -9,7 +9,7 @@ pub struct Instance<E: Engine>(PhantomData<E>);
 impl<E: Engine> SandboxInstance for Instance<E> {
     type Engine = E;
 
-    fn new(_id: String, _cfg: Option<&InstanceConfig<Self::Engine>>) -> Self {
+    fn new(_id: String, _cfg: Option<&InstanceConfig<Self::Engine>>) -> Result<Self, SandboxError> {
         todo!();
     }