Support for cgroups by cpuguy83 · Pull Request #21 · containerd/runwasi

cpuguy83 · 2022-10-19T00:53:53Z

This is a series of patches to add support for running wasm code in a cgroup.
While some cgroup controllers can support running different threads in different cgroups, the main one we are interested in, the cgroup memory controller, this doesn't make sense since threads all share memory.
Because of this we'll fork off a new process (which should have CoW version of the main shim's memory) to run the wasm code.

Of course all the issues of fork apply here since this is fairly unsafe from a multithreaded program (which the shim is always multi-threaded), so care must be taken to not try do things like take a lock in the new process because this can cause a deadlock.
There may be some interesting things to test out with the wasmtime engine just to make sure we aren't going to deadlock if there's multiple things happening when the fork occurs.

cpuguy83 · 2022-10-27T23:30:21Z

Recommend reviewing this with whitespace changes disabled.

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

danbugs

~haven't finished reviewing the src/instance.rs, crates/containerd-shim-wasm/src/sandbox/exec.rs, and crates/containerd-shim-wasm/src/sandbox/oci.rs files just, but it's getting late, so figured I'd just post what I have so far. That is:

two suggestions (regarding dependencies versioning), and
a possible mistake in a CI action!

I'll probably review the rest during the weekend!

Cargo.toml

crates/containerd-shim-wasm/Cargo.toml

.github/workflows/ci.yml

danbugs

some improvements here and there~

P.S. After applying these, I suggest running cargo fmt --all

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

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

src/instance.rs

danbugs · 2022-11-05T16:32:45Z

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

