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

Handle args for modules with out extra first argument in prep for oci articfacts #146

Closed
wants to merge 4 commits into from
Closed

Handle args for modules with out extra first argument in prep for oci articfacts #146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f2c6b97
Handle args to wasmtime in prep for oci articfacts
jsturtevant Jun 15, 2023
cfc813d
Support args for wasmedge
jsturtevant Jun 15, 2023
ab7b940
Fix for args
jsturtevant Jun 15, 2023
071776b
Add some logging for edge
jsturtevant Jun 15, 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
5 changes: 4 additions & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,4 +109,7 @@ jobs:
make test/k3s
- name: cleanup
if: always()
run: make test/k3s/clean
run: |
sudo bin/k3s kubectl logs deployments/wasi-demo
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that this will cause a long timeout if the cluster is in a broken state? I'm not opposing this but would like the cleanup to be quick and safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call, I can wrap it in a timeout? I was having trouble getting the logs on failure and this was the only way I could get it to work.

sudo bin/k3s kubectl get pods -o wide
make test/k3s/clean
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ test/out/img.tar
!crates/wasmtime/src/bin/
test/k8s/_out
release/
.vscode/settings.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just ignore the full .vscode directory while at it? Or is it possible to have some project-level settings there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

open to either... settings.json changes often for me as I switch the rust analyzer settings around as I go.

I do have a couple debug configurations that might be worth sharing if folks are interested in them

1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ test/k3s: target/wasm32-wasi/$(TARGET)/img.tar bin/wasmedge bin/k3s
sudo bin/k3s kubectl apply -f test/k8s/deploy.yaml
sudo bin/k3s kubectl wait deployment wasi-demo --for condition=Available=True --timeout=90s && \
sudo bin/k3s kubectl get pods -o wide
sudo bin/k3s kubectl logs deployments/wasi-demo

.PHONY: test/k3s/clean
test/k3s/clean: bin/wasmedge/clean bin/k3s/clean
Expand Down
248 changes: 248 additions & 0 deletions crates/containerd-shim-wasm/src/sandbox/oci.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,53 @@ pub fn get_args(spec: &Spec) -> &[String] {
}
}

pub fn get_module_args(spec: &Spec) -> &[String] {
let args = get_args(spec);

match spec.annotations().clone() {
Some(annotations)
if annotations.contains_key("application/vnd.w3c.wasm.module.v1+wasm") =>
{
args
}
_ => {
if args.len() > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, is this a workaround for the Wasmedge issue? Should we wait for them to resolve it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this logic didn't change. This is the standard processing when the wasm module is shipped in the container layers. The first argument will always be the module. The rest of the args are the args to the module.

&args[1..]
} else {
&[]
}
}
}
}

pub fn get_module(spec: &Spec) -> (Option<String>, String) {
let args = get_args(spec);

match spec.annotations().clone() {
Some(annotations)
if annotations.contains_key("application/vnd.w3c.wasm.module.v1+wasm") =>
{
(None, "_start".to_string())
}
_ => {
if !args.is_empty() {
let start = args[0].clone();
let mut iterator = start.split('#');
let mut cmd = iterator.next().unwrap().to_string();

let stripped = cmd.strip_prefix(std::path::MAIN_SEPARATOR);
if let Some(strpd) = stripped {
cmd = strpd.to_string();
}
let method = iterator.next().unwrap_or("_start");
(Some(cmd), method.to_string())
} else {
(None, "_start".to_string())
}
}
}
}

pub fn spec_from_file<P: AsRef<Path>>(path: P) -> Result<Spec> {
let file = File::open(path)?;
let cfg: Spec = json::from_reader(file)?;
Expand Down Expand Up @@ -160,3 +207,204 @@ pub fn setup_prestart_hooks(hooks: &Option<oci_spec::runtime::Hooks>) -> Result<
}
Ok(())
}

