runc client #22

ytoml · 2022-02-03T02:22:20Z

runc client
Signed-off-by: Yuna Tomida ytomida.mmm@gmail.com

Signed-off-by: Yuna Tomida <ytomida.mmm@gmail.com>

ytoml · 2022-02-03T02:22:30Z

runc client crate, corresponds to go-runc in Go
Thanks a lot to earlier efforts rust-runc
- It's archived then I re-implement based on it.
Contains runc client of sync/async version
Supported
- runc create / start / kill / delete / state
- exec is available only in sync client now
What left unimplemented
- checkpoint related commands
- console utilities (ref: Go version)

ytoml · 2022-02-03T02:23:17Z

And updated MSRV to 1.54 for using oci-spec crate: https://github.com/containers/oci-spec-rs

Signed-off-by: Yuna Tomida <ytomida.mmm@gmail.com>

AkihiroSuda · 2022-02-03T08:32:04Z

LGTM, but needs to fix the format error

Run cargo fmt --all -- --check --files-with-diff
/home/runner/work/rust-extensions/rust-extensions/crates/runc-rust/src/lib.rs
Error: Process completed with exit code 1.

https://github.com/containerd/rust-extensions/runs/5048032832?check_suite_focus=true

Thanks!

Signed-off-by: Yuna Tomida <ytomida.mmm@gmail.com>

ytoml · 2022-02-03T11:43:03Z

I fixed it on local, but I want to ask something before push it.
When I run clippy with options set on CI, this error came out:

Actually it's not what I implemented here, but is it fine fixing on this PR?
Maybe I can fix renaming it to ParseFailed.

AkihiroSuda · 2022-02-03T12:42:19Z

Actually it's not what I implemented here, but is it fine fixing on this PR?
Maybe I can fix renaming it to ParseFailed.

Yes, in a separate commit 🙏 Thanks

Signed-off-by: Yuna Tomida <ytomida.mmm@gmail.com>

mxpv · 2022-02-03T17:44:03Z

Actually it's not what I implemented here, but is it fine fixing on this PR?
Maybe I can fix renaming it to ParseFailed.

I think this was introduced after updating rust to 1.54 in rust-toolchain.toml

mxpv · 2022-02-03T17:44:19Z

Cargo.toml

    "crates/shim",
-    "crates/snapshots"
+    "crates/snapshots",
+    "crates/runc-rust"


just crates/runc ?

Ok, thank you!

mxpv · 2022-02-03T17:44:35Z

crates/runc-rust/README.md

