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

Investigate uses of cgroups.Manager.GetCgroups() (unused, perhaps can be deprecated) #3221

Closed
thaJeztah opened this issue Sep 23, 2021 · 5 comments

Comments

@thaJeztah
Copy link
Member

While reviewing #3216, I noticed the manager interface has a GetCgroups(), which is unused in our codebase.

Looking at its history to see if it was used at some point, but "no longer" used, I found that it was contributed in #2177 to be used by an external consumer. Digging further, that PR was linked to kata-containers/runtime#2351, which is in a repository that has now been archived.

We should look if GetCgroups() is actively used by external consumers, and consider deprecating (and removing) it, if there are no uses.

@thaJeztah
Copy link
Member Author

/cc @devimc perhaps you know if it's still used in your codebase (which may have been moved elsewhere)

/cc @kolyshkin

@devimc
Copy link
Contributor

devimc commented Sep 23, 2021

please go ahead and deprecate it, we are using github.com/containerd/cgroups for cgroups management, thanks

@thaJeztah
Copy link
Member Author

Thanks @devimc ! It's only a small bit of code, but I know we're looking at making more packages "internal" (as not all of them are really designed for external consumption), so this one caught my eye.

@kolyshkin
Copy link
Contributor

This is used by kubernetes (see e.g. https://sourcegraph.com/github.com/kubernetes/kubernetes/-/blob/pkg/kubelet/cm/container_manager_linux.go?L913:27#tab=references), maybe also cAdvisor, so we can't remove it.

One other thing, pretty big, which is not used by runc itself, is GetStats. We have lots of code and some tests for these methods, but never use them. The user is, again, kubernetes, and probably cAdvisor, too.

The way to solve it is to have a separate basic cgroup library which is used as a base by both runc and kubernetes. This is the subject of kubernetes/kubernetes#104325, and it's a lot of work that has not been started yet (but I have it in mind).

(Feel free to reopen if I'm missing something)

@kolyshkin
Copy link
Contributor

Ah, and the main reason why kubernetes use libcontainer is because no other cgroup library supports systemd (AFAIK).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants