Remove unnecessary chdir #2780

utam0k · 2024-05-03T13:03:32Z

These chdir are unexpected for libcontainer's users.

utam0k · 2024-05-03T13:04:46Z

I've checked the relevant tests that came from containerd were passed.
#2777 (comment)

utam0k · 2024-05-03T13:06:04Z

Vagrantfile.containerd2youki

@@ -44,8 +44,7 @@ Vagrant.configure("2") do |config|
        ./script/setup/install-cni
        ./script/setup/install-critools
        rm -rf /bin/runc /sbin/runc /usr/sbin/runc /usr/bin/runc
-
-        cp /vagrant/youki/youki /usr/bin/runc
+        ln -s /vagrant/youki/youki /usr/bin/runc


It'd be more beneficial because we don't need to copy again when we rebuild.

utam0k · 2024-05-03T13:33:58Z

I'll check CI.

Co-authored-by: jiaxiao zhou <jiazho@microsoft.com> Co-authored-by: Jorge Prendes <jorge.prendes@gmail.com> Signed-off-by: utam0k <k0ma@utam0k.jp>

utam0k · 2024-05-05T13:09:48Z

@Furisto Can you take a look at this PR?

crates/libcontainer/src/container/container_start.rs

Signed-off-by: utam0k <k0ma@utam0k.jp>

YJDoc2 · 2024-05-10T07:18:35Z

@Mossaka can you confirm that this branch's youki build resolved the original perf issues you were facing? So that once this PR is checked from the our side, we can merge and make a release.

Signed-off-by: utam0k <k0ma@utam0k.jp>

crates/libcontainer/src/container/container_start.rs

Mossaka · 2024-05-10T23:08:45Z

@Mossaka can you confirm that this branch's youki build resolved the original perf issues you were facing? So that once this PR is checked from the our side, we can merge and make a release.

Just confirmed, it solved the original perf issue we are facing

Signed-off-by: utam0k <k0ma@utam0k.jp>

YJDoc2

Hey, can you take a look at two comments I have? Apart from them, it is lgtm

crates/libcontainer/src/tty.rs

crates/libcontainer/src/hooks.rs

Signed-off-by: utam0k <k0ma@utam0k.jp>

YJDoc2

LGTM 👍
Not merging it in case you want to do something before release, or anyone else has a review comment

Thanks @utam0k !

jprendes · 2024-05-15T10:25:25Z

crates/libcontainer/src/container/builder_impl.rs

+use std::{fs, io::Write, os::unix::prelude::RawFd, path::PathBuf, rc::Rc};
+
+use libcgroups::common::CgroupManager;
+use nix::unistd::Pid;
+use oci_spec::runtime::Spec;
+


thought: We should have a rustfmt.toml to standardize this.
I personally like the one that @Mossaka put together for runwasi.
https://github.com/containerd/runwasi/blob/main/rustfmt.toml

I didn't know that. It is what I want! Thanks for sharing.

jprendes

Thanks @utam0k !
I left a minor comments.
Otherwise, LGTM.

crates/libcontainer/src/hooks.rs

Signed-off-by: utam0k <k0ma@utam0k.jp>

utam0k requested a review from Furisto May 3, 2024 13:03

utam0k added the kind/bug label May 3, 2024

utam0k commented May 3, 2024

View reviewed changes

utam0k force-pushed the remove-chdir branch from 6d0627c to e650bad Compare May 3, 2024 13:08

This was referenced May 3, 2024

Unexpected chdir invoked on container init and start #2772

Closed

Remove chdir in container init and start #2777

Closed

Mossaka mentioned this pull request May 5, 2024

feat(shim): change the current dir back to bundle path containerd/runwasi#577

Closed

utam0k force-pushed the remove-chdir branch from e650bad to 2c0736a Compare May 5, 2024 12:55

Remove unnecessary chdir

ada9cd1

Co-authored-by: jiaxiao zhou <jiazho@microsoft.com> Co-authored-by: Jorge Prendes <jorge.prendes@gmail.com> Signed-off-by: utam0k <k0ma@utam0k.jp>

utam0k force-pushed the remove-chdir branch from 2c0736a to ada9cd1 Compare May 5, 2024 12:56

Mossaka approved these changes May 6, 2024

View reviewed changes

radu-matei mentioned this pull request May 6, 2024

Startup performance spinkube/containerd-shim-spin#41

Closed

YJDoc2 reviewed May 6, 2024

View reviewed changes

