Refactor the Executor interface to allow trait object as executors (#…

…2230)

* Refactored the Executor interface yet again

I apologize having to refactor this interface yet again. Previously, we
introduced the executor to be a function pointer. This works out nicely
because the function pointer in rust can be clone-ed without hassel.
However, I realized that using function pointer is way to restrictive
for our users. The executor may wish to include additional context when
calling the exec function. The function pointer limited the input only
`oci spec`.

Signed-off-by: yihuaf <[email protected]>

* fix spell check

Signed-off-by: yihuaf <[email protected]>

* Fix doc test

Signed-off-by: yihuaf <[email protected]>

---------

Signed-off-by: yihuaf <[email protected]>

crates/libcontainer/src/container/builder.rs

@@ -22,7 +22,7 @@ pub struct ContainerBuilder {
     pub(super) preserve_fds: i32,
     /// The function that actually runs on the container init process. Default
     /// is to execute the specified command in the oci spec.
-    pub(super) executor: Executor,
+    pub(super) executor: Box<dyn Executor>,
 }
 
 /// Builder that can be used to configure the common properties of
@@ -244,16 +244,16 @@ impl ContainerBuilder {
     /// ```no_run
     /// # use libcontainer::container::builder::ContainerBuilder;
     /// # use libcontainer::syscall::syscall::SyscallType;
-    /// # use libcontainer::workload::default::get_executor;
+    /// # use libcontainer::workload::default::DefaultExecutor;
     ///
     /// ContainerBuilder::new(
     ///     "74f1a4cb3801".to_owned(),
     ///     SyscallType::default(),
     /// )
-    /// .with_executor(get_executor());
+    /// .with_executor(DefaultExecutor{});
     /// ```
-    pub fn with_executor(mut self, executor: Executor) -> Self {
-        self.executor = executor;
+    pub fn with_executor(mut self, executor: impl Executor + 'static) -> Self {
+        self.executor = Box::new(executor);
         self
     }
 }

crates/libcontainer/src/container/builder_impl.rs

@@ -47,7 +47,7 @@ pub(super) struct ContainerBuilderImpl {
     /// If the container is to be run in detached mode
     pub detached: bool,
     /// Default executes the specified execution of a generic command
-    pub executor: Executor,
+    pub executor: Box<dyn Executor>,
 }
 
 impl ContainerBuilderImpl {

crates/libcontainer/src/process/args.rs

@@ -40,5 +40,5 @@ pub struct ContainerArgs {
     /// If the container is to be run in detached mode
     pub detached: bool,
     /// Manage the functions that actually run on the container
-    pub executor: Executor,
+    pub executor: Box<dyn Executor>,
 }

crates/libcontainer/src/process/container_init_process.rs

@@ -678,16 +678,20 @@ pub fn container_init_process(
         }
     }
 
-    if proc.args().is_some() {
-        (args.executor)(spec).map_err(|err| {
-            tracing::error!(?err, "failed to execute payload");
-            err
-        })?;
-        unreachable!("should not be back here");
-    } else {
+    if proc.args().is_none() {
         tracing::error!("on non-Windows, at least one process arg entry is required");
-        Err(MissingSpecError::Args)
-    }?
+        Err(MissingSpecError::Args)?;
+    }
+
+    args.executor.exec(spec).map_err(|err| {
+        tracing::error!(?err, "failed to execute payload");
+        err
+    })?;
+
+    // Once the executor is executed without error, it should not return. For
+    // example, the default executor is expected to call `exec` and replace the
+    // current process.
+    unreachable!("the executor should not return if it is successful.");
 }
 
 // Before 3.19 it was possible for an unprivileged user to enter an user namespace,

crates/libcontainer/src/workload/default.rs

@@ -5,10 +5,11 @@ use oci_spec::runtime::Spec;
 
 use super::{Executor, ExecutorError, EMPTY};
 