#[cfg(test)]
mod oci_tests {
use super::*;
use oci_spec::runtime::{ProcessBuilder, RootBuilder, SpecBuilder};

#[test]
fn test_get_args() -> Result<()> {
let spec = SpecBuilder::default()
.root(RootBuilder::default().path("rootfs").build()?)
.process(
ProcessBuilder::default()
.cwd("/")
.args(vec!["hello.wat".to_string()])
.build()?,
)
.build()?;

let args = get_args(&spec);
assert_eq!(args.len(), 1);
assert_eq!(args[0], "hello.wat");

Ok(())
}

#[test]
fn test_get_args_return_empty() -> Result<()> {
let spec = SpecBuilder::default()
.root(RootBuilder::default().path("rootfs").build()?)
.process(ProcessBuilder::default().cwd("/").args(vec![]).build()?)
.build()?;

let args = get_args(&spec);
assert_eq!(args.len(), 0);

Ok(())
}

#[test]
fn test_get_args_returns_all() -> Result<()> {
let spec = SpecBuilder::default()
.root(RootBuilder::default().path("rootfs").build()?)
.process(
ProcessBuilder::default()
.cwd("/")
.args(vec![
"hello.wat".to_string(),
"echo".to_string(),
"hello".to_string(),
])
.build()?,
)
.build()?;

let args = get_args(&spec);
assert_eq!(args.len(), 3);
assert_eq!(args[0], "hello.wat");
assert_eq!(args[1], "echo");
assert_eq!(args[2], "hello");

Ok(())
}

#[test]
fn test_module_args_returns_module_args_only() -> Result<()> {
let spec = SpecBuilder::default()
.root(RootBuilder::default().path("rootfs").build()?)
.process(
ProcessBuilder::default()
.cwd("/")
.args(vec![
"hello.wat".to_string(),
"echo".to_string(),
"hello".to_string(),
])
.build()?,
)
.build()?;

let args = get_module_args(&spec);
assert_eq!(args.len(), 2);
assert_eq!(args[0], "echo");
assert_eq!(args[1], "hello");

Ok(())
}

#[test]
fn test_module_args_returns_empty_when_just_module() -> Result<()> {
let spec = SpecBuilder::default()
.root(RootBuilder::default().path("rootfs").build()?)
.process(
ProcessBuilder::default()
.cwd("/")
.args(vec!["hello.wat".to_string()])
.build()?,
)
.build()?;

let args = get_module_args(&spec);
assert_eq!(args.len(), 0);

Ok(())
}

#[test]
fn test_module_args_returns_module_args_only_when_oci_artifact() -> Result<()> {
let spec = SpecBuilder::default()
.root(RootBuilder::default().path("rootfs").build()?)
.process(
ProcessBuilder::default()
.cwd("/")
.args(vec!["echo".to_string(), "hello".to_string()])
.build()?,
)
.annotations(HashMap::from([(
"application/vnd.w3c.wasm.module.v1+wasm".to_string(),
"sha256:1234".to_string(),
)]))
.build()?;

let args = get_module_args(&spec);
assert_eq!(args.len(), 2);
assert_eq!(args[0], "echo");
assert_eq!(args[1], "hello");

Ok(())
}

#[test]
fn test_module_args_returns_empty_args_when_oci_artifact() -> Result<()> {
let spec = SpecBuilder::default()
.root(RootBuilder::default().path("rootfs").build()?)
.process(ProcessBuilder::default().cwd("/").args(vec![]).build()?)
.annotations(HashMap::from([(
"application/vnd.w3c.wasm.module.v1+wasm".to_string(),
"sha256:1234".to_string(),
)]))
.build()?;

let args = get_module_args(&spec);
assert_eq!(args.len(), 0);

Ok(())
}

#[test]
fn test_get_module_returns_empty_args_when_oci_artifact() -> Result<()> {
let spec = SpecBuilder::default()
.root(RootBuilder::default().path("rootfs").build()?)
.process(
ProcessBuilder::default()
.cwd("/")
.args(vec!["echo".to_string(), "hello".to_string()])
.build()?,
)
.annotations(HashMap::from([(
"application/vnd.w3c.wasm.module.v1+wasm".to_string(),
"sha256:1234".to_string(),
)]))
.build()?;

let (module, method) = get_module(&spec);
assert_eq!(module, None);
assert_eq!(method, "_start");

Ok(())
}

#[test]
fn test_get_module_returns_module() -> Result<()> {
let spec = SpecBuilder::default()
.root(RootBuilder::default().path("rootfs").build()?)
.process(
ProcessBuilder::default()
.cwd("/")
.args(vec!["hello.wat".to_string()])
.build()?,
)
.build()?;

let (module, method) = get_module(&spec);
assert_eq!(module.unwrap(), "hello.wat");
assert_eq!(method, "_start");

Ok(())
}

#[test]
fn test_get_module_returns_none_when_not_present() -> Result<()> {
let spec = SpecBuilder::default()
.root(RootBuilder::default().path("rootfs").build()?)
.process(ProcessBuilder::default().cwd("/").args(vec![]).build()?)
.build()?;

let (module, _) = get_module(&spec);
assert_eq!(module, None);

Ok(())
}
}
54 changes: 26 additions & 28 deletions crates/containerd-shim-wasmedge/src/executor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
use anyhow::Result;
use containerd_shim_wasm::sandbox::oci;
use nix::unistd::{dup, dup2};
use oci_spec::runtime::Spec;

