-
Notifications
You must be signed in to change notification settings - Fork 48
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
Correct path of cgroup when running a container in a container #132
Conversation
@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(), |
There was a problem hiding this comment.
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(), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2598e36
to
8c19687
Compare
Hi @aa624545345 LGTM, and you'd better fix the CI complained. Thanks. |
1debf3c
to
78509a2
Compare
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]>
78509a2
to
4005ad8
Compare
@@ -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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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>> { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @aa624545345
Issue can be found in #131