refactor: adding `LibcontainerInstance` for default implementation of `Instance` trait #226

Mossaka · 2023-08-03T01:33:56Z

This commits adds a new LibcontainerInstance trait in the containerd-shim-wasm crate. It refactors the common youki Instance implementation into this trait and as a consequence, all the Instance implementation from both the wasmtime and wasmedge shims moved to a common place.

This requires the shim author to implement the newly introduced LibcontainerInstance and then uses its default methods to implement Instance. The reason doing that is there are no changes to the existing Instance trait. The downside is that any new shim instance needs to implement both LibcontainerInstance and the Instance traits.

This closes #131

…nce trait This commits adds a new YoukiInstance trait in the containerd-shim-wasm crate. It refactors the common youki Instance implementation into this trait and as a consequence, all the Instance implementation from both the wasmtime and wasmedge shims moved to a common place. This closes containerd#131 Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>

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

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>

jsturtevant · 2023-08-04T21:32:03Z

I like the way this shares all the work and makes it easier for the implementors and think this is a great direction eventually.

The concern I have is I've gotten back to work on #49 and found that since our move to youki this PR could cause an issue since youki doesn't support Windows (and it appears would need some major work to get it going). If a shim such as wasmtime implements YoukiInstance then we won't be able to add the windows support at all since we take a hard dependecy on libcontainer , where as right now the youki work can be hidden behind some compile time flags.

Mossaka · 2023-08-04T21:42:07Z

@jsturtevant I can make libcontainer dependency completely optional in containerd-shim-wasm. At least this will unblock you from the Windows support work, but you still need to implement non-youki version for the wasmtime shim that does not dep on libcontainer.

jsturtevant · 2023-08-04T23:26:52Z

but you still need to implement non-youki version for the wasmtime shim that does not dep on libcontainer.

was hoping to share more but with the direction towards youki, in the short term this is probably the best option.

jsturtevant · 2023-08-04T23:29:07Z

I can make libcontainer dependency completely optional in containerd-shim-wasm.

👍

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>

jprendes

This looks great, thanks for working on it!

Some minor comments. The comments in wasmedge also apply to wasmtime.

In the PR description you mention

The downside is that any new shim instance needs to implement both YoukiInstance and the Instance traits.

But there's a blanket implementation of Instance for YoukiInstance, so there's no need to implement both traits, or am I missing something?

crates/containerd-shim-wasm/Cargo.toml

crates/containerd-shim-wasmedge/Cargo.toml

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

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

crates/containerd-shim-wasm/Cargo.toml

Makefile

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

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>

…im-wasm Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>

jprendes

I really like how this is looking, Thanks @Mossaka !

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

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

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

jprendes · 2023-08-10T09:22:06Z

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

+    type E: Send + Sync + Clone;
+
+    /// Create a new instance
+    fn new_youki(id: String, cfg: Option<&InstanceConfig<Self::E>>) -> Self;


suggestion / preference: rename this to plain new:

In L.65 do

<Self as YoukiInstance>::new(...)

In the implementations, instead of doing

use containerd_shim_wasm::sandbox:YoukiInstance; impl YoukiInstance for Wasi { ... }

do

use containerd_shim_wasm::sandbox; impl sandbox::YoukiInstance for Wasi { ... }

If we change new_youki to new, then there is a naming conflict which is pretty annoying.

Rust will complain about the usage of Wasi::new() by saying that new is referring both to LibcontainerInstance::new and Instance::new and requires users to specify which one to refer to.

That's why you would need

<Self as LibcontainerInstance>::new(...)

But I'm not sure that's better than the method being called new_libcontainer.
I guess it's just odd that the method new is part of a trait, and not part of the struct's impl.

I'm happy to keep this as new_libcontainer.

devigned

Looks great! I've added a couple comments.

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

devigned · 2023-08-10T13:47:17Z

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

+            .map_err(err_others)?;
+        // Close the fds now that they have been passed to the container process
+        // so that we don't leak them.
+        stdin.map(close);


Are there cases here where these may not get closed? For example, if one of the map_err is hit after the initial with_executor call, will there be an opportunity for the function to exit without calling close? Or, do we only care if build is called with an Ok result?

I think we only care about build calling with an Ok result, but you remind me to close file descriptors on the wasmtime shim.

