search

containerd / rust-extensions

Rust crates to extend containerd
https://containerd.io
Apache License 2.0
172 stars 66 forks source link

fix(shim): rewrite cgroups v2 parsing logic #254

Closed Mossaka closed 5 months ago

Mossaka commented 5 months ago

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 https://github.com/spinkube/containerd-shim-spin/issues/39, https://github.com/containerd/runwasi/issues/418

Mossaka commented 5 months ago

The cgroups v2 parsing logic is inspired by https://github.com/opencontainers/runc/blob/1950892f69597aa844cbf000fbdf77610dda3a44/libcontainer/cgroups/fs2/defaultpath.go#L83

jsturtevant commented 5 months ago

Couple of questions:

radu-matei commented 5 months ago

I can validate that with this patch, what we are seeing in https://github.com/spinkube/containerd-shim-spin/issues/39 is now fixed.

Mossaka commented 5 months ago

Is the path with colons like that valid?

I think runc with Go impl can actually handle path with colons. So it should be valid.

What is generating the path with colons and should this be fixed in the code generating the path to be more idiomatic?

Maybe youki's libcgroups? I am not entirely sure

Mossaka commented 5 months ago

Could you please take a look, @mxpv 🙏?

mxpv commented 5 months ago

Frankly speaking (in longer term) I think we should not carry cgroups related code in the shim crate. shim crate supposed to be the foundation for building new shim runtimes for containers on all platforms, without carrying any implementation details.

Mossaka commented 5 months ago

I think there are two pending issues which are not resolved by this PR where I made an issue for each