+Note that async client depends on [tokio](https://github.com/tokio-rs/tokio), then please use it on tokio runtime.
+
+```rust
+use runc_rust as runc;


Ok, thank you!

mxpv · 2022-02-03T18:05:59Z

crates/runc-rust/src/io.rs

+    stderr: Option<Pipe>,
+}
+
+impl RuncPipedIO {


I think we could omit these Runc prefixes everywhere.
runc::RuncPipedIO -> runc::PipedIo
Same for other structs.

Ok, thank you!

mxpv · 2022-02-03T18:18:18Z

crates/runc-rust/src/lib.rs

+                serde_json::to_string(resources).map_err(Error::JsonDeserializationFailed)?;
+            f.write(spec_json.as_bytes())
+                .map_err(Error::SpecFileCreationError)?;
+            f.flush().map_err(Error::SpecFileCreationError)?;


This block can be reduced to:

fs::write(path, spec_json)?;

Ok, thank you!

kzys · 2022-02-03T17:59:25Z

crates/runc-rust/src/container.rs

+
+    #[test]
+    fn serde_test() {
+        let j = "


How about using raw string literals here? So that you can write " without escaping.

It's good, thank you!

kzys · 2022-02-03T18:00:35Z

crates/runc-rust/src/container.rs

+            }";
+
+        let c: Container = serde_json::from_str(j).unwrap();
+        println!("{:#?}", c);


Personally prefer to keep stdout clean. If you have some properties you want to check, write them as tests.

Thank you for recommend but I'm sorry, I couldn't see your point.
This function is in mod tests and attributed as #[test], but it's not enough?

What you'd like to do with the println! output? If you want to sometimes manually check the result, it would be better to put more context or use dbg!.

That being said, it would be great if you can automate the manual checking part by writing assertions.

Oh, I see, thank you!
I'll rewrite them with assertions.

kzys · 2022-02-03T18:05:53Z

crates/runc-rust/src/io.rs

+    fn close(&self) {
+        unimplemented!()
+    }
+
+    /// Set IO for passed command.
+    /// Read side of stdin, write side of stdout and write side of stderr should be provided to command.
+    fn set(&self, _cmd: &mut Command) -> std::io::Result<()> {
+        unimplemented!()
+    }
+
+    // tokio version of set()
+    fn set_tk(&self, _cmd: &mut tokio::process::Command) -> std::io::Result<()> {
+        unimplemented!()
+    }
+
+    fn close_after_start(&self) {
+        unimplemented!()
+    }


Not clear about these unimplemented. Due to time constraints?

I' ll remove them, thank you!

kzys · 2022-02-03T18:10:59Z

crates/runc-rust/src/io.rs

+    pub fn take_read(&self) -> Option<File> {
+        let mut m = self.rd.lock().unwrap();
+        m.take()
+    }
+
+    pub fn take_write(&self) -> Option<File> {
+        let mut m = self.wr.lock().unwrap();
+        m.take()
+    }
+
+    pub fn close_read(&self) {
+        let mut m = self.rd.lock().unwrap();
+        let _ = m.take();
+    }
+
+    pub fn close_write(&self) {
+        let mut m = self.wr.lock().unwrap();
+        let _ = m.take();
+    }


lock() returns Result. Does it make sense to return the errors rather than panic?

not in case of std Mutex - https://users.rust-lang.org/t/should-i-unwrap-a-mutex-lock/61519

kzys · 2022-02-03T18:13:25Z

crates/runc-rust/src/io.rs

+}
+
+impl RuncPipedIO {
+    pub fn new(uid: isize, gid: isize, opts: IOOption) -> std::io::Result<Self> {


Should this use isize here?

Actually, I just followed Go's but it seems to be unnecessary...

Thank you, I'll change it to u32

mxpv · 2022-02-03T18:32:26Z

crates/runc-rust/src/io.rs

+    }
+
+    fn close_after_start(&self) {
+        unimplemented!()


unimplemented is not necessary here, just have it as

fn close_after_start(&self);

so the compiler will require implementation at compile time when implementing this trait.

Ok, thank you!

mxpv · 2022-02-03T18:36:21Z

crates/runc-rust/src/io.rs

+/// when one side of closed, this struct represent it with [`None`]
+#[derive(Debug)]
+pub struct Pipe {
+    // Might be ugly hack: using mutex in order to take rd/wr under immutable [`Pipe`]


Is there a particular reason why it can't be mutable?

Actually what I want to do is accessing struct containing Pipe like PipeIO here from multipile thread, and extract file, and copy the stdio to other io entry from higher layer(e.g. Fifo that passed from containerd to shim).
In such case, we should Arc it to reference from multiple thread and need Mutex to be mutable(to read/write), but any two threads will not access the same entry(e.g. stdout and stdout) and I thought it's redundant to make Mutex<PipedIO>.
Is there any other good way to do this?

Also, like close_after_write, I want to close some files(but not all) manually from threads.

mxpv · 2022-02-03T18:37:51Z

crates/runc-rust/src/io.rs

+        None
+    }
+
+    fn close(&self) {


impl Drop instead?

Ok, thank you!

As destructor automatically drop files inside structs, I will just erase this trait method.

mxpv · 2022-02-03T19:04:12Z

crates/runc-rust/src/io.rs

+    pub fn take_read(&self) -> Option<File> {
+        let mut m = self.rd.lock().unwrap();
+        m.take()
+    }
+
+    pub fn take_write(&self) -> Option<File> {
+        let mut m = self.wr.lock().unwrap();
+        m.take()
+    }
+
+    pub fn close_read(&self) {
+        let mut m = self.rd.lock().unwrap();
+        let _ = m.take();
+    }
+
+    pub fn close_write(&self) {
+        let mut m = self.wr.lock().unwrap();
+        let _ = m.take();
+    }


not in case of std Mutex - https://users.rust-lang.org/t/should-i-unwrap-a-mutex-lock/61519

Signed-off-by: Yuna Tomida <ytomida.mmm@gmail.com>

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

AkihiroSuda

CI green.

Let's merge this toward reopening the actual shim part (#21)

mxpv · 2022-02-06T21:05:48Z

@AkihiroSuda do we need 6e17be8 for this PR? Seems like runc crate has no dependencies on these.

mxpv · 2022-02-06T21:06:38Z

@kzys PTAL

AkihiroSuda · 2022-02-07T01:59:52Z

@AkihiroSuda do we need 6e17be8 for this PR? Seems like runc crate has no dependencies on these.

Cargo automatically bumped up them, so I committed.

kzys · 2022-02-08T18:14:11Z

.github/workflows/ci.yml

      - run: cargo fmt --all -- --check --files-with-diff
      - run: cargo clippy --all-targets --all-features -- -D warnings
-      - run: cargo test --all-features
+      # runc::tests::test_exec needs $XDG_RUNTIME_DIR to be set


Shouldn't the test set the environment variable if so?

No, usually set by pam_systemd (which is missing in GHA)

kzys · 2022-02-08T18:18:44Z

crates/runc/src/container.rs

+#[derive(Debug, Serialize, Deserialize)]
+pub struct Container {
+    pub id: String,
+    pub pid: usize,


u32? Response's pid is u32.

That's right, thank you!

mxpv · 2022-02-08T18:26:00Z

Cargo automatically bumped up them, so I committed.

@AkihiroSuda can we revert it? Seems like its not relevant for this PR (also looks like it depends on environment, I'll TAL later).

kzys

Mostly nits. I'm fine merging the PR as is and address them later.

kzys · 2022-02-08T18:21:07Z

crates/runc/src/lib.rs

+        let stdout = String::from_utf8(result.stdout).unwrap();
+        let stderr = String::from_utf8(result.stderr).unwrap();


Wouldn't it fail if the command outputs some non UTF-8 bytes (e.g. gzip)?

Sure. However which is preferable in such case, returning parse error or return the raw value to caller?

This is just my personal opinion but I think returning error is better. Higher level wrappers which use this crate, like shim, will bend the stdout/err of command to IO that they prepare in advance. And they can handle such non-utf8 within their procedure if needed.
In such situation, these result.stdout and result.stderr will be empty string (and never be non-utf8).

kzys · 2022-02-08T18:22:18Z

crates/runc/src/lib.rs

+        let args = ["list".to_string(), "--format-json".to_string()];
+        let res = self.launch(self.command(&args)?, true).await?;
+        let output = res.output.trim();
+        // Ugly hack to work around golang


I want to know more about the thing you need to workaround :)

Actually, this is for just handling null json. But I think this way (with comparering output and "null") is not concise...
Is it comprehensive if I change it like below?

Ok(serde_json::from_str::<Option<Vec<Container>>>(output).map_err(/*omit*/)?.unwrap_or_default())

kzys · 2022-02-08T18:22:41Z

crates/runc/src/lib.rs

+
+    fn dummy_process() -> Process {
+        serde_json::from_str(
+            "


raw string?

Sure. Thank you!

AkihiroSuda · 2022-02-08T18:31:32Z

Seems like its not relevant for this PR (also looks like it depends on environment, I'll TAL later).

Not depending on local env, simply 2.27.1 is the latest ( https://crates.io/crates/protobuf ), and Cargo picks up the latest one because we do not commit Cargo.lock in the repo (why?)

mxpv · 2022-02-08T18:56:13Z

we do not commit Cargo.lock in the repo (why?)

We generally don't commit Cargo.lock for libraries: https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries

Burning1020 · 2022-02-10T15:24:37Z

crates/runc/src/container.rs

+pub struct Container {
+    pub id: String,
+    pub pid: usize,
+    pub status: String,


should status be defined as enum type for only a handful of container status in act? @mxpv @kzys

Burning1020 · 2022-02-10T15:54:18Z

crates/runc/src/lib.rs

+
+    /// Return the state of a container
+    pub fn state(&self, id: &str) -> Result<Container> {
+        let args = ["state".to_string(), id.to_string()];


it seems that containerd-shim-runc-v2(Go version) uses its memory variable to strore container state(eg. https://github.com/containerd/containerd/blob/release%2F1.5/pkg/process/init_state.go) instead of runc state cli command, cause shim has its own state system maybe.

Burning1020 · 2022-02-10T15:59:35Z

crates/runc/src/lib.rs

+        ];
+        self.launch(self.command(&args)?, true)?;
+        Ok(())
+    }


update normaly takes stdin as arg, we should provide this way for compatibility

mxpv · 2022-02-11T17:46:01Z

Ok, I'm going to merge this in. Let's address remaining items in follow up PRs.

runc client

d34fa45

Signed-off-by: Yuna Tomida <ytomida.mmm@gmail.com>

ytoml mentioned this pull request Feb 3, 2022

[shim] minimal implementation for containerd-shim-runc-v2 #21

Closed

ytoml added 2 commits February 3, 2022 05:09

removed chrono from dependencies and applied fmt

7c2b93f

Signed-off-by: Yuna Tomida <ytomida.mmm@gmail.com>

confirmed tests

2663849

Signed-off-by: Yuna Tomida <ytomida.mmm@gmail.com>

applied fmt

43cf334

Signed-off-by: Yuna Tomida <ytomida.mmm@gmail.com>

fixed clippy warnings in shim::args and example in client

8bbd4e7

Signed-off-by: Yuna Tomida <ytomida.mmm@gmail.com>

mxpv reviewed Feb 3, 2022

View reviewed changes

kzys requested changes Feb 3, 2022

View reviewed changes

mxpv reviewed Feb 3, 2022

View reviewed changes

ytoml and others added 4 commits February 4, 2022 00:39

renamed crate and applied recommended changes

8f3e556

Signed-off-by: Yuna Tomida <ytomida.mmm@gmail.com>

remove unnecessary 'forget'

eddd107

Signed-off-by: Yuna Tomida <ytomida.mmm@gmail.com>

crates/runc/src/lib.rs: fix compilation on non-Linux

522e26f

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

crates/shim-protos: regenerate with rust-protobuf 2.27.1

6e17be8

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

AkihiroSuda force-pushed the runc-client branch from 228fc2b to 6e17be8 Compare February 6, 2022 09:34

CI: set $XDG_RUNTIME_DIR

18b5b99

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

AkihiroSuda approved these changes Feb 6, 2022

View reviewed changes

AkihiroSuda requested a review from mxpv February 6, 2022 10:20

mxpv approved these changes Feb 6, 2022

View reviewed changes

AkihiroSuda requested a review from kzys February 8, 2022 07:03

kzys reviewed Feb 8, 2022

View reviewed changes

Burning1020 suggested changes Feb 10, 2022

View reviewed changes

mxpv merged commit 73d1dfa into containerd:main Feb 11, 2022

AkihiroSuda mentioned this pull request Feb 12, 2022

Minimum runc shim implementation #24

Merged

utam0k mentioned this pull request Feb 23, 2022

need rust shim youki-dev/youki#633

Closed

		let stdout = String::from_utf8(result.stdout).unwrap();
		let stderr = String::from_utf8(result.stderr).unwrap();

runc client #22

runc client #22

Uh oh!

Conversation

ytoml commented Feb 3, 2022

Uh oh!

ytoml commented Feb 3, 2022

Uh oh!

ytoml commented Feb 3, 2022

Uh oh!

AkihiroSuda commented Feb 3, 2022

Uh oh!

ytoml commented Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Feb 3, 2022

Uh oh!

mxpv commented Feb 3, 2022

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!

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!

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!

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!

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

ytoml commented Feb 3, 2022 •

edited

Loading

mxpv commented Feb 6, 2022 •

edited

Loading

ytoml Feb 9, 2022 •

edited

Loading