-
Notifications
You must be signed in to change notification settings - Fork 68
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
fix(shim): rewrite cgroups v2 parsing logic #254
Conversation
The cgroups v2 parsing logic is inspired by https://github.com/opencontainers/runc/blob/1950892f69597aa844cbf000fbdf77610dda3a44/libcontainer/cgroups/fs2/defaultpath.go#L83 |
Couple of questions:
|
I can validate that with this patch, what we are seeing in spinkube/containerd-shim-spin#39 is now fixed. |
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.
Left minor comments.
I think runc with Go impl can actually handle path with colons. So it should be valid.
Maybe youki's libcgroups? I am not entirely sure |
Could you please take a look, @mxpv 🙏? |
this commit rewrites the cgroups v2 parsing logic in get_cgroup function which is used to fetch stats of a container. The reason for the rewrite was that in some cases the original logic would panic due to index of bound for parsing paths like 0::/kubepods-besteffort-pod162385e5_7f69_4c38_ba9c_db0a8f02b35e.slice:cri-containerd:278a0aac1fff30dfbc41b4a32ba9de4519928fe7480213dba87aa1498838ef34 we ran into this issue in deleting a spin container in the spin shim. the rewrite replaces index access to properly propogate the error to the caller of the function and added a few unit tests for the parsing logic. Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>
Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>
Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>
7613f93
to
7972c9c
Compare
let content = fs::read_to_string(path).map_err(io_error!(e, "read cgroup"))?; | ||
let content = content.lines().next().unwrap_or(""); | ||
|
||
let Ok(path) = parse_cgroups_v2_path(content)?.canonicalize() else { |
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.
is there any concern about resolving symlinks or relative paths via canonicalize here?
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.
In the go implementation the path is cleaned here https://github.com/opencontainers/runc/blob/1950892f69597aa844cbf000fbdf77610dda3a44/libcontainer/cgroups/fs2/defaultpath.go#L48
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.
Testing is harder if the actual path has to exist. Maybe we can just normalize it instead of canonicalizing it, although rust's stdlib doesnt have a normalize method, that would require a 3rd party 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.
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.
If I understand the Go's CleanPath correctly, it's like normalizing the path but without verifying it exists in the filesystem, unlick fs::canonicalize()
right?
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 think the key is:
// "This ensures that a
// path resulting from prepending another path will always resolve to lexically
// be a subdirectory of the prefixed 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.
In the go implementation the path is cleaned here https://github.com/opencontainers/runc/blob/1950892f69597aa844cbf000fbdf77610dda3a44/libcontainer/cgroups/fs2/defaultpath.go#L48
@Burning1020 @mxpv does this path need be cleaned? It is there in the go code but I am not sure if I completely understand why it is needed. If not we should be ok to merge this.
Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>
Co-authored-by: James Sturtevant <jsturtevant@gmail.com> Signed-off-by: Jiaxiao Zhou <duibao55328@gmail.com>
Frankly speaking (in longer term) I think we should not carry cgroups related code in the |
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
I think there are two pending issues which are not resolved by this PR where I made an issue for each |
this commit rewrites the cgroups v2 parsing logic in get_cgroup function which is used to fetch stats of a container. The reason for the rewrite was that in some cases the original logic would panic due to index of bound for parsing paths like
0::/kubepods-besteffort-pod162385e5_7f69_4c38_ba9c_db0a8f02b35e.slice:cri-containerd:278a0aac1fff30dfbc41b4a32ba9de4519928fe7480213dba87aa1498838ef34
we ran into this issue in deleting a spin container in the spin shim.
the rewrite replaces index access to properly propogate the error to the caller of the function and added a few unit tests for the parsing logic.
see more discussion spinkube/containerd-shim-spin#39, containerd/runwasi#418