crates/libcontainer/src/container/container_start.rs Show resolved Hide resolved

fixup! Remove unnecessary chdir

874414a

Signed-off-by: utam0k <k0ma@utam0k.jp>

utam0k force-pushed the remove-chdir branch from 444146e to 874414a Compare May 8, 2024 13:03

fixup! Remove unnecessary chdir

524ae7a

Signed-off-by: utam0k <k0ma@utam0k.jp>

utam0k mentioned this pull request May 9, 2024

deps: Disable unused/unnecessary regex features in libcontainer #2781

Merged

YJDoc2 mentioned this pull request May 10, 2024

Move chdir to post start hooks utam0k/youki#528

Closed

fixup! Remove unnecessary chdir

8de2233

Signed-off-by: utam0k <k0ma@utam0k.jp>

jprendes reviewed May 10, 2024

View reviewed changes

crates/libcontainer/src/container/container_start.rs Outdated Show resolved Hide resolved

Mossaka approved these changes May 10, 2024

View reviewed changes

Enable run_hooks to be passed cwd

b536591

Signed-off-by: utam0k <k0ma@utam0k.jp>

utam0k requested review from YJDoc2 and jprendes May 11, 2024 06:31

YJDoc2 reviewed May 13, 2024

View reviewed changes

crates/libcontainer/src/tty.rs Show resolved Hide resolved

crates/libcontainer/src/hooks.rs Outdated Show resolved Hide resolved

jprendes reviewed May 14, 2024

View reviewed changes

crates/libcontainer/src/hooks.rs Outdated Show resolved Hide resolved

crates/libcontainer/src/hooks.rs Outdated Show resolved Hide resolved

crates/libcontainer/src/hooks.rs Outdated Show resolved Hide resolved

fixup! Enable run_hooks to be passed cwd

a816712

Signed-off-by: utam0k <k0ma@utam0k.jp>

utam0k force-pushed the remove-chdir branch from 73cffe3 to a816712 Compare May 14, 2024 12:55

utam0k requested review from jprendes, Mossaka and YJDoc2 May 14, 2024 12:55

YJDoc2 approved these changes May 14, 2024

View reviewed changes

jprendes reviewed May 15, 2024

View reviewed changes

jprendes approved these changes May 15, 2024

View reviewed changes

crates/libcontainer/src/hooks.rs Outdated Show resolved Hide resolved

fixup! Enable run_hooks to be passed cwd

4a49759

Signed-off-by: utam0k <k0ma@utam0k.jp>

utam0k enabled auto-merge (squash) May 15, 2024 11:53

utam0k merged commit 25edc95 into youki-dev:main May 15, 2024
28 checks passed

github-actions bot mentioned this pull request May 15, 2024

Release for v0.3.3 #2665

Merged

Mossaka mentioned this pull request May 16, 2024

update youki to v0.3.3 to pick up the changes that remove chdir containerd/runwasi#590

Closed

utam0k mentioned this pull request Jun 16, 2024

Wrong directory when using a tenant container and w/ a mount namespace #2751

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unnecessary chdir #2780

Remove unnecessary chdir #2780

utam0k commented May 3, 2024

utam0k commented May 3, 2024

utam0k May 3, 2024

utam0k commented May 3, 2024

utam0k commented May 5, 2024

YJDoc2 commented May 10, 2024

Mossaka commented May 10, 2024

YJDoc2 left a comment

YJDoc2 left a comment

jprendes May 15, 2024 •

edited

Loading

utam0k May 15, 2024

jprendes May 15, 2024

jprendes left a comment •

edited

Loading

Remove unnecessary chdir #2780

Remove unnecessary chdir #2780

Conversation

utam0k commented May 3, 2024

utam0k commented May 3, 2024

utam0k May 3, 2024

Choose a reason for hiding this comment

utam0k commented May 3, 2024

utam0k commented May 5, 2024

YJDoc2 commented May 10, 2024

Mossaka commented May 10, 2024

YJDoc2 left a comment

Choose a reason for hiding this comment

YJDoc2 left a comment

Choose a reason for hiding this comment

jprendes May 15, 2024 • edited Loading

Choose a reason for hiding this comment

utam0k May 15, 2024

Choose a reason for hiding this comment

jprendes May 15, 2024

Choose a reason for hiding this comment

jprendes left a comment • edited Loading

Choose a reason for hiding this comment

jprendes May 15, 2024 •

edited

Loading

jprendes left a comment •

edited

Loading