Merge pull request #763 from jprendes/cleanup

[chore] minor cleanup

crates/containerd-shim-wasm/CHANGELOG.md

@@ -7,6 +7,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 ### Added
 - Added test for signal handling issue [#755](https://github.com/containerd/runwasi/issues/755) ([#756](https://github.com/containerd/runwasi/pull/756))
 
+### Changed
+- Reuse and synchronise access to `Container` object instead of reloading form disk ([#763](https://github.com/containerd/runwasi/pull/763))
+
+### Removed
+- Removed `containerd_shim_wasm::sandbox::instance_utils::get_instance_root` and `containerd_shim_wasm::sandbox::instance_utils::instance_exists` functions ([#763](https://github.com/containerd/runwasi/pull/763))
+
 ## [v0.8.0] — 2024-12-04
 
 ### Added

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

@@ -3,35 +3,10 @@ use std::fs::File;
 use std::io::ErrorKind;
 use std::path::{Path, PathBuf};
 
-use anyhow::{bail, Context, Result};
 use serde::{Deserialize, Serialize};
 
 use super::Error;
 
-/// Return the root path for the instance.
-///
-/// The root path is the path to the directory containing the container's state.
-#[cfg_attr(feature = "tracing", tracing::instrument(parent = tracing::Span::current(), skip_all, level = "Info"))]
-pub fn get_instance_root<P: AsRef<Path>>(
-    root_path: P,
-    instance_id: &str,
-) -> Result<PathBuf, anyhow::Error> {
-    let instance_root = construct_instance_root(root_path, instance_id)?;
-    if !instance_root.exists() {
-        bail!("container {} does not exist.", instance_id)
-    }
-    Ok(instance_root)
-}
-
-/// Checks if the container exists.
-///
-/// The root path is the path to the directory containing the container's state.
-#[cfg_attr(feature = "tracing", tracing::instrument(parent = tracing::Span::current(), skip_all, level = "Info"))]
-pub fn instance_exists<P: AsRef<Path>>(root_path: P, container_id: &str) -> Result<bool> {
-    let instance_root = construct_instance_root(root_path, container_id)?;
-    Ok(instance_root.exists())
-}
-
 #[derive(Serialize, Deserialize)]
 struct Options {
     root: Option<PathBuf>,
@@ -56,18 +31,6 @@ pub fn determine_rootdir(
     Ok(path)
 }
 
-#[cfg_attr(feature = "tracing", tracing::instrument(parent = tracing::Span::current(), skip_all, level = "Info"))]
-fn construct_instance_root<P: AsRef<Path>>(root_path: P, container_id: &str) -> Result<PathBuf> {
-    let root_path = root_path.as_ref().canonicalize().with_context(|| {
-        format!(
-            "failed to canonicalize {} for container {}",
-            root_path.as_ref().display(),
-            container_id
-        )
-    })?;
-    Ok(root_path.join(container_id))
-}
-
 #[cfg(unix)]
 #[cfg(test)]
 mod tests {

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

@@ -1,4 +1,4 @@
-use std::sync::{Arc, OnceLock, RwLock};
+use std::sync::{OnceLock, RwLock};
 use std::time::Duration;
 
 use chrono::{DateTime, Utc};
@@ -10,7 +10,7 @@ pub(super) struct InstanceData<T: Instance> {
     pub instance: T,
     cfg: InstanceConfig<T::Engine>,
     pid: OnceLock<u32>,
-    state: Arc<RwLock<TaskState>>,
+    state: RwLock<TaskState>,
 }
 
 impl<T: Instance> InstanceData<T> {
@@ -22,7 +22,7 @@ impl<T: Instance> InstanceData<T> {
             instance,
             cfg,
             pid: OnceLock::default(),
-            state: Arc::new(RwLock::new(TaskState::Created)),
+            state: RwLock::new(TaskState::Created),
         })
     }
 

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

@@ -1,5 +1,6 @@
 use std::marker::PhantomData;
-use std::path::{Path, PathBuf};
+use std::path::Path;
+use std::sync::Mutex;
 use std::thread;
 use std::time::Duration;
 
@@ -16,7 +17,7 @@ use oci_spec::image::Platform;
 
 use crate::container::Engine;
 use crate::sandbox::async_utils::AmbientRuntime as _;
-use crate::sandbox::instance_utils::{determine_rootdir, get_instance_root, instance_exists};
+use crate::sandbox::instance_utils::determine_rootdir;
 use crate::sandbox::sync::WaitableCell;
 use crate::sandbox::{
     containerd, Error as SandboxError, Instance as SandboxInstance, InstanceConfig, Stdio,
@@ -27,7 +28,7 @@ static DEFAULT_CONTAINER_ROOT_DIR: &str = "/run/containerd";
 
 pub struct Instance<E: Engine> {
     exit_code: WaitableCell<(u32, DateTime<Utc>)>,
-    rootdir: PathBuf,
+    container: Mutex<Container>,
     id: String,
     _phantom: PhantomData<E>,
 }
@@ -54,7 +55,7 @@ impl<E: Engine> SandboxInstance for Instance<E> {
                 (vec![], Platform::default())
             });
 
-        ContainerBuilder::new(id.clone(), SyscallType::Linux)
+        let container = ContainerBuilder::new(id.clone(), SyscallType::Linux)
             .with_executor(Executor::new(engine, stdio, modules, platform))
             .with_root_path(rootdir.clone())?
             .as_init(&bundle)
@@ -64,7 +65,7 @@ impl<E: Engine> SandboxInstance for Instance<E> {
         Ok(Self {
             id,
             exit_code: WaitableCell::new(),
-            rootdir,
+            container: Mutex::new(container),
             _phantom: Default::default(),
         })
     }
@@ -78,8 +79,7 @@ impl<E: Engine> SandboxInstance for Instance<E> {
         // make sure we have an exit code by the time we finish (even if there's a panic)
         let guard = self.exit_code.set_guard_with(|| (137, Utc::now()));
 
-        let container_root = get_instance_root(&self.rootdir, &self.id)?;
-        let mut container = Container::load(container_root)?;
+        let mut container = self.container.lock().expect("Poisoned mutex");
         let pid = container.pid().context("failed to get pid")?.as_raw();
 
         container.start()?;
@@ -115,11 +115,11 @@ impl<E: Engine> SandboxInstance for Instance<E> {
         let signal = Signal::try_from(signal as i32).map_err(|err| {
             SandboxError::InvalidArgument(format!("invalid signal number: {}", err))
         })?;
-        let container_root = get_instance_root(&self.rootdir, &self.id)?;
-        let mut container = Container::load(container_root)
-            .with_context(|| format!("could not load state for container {}", self.id))?;
 
-        container.kill(signal, true)?;
+        self.container
+            .lock()
+            .expect("Poisoned mutex")
+            .kill(signal, true)?;
 
         Ok(())
     }
@@ -129,23 +129,10 @@ impl<E: Engine> SandboxInstance for Instance<E> {
     #[cfg_attr(feature = "tracing", tracing::instrument(parent = tracing::Span::current(), skip_all, level = "Info"))]
     fn delete(&self) -> Result<(), SandboxError> {
         log::info!("deleting instance: {}", self.id);
-        match instance_exists(&self.rootdir, &self.id) {
-            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)?;
-        match Container::load(container_root) {
-            Ok(mut container) => {
-                container.delete(true)?;
-            }
-            Err(err) => {
-                log::error!("could not find the container, skipping cleanup: {}", err);
-            }
-        }
+        self.container
+            .lock()
+            .expect("Poisoned mutex")
+            .delete(true)?;
         Ok(())
     }