I woiuld be nice if maybe_open_stdio returned an RAII wrapper.
A simple approach would be to return the File instead of the RawFd, and using as_raw_fd() when needed instead of using into_raw_fd() early on.

I woiuld be nice if maybe_open_stdio returned an RAII wrapper. A simple approach would be to return the File instead of the RawFd, and using as_raw_fd() when needed instead of using into_raw_fd() early on.

#240 is similar to what you reference here, thought I am not sure what RAII is

Sorry, that's my inner C++ betraying me.
It's basically when you have a struct implementing Drop to cleanup after itself.
When you drop a File, it automatically closes the file for you.
It is kind of what #240 does, but instead of using into_raw_fd the executors would hold a Option<File> instead of an Option<RawFd>, and the conversion to RawFd would be done at le last possible moment.

I tried this but ran into some lifetime issues with the File and opted for a smaller change but if you have any suggestions be happy to fix that up 👍

I was thinking something like this jsturtevant/runwasi@use-cross-platfrom-file...jprendes:runwasi:use-cross-platfrom-file

crates/containerd-shim-wasmtime/Cargo.toml

jsturtevant · 2023-08-10T15:51:23Z

Cargo.toml

 oci-spec = { version = "0.6.1", features = ["runtime"] }
 sha256 = "1.3.0"
-libcontainer = "0.1"
+libcontainer = { version = "0.1", default-features = false }


is default-features=false needed?

Actually not, let me remove it.

Actually it is needed because otherwise you will get this error

warning: /home/mossaka/developer/runwasi/crates/containerd-shim-wasm/Cargo.toml: `default-features` is ignored for libcontainer, since `default-features` was not specified for `workspace.dependencies.libcontainer`, this could become a hard error in the future

jsturtevant

looking really good! I think once the last few open comments are addressed this is in good shape

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>

Mossaka · 2023-08-10T23:02:20Z

But there's a blanket implementation of Instance for YoukiInstance, so there's no need to implement both traits, or am I missing something?

Ah that was a mistaken. I used a different implementation so that shim author only needs to implement LibcontainerInstance trait now.

jsturtevant

LGTM

jsturtevant · 2023-08-11T16:29:04Z

not sure what our current set up is in regards to CI and commits, but maybe squash the commits?

jprendes

LGTM!

utam0k · 2023-08-14T12:16:41Z

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

+                    }
+                }
+            } as u32;
+            let mut ec = lock.lock().unwrap();


Suggested change

let mut ec = lock.lock().unwrap();

let mut ec = lock.lock().map_err(|err| Error::Any(anyhow::anyhow!("failed get a lockr: {}", err)))?;

The error isn't able to automatically transfer so I will leave it as is.

I think its good to let this panic anyway.
If we can't lock something is very wrong (dirty mutex state) and this should crash the program.

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

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>

Mossaka · 2023-08-14T18:11:25Z

Going to merge this in. Thanks everyone for reviewing it!! Really appreciate it.

cpuguy83

I haven't given this a thorough review yet, but I like where this is going at lot.

cpuguy83 · 2023-08-14T18:15:10Z

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

+    fn get_root_dir(&self) -> Result<PathBuf, Error>;
+
+    /// Build the container
+    fn build_container(&self) -> Result<Container, Error>;


build_container seems like a fairly heavy trait function.

I think if we add something like open_stdio to the trait we can implement common build_container?

IIUC, that would move the stdio redirection work from the executor to the instance.
Is there any instance / executor that would not want to do the stdio redirection trick (slight/spin/wws)?
Can that be handled by default in Instance instead of LibcontainerInstance?

cpuguy83 · 2023-08-14T18:17:20Z

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

+                    }
+                }
+            } as u32;
+            let mut ec = lock.lock().unwrap();


I think its good to let this panic anyway.
If we can't lock something is very wrong (dirty mutex state) and this should crash the program.

Mossaka requested review from cpuguy83 and utam0k August 3, 2023 01:34

Mossaka added 2 commits August 3, 2023 01:36

docs: add some comments on the YoukiInstance trait

355a267

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>

implement Instance for all instances of YoukiInstance

f415b62

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>

jsturtevant reviewed Aug 3, 2023

View reviewed changes

