feat(wamr): add wasm-micro-runtime shim implementation #716

Mossaka · 2024-11-03T22:55:36Z

Follow up on the work done by #508 and #642. The runtime execution "thread signal env initialized failed" error was resolved by the upstream Wamr SDK and thus I used the latest sdk to re-build the wamr shim.

Now it actually worked!

sudo ctr run  --net-host --rm --runtime=io.containerd.wamr.v1 ghcr.io/containerd/runwasi/wasi-demo-app:latest testwasm /wasi-demo-app.wasm echo "hi"
hi
exiting

TODO

pass all the tests, see 5030afa

…error upstream to the official wamr SDK solves this issue Signed-off-by: jiaxiao zhou <[email protected]>

I believe there are more work to be done to parse the runtimeError to appropriate exit code. failures: wamr_tests::test_custom_entrypoint wamr_tests::test_exit_code wamr_tests::test_hello_world_oci wamr_tests::test_seccomp wamr_tests::test_unreachable Signed-off-by: jiaxiao zhou <[email protected]>

…lows Signed-off-by: jiaxiao zhou <[email protected]>

Signed-off-by: jiaxiao zhou <[email protected]>

Mossaka · 2024-11-03T22:58:00Z

crates/containerd-shim-wamr/src/instance.rs

+            .or_else(|err| {
+                log::error!("Error: {:?}", err);
+                Err(err)
+            })


we should parse the RuntimeError to appropriate exit code. Any thoughts on this one, @lum1n0us?

Regarding that, I just noticed that WAMR only uses error strings to convey information if something goes wrong during the execution of a Wasm function. So, if it's okay, can we continue to display the error string in the log and use a simple -1 and 0 to indicate failure and success, respectively?

I was looking for exit code that can pass this test: https://github.com/containerd/runwasi/blob/main/crates/containerd-shim-wasmer/src/tests.rs#L79-L88

For example, how can I get exit code 42 from running this wasm module?

(module ;; Import the required proc_exit WASI function which terminates the program with an exit code. ;; The function signature for proc_exit is: ;; (exit_code: i32) -> ! (import "wasi_snapshot_preview1" "proc_exit" (func $proc_exit (param i32))) (memory 1) (export "memory" (memory 0)) (func $main (export "_start") (call $proc_exit (i32.const 42)) unreachable ) )

We have a fix for this situation. Please have a look.

Signed-off-by: jiaxiao zhou <[email protected]>

Mossaka · 2024-11-05T21:39:18Z

@lum1n0us do you know why the entrypoint_test failed?

thread 'wamr_tests::test_custom_entrypoint' panicked at crates/containerd-shim-wamr/src/tests.rs:58:5:
assertion `left == right` failed
  left: "hello world\n[00:08:01:896 - 7FA1CA910640]: warning: a module with WASI apis should be either a command or a reactor\n"
 right: "hello world\n"

Signed-off-by: jiaxiao zhou <[email protected]>

Mossaka · 2024-11-05T21:49:38Z

Also, it doesn't seem like it runs on Windows: https://github.com/containerd/runwasi/actions/runs/11692971119/job/32563482903?pr=716#step:8:167

Signed-off-by: jiaxiao zhou <[email protected]>

lum1n0us · 2024-11-05T23:21:18Z

Yes, that's because WAMR enforces a strict application ABI check. Every wasm32-wasi .wasm should be classified as either a command or a reactor.

For your case,

either patch the case as a command, for example. Add a function like (func $start (export "_start"))
or WAMR turns off the WARNING messages by setting a higher log level.

Signed-off-by: jiaxiao zhou <[email protected]>

lum1n0us · 2024-11-13T01:44:07Z

So, which option?

Mossaka · 2024-11-13T04:36:17Z

I disabled that test for now and I don't think we need to pass it to get this PR merged. Let me fix the linting issue.

if the target is windows, do not import the APIs changed the compiler_error to panic! Signed-off-by: jiaxiao zhou <[email protected]>

Mossaka · 2024-11-13T15:40:00Z

@jprendes @andreiltd can you give this PR a review please 🙏

jprendes

LGTM, left a few comments and suggestions

crates/containerd-shim-wamr/Cargo.toml

jprendes · 2024-11-13T15:56:35Z

crates/containerd-shim-wamr/src/instance.rs

+unsafe impl Send for WamrEngine {}
+unsafe impl Sync for WamrEngine {}


question: Could you add a comment as to why this is safe to do? I guess Runtime is not Send/Sync, should Runtime be made Send/Sync upstream?

I believe the Runtime should be made Send / Sync upstream

@lum1n0us is this the case? Does Wamr Runtime contain any thread-local storage? Does it have any shared internal states?

jprendes · 2024-11-13T15:58:52Z

crates/containerd-shim-wamr/src/instance.rs

+impl Clone for WamrEngine {
+    fn clone(&self) -> Self {
+        let runtime = Runtime::new().unwrap();
+        Self { runtime }
+    }
+}


question: Is this correct? doesn't Runtime hold any state? I don't remember why we require the engine to be Clone :-\

We want the Engine to be Clone because we want cheap copies of the Runtime for each container in the pod. This is the case for Wasmtime, WasmEdge and Wasmer. I believe it should be the case for Wamr as well.

But we don't clone the engine for each container, we clone the process...
In any case, if we create a new runtime with each clone, we might as well use a marker struct for the Engine and create a new runtime inside the run_wasi method.

yeah I agree with you that we clone the entire process so there is no point to clone the engine.

In any case, if we create a new runtime with each clone,

