Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bump libcontainer to tip of tree #290

Merged
merged 4 commits into from
Aug 30, 2023
Merged

bump libcontainer to tip of tree #290

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
28c93dd
bump libcontainer to tip of tree
yihuaf Aug 29, 2023
109141d
fix can_handle function to return Can't Handle
yihuaf Aug 29, 2023
193205b
Add a TODO to use released version of `youki` later.
yihuaf Aug 29, 2023
7cb992f
move `can_handle` from method to function
yihuaf Aug 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 17 additions & 20 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ thiserror = "1.0"
libc = "0.2.147"
oci-spec = { version = "0.6.1", features = ["runtime"] }
sha256 = "1.4.0"
libcontainer = { version = "0.1", default-features = false }
# TODO: once lincontainer releases 0.2, switch to released version. The current commit is the tip of the tree from `youki` and a release candidate.
libcontainer = { git = "https://github.com/containers/youki", rev = "09e67372a892f22a89eeef62ff429c3cbcac6d41", default-features = false }
yihuaf marked this conversation as resolved.
Show resolved Hide resolved
windows-sys = { version = "0.48" }

[profile.release]
Expand Down
22 changes: 15 additions & 7 deletions crates/containerd-shim-wasm/src/libcontainer_instance/container_executor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ use std::io::Read;
use std::path::PathBuf;

use libcontainer::workload::default::DefaultExecutor;
use libcontainer::workload::{Executor, ExecutorError};
use libcontainer::workload::{Executor, ExecutorError, ExecutorValidationError};
use oci_spec::runtime::Spec;

use crate::sandbox::{oci, Stdio};

#[derive(Default)]
#[derive(Clone)]
pub struct LinuxContainerExecutor {
stdio: Stdio,
default_executor: DefaultExecutor,
Expand All @@ -18,7 +18,7 @@ impl LinuxContainerExecutor {
pub fn new(stdio: Stdio) -> Self {
Self {
stdio,
..Default::default()
default_executor: DefaultExecutor {},
}
}
}
Expand All @@ -32,7 +32,19 @@ impl Executor for LinuxContainerExecutor {
self.default_executor.exec(spec)
}

fn validate(&self, spec: &Spec) -> Result<(), ExecutorValidationError> {
self.default_executor.validate(spec)?;

self.can_handle(spec)
.then_some(())
.ok_or(ExecutorValidationError::InvalidArg)
}
}

impl LinuxContainerExecutor {
fn can_handle(&self, spec: &Spec) -> bool {
// TODO: some of these logic is now implemented inside the default
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should address this TODO before merging.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok to completely remove this function now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some ELF magic number checking that is specific to runwasi. I decided to leave the function in for now and we can address the duplication in future PR.

// executor. Should we de-dup?
let args = oci::get_args(spec);

if args.is_empty() {
Expand Down Expand Up @@ -106,8 +118,4 @@ impl Executor for LinuxContainerExecutor {
}
}
}

fn name(&self) -> &'static str {
self.default_executor.name()
}
}
93 changes: 58 additions & 35 deletions crates/containerd-shim-wasmedge/src/executor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
use std::path::PathBuf;

use anyhow::Result;
use containerd_shim_wasm::libcontainer_instance::LinuxContainerExecutor;
use containerd_shim_wasm::sandbox::{oci, Stdio};
use libcontainer::workload::{Executor, ExecutorError};
use libcontainer::workload::{Executor, ExecutorError, ExecutorValidationError};
use log::debug;
use oci_spec::runtime::Spec;
use wasmedge_sdk::config::{CommonConfigOptions, ConfigBuilder, HostRegistrationConfigOptions};
use wasmedge_sdk::{params, VmBuilder};

const EXECUTOR_NAME: &str = "wasmedge";