use libc::{STDERR_FILENO, STDIN_FILENO, STDOUT_FILENO};
use libcontainer::workload::{Executor, ExecutorError};
use log::debug;
use std::os::unix::io::RawFd;

use wasmedge_sdk::{
config::{CommonConfigOptions, ConfigBuilder, HostRegistrationConfigOptions},
params, VmBuilder,
Expand All @@ -21,16 +22,6 @@ pub struct WasmEdgeExecutor {

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

let mut cmd = args[0].clone();
if let Some(stripped) = args[0].strip_prefix(std::path::MAIN_SEPARATOR) {
cmd = stripped.to_string();
}
let envs = env_to_wasi(spec);

// create configuration with `wasi` option enabled
Expand All @@ -51,14 +42,34 @@ impl Executor for WasmEdgeExecutor {
.ok_or_else(|| anyhow::Error::msg("Not found wasi module"))
.map_err(|err| ExecutorError::Execution(err.into()))?;

let args = oci::get_module_args(spec);
let mut module_args = None;
if !args.is_empty() {
module_args = Some(args.iter().map(|s| s as &str).collect())
}

debug!("module args: {:?}", module_args);
Copy link
Contributor

Choose a reason for hiding this comment

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

At some point we need to go through the codebase and look at the logging, because I'm worried that someone might pass secrets as environment variables or module arguments and we'll accidentally log them. It's not a problem yet, just something to keep in mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, we might want to take a more structure approach to logging as well.

I can drop some of these debug statements. It was helpful when figuring this out but your concern is valid.

wasi_module.initialize(
Some(args.iter().map(|s| s as &str).collect()),
module_args,
Some(envs.iter().map(|s| s as &str).collect()),
None,
);

let (module_name, method) = oci::get_module(spec);
let module_name = match module_name {
Some(m) => m,
None => {
return Err(ExecutorError::Execution(
anyhow::Error::msg(
"no module provided, cannot load module from file within container",
)
.into(),
))
}
};

let vm = vm
.register_module_from_file("main", cmd)
.register_module_from_file("main", module_name.clone())
.map_err(|err| ExecutorError::Execution(err))?;

if let Some(stdin) = self.stdin {
Expand All @@ -74,9 +85,8 @@ impl Executor for WasmEdgeExecutor {
let _ = dup2(stderr, STDERR_FILENO);
}

// TODO: How to get exit code?
// This was relatively straight forward in go, but wasi and wasmtime are totally separate things in rust
match vm.run_func(Some("main"), "_start", params!()) {
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),
};
Expand All @@ -91,18 +101,6 @@ impl Executor for WasmEdgeExecutor {
}
}

fn get_args(spec: &Spec) -> &[String] {
let p = match spec.process() {
None => return &[],
Some(p) => p,
};

match p.args() {
None => &[],
Some(args) => args.as_slice(),
}
}

fn env_to_wasi(spec: &Spec) -> Vec<String> {
let default = vec![];
let env = spec
Expand Down
Loading