at least for wasmtime, Engine::clone() is a cheap operation: https://docs.wasmtime.dev/api/wasmtime/struct.Engine.html#engines-and-clone

crates/containerd-shim-wamr/src/instance.rs

crates/containerd-shim-wamr/src/tests.rs

Signed-off-by: Jiaxiao Zhou <[email protected]>

Co-authored-by: Jorge Prendes <[email protected]> Signed-off-by: Jiaxiao Zhou <[email protected]>

Signed-off-by: jiaxiao zhou <[email protected]>

…ance to wamrInst Signed-off-by: jiaxiao zhou <[email protected]>

Maciej and others added 8 commits November 3, 2024 22:01

crates/containerd-shim-wamr: add WAMR shim implementation

8f93996

containerd-shim-wamr: use my sdk

e2fb956

containerd-shim-wamr: minor cleanup

7532700

containerd-shim-wamr: minor cleanup

11dffb0

containerd-shim-wamr: fixed the thread signal env initialized failed …

d5fe22e

…error upstream to the official wamr SDK solves this issue Signed-off-by: jiaxiao zhou <[email protected]>

.github/workflows/{ci, release}: Add wamr to the CI and release workf…

49455b9

…lows Signed-off-by: jiaxiao zhou <[email protected]>

CONTRIBUTING, README: update the contributing and readme for WAMR shim

2ca5ea7

Signed-off-by: jiaxiao zhou <[email protected]>

Mossaka mentioned this pull request Nov 3, 2024

feat(wamr): add wasm-mico-runtime shim implementation #508

Closed

Mossaka commented Nov 3, 2024

View reviewed changes

containerd-shim-wamr/Cargo: remove unused deps

bc9f649

Signed-off-by: jiaxiao zhou <[email protected]>

containerd-shim-wamr/tests: disable failed tests

9e964e7

Signed-off-by: jiaxiao zhou <[email protected]>

Mossaka added 3 commits November 5, 2024 23:06

containerd-shim-wamr: rustfmt

99f5a39

Signed-off-by: jiaxiao zhou <[email protected]>

.github/workflows: remove wamr shim from windows build

dc777e5

Signed-off-by: jiaxiao zhou <[email protected]>

containerd-shim-wamr: clippy

77db97b

Signed-off-by: jiaxiao zhou <[email protected]>

containerd-shim-wamr: add conditional compilation for unix platform

3046d3e

Signed-off-by: jiaxiao zhou <[email protected]>

containerd-shim-wamr: fix linting issue

d1d062c

if the target is windows, do not import the APIs changed the compiler_error to panic! Signed-off-by: jiaxiao zhou <[email protected]>

jprendes changed the title ~~feat(wamr): add wasm-mico-runtime shim implementation~~ feat(wamr): add wasm-micro-runtime shim implementation Nov 13, 2024

jprendes previously approved these changes Nov 13, 2024

View reviewed changes

Merge branch 'main' into wamr-2

89eac6e

Signed-off-by: Jiaxiao Zhou <[email protected]>

Mossaka dismissed jprendes’s stale review via 89eac6e November 14, 2024 05:14

Mossaka and others added 3 commits November 13, 2024 22:15

Apply suggestions from code review

a47e85a

Co-authored-by: Jorge Prendes <[email protected]> Signed-off-by: Jiaxiao Zhou <[email protected]>

containerd-shim-wamr: apply rustfmt

0b1d720

Signed-off-by: jiaxiao zhou <[email protected]>

containerd-shim-wamr/src/instance: fixed a typo

dd1d892

Signed-off-by: jiaxiao zhou <[email protected]>

containerd-shim-wasm/src/sandbox/shim/cli: rename duplicated wamrInst…

dea9609

…ance to wamrInst Signed-off-by: jiaxiao zhou <[email protected]>

Mossaka requested a review from jprendes November 16, 2024 02:41

Mossaka merged commit 01ff775 into containerd:main Nov 16, 2024
69 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(wamr): add wasm-micro-runtime shim implementation #716

feat(wamr): add wasm-micro-runtime shim implementation #716

Mossaka commented Nov 3, 2024 •

edited

Loading

Mossaka Nov 3, 2024

lum1n0us Nov 4, 2024

Mossaka Nov 4, 2024 •

edited

Loading

Mossaka Nov 5, 2024

lum1n0us Nov 5, 2024

Mossaka Nov 5, 2024

lum1n0us Nov 5, 2024

Mossaka commented Nov 5, 2024

Mossaka commented Nov 5, 2024

lum1n0us commented Nov 5, 2024

lum1n0us commented Nov 13, 2024

Mossaka commented Nov 13, 2024

Mossaka commented Nov 13, 2024

jprendes left a comment

jprendes Nov 13, 2024

Mossaka Nov 14, 2024

Mossaka Nov 14, 2024

jprendes Nov 13, 2024

Mossaka Nov 14, 2024

jprendes Nov 14, 2024

Mossaka Nov 16, 2024

		unsafe impl Send for WamrEngine {}
		unsafe impl Sync for WamrEngine {}

feat(wamr): add wasm-micro-runtime shim implementation #716

feat(wamr): add wasm-micro-runtime shim implementation #716

Conversation

Mossaka commented Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mossaka Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mossaka commented Nov 5, 2024

Mossaka commented Nov 5, 2024

lum1n0us commented Nov 5, 2024

lum1n0us commented Nov 13, 2024

Mossaka commented Nov 13, 2024

Mossaka commented Nov 13, 2024

jprendes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mossaka commented Nov 3, 2024 •

edited

Loading

Mossaka Nov 4, 2024 •

edited

Loading