crates/containerd-shim-wasm/src/sandbox/instance.rs Outdated Show resolved Hide resolved

jsturtevant reviewed Aug 3, 2023

View reviewed changes

crates/containerd-shim-wasm/src/sandbox/instance.rs Outdated Show resolved Hide resolved

docs: add more documentation to the youkiinstance

bfb6819

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>

Mossaka requested review from jprendes and jsturtevant August 3, 2023 22:20

Mossaka mentioned this pull request Aug 4, 2023

Add LinuxContainerExecutor to Wasmtime and WasmEdge shims #156

Merged

Mossaka added 2 commits August 4, 2023 23:35

feat: make libcontainer optional in containerd-shim-wasm

157b888

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>

group imports with conditional compilation

21d8c67

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>

jprendes approved these changes Aug 9, 2023

View reviewed changes

jsturtevant reviewed Aug 9, 2023

View reviewed changes

crates/containerd-shim-wasm/Cargo.toml Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

crates/containerd-shim-wasm/src/sandbox/instance.rs Outdated Show resolved Hide resolved

Mossaka added 4 commits August 9, 2023 22:47

refactor: apply Jorge's comments

2c310e0

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>

move youkiinstance to its own module

401d1fd

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>

make libcontainer an opt in feature

0c944a4

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>

Merge remote-tracking branch 'upstream/main' into youki-containerd-sh…

644afd6

…im-wasm Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>

jprendes approved these changes Aug 10, 2023

View reviewed changes

devigned reviewed Aug 10, 2023

View reviewed changes

jsturtevant reviewed Aug 10, 2023

View reviewed changes

crates/containerd-shim-wasmtime/Cargo.toml Show resolved Hide resolved

jsturtevant reviewed Aug 10, 2023

View reviewed changes

jsturtevant mentioned this pull request Aug 10, 2023

Refactor use statements to enable building Windows #238

Merged

1 task

refactor: resolve more comments

013247b

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>

Mossaka changed the title ~~refactor: adding YoukiInstance for default implementation of Instance trait~~ refactor: adding LibcontainerInstance for default implementation of Instance trait Aug 11, 2023

Mossaka requested review from devigned, jprendes and jsturtevant August 11, 2023 00:25

jsturtevant approved these changes Aug 11, 2023

View reviewed changes

jprendes approved these changes Aug 14, 2023

View reviewed changes

utam0k reviewed Aug 14, 2023

View reviewed changes

crates/containerd-shim-wasm/src/sandbox/libcontainer_instance.rs Outdated Show resolved Hide resolved

fixed a typo in the docs

606f177

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>

Mossaka merged commit d04bf6b into containerd:main Aug 14, 2023

Mossaka deleted the youki-containerd-shim-wasm branch August 14, 2023 18:11

cpuguy83 requested changes Aug 14, 2023

View reviewed changes

This was referenced Aug 15, 2023

Make the utility maybe_open_stdio cross platform #240

Merged

Experiment with the Libcontainer trait for build_container to something like open_stdio #243

Closed

	let mut ec = lock.lock().unwrap();
	let mut ec = lock.lock().map_err(\|err\| Error::Any(anyhow::anyhow!("failed get a lockr: {}", err)))?;

refactor: adding LibcontainerInstance for default implementation of Instance trait #226

refactor: adding LibcontainerInstance for default implementation of Instance trait #226

Uh oh!

Conversation

Mossaka commented Aug 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsturtevant commented Aug 4, 2023

Uh oh!

Mossaka commented Aug 4, 2023

Uh oh!

jsturtevant commented Aug 4, 2023

Uh oh!

jsturtevant commented Aug 4, 2023

Uh oh!

jprendes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jprendes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mossaka Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devigned left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsturtevant left a comment

Choose a reason for hiding this comment

Uh oh!

Mossaka commented Aug 10, 2023

Uh oh!

jsturtevant left a comment

Choose a reason for hiding this comment

Uh oh!

jsturtevant commented Aug 11, 2023

Uh oh!

jprendes left a comment

refactor: adding `LibcontainerInstance` for default implementation of `Instance` trait #226

refactor: adding `LibcontainerInstance` for default implementation of `Instance` trait #226

Mossaka commented Aug 3, 2023 •

edited

Loading

Mossaka Aug 10, 2023 •

edited

Loading