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

cgroups/v1: optimize path usage #2494

Merged
merged 3 commits into from
Sep 11, 2020
Merged

cgroups/v1: optimize path usage #2494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fad92bb
cgroupv1/Apply: do not overuse d.path/getSubsystemPath
kolyshkin May 30, 2020
f075084
cgroupv1/systemd: rework Apply/joinCgroups
kolyshkin May 30, 2020
940e154
cgroupv1/systemd: (re)use m.paths
kolyshkin May 30, 2020
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
24 changes: 12 additions & 12 deletions libcontainer/cgroups/fs/fs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,26 +220,26 @@ func (m *manager) Apply(pid int) (err error) {

var c = m.cgroups

d, err := getCgroupData(m.cgroups, pid)
if err != nil {
return err
}

m.paths = make(map[string]string)
if c.Paths != nil {
cgMap, err := cgroups.ParseCgroupFile("/proc/self/cgroup")
if err != nil {
return err
}
for name, path := range c.Paths {
_, err := d.path(name)
if err != nil {
if cgroups.IsNotFound(err) {
continue
}
return err
// XXX(kolyshkin@): why this check is needed?
Copy link
Member

Choose a reason for hiding this comment

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

What happens without this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can see, nothing bad. The paths map is then used to write pid to cgroups, and WriteCgroupProc has a check that path exists.

There may be other scenarios (with software that uses runc/libcontainer) I don't envision, and I have broken stuff in the past, so I am trying to be extra careful here.

So, ideally, I'd remove this, but maybe later, thus TODO.

The new check is already 100x better than the old one, so this patch is a big improvement -- I'm just not sure if the check is needed at all.

if _, ok := cgMap[name]; ok {
m.paths[name] = path
}
m.paths[name] = path
}
return cgroups.EnterPid(m.paths, pid)
}

d, err := getCgroupData(m.cgroups, pid)
if err != nil {
return err
}

for _, sys := range subsystems {
p, err := d.path(sys.Name())
if err != nil {
Expand Down
104 changes: 43 additions & 61 deletions libcontainer/cgroups/systemd/v1.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,16 +105,15 @@ func (m *legacyManager) Apply(pid int) error {
defer m.mu.Unlock()
if c.Paths != nil {
paths := make(map[string]string)
cgMap, err := cgroups.ParseCgroupFile("/proc/self/cgroup")
if err != nil {
return err
}
// XXX(kolyshkin@): why this check is needed?
for name, path := range c.Paths {
_, err := getSubsystemPath(m.cgroups, name)
if err != nil {
// Don't fail if a cgroup hierarchy was not found, just skip this subsystem
if cgroups.IsNotFound(err) {
continue
}
return err
if _, ok := cgMap[name]; ok {
paths[name] = path
}
paths[name] = path
}
m.paths = paths
return cgroups.EnterPid(m.paths, pid)
Expand Down Expand Up @@ -179,14 +178,16 @@ func (m *legacyManager) Apply(pid int) error {
return err
}

if err := joinCgroups(c, pid); err != nil {
return err
}

paths := make(map[string]string)
for _, s := range legacySubsystems {
subsystemPath, err := getSubsystemPath(m.cgroups, s.Name())
if err != nil {
// Even if it's `not found` error, we'll return err
// because devices cgroup is hard requirement for
// container security.
if s.Name() == "devices" {
return err
}
// Don't fail if a cgroup hierarchy was not found, just skip this subsystem
if cgroups.IsNotFound(err) {
continue
Expand All @@ -196,6 +197,11 @@ func (m *legacyManager) Apply(pid int) error {
paths[s.Name()] = subsystemPath
}
m.paths = paths

if err := m.joinCgroups(pid); err != nil {
return err
}

return nil
}

Expand Down Expand Up @@ -228,48 +234,25 @@ func (m *legacyManager) Path(subsys string) string {
return m.paths[subsys]
}

func join(c *configs.Cgroup, subsystem string, pid int) (string, error) {
path, err := getSubsystemPath(c, subsystem)
if err != nil {
return "", err
}

if err := os.MkdirAll(path, 0755); err != nil {
return "", err
}
if err := cgroups.WriteCgroupProc(path, pid); err != nil {
return "", err
}
return path, nil
}

func joinCgroups(c *configs.Cgroup, pid int) error {
func (m *legacyManager) joinCgroups(pid int) error {
for _, sys := range legacySubsystems {
name := sys.Name()
switch name {
case "name=systemd":
// let systemd handle this
case "cpuset":
path, err := getSubsystemPath(c, name)
if err != nil && !cgroups.IsNotFound(err) {
return err
}
s := &fs.CpusetGroup{}
if err := s.ApplyDir(path, c, pid); err != nil {
return err
if path, ok := m.paths[name]; ok {
s := &fs.CpusetGroup{}
if err := s.ApplyDir(path, m.cgroups, pid); err != nil {
return err
}
}
default:
_, err := join(c, name, pid)
if err != nil {
// Even if it's `not found` error, we'll return err
// because devices cgroup is hard requirement for
// container security.
if name == "devices" {
if path, ok := m.paths[name]; ok {
if err := os.MkdirAll(path, 0755); err != nil {
return err
}
// For other subsystems, omit the `not found` error
// because they are optional.
if !cgroups.IsNotFound(err) {
if err := cgroups.WriteCgroupProc(path, pid); err != nil {
return err
}
}
Expand Down Expand Up @@ -306,33 +289,32 @@ func getSubsystemPath(c *configs.Cgroup, subsystem string) (string, error) {
}

func (m *legacyManager) Freeze(state configs.FreezerState) error {
path, err := getSubsystemPath(m.cgroups, "freezer")
if err != nil {
return err
path, ok := m.paths["freezer"]
if !ok {
return errSubsystemDoesNotExist
}
prevState := m.cgroups.Resources.Freezer
m.cgroups.Resources.Freezer = state
freezer := &fs.FreezerGroup{}
err = freezer.Set(path, m.cgroups)
if err != nil {
if err := freezer.Set(path, m.cgroups); err != nil {
m.cgroups.Resources.Freezer = prevState
return err
}
return nil
}

func (m *legacyManager) GetPids() ([]int, error) {
path, err := getSubsystemPath(m.cgroups, "devices")
if err != nil {
return nil, err
path, ok := m.paths["devices"]
if !ok {
return nil, errSubsystemDoesNotExist
}
return cgroups.GetPids(path)
}

func (m *legacyManager) GetAllPids() ([]int, error) {
path, err := getSubsystemPath(m.cgroups, "devices")
if err != nil {
return nil, err
path, ok := m.paths["devices"]
if !ok {
return nil, errSubsystemDoesNotExist
}
return cgroups.GetAllPids(path)
}
Expand Down Expand Up @@ -403,9 +385,9 @@ func (m *legacyManager) Set(container *configs.Config) error {

for _, sys := range legacySubsystems {
// Get the subsystem path, but don't error out for not found cgroups.
path, err := getSubsystemPath(container.Cgroups, sys.Name())
if err != nil && !cgroups.IsNotFound(err) {
return err
path, ok := m.paths[sys.Name()]
if !ok {
continue
}
if err := sys.Set(path, container.Cgroups); err != nil {
return err
Expand Down Expand Up @@ -447,9 +429,9 @@ func (m *legacyManager) GetCgroups() (*configs.Cgroup, error) {
}

func (m *legacyManager) GetFreezerState() (configs.FreezerState, error) {
path, err := getSubsystemPath(m.cgroups, "freezer")
if err != nil && !cgroups.IsNotFound(err) {
return configs.Undefined, err
path, ok := m.paths["freezer"]
if !ok {
return configs.Undefined, nil
}
freezer := &fs.FreezerGroup{}
return freezer.GetState(path)
Expand Down