#[derive(Clone)]
pub struct WasmEdgeExecutor {
stdio: Stdio,
}
Expand All @@ -22,45 +24,45 @@ impl WasmEdgeExecutor {

impl Executor for WasmEdgeExecutor {
fn exec(&self, spec: &Spec) -> Result<(), ExecutorError> {
// parse wasi parameters
let args = oci::get_args(spec);
if args.is_empty() {
return Err(ExecutorError::InvalidArg);
}
match self.can_handle(spec) {
Ok(()) => {
// parse wasi parameters
let args = oci::get_args(spec);
if args.is_empty() {
return Err(ExecutorError::InvalidArg);
}

let vm = self
.prepare(args, spec)
.map_err(|err| ExecutorError::Other(format!("failed to prepare function: {}", err)))?;

// TODO: How to get exit code?
// This was relatively straight forward in go, but wasi and wasmtime are totally separate things in rust
let (module_name, method) = oci::get_module(spec);
debug!("running {:?} with method {}", module_name, method);
match vm.run_func(Some("main"), method, params!()) {
Ok(_) => std::process::exit(0),
Err(_) => std::process::exit(137),
};
}
let vm = self.prepare(args, spec).map_err(|err| {
ExecutorError::Other(format!("failed to prepare function: {}", err))
})?;

fn can_handle(&self, spec: &Spec) -> bool {
// check if the entrypoint of the spec is a wasm binary.
let (module_name, _method) = oci::get_module(spec);
let module_name = match module_name {
Some(m) => m,
None => {
log::info!("WasmEdge cannot handle this workload, no arguments provided");
return false;
// TODO: How to get exit code?
// This was relatively straight forward in go, but wasi and wasmtime are totally separate things in rust
let (module_name, method) = oci::get_module(spec);
debug!("running {:?} with method {}", module_name, method);
match vm.run_func(Some("main"), method, params!()) {
Ok(_) => std::process::exit(0),
Err(_) => std::process::exit(137),
};
}
};
let path = PathBuf::from(module_name);

path.extension()
.map(|ext| ext.to_ascii_lowercase())
.is_some_and(|ext| ext == "wasm" || ext == "wat")
Err(ExecutorValidationError::CantHandle(_)) => {
LinuxContainerExecutor::new(self.stdio.clone()).exec(spec)?;
Ok(())
}
Err(_) => Err(ExecutorError::InvalidArg),
}
}

fn name(&self) -> &'static str {
EXECUTOR_NAME
fn validate(&self, spec: &Spec) -> std::result::Result<(), ExecutorValidationError> {
match self.can_handle(spec) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'm not a huge fan of doing this check twice, here and on exec. But it works.
I'll try to think if we can avoid it or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of the downside/trade off of this interface. In both valid and exec, we have to run the can_handle logic to figure out the correct exec. Also due to rust safety features, in exec we have to run a lot of the similar checks to return InvalidArg error. For example, if there is no process block in the spec, we have to check it again inside exec even though we already checked in the valid. It doesn't make sense for us to panic.

Ok(()) => Ok(()),
Err(ExecutorValidationError::CantHandle(_)) => {
LinuxContainerExecutor::new(self.stdio.clone()).validate(spec)?;

Ok(())
}
Err(err) => Err(err),
}
}
}

Expand Down Expand Up @@ -98,6 +100,27 @@ impl WasmEdgeExecutor {

Ok(vm)
}

fn can_handle(&self, spec: &Spec) -> Result<(), ExecutorValidationError> {
yihuaf marked this conversation as resolved.
Show resolved Hide resolved
// check if the entrypoint of the spec is a wasm binary.
let (module_name, _method) = oci::get_module(spec);
let module_name = match module_name {
Some(m) => m,
None => {
log::info!("WasmEdge cannot handle this workload, no arguments provided");
return Err(ExecutorValidationError::CantHandle(EXECUTOR_NAME));
}
};
let path = PathBuf::from(module_name);

path.extension()
.map(|ext| ext.to_ascii_lowercase())
.is_some_and(|ext| ext == "wasm" || ext == "wat")
.then_some(())
.ok_or(ExecutorValidationError::CantHandle(EXECUTOR_NAME))?;

Ok(())
}
}

fn env_to_wasi(spec: &Spec) -> Vec<String> {
Expand Down
13 changes: 4 additions & 9 deletions crates/containerd-shim-wasmedge/src/instance/instance_linux.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ use std::fs;
use std::path::PathBuf;
use std::sync::{Arc, Condvar, Mutex};

use containerd_shim_wasm::libcontainer_instance::{LibcontainerInstance, LinuxContainerExecutor};
use containerd_shim_wasm::libcontainer_instance::LibcontainerInstance;
use containerd_shim_wasm::sandbox::error::Error;
use containerd_shim_wasm::sandbox::instance::ExitCode;
use containerd_shim_wasm::sandbox::instance_utils::determine_rootdir;
use containerd_shim_wasm::sandbox::{InstanceConfig, Stdio};
use libcontainer::container::builder::ContainerBuilder;
use libcontainer::container::Container;
use libcontainer::syscall::syscall::create_syscall;
use libcontainer::syscall::syscall::SyscallType;

use crate::executor::WasmEdgeExecutor;

Expand Down Expand Up @@ -63,14 +63,9 @@ impl LibcontainerInstance for Wasi {
fn build_container(&self) -> std::result::Result<Container, Error> {
fs::create_dir_all(&self.rootdir)?;

let syscall = create_syscall();
let err_others = |err| Error::Others(format!("failed to create container: {}", err));
let default_executor = Box::new(LinuxContainerExecutor::new(self.stdio.clone()));
let wasmedge_executor = Box::new(WasmEdgeExecutor::new(self.stdio.clone()));

let container = ContainerBuilder::new(self.id.clone(), syscall.as_ref())
.with_executor(vec![default_executor, wasmedge_executor])
.map_err(err_others)?
let container = ContainerBuilder::new(self.id.clone(), SyscallType::Linux)
.with_executor(WasmEdgeExecutor::new(self.stdio.clone()))
.with_root_path(self.rootdir.clone())
.map_err(err_others)?
.as_init(&self.bundle)
Expand Down
Loading