-/// Return the default executor. The default executor will execute the command
-/// specified in the oci spec.
-pub fn get_executor() -> Executor {
-    Box::new(|spec: &Spec| -> Result<(), ExecutorError> {
+#[derive(Clone)]
+pub struct DefaultExecutor {}
+
+impl Executor for DefaultExecutor {
+    fn exec(&self, spec: &Spec) -> Result<(), ExecutorError> {
         tracing::debug!("executing workload with default handler");
         let args = spec
             .process()
@@ -38,5 +39,9 @@ pub fn get_executor() -> Executor {
         // After execvp is called, the process is replaced with the container
         // payload through execvp, so it should never reach here.
         unreachable!();
-    })
+    }
+}
+
+pub fn get_executor() -> Box<dyn Executor> {
+    Box::new(DefaultExecutor {})
 }

crates/libcontainer/src/workload/mod.rs

@@ -16,4 +16,49 @@ pub enum ExecutorError {
     CantHandle(&'static str),
 }
 
-pub type Executor = Box<fn(&Spec) -> Result<(), ExecutorError>>;
+// Here is an explanation about the complexity below regarding to
+// CloneBoxExecutor and Executor traits. This is one of the places rust actually
+// makes our life harder. The usecase for the executor is to allow users of
+// `libcontainer` to pass in a closure like function where the actual execution
+// of the container workload can be defined by user. To maximize the flexibility
+// for the users, we use trait object to allow users to pass in a closure like
+// objects, so the function can container any number of variables through the
+// structure. This is similar to the Fn family of traits that rust std lib has.
+// However, our usecase has a little bit more complexity than the Fn family of
+// traits. We require the struct implementing this Executor traits to be
+// cloneable, so we can pass the struct across fork/clone process boundary with
+// memory safety. We can't make the Executor trait to require Clone trait
+// because doing so will make the Executor trait not object safe. Part of the
+// reason is that without the CloneBoxExecutor trait, the default clone
+// implementation for Box<dyn trait> will first unwrap the box. However, the
+// `dyn trait` inside the box doesn't have a size, which violates the object
+// safety requirement for a trait. To work around this, we implement our own
+// CloneBoxExecutor trait, which is object safe.
+//
+// Note to future maintainers: if you find a better way to do this or Rust
+// introduced some new magical feature to simplify this logic, please consider
+// to refactor this part.
+
+pub trait CloneBoxExecutor {
+    fn clone_box(&self) -> Box<dyn Executor>;
+}
+
+pub trait Executor: CloneBoxExecutor {
+    /// Executes the workload
+    fn exec(&self, spec: &Spec) -> Result<(), ExecutorError>;
+}
+
+impl<T> CloneBoxExecutor for T
+where
+    T: 'static + Executor + Clone,
+{
+    fn clone_box(&self) -> Box<dyn Executor> {
+        Box::new(self.clone())
+    }
+}
+
+impl Clone for Box<dyn Executor> {
+    fn clone(&self) -> Self {
+        self.clone_box()
+    }
+}

crates/youki/src/workload/executor.rs

@@ -1,29 +1,36 @@
 use libcontainer::oci_spec::runtime::Spec;
 use libcontainer::workload::{Executor, ExecutorError};
 
-pub fn default_executor() -> Executor {
-    Box::new(|spec: &Spec| -> Result<(), ExecutorError> {
+#[derive(Clone)]
+pub struct DefaultExecutor {}
+
+impl Executor for DefaultExecutor {
+    fn exec(&self, spec: &Spec) -> Result<(), ExecutorError> {
         #[cfg(feature = "wasm-wasmer")]
-        match super::wasmer::get_executor()(spec) {
+        match super::wasmer::get_executor().exec(spec) {
             Ok(_) => return Ok(()),
             Err(ExecutorError::CantHandle(_)) => (),
             Err(err) => return Err(err),
         }
         #[cfg(feature = "wasm-wasmedge")]
-        match super::wasmedge::get_executor()(spec) {
+        match super::wasmedge::get_executor().exec(spec) {
             Ok(_) => return Ok(()),
             Err(ExecutorError::CantHandle(_)) => (),
             Err(err) => return Err(err),
         }
         #[cfg(feature = "wasm-wasmtime")]
-        match super::wasmtime::get_executor()(spec) {
+        match super::wasmtime::get_executor().exec(spec) {
             Ok(_) => return Ok(()),
             Err(ExecutorError::CantHandle(_)) => (),
             Err(err) => return Err(err),
         }
 
         // Leave the default executor as the last option, which executes normal
         // container workloads.
-        libcontainer::workload::default::get_executor()(spec)
-    })
+        libcontainer::workload::default::get_executor().exec(spec)
+    }
+}
+
+pub fn default_executor() -> DefaultExecutor {
+    DefaultExecutor {}
 }

crates/youki/src/workload/wasmedge.rs

@@ -8,8 +8,11 @@ use libcontainer::workload::{Executor, ExecutorError};
 
 const EXECUTOR_NAME: &str = "wasmedge";
 
-pub fn get_executor() -> Executor {
-    Box::new(|spec: &Spec| -> Result<(), ExecutorError> {
+#[derive(Clone)]
+pub struct WasmedgeExecutor {}
+
+impl Executor for WasmedgeExecutor {
+    fn exec(&self, spec: &Spec) -> Result<(), ExecutorError> {
         if !can_handle(spec) {
             return Err(ExecutorError::CantHandle(EXECUTOR_NAME));
         }
@@ -58,7 +61,11 @@ pub fn get_executor() -> Executor {
             .map_err(|err| ExecutorError::Execution(err))?;
 
         Ok(())
-    })
+    }
+}
+
+pub fn get_executor() -> WasmedgeExecutor {
+    WasmedgeExecutor {}
 }
 
 fn can_handle(spec: &Spec) -> bool {

crates/youki/src/workload/wasmer.rs

@@ -6,8 +6,11 @@ use libcontainer::workload::{Executor, ExecutorError, EMPTY};
 
 const EXECUTOR_NAME: &str = "wasmer";
 
-pub fn get_executor() -> Executor {
-    Box::new(|spec: &Spec| -> Result<(), ExecutorError> {
+#[derive(Clone)]
+pub struct WasmerExecutor {}
+
+impl Executor for WasmerExecutor {
+    fn exec(&self, spec: &Spec) -> Result<(), ExecutorError> {
         if !can_handle(spec) {
             return Err(ExecutorError::CantHandle(EXECUTOR_NAME));
         }
@@ -76,7 +79,11 @@ pub fn get_executor() -> Executor {
         wasi_env.cleanup(&mut store, None);
 
         Ok(())
-    })
+    }
+}
+
+pub fn get_executor() -> WasmerExecutor {
+    WasmerExecutor {}
 }
 
 fn can_handle(spec: &Spec) -> bool {

crates/youki/src/workload/wasmtime.rs

@@ -6,8 +6,11 @@ use libcontainer::workload::{Executor, ExecutorError, EMPTY};
 
 const EXECUTOR_NAME: &str = "wasmtime";
 
-pub fn get_executor() -> Executor {
-    Box::new(|spec: &Spec| -> Result<(), ExecutorError> {
+#[derive(Clone)]
+pub struct WasmtimeExecutor {}
+
+impl Executor for WasmtimeExecutor {
+    fn exec(&self, spec: &Spec) -> Result<(), ExecutorError> {
         if !can_handle(spec) {
             return Err(ExecutorError::CantHandle(EXECUTOR_NAME));
         }
@@ -86,7 +89,11 @@ pub fn get_executor() -> Executor {
         start
             .call(&mut store, &[], &mut [])
             .map_err(|err| ExecutorError::Execution(err.into()))
-    })
+    }
+}
+
+pub fn get_executor() -> WasmtimeExecutor {
+    WasmtimeExecutor {}
 }
 
 fn can_handle(spec: &Spec) -> bool {