From 5efa14e3dbc5c34d93de6c706af24fea4a30de46 Mon Sep 17 00:00:00 2001 From: bpopovschi Date: Tue, 19 Nov 2019 00:14:40 +0200 Subject: [PATCH 1/7] Added systemd manager Signed-off-by: bpopovschi --- v2/manager.go | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++ v2/utils.go | 23 ++++++++++ 2 files changed, 148 insertions(+) diff --git a/v2/manager.go b/v2/manager.go index 85f7d32e..16e14482 100644 --- a/v2/manager.go +++ b/v2/manager.go @@ -25,6 +25,7 @@ import ( "path/filepath" "strconv" "strings" + "sync" "syscall" "time" @@ -33,12 +34,23 @@ import ( "golang.org/x/sys/unix" "github.com/containerd/cgroups/v2/stats" + "github.com/godbus/dbus" + "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" + "github.com/sirupsen/logrus" + + systemdDbus "github.com/coreos/go-systemd/dbus" ) const ( subtreeControl = "cgroup.subtree_control" controllersFile = "cgroup.controllers" + defaultSlice = "system.slice" +) + +var ( + canDelegate bool + once sync.Once ) type cgValuer interface { @@ -583,3 +595,116 @@ func setDevices(path string, devices []specs.LinuxDeviceCgroup) error { } return nil } + +func NewSystemd(path string, resources *specs.LinuxResources) (*Manager, error) { + conn, err := systemdDbus.New() + if err != nil { + return &Manager{}, err + } + defer conn.Close() + slice, name := splitName(path) + + // We need to see if systemd can handle the delegate property + // Systemd will return an error if it cannot handle delegate regardless + // of its bool setting. + checkDelegate := func() { + canDelegate = true + dlSlice := newSystemdProperty("Delegate", true) + if _, err := conn.StartTransientUnit(slice, "testdelegate", []systemdDbus.Property{dlSlice}, nil); err != nil { + if dbusError, ok := err.(dbus.Error); ok { + // Starting with systemd v237, Delegate is not even a property of slices anymore, + // so the D-Bus call fails with "InvalidArgs" error. + if strings.Contains(dbusError.Name, "org.freedesktop.DBus.Error.PropertyReadOnly") || strings.Contains(dbusError.Name, "org.freedesktop.DBus.Error.InvalidArgs") { + canDelegate = false + } + } + } + + conn.StopUnit(slice, "testDelegate", nil) + } + once.Do(checkDelegate) + + properties := []systemdDbus.Property{ + systemdDbus.PropDescription(fmt.Sprintf("cgroup %s", name)), + systemdDbus.PropWants(slice), + newSystemdProperty("DefaultDependencies", false), + newSystemdProperty("MemoryAccounting", true), + newSystemdProperty("CPUAccounting", true), + newSystemdProperty("BlockIOAccounting", true), + } + + // cpu.cfs_quota_us and cpu.cfs_period_us are controlled by systemd. + if *resources.CPU.Quota != 0 && *resources.CPU.Period != 0 { + // corresponds to USEC_INFINITY in systemd + // if USEC_INFINITY is provided, CPUQuota is left unbound by systemd + // always setting a property value ensures we can apply a quota and remove it later + cpuQuotaPerSecUSec := uint64(math.MaxUint64) + if *resources.CPU.Quota > 0 { + // systemd converts CPUQuotaPerSecUSec (microseconds per CPU second) to CPUQuota + // (integer percentage of CPU) internally. This means that if a fractional percent of + // CPU is indicated by Resources.CpuQuota, we need to round up to the nearest + // 10ms (1% of a second) such that child cgroups can set the cpu.cfs_quota_us they expect. + cpuQuotaPerSecUSec = uint64(*resources.CPU.Quota*1000000) / *resources.CPU.Period + if cpuQuotaPerSecUSec%10000 != 0 { + cpuQuotaPerSecUSec = ((cpuQuotaPerSecUSec / 10000) + 1) * 10000 + } + } + properties = append(properties, + newSystemdProperty("CPUQuotaPerSecUSec", cpuQuotaPerSecUSec)) + } + + // If we can delegate, we add the property back in + if canDelegate { + properties = append(properties, newSystemdProperty("Delegate", true)) + } + + if resources.Pids.Limit > 0 { + properties = append(properties, + newSystemdProperty("TasksAccounting", true), + newSystemdProperty("TasksMax", uint64(resources.Pids.Limit))) + } + + statusChan := make(chan string, 1) + if _, err := conn.StartTransientUnit(name, "replace", properties, statusChan); err == nil { + select { + case <-statusChan: + case <-time.After(time.Second): + logrus.Warnf("Timed out while waiting for StartTransientUnit(%s) completion signal from dbus. Continuing...", name) + } + } else if !isUnitExists(err) { + return &Manager{}, err + } + + return &Manager{ + path: path, + }, nil +} + +func LoadSystemd(path string) (*Manager, error) { + return &Manager{ + path: path, + }, nil +} + +func (c *Manager) DeleteSystemd() error { + conn, err := systemdDbus.New() + if err != nil { + return err + } + defer conn.Close() + _, name := splitName(c.path) + ch := make(chan string) + _, err = conn.StopUnit(name, "replace", ch) + if err != nil { + return err + } + <-ch + return nil +} + +func newSystemdProperty(name string, units interface{}) systemdDbus.Property { + return systemdDbus.Property{ + Name: name, + Value: dbus.MakeVariant(units), + } +} diff --git a/v2/utils.go b/v2/utils.go index a673211d..596ad40a 100644 --- a/v2/utils.go +++ b/v2/utils.go @@ -19,6 +19,7 @@ package v2 import ( "bufio" "fmt" + "github.com/godbus/dbus" "io" "io/ioutil" "math" @@ -361,3 +362,25 @@ func toRdmaEntry(strEntries []string) []*stats.RdmaEntry { } return rdmaEntries } + +func splitName(path string) (slice string, unit string) { + slice, unit = filepath.Split(path) + return strings.TrimSuffix(slice, "/"), unit +} + +func Slice(slice, name string) string { + if slice == "" { + slice = defaultSlice + } + return filepath.Join(slice, name) +} + +// isUnitExists returns true if the error is that a systemd unit already exists. +func isUnitExists(err error) bool { + if err != nil { + if dbusError, ok := err.(dbus.Error); ok { + return strings.Contains(dbusError.Name, "org.freedesktop.systemd1.UnitExists") + } + } + return false +} From 3a32c65f78c12eb7934c4d4ceda4729f2970c147 Mon Sep 17 00:00:00 2001 From: bpopovschi Date: Tue, 10 Dec 2019 16:36:34 +0200 Subject: [PATCH 2/7] Systemd refactoring Signed-off-by: bpopovschi --- cmd/cgctl/main.go | 33 +++++++++++++++++++++++++++ v2/manager.go | 46 +++++++++++++++++++++++++++++-------- v2/utils.go | 58 +++++++++++++++++++++++++++++++++++++++-------- 3 files changed, 119 insertions(+), 18 deletions(-) diff --git a/cmd/cgctl/main.go b/cmd/cgctl/main.go index 5e102349..93779975 100644 --- a/cmd/cgctl/main.go +++ b/cmd/cgctl/main.go @@ -23,6 +23,7 @@ import ( "github.com/containerd/cgroups" v2 "github.com/containerd/cgroups/v2" + "github.com/opencontainers/runtime-spec/specs-go" "github.com/sirupsen/logrus" "github.com/urfave/cli" ) @@ -50,6 +51,8 @@ func main() { listCommand, listControllersCommand, statCommand, + newSystemdCommand, + deleteSystemdCommand, } app.Before = func(clix *cli.Context) error { if clix.GlobalBool("debug") { @@ -161,6 +164,36 @@ var statCommand = cli.Command{ }, } +var newSystemdCommand = cli.Command{ + Name: "systemd", + Usage: "create a new systemd managed cgroup", + Action: func(clix *cli.Context) error { + path := clix.Args().First() + _, err := v2.NewSystemd(path, os.Getpid(), &specs.LinuxResources{}) + if err != nil { + return err + } + return nil + }, +} + +var deleteSystemdCommand = cli.Command{ + Name: "del-systemd", + Usage: "delete a systemd managed cgroup", + Action: func(clix *cli.Context) error { + path := clix.Args().First() + m, err := v2.LoadSystemd(path) + if err != nil { + return err + } + err = m.DeleteSystemd() + if err != nil { + return err + } + return nil + }, +} + var modeCommand = cli.Command{ Name: "mode", Usage: "return the cgroup mode that is mounted on the system", diff --git a/v2/manager.go b/v2/manager.go index 16e14482..6715b13f 100644 --- a/v2/manager.go +++ b/v2/manager.go @@ -43,9 +43,9 @@ import ( ) const ( - subtreeControl = "cgroup.subtree_control" - controllersFile = "cgroup.controllers" - defaultSlice = "system.slice" + subtreeControl = "cgroup.subtree_control" + controllersFile = "cgroup.controllers" + defaultCgroup2Path = "/sys/fs/cgroup" ) var ( @@ -596,13 +596,17 @@ func setDevices(path string, devices []specs.LinuxDeviceCgroup) error { return nil } -func NewSystemd(path string, resources *specs.LinuxResources) (*Manager, error) { +func NewSystemd(group string, pid int, resources *specs.LinuxResources) (*Manager, error) { + if err := VerifyGroupPath(group); err != nil { + return nil, err + } + path := filepath.Join(defaultCgroup2Path, group) conn, err := systemdDbus.New() if err != nil { return &Manager{}, err } defer conn.Close() - slice, name := splitName(path) + slice, name := splitName(group) // We need to see if systemd can handle the delegate property // Systemd will return an error if it cannot handle delegate regardless @@ -630,11 +634,26 @@ func NewSystemd(path string, resources *specs.LinuxResources) (*Manager, error) newSystemdProperty("DefaultDependencies", false), newSystemdProperty("MemoryAccounting", true), newSystemdProperty("CPUAccounting", true), - newSystemdProperty("BlockIOAccounting", true), + newSystemdProperty("IOAccounting", true), + } + + // only add pid if its valid, -1 is used w/ general slice creation. + if pid != -1 { + properties = append(properties, newSystemdProperty("PIDs", []uint32{uint32(pid)})) + } + + if resources.Memory != nil && *resources.Memory.Limit != 0 { + properties = append(properties, + newSystemdProperty("MemoryMax", uint64(*resources.Memory.Limit))) + } + + if resources.CPU != nil && *resources.CPU.Shares != 0 { + properties = append(properties, + newSystemdProperty("CPUWeight", *resources.CPU.Shares)) } // cpu.cfs_quota_us and cpu.cfs_period_us are controlled by systemd. - if *resources.CPU.Quota != 0 && *resources.CPU.Period != 0 { + if resources.CPU != nil && *resources.CPU.Quota != 0 && *resources.CPU.Period != 0 { // corresponds to USEC_INFINITY in systemd // if USEC_INFINITY is provided, CPUQuota is left unbound by systemd // always setting a property value ensures we can apply a quota and remove it later @@ -658,7 +677,7 @@ func NewSystemd(path string, resources *specs.LinuxResources) (*Manager, error) properties = append(properties, newSystemdProperty("Delegate", true)) } - if resources.Pids.Limit > 0 { + if resources.Pids != nil && resources.Pids.Limit > 0 { properties = append(properties, newSystemdProperty("TasksAccounting", true), newSystemdProperty("TasksMax", uint64(resources.Pids.Limit))) @@ -675,12 +694,21 @@ func NewSystemd(path string, resources *specs.LinuxResources) (*Manager, error) return &Manager{}, err } + err = createCgroupsv2Path(path) + if err != nil { + return &Manager{}, err + } + return &Manager{ path: path, }, nil } -func LoadSystemd(path string) (*Manager, error) { +func LoadSystemd(group string) (*Manager, error) { + if err := VerifyGroupPath(group); err != nil { + return nil, err + } + path := filepath.Join(defaultCgroup2Path, group) return &Manager{ path: path, }, nil diff --git a/v2/utils.go b/v2/utils.go index 596ad40a..0396f61b 100644 --- a/v2/utils.go +++ b/v2/utils.go @@ -19,7 +19,6 @@ package v2 import ( "bufio" "fmt" - "github.com/godbus/dbus" "io" "io/ioutil" "math" @@ -29,6 +28,8 @@ import ( "strings" "time" + "github.com/godbus/dbus" + "github.com/containerd/cgroups/v2/stats" "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" @@ -365,14 +366,7 @@ func toRdmaEntry(strEntries []string) []*stats.RdmaEntry { func splitName(path string) (slice string, unit string) { slice, unit = filepath.Split(path) - return strings.TrimSuffix(slice, "/"), unit -} - -func Slice(slice, name string) string { - if slice == "" { - slice = defaultSlice - } - return filepath.Join(slice, name) + return strings.TrimPrefix(strings.TrimSuffix(slice, "/"), "/"), unit } // isUnitExists returns true if the error is that a systemd unit already exists. @@ -384,3 +378,49 @@ func isUnitExists(err error) bool { } return false } + +func createCgroupsv2Path(path string) (Err error) { + content, err := ioutil.ReadFile("/sys/fs/cgroup/cgroup.controllers") + if err != nil { + return err + } + if !filepath.HasPrefix(path, "/sys/fs/cgroup") { + return fmt.Errorf("invalid cgroup path %s", path) + } + + res := "" + for i, c := range strings.Split(strings.TrimSpace(string(content)), " ") { + if i == 0 { + res = fmt.Sprintf("+%s", c) + } else { + res = res + fmt.Sprintf(" +%s", c) + } + } + resByte := []byte(res) + + current := "/sys/fs" + elements := strings.Split(path, "/") + for i, e := range elements[3:] { + current = filepath.Join(current, e) + if i > 0 { + if err := os.Mkdir(current, defaultDirPerm); err != nil { + if !os.IsExist(err) { + return err + } + } else { + // If the directory was created, be sure it is not left around on errors. + defer func() { + if Err != nil { + os.Remove(current) + } + }() + } + } + if i < len(elements[3:])-1 { + if err := ioutil.WriteFile(filepath.Join(current, "cgroup.subtree_control"), resByte, defaultDirPerm); err != nil { + return err + } + } + } + return nil +} From b0a15b1d1682d7555d4793bc2122c8f9b162fb4f Mon Sep 17 00:00:00 2001 From: bpopovschi Date: Tue, 10 Dec 2019 17:20:17 +0200 Subject: [PATCH 3/7] conflict fix Signed-off-by: bpopovschi --- v2/testutils_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/v2/testutils_test.go b/v2/testutils_test.go index 60147682..ae26b45c 100644 --- a/v2/testutils_test.go +++ b/v2/testutils_test.go @@ -28,8 +28,6 @@ import ( "golang.org/x/sys/unix" ) -const defaultCgroup2Path = "/sys/fs/cgroup" - func checkCgroupMode(t *testing.T) { var st syscall.Statfs_t if err := syscall.Statfs(defaultCgroup2Path, &st); err != nil { From 7b4fbc7a0b3877704bdc4b82bc4ded8bc42f18e7 Mon Sep 17 00:00:00 2001 From: bpopovschi Date: Wed, 11 Dec 2019 12:18:15 +0200 Subject: [PATCH 4/7] Toggle controllers fix Signed-off-by: bpopovschi --- cmd/cgctl/main.go | 9 ++++++++- v2/manager.go | 8 ++------ v2/utils.go | 46 ---------------------------------------------- 3 files changed, 10 insertions(+), 53 deletions(-) diff --git a/cmd/cgctl/main.go b/cmd/cgctl/main.go index 93779975..16a55545 100644 --- a/cmd/cgctl/main.go +++ b/cmd/cgctl/main.go @@ -169,10 +169,17 @@ var newSystemdCommand = cli.Command{ Usage: "create a new systemd managed cgroup", Action: func(clix *cli.Context) error { path := clix.Args().First() - _, err := v2.NewSystemd(path, os.Getpid(), &specs.LinuxResources{}) + c, err := v2.NewSystemd(path, os.Getpid(), &specs.LinuxResources{}) if err != nil { return err } + controllers, err := c.RootControllers() + if err != nil { + return err + } + if err := c.ToggleControllers(controllers, v2.Enable); err != nil { + return err + } return nil }, } diff --git a/v2/manager.go b/v2/manager.go index 6715b13f..ee01b472 100644 --- a/v2/manager.go +++ b/v2/manager.go @@ -648,8 +648,9 @@ func NewSystemd(group string, pid int, resources *specs.LinuxResources) (*Manage } if resources.CPU != nil && *resources.CPU.Shares != 0 { + convertedWeight := (1 + ((*resources.CPU.Shares-2)*9999)/262142) properties = append(properties, - newSystemdProperty("CPUWeight", *resources.CPU.Shares)) + newSystemdProperty("CPUWeight", &convertedWeight)) } // cpu.cfs_quota_us and cpu.cfs_period_us are controlled by systemd. @@ -694,11 +695,6 @@ func NewSystemd(group string, pid int, resources *specs.LinuxResources) (*Manage return &Manager{}, err } - err = createCgroupsv2Path(path) - if err != nil { - return &Manager{}, err - } - return &Manager{ path: path, }, nil diff --git a/v2/utils.go b/v2/utils.go index 0396f61b..315838e9 100644 --- a/v2/utils.go +++ b/v2/utils.go @@ -378,49 +378,3 @@ func isUnitExists(err error) bool { } return false } - -func createCgroupsv2Path(path string) (Err error) { - content, err := ioutil.ReadFile("/sys/fs/cgroup/cgroup.controllers") - if err != nil { - return err - } - if !filepath.HasPrefix(path, "/sys/fs/cgroup") { - return fmt.Errorf("invalid cgroup path %s", path) - } - - res := "" - for i, c := range strings.Split(strings.TrimSpace(string(content)), " ") { - if i == 0 { - res = fmt.Sprintf("+%s", c) - } else { - res = res + fmt.Sprintf(" +%s", c) - } - } - resByte := []byte(res) - - current := "/sys/fs" - elements := strings.Split(path, "/") - for i, e := range elements[3:] { - current = filepath.Join(current, e) - if i > 0 { - if err := os.Mkdir(current, defaultDirPerm); err != nil { - if !os.IsExist(err) { - return err - } - } else { - // If the directory was created, be sure it is not left around on errors. - defer func() { - if Err != nil { - os.Remove(current) - } - }() - } - } - if i < len(elements[3:])-1 { - if err := ioutil.WriteFile(filepath.Join(current, "cgroup.subtree_control"), resByte, defaultDirPerm); err != nil { - return err - } - } - } - return nil -} From fe689749600616370376829ff953d126d2d9abba Mon Sep 17 00:00:00 2001 From: Boris Popovschi Date: Thu, 19 Dec 2019 14:53:19 +0200 Subject: [PATCH 5/7] tests for systemd controllers Signed-off-by: Boris Popovschi --- cmd/cgctl/main.go | 16 ++++++------ v2/cpuv2_test.go | 17 ++++++++++++ v2/manager.go | 63 +++++++++++++++++---------------------------- v2/memoryv2_test.go | 15 +++++++++++ v2/utils.go | 11 ++++---- 5 files changed, 69 insertions(+), 53 deletions(-) diff --git a/cmd/cgctl/main.go b/cmd/cgctl/main.go index 16a55545..5d0b37d3 100644 --- a/cmd/cgctl/main.go +++ b/cmd/cgctl/main.go @@ -20,6 +20,7 @@ import ( "encoding/json" "fmt" "os" + "strconv" "github.com/containerd/cgroups" v2 "github.com/containerd/cgroups/v2" @@ -169,17 +170,16 @@ var newSystemdCommand = cli.Command{ Usage: "create a new systemd managed cgroup", Action: func(clix *cli.Context) error { path := clix.Args().First() - c, err := v2.NewSystemd(path, os.Getpid(), &specs.LinuxResources{}) - if err != nil { - return err + pidStr := clix.Args().Get(1) + pid := os.Getpid() + if pidStr != "" { + pid, _ = strconv.Atoi(pidStr) } - controllers, err := c.RootControllers() + + _, err := v2.NewSystemd("", path, pid, &specs.LinuxResources{}) if err != nil { return err } - if err := c.ToggleControllers(controllers, v2.Enable); err != nil { - return err - } return nil }, } @@ -189,7 +189,7 @@ var deleteSystemdCommand = cli.Command{ Usage: "delete a systemd managed cgroup", Action: func(clix *cli.Context) error { path := clix.Args().First() - m, err := v2.LoadSystemd(path) + m, err := v2.LoadSystemd("", path) if err != nil { return err } diff --git a/v2/cpuv2_test.go b/v2/cpuv2_test.go index f72ef2c8..5446184c 100644 --- a/v2/cpuv2_test.go +++ b/v2/cpuv2_test.go @@ -21,6 +21,8 @@ import ( "os" "strconv" "testing" + + "github.com/opencontainers/runtime-spec/specs-go" ) func TestCgroupv2CpuStats(t *testing.T) { @@ -52,3 +54,18 @@ func TestCgroupv2CpuStats(t *testing.T) { checkFileContent(t, c.path, "cpuset.cpus", "0") checkFileContent(t, c.path, "cpuset.mems", "0") } + +func TestSystemdCgroupCpuController(t *testing.T) { + checkCgroupMode(t) + group := fmt.Sprintf("testing-cpu-%d.scope", os.Getpid()) + var shares uint64 = 100 + res := specs.LinuxResources{ + CPU: &specs.LinuxCPU{Shares: &shares}, + } + c, err := NewSystemd("", group, os.Getpid(), &res) + if err != nil { + t.Fatal("failed to init new cgroup systemd manager: ", err) + } + convertedWeight := (1 + ((shares-2)*9999)/262142) + checkFileContent(t, c.path, "cpu.weight", strconv.FormatUint(convertedWeight, 10)) +} diff --git a/v2/manager.go b/v2/manager.go index ee01b472..1129c665 100644 --- a/v2/manager.go +++ b/v2/manager.go @@ -29,8 +29,6 @@ import ( "syscall" "time" - "github.com/opencontainers/runtime-spec/specs-go" - "golang.org/x/sys/unix" "github.com/containerd/cgroups/v2/stats" @@ -46,6 +44,7 @@ const ( subtreeControl = "cgroup.subtree_control" controllersFile = "cgroup.controllers" defaultCgroup2Path = "/sys/fs/cgroup" + defaultSlice = "system.slice" ) var ( @@ -596,47 +595,33 @@ func setDevices(path string, devices []specs.LinuxDeviceCgroup) error { return nil } -func NewSystemd(group string, pid int, resources *specs.LinuxResources) (*Manager, error) { - if err := VerifyGroupPath(group); err != nil { - return nil, err +func NewSystemd(slice, group string, pid int, resources *specs.LinuxResources) (*Manager, error) { + if slice == "" { + slice = defaultSlice } - path := filepath.Join(defaultCgroup2Path, group) + path := filepath.Join(defaultCgroup2Path, slice, group) conn, err := systemdDbus.New() if err != nil { return &Manager{}, err } defer conn.Close() - slice, name := splitName(group) - - // We need to see if systemd can handle the delegate property - // Systemd will return an error if it cannot handle delegate regardless - // of its bool setting. - checkDelegate := func() { - canDelegate = true - dlSlice := newSystemdProperty("Delegate", true) - if _, err := conn.StartTransientUnit(slice, "testdelegate", []systemdDbus.Property{dlSlice}, nil); err != nil { - if dbusError, ok := err.(dbus.Error); ok { - // Starting with systemd v237, Delegate is not even a property of slices anymore, - // so the D-Bus call fails with "InvalidArgs" error. - if strings.Contains(dbusError.Name, "org.freedesktop.DBus.Error.PropertyReadOnly") || strings.Contains(dbusError.Name, "org.freedesktop.DBus.Error.InvalidArgs") { - canDelegate = false - } - } - } - - conn.StopUnit(slice, "testDelegate", nil) - } - once.Do(checkDelegate) properties := []systemdDbus.Property{ - systemdDbus.PropDescription(fmt.Sprintf("cgroup %s", name)), - systemdDbus.PropWants(slice), + systemdDbus.PropDescription(fmt.Sprintf("cgroup %s", group)), newSystemdProperty("DefaultDependencies", false), newSystemdProperty("MemoryAccounting", true), newSystemdProperty("CPUAccounting", true), newSystemdProperty("IOAccounting", true), } + // if we create a slice, the parent is defined via a Wants= + if strings.HasSuffix(group, ".slice") { + properties = append(properties, systemdDbus.PropWants(defaultSlice)) + } else { + // otherwise, we use Slice= + properties = append(properties, systemdDbus.PropSlice(defaultSlice)) + } + // only add pid if its valid, -1 is used w/ general slice creation. if pid != -1 { properties = append(properties, newSystemdProperty("PIDs", []uint32{uint32(pid)})) @@ -654,7 +639,7 @@ func NewSystemd(group string, pid int, resources *specs.LinuxResources) (*Manage } // cpu.cfs_quota_us and cpu.cfs_period_us are controlled by systemd. - if resources.CPU != nil && *resources.CPU.Quota != 0 && *resources.CPU.Period != 0 { + if resources.CPU != nil && resources.CPU.Quota != nil && resources.CPU.Period != nil { // corresponds to USEC_INFINITY in systemd // if USEC_INFINITY is provided, CPUQuota is left unbound by systemd // always setting a property value ensures we can apply a quota and remove it later @@ -685,11 +670,11 @@ func NewSystemd(group string, pid int, resources *specs.LinuxResources) (*Manage } statusChan := make(chan string, 1) - if _, err := conn.StartTransientUnit(name, "replace", properties, statusChan); err == nil { + if _, err := conn.StartTransientUnit(group, "replace", properties, statusChan); err == nil { select { case <-statusChan: case <-time.After(time.Second): - logrus.Warnf("Timed out while waiting for StartTransientUnit(%s) completion signal from dbus. Continuing...", name) + logrus.Warnf("Timed out while waiting for StartTransientUnit(%s) completion signal from dbus. Continuing...", group) } } else if !isUnitExists(err) { return &Manager{}, err @@ -700,13 +685,13 @@ func NewSystemd(group string, pid int, resources *specs.LinuxResources) (*Manage }, nil } -func LoadSystemd(group string) (*Manager, error) { - if err := VerifyGroupPath(group); err != nil { - return nil, err +func LoadSystemd(slice, group string) (*Manager, error) { + if slice == "" { + slice = defaultSlice } - path := filepath.Join(defaultCgroup2Path, group) + group = filepath.Join(defaultCgroup2Path, slice, group) return &Manager{ - path: path, + path: group, }, nil } @@ -716,9 +701,9 @@ func (c *Manager) DeleteSystemd() error { return err } defer conn.Close() - _, name := splitName(c.path) + group := systemdUnitFromPath(c.path) ch := make(chan string) - _, err = conn.StopUnit(name, "replace", ch) + _, err = conn.StopUnit(group, "replace", ch) if err != nil { return err } diff --git a/v2/memoryv2_test.go b/v2/memoryv2_test.go index 56ba8454..b4eb72c8 100644 --- a/v2/memoryv2_test.go +++ b/v2/memoryv2_test.go @@ -21,6 +21,8 @@ import ( "os" "testing" + "github.com/opencontainers/runtime-spec/specs-go" + "github.com/stretchr/testify/assert" ) @@ -50,3 +52,16 @@ func TestCgroupv2MemoryStats(t *testing.T) { checkFileContent(t, c.path, "memory.swap.max", "314572800") checkFileContent(t, c.path, "memory.max", "629145600") } + +func TestSystemdCgroupMemoryController(t *testing.T) { + checkCgroupMode(t) + group := fmt.Sprintf("testing-memory-%d.scope", os.Getpid()) + res := specs.LinuxResources{ + Memory: &specs.LinuxMemory{Limit: pointerInt64(629145600)}, + } + c, err := NewSystemd("", group, os.Getpid(), &res) + if err != nil { + t.Fatal("failed to init new cgroup systemd manager: ", err) + } + checkFileContent(t, c.path, "memory.max", "629145600") +} diff --git a/v2/utils.go b/v2/utils.go index 315838e9..0594d6b2 100644 --- a/v2/utils.go +++ b/v2/utils.go @@ -240,7 +240,6 @@ func ToResources(spec *specs.LinuxResources) *Resources { func getStatFileContentUint64(filePath string) uint64 { contents, err := ioutil.ReadFile(filePath) if err != nil { - logrus.Error(err) return 0 } trimmed := strings.TrimSpace(string(contents)) @@ -364,11 +363,6 @@ func toRdmaEntry(strEntries []string) []*stats.RdmaEntry { return rdmaEntries } -func splitName(path string) (slice string, unit string) { - slice, unit = filepath.Split(path) - return strings.TrimPrefix(strings.TrimSuffix(slice, "/"), "/"), unit -} - // isUnitExists returns true if the error is that a systemd unit already exists. func isUnitExists(err error) bool { if err != nil { @@ -378,3 +372,8 @@ func isUnitExists(err error) bool { } return false } + +func systemdUnitFromPath(path string) string { + _, unit := filepath.Split(path) + return unit +} From 7d585c40f10b87650d114877dbcec2b72a852357 Mon Sep 17 00:00:00 2001 From: Boris Popovschi Date: Tue, 24 Dec 2019 14:12:31 +0200 Subject: [PATCH 6/7] Resource rework + path fix Signed-off-by: Boris Popovschi --- cmd/cgctl/main.go | 3 +-- v2/cpu.go | 16 ++++++++++++++++ v2/cpuv2_test.go | 33 ++++++++++++++++++++++++++------- v2/manager.go | 24 ++++++++++++------------ v2/memoryv2_test.go | 8 ++++---- 5 files changed, 59 insertions(+), 25 deletions(-) diff --git a/cmd/cgctl/main.go b/cmd/cgctl/main.go index 5d0b37d3..67243703 100644 --- a/cmd/cgctl/main.go +++ b/cmd/cgctl/main.go @@ -24,7 +24,6 @@ import ( "github.com/containerd/cgroups" v2 "github.com/containerd/cgroups/v2" - "github.com/opencontainers/runtime-spec/specs-go" "github.com/sirupsen/logrus" "github.com/urfave/cli" ) @@ -176,7 +175,7 @@ var newSystemdCommand = cli.Command{ pid, _ = strconv.Atoi(pidStr) } - _, err := v2.NewSystemd("", path, pid, &specs.LinuxResources{}) + _, err := v2.NewSystemd("", path, pid, &v2.Resources{}) if err != nil { return err } diff --git a/v2/cpu.go b/v2/cpu.go index 648a93d3..65282ff0 100644 --- a/v2/cpu.go +++ b/v2/cpu.go @@ -17,6 +17,7 @@ package v2 import ( + "math" "strconv" "strings" ) @@ -38,6 +39,21 @@ type CPU struct { Mems string } +func (c CPUMax) extractQuotaAndPeriod() (int64, uint64) { + var ( + quota int64 + period uint64 + ) + values := strings.Split(string(c), " ") + if values[0] == "max" { + quota = math.MaxInt64 + } else { + quota, _ = strconv.ParseInt(values[0], 10, 64) + } + period, _ = strconv.ParseUint(values[1], 10, 64) + return quota, period +} + func (r *CPU) Values() (o []Value) { if r.Weight != nil { o = append(o, Value{ diff --git a/v2/cpuv2_test.go b/v2/cpuv2_test.go index 5446184c..e5ace2f3 100644 --- a/v2/cpuv2_test.go +++ b/v2/cpuv2_test.go @@ -18,11 +18,12 @@ package v2 import ( "fmt" + "math" "os" "strconv" "testing" - "github.com/opencontainers/runtime-spec/specs-go" + "github.com/stretchr/testify/assert" ) func TestCgroupv2CpuStats(t *testing.T) { @@ -58,14 +59,32 @@ func TestCgroupv2CpuStats(t *testing.T) { func TestSystemdCgroupCpuController(t *testing.T) { checkCgroupMode(t) group := fmt.Sprintf("testing-cpu-%d.scope", os.Getpid()) - var shares uint64 = 100 - res := specs.LinuxResources{ - CPU: &specs.LinuxCPU{Shares: &shares}, - } + var weight uint64 = 100 + res := Resources{CPU: &CPU{Weight: &weight}} c, err := NewSystemd("", group, os.Getpid(), &res) if err != nil { t.Fatal("failed to init new cgroup systemd manager: ", err) } - convertedWeight := (1 + ((shares-2)*9999)/262142) - checkFileContent(t, c.path, "cpu.weight", strconv.FormatUint(convertedWeight, 10)) + checkFileContent(t, c.path, "cpu.weight", strconv.FormatUint(weight, 10)) +} + +func TestExtractQuotaAndPeriod(t *testing.T) { + var ( + period uint64 + quota int64 + ) + quota = 10000 + period = 8000 + cpuMax := NewCPUMax("a, &period) + tquota, tPeriod := cpuMax.extractQuotaAndPeriod() + + assert.Equal(t, quota, tquota) + assert.Equal(t, period, tPeriod) + + //case with nil quota which makes it "max" - max int val + cpuMax2 := NewCPUMax(nil, &period) + tquota2, tPeriod2 := cpuMax2.extractQuotaAndPeriod() + + assert.Equal(t, int64(math.MaxInt64), tquota2) + assert.Equal(t, period, tPeriod2) } diff --git a/v2/manager.go b/v2/manager.go index 1129c665..01f84fa6 100644 --- a/v2/manager.go +++ b/v2/manager.go @@ -595,7 +595,7 @@ func setDevices(path string, devices []specs.LinuxDeviceCgroup) error { return nil } -func NewSystemd(slice, group string, pid int, resources *specs.LinuxResources) (*Manager, error) { +func NewSystemd(slice, group string, pid int, resources *Resources) (*Manager, error) { if slice == "" { slice = defaultSlice } @@ -627,29 +627,29 @@ func NewSystemd(slice, group string, pid int, resources *specs.LinuxResources) ( properties = append(properties, newSystemdProperty("PIDs", []uint32{uint32(pid)})) } - if resources.Memory != nil && *resources.Memory.Limit != 0 { + if resources.Memory != nil && *resources.Memory.Max != 0 { properties = append(properties, - newSystemdProperty("MemoryMax", uint64(*resources.Memory.Limit))) + newSystemdProperty("MemoryMax", uint64(*resources.Memory.Max))) } - if resources.CPU != nil && *resources.CPU.Shares != 0 { - convertedWeight := (1 + ((*resources.CPU.Shares-2)*9999)/262142) + if resources.CPU != nil && *resources.CPU.Weight != 0 { properties = append(properties, - newSystemdProperty("CPUWeight", &convertedWeight)) + newSystemdProperty("CPUWeight", *resources.CPU.Weight)) } - // cpu.cfs_quota_us and cpu.cfs_period_us are controlled by systemd. - if resources.CPU != nil && resources.CPU.Quota != nil && resources.CPU.Period != nil { + if resources.CPU != nil && resources.CPU.Max != "" { + quota, period := resources.CPU.Max.extractQuotaAndPeriod() + // cpu.cfs_quota_us and cpu.cfs_period_us are controlled by systemd. // corresponds to USEC_INFINITY in systemd // if USEC_INFINITY is provided, CPUQuota is left unbound by systemd // always setting a property value ensures we can apply a quota and remove it later cpuQuotaPerSecUSec := uint64(math.MaxUint64) - if *resources.CPU.Quota > 0 { + if quota > 0 { // systemd converts CPUQuotaPerSecUSec (microseconds per CPU second) to CPUQuota // (integer percentage of CPU) internally. This means that if a fractional percent of // CPU is indicated by Resources.CpuQuota, we need to round up to the nearest // 10ms (1% of a second) such that child cgroups can set the cpu.cfs_quota_us they expect. - cpuQuotaPerSecUSec = uint64(*resources.CPU.Quota*1000000) / *resources.CPU.Period + cpuQuotaPerSecUSec = uint64(quota*1000000) / period if cpuQuotaPerSecUSec%10000 != 0 { cpuQuotaPerSecUSec = ((cpuQuotaPerSecUSec / 10000) + 1) * 10000 } @@ -663,10 +663,10 @@ func NewSystemd(slice, group string, pid int, resources *specs.LinuxResources) ( properties = append(properties, newSystemdProperty("Delegate", true)) } - if resources.Pids != nil && resources.Pids.Limit > 0 { + if resources.Pids != nil && resources.Pids.Max > 0 { properties = append(properties, newSystemdProperty("TasksAccounting", true), - newSystemdProperty("TasksMax", uint64(resources.Pids.Limit))) + newSystemdProperty("TasksMax", uint64(resources.Pids.Max))) } statusChan := make(chan string, 1) diff --git a/v2/memoryv2_test.go b/v2/memoryv2_test.go index b4eb72c8..58703a95 100644 --- a/v2/memoryv2_test.go +++ b/v2/memoryv2_test.go @@ -21,8 +21,6 @@ import ( "os" "testing" - "github.com/opencontainers/runtime-spec/specs-go" - "github.com/stretchr/testify/assert" ) @@ -56,8 +54,10 @@ func TestCgroupv2MemoryStats(t *testing.T) { func TestSystemdCgroupMemoryController(t *testing.T) { checkCgroupMode(t) group := fmt.Sprintf("testing-memory-%d.scope", os.Getpid()) - res := specs.LinuxResources{ - Memory: &specs.LinuxMemory{Limit: pointerInt64(629145600)}, + res := Resources{ + Memory: &Memory{ + Max: pointerInt64(629145600), + }, } c, err := NewSystemd("", group, os.Getpid(), &res) if err != nil { From acbc8024b3f44e265c066ff60c0b5065b3b0205f Mon Sep 17 00:00:00 2001 From: Boris Popovschi Date: Tue, 7 Jan 2020 12:59:39 +0200 Subject: [PATCH 7/7] rebase on master Signed-off-by: Boris Popovschi --- v2/manager.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/v2/manager.go b/v2/manager.go index 01f84fa6..c15dd31f 100644 --- a/v2/manager.go +++ b/v2/manager.go @@ -32,12 +32,12 @@ import ( "golang.org/x/sys/unix" "github.com/containerd/cgroups/v2/stats" - "github.com/godbus/dbus" + "github.com/godbus/dbus/v5" "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" "github.com/sirupsen/logrus" - systemdDbus "github.com/coreos/go-systemd/dbus" + systemdDbus "github.com/coreos/go-systemd/v22/dbus" ) const (