+    cgroups::new(p.as_ref().unwrap().to_str().unwrap().to_string())
+}
+
+pub fn setup_cgroup(cg: &Box<dyn cgroups::Cgroup>, spec: &Spec) -> Result<()> {


Suggested change

pub fn setup_cgroup(cg: &Box<dyn cgroups::Cgroup>, spec: &Spec) -> Result<()> {

pub fn setup_cgroup(cg: &dyn cgroups::Cgroup, spec: &Spec) -> Result<()> {

~don't need Box if we are passing a reference, will suggest fixes on the function calls of this too.

danbugs · 2022-11-05T16:33:23Z

src/instance.rs

-                            *ec = Some((137, Utc::now()));
-                        }
-                    };
+        oci::setup_cgroup(&cg, &spec)?;


Suggested change

oci::setup_cgroup(&cg, &spec)?;

oci::setup_cgroup(&*cg, &spec)?;

danbugs · 2022-11-05T17:42:45Z

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

@@ -52,9 +55,6 @@ impl From<Error> for ttrpc::Error {
                ttrpc::Error::RpcStatus(ttrpc::get_status(ttrpc::Code::FAILED_PRECONDITION, s))
            }
            Error::Oci(ref s) => match s {


Suggested change

Error::Oci(ref s) => match s {

Error::Oci(ref s) => {

danbugs · 2022-11-05T17:44:43Z

I'll come back to do a review of the functionality tomorrow~ lemme know if there's anything specific you'd like me to do locally to test the cgroups!

Mossaka

Great work, @cpuguy83 ! Left some comments on cgroups.rs file~

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

Mossaka · 2022-11-07T00:41:40Z

For some reason, when I retried to run a testwasm container, I got the following error

➜  runwasi git:(pr-21) ✗ sudo ctr run --rm --runtime=io.containerd.wasmtime.v1 docker.io/library/wasmtest:v1 testwasm wasm echo hello world 
ctr: Others("could not open cgroup file /default/testwasm/cpu.shares: No such file or directory (os error 2)"): unknown

Mossaka

exec.rs lgtm!

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

src/instance.rs

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

some more clippy-ing I noticed while reviewing #21

Some of the tests really require the CAP_SYS_ADMIN capability and behavior differently without it.

This is really what it is and the function makes more sense named this way. Also updated the doc for this since it was outdated already.

This allows us to return an object that will be properly closed when out of scope as well as do more interaction with the process, such as sending signals.

Since we're using fork now there is no need to spin up a new thread for most of the module setup. We only need a new thread to wait on the wasi function call to return to report the status. We also store the pidfd and use that for sending signals to.

Now the shim just calls `kill -9` on the process running the module. No more need for the deprecated feature from wasmtime.

This is better left to the caller, or perhaps some better integration with the oci spec.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Co-authored-by: Dan Chiarlone <danilochiarlone@gmail.com> Co-authored-by: Jiaxiao Zhou <duibao55328@gmail.com>

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Co-authored-by: Dan Chiarlone <danilochiarlone@gmail.com>

This just makes the intention of this function more obvious. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

This makes the implementations a little easier to read and other changes to make it much easier to test certain things.

This re-execs the test binary to run just the single test that requires sudo. This should just work in CI since there's no need to enter a password there. For local machines you'll just need to enter your sudo password to run these tests.

This adds some missing controllers, adds constants for cgroup files, and some minor cleanups. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 · 2022-11-16T02:20:25Z

Updated:

Fixed issue with absolute group paths (e.g. /foo/bar) causing the base path of the group to be overwritten (along with tests)
Split cgroup impls into separate modules
Re-wrote cgroup detection logic to be more robust
Support for v1 cgroup controllers which do not share a common root
Support for more cgroupv1 controllers
Added more tests around group setup

This should have a follow-up to add CI for cgroupv2 to make sure those code paths are exercised.
This will require setting up nested virt so we have control over the kernel.

Mossaka

lgtm!

Mossaka · 2022-11-18T03:15:23Z

Hey @cpuguy83, in our last meeting, I asked you feature parity between runwasi and runc. I remember you said there are features like namespaces and OCI hooks which are not implemented in runwasi. Do we plan to add them in the future? If yes, we could create issues for them.

danbugs

LGTM!

cpuguy83 · 2022-11-18T23:17:02Z

2 LGTM's, I'm gonna go ahead and merge.

…etch_cargo.io fix 'Unable to update registry crates-io' issue

cpuguy83 force-pushed the cgroups branch from 63d17cd to 8de5ab3 Compare October 19, 2022 02:13

cpuguy83 marked this pull request as ready for review October 26, 2022 21:58

cpuguy83 force-pushed the cgroups branch 10 times, most recently from 863822c to 968c783 Compare October 27, 2022 23:26

cpuguy83 force-pushed the cgroups branch from 5de337f to 5474fd8 Compare October 28, 2022 00:00

cpuguy83 commented Nov 1, 2022

View reviewed changes

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

cpuguy83 commented Nov 1, 2022

View reviewed changes

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

danbugs reviewed Nov 5, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

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

.github/workflows/ci.yml Outdated Show resolved Hide resolved

danbugs requested changes Nov 5, 2022

View reviewed changes

danbugs reviewed Nov 5, 2022

View reviewed changes

Mossaka requested changes Nov 7, 2022

View reviewed changes

Mossaka reviewed Nov 7, 2022

View reviewed changes

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

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

src/instance.rs Show resolved Hide resolved

src/instance.rs Show resolved Hide resolved

cpuguy83 force-pushed the cgroups branch 4 times, most recently from ee18818 to 929be45 Compare November 8, 2022 00:09

cpuguy83 commented Nov 8, 2022

View reviewed changes

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

cpuguy83 force-pushed the cgroups branch from d2a5875 to 8dc33b9 Compare November 8, 2022 01:05

danbugs added a commit that referenced this pull request Nov 8, 2022

Merge pull request #22 from danbugs/danbugs/clippy-ing

5e8fd1b

some more clippy-ing I noticed while reviewing #21

cpuguy83 and others added 17 commits November 15, 2022 23:12

CI: also run tests as root

1191ae7

Some of the tests really require the CAP_SYS_ADMIN capability and behavior differently without it.

cgroups: only write to v1 cpuset if it's been initialized

a623449

Rename perform_start to fork

be149d4

This is really what it is and the function makes more sense named this way. Also updated the doc for this since it was outdated already.

cgroups: v1: rewrite apply using helper

f07a35c

Add stronger type for pidfd

3cc046e

This allows us to return an object that will be properly closed when out of scope as well as do more interaction with the process, such as sending signals.

Remove un-needed interupt config for wasmtime

d8e7783

Now the shim just calls `kill -9` on the process running the module. No more need for the deprecated feature from wasmtime.

fork: do not create new namespaces

244c013

This is better left to the caller, or perhaps some better integration with the oci spec.

Fix-up some cgroup, especially v2, settings

84f20f3

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Apply suggestions from code review

d14084e

Co-authored-by: Dan Chiarlone <danilochiarlone@gmail.com> Co-authored-by: Jiaxiao Zhou <duibao55328@gmail.com>

Don't box reference to cgroup

685fac9

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Improve documentation for fork

7ab39e0

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Remove patch versions in cargo.toml

ab052fe

Co-authored-by: Dan Chiarlone <danilochiarlone@gmail.com>

Replace pidfd wait_no_hang with is_running

0aed7f9

This just makes the intention of this function more obvious. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Re-write cgroup detection logic for easier testing

466b805

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Refactor cgroups into separate nmdules

4372047

This makes the implementations a little easier to read and other changes to make it much easier to test certain things.

Run tests that need CAP_SYS_ADMIN under sudo

f147d07

This re-execs the test binary to run just the single test that requires sudo. This should just work in CI since there's no need to enter a password there. For local machines you'll just need to enter your sudo password to run these tests.

cpuguy83 force-pushed the cgroups branch from 01f0584 to f147d07 Compare November 15, 2022 23:15

cpuguy83 added 2 commits November 16, 2022 01:57

Round out cgroupv1 support

26f9c6e

This adds some missing controllers, adds constants for cgroup files, and some minor cleanups. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Add consts for cgroupv2 files

6a44170

cpuguy83 requested review from Mossaka and danbugs November 16, 2022 02:20

Mossaka approved these changes Nov 17, 2022

View reviewed changes

cpuguy83 mentioned this pull request Nov 18, 2022

Full Linux OCI runtime spec support #23

Closed

18 tasks

danbugs approved these changes Nov 18, 2022

View reviewed changes

cpuguy83 merged commit aa7fe9f into containerd:main Nov 18, 2022

rumpl pushed a commit to rumpl/runwasi that referenced this pull request Nov 23, 2022

Merge pull request containerd#21 from second-state/hydai/fix_cannot_f…

76c11f8

…etch_cargo.io fix 'Unable to update registry crates-io' issue

	pub fn setup_cgroup(cg: &Box<dyn cgroups::Cgroup>, spec: &Spec) -> Result<()> {
	pub fn setup_cgroup(cg: &dyn cgroups::Cgroup, spec: &Spec) -> Result<()> {

	oci::setup_cgroup(&cg, &spec)?;
	oci::setup_cgroup(&*cg, &spec)?;

Conversation

cpuguy83 commented Oct 19, 2022

Uh oh!

cpuguy83 commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danbugs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danbugs left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danbugs Nov 5, 2022

Choose a reason for hiding this comment

Uh oh!

danbugs Nov 5, 2022

Choose a reason for hiding this comment

Uh oh!

danbugs Nov 5, 2022

Choose a reason for hiding this comment

Uh oh!

danbugs commented Nov 5, 2022

Uh oh!

Mossaka 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!

Uh oh!

Mossaka commented Nov 7, 2022

Uh oh!

Mossaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpuguy83 commented Nov 16, 2022

Uh oh!

Mossaka left a comment

Choose a reason for hiding this comment

Uh oh!

Mossaka commented Nov 18, 2022

Uh oh!

danbugs left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Nov 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cpuguy83 commented Oct 27, 2022 •

edited

Loading

danbugs left a comment •

edited

Loading