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.

Sign up for GitHub

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

Jump to bottom

Move the cgroups v2 hybrid check from init() #3432

Closed
wants to merge 2 commits into from
Closed

Move the cgroups v2 hybrid check from init() #3432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
98c7435
Move the cgroups v2 hybrid check from init()
mrunalp Mar 25, 2022
670e0e2
Move the subsytems under the fs manager instance
mrunalp Mar 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 34 additions & 34 deletions libcontainer/cgroups/fs/fs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,33 +13,8 @@ import (
"github.com/opencontainers/runc/libcontainer/configs"
)

var subsystems = []subsystem{
&CpusetGroup{},
&DevicesGroup{},
&MemoryGroup{},
&CpuGroup{},
&CpuacctGroup{},
&PidsGroup{},
&BlkioGroup{},
&HugetlbGroup{},
&NetClsGroup{},
&NetPrioGroup{},
&PerfEventGroup{},
&FreezerGroup{},
&RdmaGroup{},
&NameGroup{GroupName: "name=systemd", Join: true},
}

var errSubsystemDoesNotExist = errors.New("cgroup: subsystem does not exist")

func init() {
// If using cgroups-hybrid mode then add a "" controller indicating
// it should join the cgroups v2.
if cgroups.IsCgroup2HybridMode() {
subsystems = append(subsystems, &NameGroup{GroupName: "", Join: true})
}
}

type subsystem interface {
// Name returns the name of the subsystem.
Name() string
Expand All @@ -54,9 +29,10 @@ type subsystem interface {
}

type manager struct {
mu sync.Mutex
cgroups *configs.Cgroup
paths map[string]string
mu sync.Mutex
cgroups *configs.Cgroup
paths map[string]string
subsystems []subsystem
Copy link
Contributor

Choose a reason for hiding this comment

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

It does not make sense to have a per-manager subsystem list.

Instead, I think, we should keep subsystems global, and wrap their initialization into once.Do.

}

func NewManager(cg *configs.Cgroup, paths map[string]string) (cgroups.Manager, error) {
Expand All @@ -69,17 +45,41 @@ func NewManager(cg *configs.Cgroup, paths map[string]string) (cgroups.Manager, e
return nil, cgroups.ErrV1NoUnified
}

subsystems := []subsystem{
&CpusetGroup{},
&DevicesGroup{},
&MemoryGroup{},
&CpuGroup{},
&CpuacctGroup{},
&PidsGroup{},
&BlkioGroup{},
&HugetlbGroup{},
&NetClsGroup{},
&NetPrioGroup{},
&PerfEventGroup{},
&FreezerGroup{},
&RdmaGroup{},
&NameGroup{GroupName: "name=systemd", Join: true},
}

// If using cgroups-hybrid mode then add a "" controller indicating
// it should join the cgroups v2.
if cgroups.IsCgroup2HybridMode() {
subsystems = append(subsystems, &NameGroup{GroupName: "", Join: true})
Copy link
Contributor

Choose a reason for hiding this comment

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

This is mutating a package var in a constructor. Can that race with other readers of subsystems? If the constructor is called multiple times, this will append multiple times, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, yeah, from a quick look, we could move the subsystems under the manager instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed a commit to test that out (will check later).

}

if paths == nil {
var err error
paths, err = initPaths(cg)
paths, err = initPaths(cg, subsystems)
if err != nil {
return nil, err
}
}

return &manager{
cgroups: cg,
paths: paths,
cgroups: cg,
paths: paths,
subsystems: subsystems,
}, nil
}

Expand Down Expand Up @@ -110,7 +110,7 @@ func (m *manager) Apply(pid int) (err error) {

c := m.cgroups

for _, sys := range subsystems {
for _, sys := range m.subsystems {
name := sys.Name()
p, ok := m.paths[name]
if !ok {
Expand Down Expand Up @@ -154,7 +154,7 @@ func (m *manager) GetStats() (*cgroups.Stats, error) {
m.mu.Lock()
defer m.mu.Unlock()
stats := cgroups.NewStats()
for _, sys := range subsystems {
for _, sys := range m.subsystems {
path := m.paths[sys.Name()]
if path == "" {
continue
Expand All @@ -177,7 +177,7 @@ func (m *manager) Set(r *configs.Resources) error {

m.mu.Lock()
defer m.mu.Unlock()
for _, sys := range subsystems {
for _, sys := range m.subsystems {
path := m.paths[sys.Name()]
if err := sys.Set(path, r); err != nil {
// When rootless is true, errors from the device subsystem
Expand Down
2 changes: 1 addition & 1 deletion libcontainer/cgroups/fs/paths.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ var (

const defaultCgroupRoot = "/sys/fs/cgroup"

func initPaths(cg *configs.Cgroup) (map[string]string, error) {
func initPaths(cg *configs.Cgroup, subsystems []subsystem) (map[string]string, error) {
root, err := rootPath()
if err != nil {
return nil, err
Expand Down