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

Fix lxcfs cpuinfo #8419

Merged
merged 1 commit into from
Feb 19, 2025
Merged

Fix lxcfs cpuinfo #8419

merged 1 commit into from
Feb 19, 2025

Conversation

bduffany
Copy link
Member

@bduffany bduffany commented Feb 16, 2025

Fix lxcfs cpuinfo not respecting cpuset when executor.child_cgroups_enabled is set.

On initialization, lxcfs looks up the current cgroup for pid 1 (in the current pid namespace) and makes an assumption that this cgroup's controllers will be the same controllers that are enabled for all child cgroups: https://github.com/lxc/lxcfs/blob/42b57f265a31261a0b69dc1f040cbcfe58a9c6a1/src/cgroups/cgfsng.c#L1036-L1054

So to make lxcfs happy, we need to enable cgroup controllers in the executor cgroup before starting lxcfs.

The fact that we aren't explicitly enabling cgroup controllers in the child cgroups made me wonder whether cgroups were working at all. After some experimentation, it seems that these cgroup controllers are only getting enabled because crun automatically enables them for us (based on the contents of buildbuddy.executor/cgroup.controllers before and after running a container). We can't rely on this automatic setup for lxcfs though, because at the time when we start lxcfs, we haven't yet invoked crun for the first time, so it hasn't set up the controllers for us.

@bduffany bduffany marked this pull request as ready for review February 18, 2025 15:23
Comment on lines +102 to +104
// EnabledControllers returns the controllers enabled for the cgroup at the
// given absolute path.
func EnabledControllers(path string) (map[string]bool, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is not strictly needed but I realized this function name and documentation are misleading. ../cgroup.subtree_control returns the set of cgroup controllers that are enabled for children of the parent cgroup, not the controllers enabled for the parent cgroup itself. So it should really be called "EnabledControllers" not "ParentEnabledControllers." Also it's probably clearer to read ./cgroup.controllers rather than ../cgroup.subtree_control, even though (iiuc) they should always return the same set of controllers.

@bduffany bduffany merged commit 08e4dca into master Feb 19, 2025
15 checks passed
@bduffany bduffany deleted the lxcfs-fix branch February 19, 2025 22:09
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.

3 participants