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

Cargo.toml

@@ -1,7 +1,7 @@
 [workspace]
 members = [
     "crates/containerd-shim-wasm",
-    "crates/containerd-shim-wasm-test",
+    "crates/containerd-shim-wasm-test-modules",
     "crates/wasi-demo-app",
     "crates/oci-tar-builder",
     "crates/containerd-shim-wasmedge",
@@ -24,7 +24,7 @@ cap-std = "1.0"
 chrono = { version = "0.4", default-features = false, features = ["clock"] }
 containerd-shim = "0.5.0"
 containerd-shim-wasm = { path = "crates/containerd-shim-wasm" }
-containerd-shim-wasm-test = { path = "crates/containerd-shim-wasm-test" }
+containerd-shim-wasm-test-modules = { path = "crates/containerd-shim-wasm-test-modules" }
 crossbeam = { version = "0.8.2", default-features = false }
 env_logger = "0.10"
 libc = "0.2.147"
@@ -36,6 +36,7 @@ serde = "1.0"
 serde_json = "1.0"
 sha256 = "1.4.0"
 tar = "0.4"
+tempfile = "3.8"
 thiserror = "1.0"
 ttrpc = "0.8.0"
 wat = "*" # Use whatever version wasmtime will make us pull

Makefile

@@ -47,8 +47,8 @@ check: check-wasm $(RUNTIMES:%=check-%);
 
 check-common: check-wasm;
 check-wasm:
-	$(CARGO) +nightly fmt $(TARGET_FLAG) -p oci-tar-builder -p wasi-demo-app -p containerd-shim-wasm -p containerd-shim-wasm-test -- --check
-	$(CARGO) clippy $(TARGET_FLAG) $(FEATURES_wasm) -p oci-tar-builder -p wasi-demo-app -p containerd-shim-wasm -p containerd-shim-wasm-test -- $(WARNINGS)
+	$(CARGO) +nightly fmt $(TARGET_FLAG) -p oci-tar-builder -p wasi-demo-app -p containerd-shim-wasm -p containerd-shim-wasm-test-modules -- --check
+	$(CARGO) clippy $(TARGET_FLAG) $(FEATURES_wasm) -p oci-tar-builder -p wasi-demo-app -p containerd-shim-wasm -p containerd-shim-wasm-test-modules -- $(WARNINGS)
 
 check-%:
 	$(CARGO) +nightly fmt $(TARGET_FLAG) -p containerd-shim-$* -- --check
@@ -59,8 +59,8 @@ fix: fix-wasm $(RUNTIMES:%=fix-%);
 
 fix-common: fix-wasm;
 fix-wasm:
-	$(CARGO) +nightly fmt $(TARGET_FLAG) -p oci-tar-builder -p wasi-demo-app -p containerd-shim-wasm -p containerd-shim-wasm-test
-	$(CARGO) clippy $(TARGET_FLAG) $(FEATURES_wasm) --fix -p oci-tar-builder -p wasi-demo-app -p containerd-shim-wasm -p containerd-shim-wasm-test -- $(WARNINGS)
+	$(CARGO) +nightly fmt $(TARGET_FLAG) -p oci-tar-builder -p wasi-demo-app -p containerd-shim-wasm -p containerd-shim-wasm-test-modules
+	$(CARGO) clippy $(TARGET_FLAG) $(FEATURES_wasm) --fix -p oci-tar-builder -p wasi-demo-app -p containerd-shim-wasm -p containerd-shim-wasm-test-modules -- $(WARNINGS)
 
 fix-%:
 	$(CARGO) +nightly fmt $(TARGET_FLAG) -p containerd-shim-$*
@@ -72,7 +72,7 @@ test: test-wasm $(RUNTIMES:%=test-%);
 test-common: test-wasm;
 test-wasm:
 	# oci-tar-builder and wasi-demo-app have no tests
-	RUST_LOG=trace $(CARGO) test $(TARGET_FLAG) --package containerd-shim-wasm $(FEATURES_wasm) --verbose -- --nocapture
+	RUST_LOG=trace $(CARGO) test $(TARGET_FLAG) --package containerd-shim-wasm $(FEATURES_wasm) --verbose -- --nocapture --test-threads=1
 
 test-wasmedge:
 	# run tests in one thread to prevent paralellism

crates/containerd-shim-wasm-test/Cargo.toml → crates/containerd-shim-wasm-test-modules/Cargo.toml

@@ -1,13 +1,12 @@
 [package]
-name = "containerd-shim-wasm-test"
-description = "Library for testing containerd shims for wasm"
+name = "containerd-shim-wasm-test-modules"
+description = "Set of WebAssembly modules for testing containerd shims for wasm"
 version.workspace = true
 edition.workspace = true
 license.workspace = true
 
 [dependencies]
 anyhow = { workspace = true }
-containerd-shim-wasm = { workspace = true }
 env_logger = "0.10"
 libc = { workspace = true }
 log = { workspace = true }

crates/containerd-shim-wasm/Cargo.toml

@@ -14,15 +14,18 @@ doctest = false
 [dependencies]
 anyhow = { workspace = true }
 chrono = { workspace = true }
-git-version = "0.3.5"
 containerd-shim = { workspace = true }
+containerd-shim-wasm-test-modules = { workspace = true, optional = true }
 crossbeam = { workspace = true }
+env_logger = { workspace = true, optional = true }
+git-version = "0.3.5"
 libc = { workspace = true }
 log = { workspace = true }
 oci-spec = { workspace = true }
 protobuf = "3.2"
 serde = { workspace = true }
 serde_json = { workspace = true }
+tempfile = { workspace = true, optional = true }
 thiserror = { workspace = true }
 ttrpc = { workspace = true }
 wat = { workspace = true }
@@ -40,8 +43,11 @@ windows-sys = { workspace = true, features = ["Win32_Foundation", "Win32_Storage
 ttrpc-codegen = { version = "0.4.2", optional = true }
 
 [dev-dependencies]
-tempfile = "3.8"
+containerd-shim-wasm-test-modules = { workspace = true }
+env_logger = { workspace = true }
+tempfile = { workspace = true }
 
 [features]
+testing = ["dep:containerd-shim-wasm-test-modules", "dep:env_logger", "dep:tempfile"]
 generate_bindings = ["ttrpc-codegen"]
 generate_doc = []

crates/containerd-shim-wasm/src/container/mod.rs

@@ -21,3 +21,6 @@ pub use path::PathResolve;
 
 pub use crate::sandbox::stdio::Stdio;
 use crate::sys::container::instance;
+
+#[cfg(test)]
+mod tests;

crates/containerd-shim-wasm/src/container/tests.rs

@@ -0,0 +1,38 @@
+use anyhow::bail;
+
+use crate::container::{Engine, RuntimeContext, Stdio};
+use crate::sys::container::instance::Instance;
+use crate::testing::WasiTest;
+
+#[derive(Clone, Default)]
+struct EngineFailingValidation;
+
+impl Engine for EngineFailingValidation {
+    fn name() -> &'static str {
+        "wasi_instance"
+    }
+    fn can_handle(&self, _ctx: &impl RuntimeContext) -> anyhow::Result<()> {
+        bail!("can't handle");
+    }
+    fn run_wasi(&self, _ctx: &impl RuntimeContext, _stdio: Stdio) -> anyhow::Result<i32> {
+        Ok(0)
+    }
+}
+
+type InstanceFailingValidation = Instance<EngineFailingValidation>;
+
+#[test]
+#[cfg(unix)] // not yet implemented on Windows
+fn test_validation_error() -> anyhow::Result<()> {
+    // A validation error should fail when creating the container
+    // as opposed to failing when starting it.
+
+    let result = WasiTest::<InstanceFailingValidation>::builder()?
+        .with_start_fn("foo")?
+        .with_wasm("/invalid_entrypoint.wasm")?
+        .build();
+
+    assert!(result.is_err());
+
+    Ok(())
+}

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

@@ -10,3 +10,6 @@ pub mod services;
 #[cfg_attr(unix, path = "sys/unix/mod.rs")]
 #[cfg_attr(windows, path = "sys/windows/mod.rs")]
 pub(crate) mod sys;
+
+#[cfg(any(test, feature = "testing"))]
+pub mod testing;

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;