Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct path of cgroup when running a container in a container #132

Merged
merged 1 commit into from
May 13, 2024

Conversation

aa624545345
Copy link

Issue can be found in #131

@aa624545345
Copy link
Author

@liwei @jbryce @gnawux @bergwolf Hello, take a look here?

@aa624545345
Copy link
Author

@Tim-Zhang Hello, take a look here?

src/blkio.rs Outdated
@@ -432,6 +436,16 @@ impl BlkIoController {
/// Constructs a new `BlkIoController` with `root` serving as the root of the control group.
pub fn new(root: PathBuf, v2: bool) -> Self {
Self {
mount_root: Default::default(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the controllers have the exact same code pattern. Shouldn't we try to factor out that code a little more?

src/blkio.rs Outdated
@@ -432,6 +436,16 @@ impl BlkIoController {
/// Constructs a new `BlkIoController` with `root` serving as the root of the control group.
pub fn new(root: PathBuf, v2: bool) -> Self {
Self {
mount_root: Default::default(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I found the base field wasn't used, was it better to use base field as the cgroup's mount root path?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@c3d @lifupan Thank you both for the review, i find that it's true that base is not used. I'll modify and push a new version later.

@aa624545345
Copy link
Author

@c3d @lifupan Modification is pushed, please review, thank u!

@aa624545345 aa624545345 changed the title Collect path of cgroup when running a container in a container Correct path of cgroup when running a container in a container Apr 8, 2024
@lifupan
Copy link
Member

lifupan commented Apr 15, 2024

@c3d @lifupan Modification is pushed, please review, thank u!

Hi @aa624545345

LGTM, and you'd better fix the CI complained.

Thanks.

@aa624545345 aa624545345 force-pushed the collect-cgroup-path branch 2 times, most recently from 1debf3c to 78509a2 Compare April 16, 2024 02:07
Path of cgroup is wrong when running a container in a container. Use
the root path of mountinfo fetched from /proc/$(shim_pid)/mountinfo
to trim the path obtained from /proc/self/mountinfo.

Fixes: kata-containers#131

Signed-off-by: 乔琛 10307740 <[email protected]>
@aa624545345
Copy link
Author

@lifupan @c3d Hello, ci was passed, please review, thx!

@@ -588,6 +589,33 @@ pub fn get_cgroups_relative_paths_by_pid(pid: u32) -> Result<HashMap<String, Str
get_cgroups_relative_paths_by_path(path)
}

fn get_cgroup_destination(mut mount_root: String, pidpath: String) -> String {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the parameter mount_root and pidpath can be &str?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, i can update it here.

pidpath.trim_start_matches(&mount_root).to_string()
}

pub fn existing_path(paths: HashMap<String, String>) -> Result<HashMap<String, String>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see where this fn was called?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is a public function. In my design, this function is optional, it can be called after get_cgroups_relative_paths or get_cgroups_relative_paths_by_pid for coder using this crate.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lifupan
Copy link
Member

lifupan commented May 13, 2024

/test

Copy link
Member

@Tim-Zhang Tim-Zhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @aa624545345

@Tim-Zhang Tim-Zhang merged commit eb3e37a into kata-containers:main May 13, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants