Skip to content

Commit

Permalink
Fix lxcfs cpuinfo (#8419)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
bduffany authored Feb 19, 2025
1 parent 6c6d994 commit 08e4dca
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 15 deletions.
6 changes: 6 additions & 0 deletions enterprise/server/cmd/executor/executor_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,12 @@ func setupCgroups() (string, error) {
}
log.Infof("Set up task cgroup at %s", taskCgroupPath)

// Enable the same controllers for the child cgroups that were enabled
// for the starting cgroup.
if err := cgroup.DelegateControllers(filepath.Join(cgroup.RootPath, startingCgroup)); err != nil {
return "", fmt.Errorf("inherit subtree control: %w", err)
}

taskCgroupRelpath := filepath.Join(startingCgroup, taskCgroupName)
return taskCgroupRelpath, nil
}
Expand Down
24 changes: 19 additions & 5 deletions enterprise/server/remote_execution/cgroup/cgroup.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func Setup(ctx context.Context, path string, s *scpb.CgroupSettings, blockDevice
if len(m) == 0 {
return nil
}
enabledControllers, err := ParentEnabledControllers(path)
enabledControllers, err := EnabledControllers(path)
if err != nil {
return fmt.Errorf("read enabled controllers: %w", err)
}
Expand All @@ -99,10 +99,10 @@ func Setup(ctx context.Context, path string, s *scpb.CgroupSettings, blockDevice
return nil
}

// ParentEnabledControllers returns the cgroup controllers that are enabled for
// the parent cgroup of a given cgroup.
func ParentEnabledControllers(path string) (map[string]bool, error) {
b, err := os.ReadFile(filepath.Join(path, "..", "cgroup.subtree_control"))
// EnabledControllers returns the controllers enabled for the cgroup at the
// given absolute path.
func EnabledControllers(path string) (map[string]bool, error) {
b, err := os.ReadFile(filepath.Join(path, "cgroup.controllers"))
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -133,6 +133,20 @@ func WriteSubtreeControl(path string, settings map[string]bool) error {
return os.WriteFile(filepath.Join(path, "cgroup.subtree_control"), b, 0)
}

// DelegateControllers reads the currently enabled controllers for the given
// cgroup absolute path and makes those controllers available to child cgroups
// by writing to the "cgroup.subtree_control" file.
func DelegateControllers(path string) error {
controllers, err := EnabledControllers(path)
if err != nil {
return fmt.Errorf("read enabled controllers for %q: %w", path, err)
}
if err := WriteSubtreeControl(path, controllers); err != nil {
return fmt.Errorf("write cgroup.subtree_control for %q: %w", path, err)
}
return nil
}

func settingsMap(s *scpb.CgroupSettings, blockDevice *block_io.Device) (map[string]string, error) {
m := map[string]string{}
if s == nil {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -695,16 +695,10 @@ func (c *ociContainer) setupCgroup(ctx context.Context) error {
if err := os.MkdirAll(path, 0755); err != nil {
return fmt.Errorf("create cgroup: %w", err)
}
// Before setting up the container cgroup we need to enable subtree
// controllers on the parent cgroup. For now we inherit the enabled
// controllers from the parent of the parent.
parentPath := filepath.Dir(path)
controllers, err := cgroup.ParentEnabledControllers(parentPath)
if err != nil {
return fmt.Errorf("read enabled controllers for parent of %q: %w", parentPath, err)
}
if err := cgroup.WriteSubtreeControl(parentPath, controllers); err != nil {
return fmt.Errorf("write cgroup.subtree_control for %q: %w", parentPath, err)
// Propagate enabled controllers to the child cgroup before performing
// cgroup setup.
if err := cgroup.DelegateControllers(filepath.Dir(path)); err != nil {
return fmt.Errorf("delegate controllers: %w", err)
}
if err := cgroup.Setup(ctx, path, c.cgroupSettings, c.blockDevice); err != nil {
return fmt.Errorf("configure cgroup: %w", err)
Expand Down

0 comments on commit 08e4dca

Please sign in to comment.