Skip to content

Commit

Permalink
libct/cg: simplify getting cgroup manager
Browse files Browse the repository at this point in the history
1. Make Rootless and Systemd flags part of config.Cgroups.

2. Make all cgroup managers (not just fs2) return error (so it can do
   more initialization -- added by the following commits).

3. Replace complicated cgroup manager instantiation in factory_linux
   by a single (and simple) libcontainer/cgroups/manager.New() function.

4. getUnifiedPath is simplified to check that only a single path is
   supplied (rather than checking that other paths, if supplied,
   are the same).

[v2: can't -> cannot]

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
  • Loading branch information
kolyshkin committed Sep 23, 2021
1 parent 147ad56 commit 097c6d7
Show file tree
Hide file tree
Showing 17 changed files with 188 additions and 222 deletions.
9 changes: 6 additions & 3 deletions contrib/cmd/sd-helper/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,9 @@ func main() {
}
}

func newManager(config *configs.Cgroup) cgroups.Manager {
func newManager(config *configs.Cgroup) (cgroups.Manager, error) {
if cgroups.IsCgroup2UnifiedMode() {
return systemd.NewUnifiedManager(config, "", false)
return systemd.NewUnifiedManager(config, "")
}
return systemd.NewLegacyManager(config, nil)
}
Expand All @@ -70,7 +70,10 @@ func unitCommand(cmd, name, parent string) error {
Parent: parent,
Resources: &configs.Resources{},
}
pm := newManager(podConfig)
pm, err := newManager(podConfig)
if err != nil {
return err
}

switch cmd {
case "start":
Expand Down
24 changes: 11 additions & 13 deletions libcontainer/cgroups/fs/fs.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,18 +49,16 @@ type subsystem interface {
}

type manager struct {
mu sync.Mutex
cgroups *configs.Cgroup
rootless bool // ignore permission-related errors
paths map[string]string
mu sync.Mutex
cgroups *configs.Cgroup
paths map[string]string
}

func NewManager(cg *configs.Cgroup, paths map[string]string, rootless bool) cgroups.Manager {
func NewManager(cg *configs.Cgroup, paths map[string]string) (cgroups.Manager, error) {
return &manager{
cgroups: cg,
paths: paths,
rootless: rootless,
}
cgroups: cg,
paths: paths,
}, nil
}

// isIgnorableError returns whether err is a permission error (in the loose
Expand Down Expand Up @@ -121,7 +119,7 @@ func (m *manager) Apply(pid int) (err error) {
// explicit cgroup path hasn't been set, we don't bail on error in
// case of permission problems. Cases where limits have been set
// (and we couldn't create our own cgroup) are handled by Set.
if isIgnorableError(m.rootless, err) && m.cgroups.Path == "" {
if isIgnorableError(c.Rootless, err) && m.cgroups.Path == "" {
delete(m.paths, sys.Name())
continue
}
Expand Down Expand Up @@ -174,10 +172,10 @@ func (m *manager) Set(r *configs.Resources) error {
for _, sys := range subsystems {
path := m.paths[sys.Name()]
if err := sys.Set(path, r); err != nil {
if m.rootless && sys.Name() == "devices" {
if m.cgroups.Rootless && sys.Name() == "devices" {
continue
}
// When m.rootless is true, errors from the device subsystem are ignored because it is really not expected to work.
// When rootless is true, errors from the device subsystem are ignored because it is really not expected to work.
// However, errors from other subsystems are not ignored.
// see @test "runc create (rootless + limits + no cgrouppath + no permission) fails with informative error"
if path == "" {
Expand Down Expand Up @@ -249,7 +247,7 @@ func OOMKillCount(path string) (uint64, error) {
func (m *manager) OOMKillCount() (uint64, error) {
c, err := OOMKillCount(m.Path("memory"))
// Ignore ENOENT when rootless as it couldn't create cgroup.
if err != nil && m.rootless && os.IsNotExist(err) {
if err != nil && m.cgroups.Rootless && os.IsNotExist(err) {
err = nil
}

Expand Down
7 changes: 5 additions & 2 deletions libcontainer/cgroups/fs/fs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,11 @@ func BenchmarkGetStats(b *testing.B) {
Path: "/some/kind/of/a/path/here",
Resources: &configs.Resources{},
}
m := NewManager(cg, nil, false)
err := m.Apply(-1)
m, err := NewManager(cg, nil)
if err != nil {
b.Fatal(err)
}
err = m.Apply(-1)
if err != nil {
b.Fatal(err)
}
Expand Down
23 changes: 9 additions & 14 deletions libcontainer/cgroups/fs2/fs2.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,12 @@ type manager struct {
// controllers is content of "cgroup.controllers" file.
// excludes pseudo-controllers ("devices" and "freezer").
controllers map[string]struct{}
rootless bool
}

// NewManager creates a manager for cgroup v2 unified hierarchy.
// dirPath is like "/sys/fs/cgroup/user.slice/user-1001.slice/session-1.scope".
// If dirPath is empty, it is automatically set using config.
func NewManager(config *configs.Cgroup, dirPath string, rootless bool) (cgroups.Manager, error) {
if config == nil {
config = &configs.Cgroup{}
}
func NewManager(config *configs.Cgroup, dirPath string) (cgroups.Manager, error) {
if dirPath == "" {
var err error
dirPath, err = defaultDirPath(config)
Expand All @@ -39,9 +35,8 @@ func NewManager(config *configs.Cgroup, dirPath string, rootless bool) (cgroups.
}

m := &manager{
config: config,
dirPath: dirPath,
rootless: rootless,
config: config,
dirPath: dirPath,
}
return m, nil
}
Expand All @@ -53,7 +48,7 @@ func (m *manager) getControllers() error {

data, err := cgroups.ReadFile(m.dirPath, "cgroup.controllers")
if err != nil {
if m.rootless && m.config.Path == "" {
if m.config.Rootless && m.config.Path == "" {
return nil
}
return err
Expand All @@ -73,7 +68,7 @@ func (m *manager) Apply(pid int) error {
// - "runc create (no limits + no cgrouppath + no permission) succeeds"
// - "runc create (rootless + no limits + cgrouppath + no permission) fails with permission error"
// - "runc create (rootless + limits + no cgrouppath + no permission) fails with informative error"
if m.rootless {
if m.config.Rootless {
if m.config.Path == "" {
if blNeed, nErr := needAnyControllers(m.config.Resources); nErr == nil && !blNeed {
return nil
Expand Down Expand Up @@ -127,7 +122,7 @@ func (m *manager) GetStats() (*cgroups.Stats, error) {
if err := fscommon.RdmaGetStats(m.dirPath, st); err != nil && !os.IsNotExist(err) {
errs = append(errs, err)
}
if len(errs) > 0 && !m.rootless {
if len(errs) > 0 && !m.config.Rootless {
return st, fmt.Errorf("error while statting cgroup v2: %+v", errs)
}
return st, nil
Expand Down Expand Up @@ -171,10 +166,10 @@ func (m *manager) Set(r *configs.Resources) error {
}
// devices (since kernel 4.15, pseudo-controller)
//
// When m.rootless is true, errors from the device subsystem are ignored because it is really not expected to work.
// When rootless is true, errors from the device subsystem are ignored because it is really not expected to work.
// However, errors from other subsystems are not ignored.
// see @test "runc create (rootless + limits + no cgrouppath + no permission) fails with informative error"
if err := setDevices(m.dirPath, r); err != nil && !m.rootless {
if err := setDevices(m.dirPath, r); err != nil && !m.config.Rootless {
return err
}
// cpuset (since kernel 5.0)
Expand Down Expand Up @@ -250,7 +245,7 @@ func OOMKillCount(path string) (uint64, error) {

func (m *manager) OOMKillCount() (uint64, error) {
c, err := OOMKillCount(m.dirPath)
if err != nil && m.rootless && os.IsNotExist(err) {
if err != nil && m.config.Rootless && os.IsNotExist(err) {
err = nil
}

Expand Down
78 changes: 78 additions & 0 deletions libcontainer/cgroups/manager/new.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
package manager

import (
"errors"
"fmt"
"path/filepath"

"github.com/opencontainers/runc/libcontainer/cgroups"
"github.com/opencontainers/runc/libcontainer/cgroups/fs"
"github.com/opencontainers/runc/libcontainer/cgroups/fs2"
"github.com/opencontainers/runc/libcontainer/cgroups/systemd"
"github.com/opencontainers/runc/libcontainer/configs"
)

// New returns the instance of a cgroup manager, which is chosen
// based on the local environment (whether cgroup v1 or v2 is used)
// and the config (whether config.Systemd is set or not).
func New(config *configs.Cgroup) (cgroups.Manager, error) {
return NewWithPaths(config, nil)
}

// NewWithPaths is similar to New, and can be used in case cgroup paths
// are already well known, which can save some resources.
//
// For cgroup v1, the keys are controller/subsystem name, and the values
// are absolute filesystem paths to the appropriate cgroups.
//
// For cgroup v2, the only key allowed is "" (empty string), and the value
// is the unified cgroup path.
func NewWithPaths(config *configs.Cgroup, paths map[string]string) (cgroups.Manager, error) {
if config == nil {
return nil, errors.New("cgroups/manager.New: config must not be nil")
}
if config.Systemd && !systemd.IsRunningSystemd() {
return nil, errors.New("systemd not running on this host, cannot use systemd cgroups manager")
}

// Cgroup v2 aka unified hierarchy.
if cgroups.IsCgroup2UnifiedMode() {
path, err := getUnifiedPath(paths)
if err != nil {
return nil, fmt.Errorf("manager.NewWithPaths: inconsistent paths: %w", err)
}
if config.Systemd {
return systemd.NewUnifiedManager(config, path)
}
return fs2.NewManager(config, path)
}

// Cgroup v1.
if config.Systemd {
return systemd.NewLegacyManager(config, paths)
}

return fs.NewManager(config, paths)
}

// getUnifiedPath is an implementation detail of libcontainer factory.
// Historically, it saves cgroup paths as per-subsystem path map (as returned
// by cm.GetPaths(""), but with v2 we only have one single unified path
// (with "" as a key).
//
// This function converts from that map to string (using "" as a key),
// and also checks that the map itself is sane.
func getUnifiedPath(paths map[string]string) (string, error) {
if len(paths) > 1 {
return "", fmt.Errorf("expected a single path, got %+v", paths)
}
path := paths[""]
// can be empty
if path != "" {
if filepath.Clean(path) != path || !filepath.IsAbs(path) {
return "", fmt.Errorf("invalid path: %q", path)
}
}

return path, nil
}
52 changes: 27 additions & 25 deletions libcontainer/cgroups/systemd/systemd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,23 @@ import (
"github.com/opencontainers/runc/libcontainer/devices"
)

func newManager(t *testing.T, config *configs.Cgroup) (m cgroups.Manager) {
t.Helper()
var err error

if cgroups.IsCgroup2UnifiedMode() {
m, err = NewUnifiedManager(config, "")
} else {
m, err = NewLegacyManager(config, nil)
}
if err != nil {
t.Fatal(err)
}
t.Cleanup(func() { _ = m.Destroy() })

return m
}

func TestSystemdVersion(t *testing.T) {
systemdVersionTests := []struct {
verStr string
Expand Down Expand Up @@ -57,13 +74,6 @@ func TestValidUnitTypes(t *testing.T) {
}
}

func newManager(config *configs.Cgroup) cgroups.Manager {
if cgroups.IsCgroup2UnifiedMode() {
return NewUnifiedManager(config, "", false)
}
return NewLegacyManager(config, nil)
}

// TestPodSkipDevicesUpdate checks that updating a pod having SkipDevices: true
// does not result in spurious "permission denied" errors in a container
// running under the pod. The test is somewhat similar in nature to the
Expand All @@ -79,17 +89,17 @@ func TestPodSkipDevicesUpdate(t *testing.T) {

podName := "system-runc_test_pod" + t.Name() + ".slice"
podConfig := &configs.Cgroup{
Parent: "system.slice",
Name: podName,
Systemd: true,
Parent: "system.slice",
Name: podName,
Resources: &configs.Resources{
PidsLimit: 42,
Memory: 32 * 1024 * 1024,
SkipDevices: true,
},
}
// Create "pod" cgroup (a systemd slice to hold containers).
pm := newManager(podConfig)
defer pm.Destroy() //nolint:errcheck
pm := newManager(t, podConfig)
if err := pm.Apply(-1); err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -132,9 +142,7 @@ func TestPodSkipDevicesUpdate(t *testing.T) {
}()

// Put the process into a cgroup.
cm := newManager(containerConfig)
defer cm.Destroy() //nolint:errcheck

cm := newManager(t, containerConfig)
if err := cm.Apply(cmd.Process.Pid); err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -184,8 +192,7 @@ func testSkipDevices(t *testing.T, skipDevices bool, expected []string) {
},
}
// Create "pods" cgroup (a systemd slice to hold containers).
pm := newManager(podConfig)
defer pm.Destroy() //nolint:errcheck
pm := newManager(t, podConfig)
if err := pm.Apply(-1); err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -236,9 +243,7 @@ func testSkipDevices(t *testing.T, skipDevices bool, expected []string) {
}()

// Put the process into a cgroup.
m := newManager(config)
defer m.Destroy() //nolint:errcheck

m := newManager(t, config)
if err := m.Apply(cmd.Process.Pid); err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -305,8 +310,7 @@ func TestUnitExistsIgnored(t *testing.T) {
Resources: &configs.Resources{},
}
// Create "pods" cgroup (a systemd slice to hold containers).
pm := newManager(podConfig)
defer pm.Destroy() //nolint:errcheck
pm := newManager(t, podConfig)

// create twice to make sure "UnitExists" error is ignored.
for i := 0; i < 2; i++ {
Expand Down Expand Up @@ -334,8 +338,7 @@ func TestFreezePodCgroup(t *testing.T) {
}
// Create a "pod" cgroup (a systemd slice to hold containers),
// which is frozen initially.
pm := newManager(podConfig)
defer pm.Destroy() //nolint:errcheck
pm := newManager(t, podConfig)
if err := pm.Apply(-1); err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -402,8 +405,7 @@ func TestFreezePodCgroup(t *testing.T) {
}()

// Put the process into a cgroup.
cm := newManager(containerConfig)
defer cm.Destroy() //nolint:errcheck
cm := newManager(t, containerConfig)

if err := cm.Apply(cmd.Process.Pid); err != nil {
t.Fatal(err)
Expand Down
7 changes: 5 additions & 2 deletions libcontainer/cgroups/systemd/v1.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,15 @@ type legacyManager struct {
dbus *dbusConnManager
}

func NewLegacyManager(cg *configs.Cgroup, paths map[string]string) cgroups.Manager {
func NewLegacyManager(cg *configs.Cgroup, paths map[string]string) (cgroups.Manager, error) {
if cg.Rootless {
return nil, errors.New("cannot use rootless systemd cgroups manager on cgroup v1")
}
return &legacyManager{
cgroups: cg,
paths: paths,
dbus: newDbusConnManager(false),
}
}, nil
}

type subsystem interface {
Expand Down
Loading

0 comments on commit 097c6d7

Please sign in to comment.