From 8adc1f3aafd140fdb8c9d3302f14fff3cb327659 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 27 Mar 2023 10:59:40 -0700 Subject: [PATCH] libct/cg/sd: support setting cpu.idle via systemd Systemd v252 (available in CentOS Stream 9 in our CI) added support for setting cpu.idle (see [1]). The way it works is: - if CPUWeight == 0, cpu.idle is set to 1; - if CPUWeight != 0, cpu.idle is set to 0. This commit implements setting cpu.idle in systemd cgroup driver via a unit property. In case CPUIdle is set to non-zero value, the driver sets adds CPUWeight=0 property, which will result in systemd setting cpu.idle to 1. Unfortunately, there's no way to set cpu.idle to 0 without also changing the CPUWeight value, so the driver doesn't do anything if CPUIdle is explicitly set to 0. This case is handled by the fs driver which is always used as a followup to setting systemd unit properties. Also, handle cpu.idle set via unified map. In case it is set to non-zero value, add CPUWeight=0 property, and ignore cpu.weight (otherwise we'll get two different CPUWeight properties set). Add a unit test for new values in unified map, and an integration test case. [1] https://github.com/systemd/systemd/pull/23299 [2] https://github.com/opencontainers/runc/issues/3786 Signed-off-by: Kir Kolyshkin --- docs/systemd.md | 1 + libcontainer/cgroups/systemd/systemd_test.go | 84 ++++++++++++++++++++ libcontainer/cgroups/systemd/v2.go | 42 +++++++++- tests/integration/update.bats | 45 +++++++++++ 4 files changed, 170 insertions(+), 2 deletions(-) diff --git a/docs/systemd.md b/docs/systemd.md index c74e2e2f407..a119d935a41 100644 --- a/docs/systemd.md +++ b/docs/systemd.md @@ -91,6 +91,7 @@ The following tables summarize which properties are translated. | cpu.mems | AllowedMemoryNodes | v244 | | unified.cpu.max | CPUQuota, CPUQuotaPeriodSec | v242 | | unified.cpu.weight | CPUWeight | | +| unified.cpu.idle | CPUWeight | v252 | | unified.cpuset.cpus | AllowedCPUs | v244 | | unified.cpuset.mems | AllowedMemoryNodes | v244 | | unified.memory.high | MemoryHigh | | diff --git a/libcontainer/cgroups/systemd/systemd_test.go b/libcontainer/cgroups/systemd/systemd_test.go index 40584f78eba..8e333a00649 100644 --- a/libcontainer/cgroups/systemd/systemd_test.go +++ b/libcontainer/cgroups/systemd/systemd_test.go @@ -2,8 +2,10 @@ package systemd import ( "os" + "reflect" "testing" + systemdDbus "github.com/coreos/go-systemd/v22/dbus" "github.com/opencontainers/runc/libcontainer/cgroups" "github.com/opencontainers/runc/libcontainer/configs" ) @@ -95,3 +97,85 @@ func TestUnitExistsIgnored(t *testing.T) { } } } + +func TestUnifiedResToSystemdProps(t *testing.T) { + if !IsRunningSystemd() { + t.Skip("Test requires systemd.") + } + if !cgroups.IsCgroup2UnifiedMode() { + t.Skip("cgroup v2 is required") + } + + cm := newDbusConnManager(os.Geteuid() != 0) + + testCases := []struct { + name string + minVer int + res map[string]string + expError bool + expProps []systemdDbus.Property + }{ + { + name: "empty map", + res: map[string]string{}, + }, + { + name: "only cpu.idle=1", + minVer: cpuIdleSupportedVersion, + res: map[string]string{ + "cpu.idle": "1", + }, + expProps: []systemdDbus.Property{ + newProp("CPUWeight", uint64(0)), + }, + }, + { + name: "only cpu.idle=0", + minVer: cpuIdleSupportedVersion, + res: map[string]string{ + "cpu.idle": "0", + }, + }, + { + name: "cpu.idle=1 and cpu.weight=1000", + minVer: cpuIdleSupportedVersion, + res: map[string]string{ + "cpu.idle": "1", + "cpu.weight": "1000", + }, + expProps: []systemdDbus.Property{ + newProp("CPUWeight", uint64(0)), + }, + }, + { + name: "cpu.idle=0 and cpu.weight=1000", + minVer: cpuIdleSupportedVersion, + res: map[string]string{ + "cpu.idle": "0", + "cpu.weight": "1000", + }, + expProps: []systemdDbus.Property{ + newProp("CPUWeight", uint64(1000)), + }, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + if tc.minVer != 0 && systemdVersion(cm) < tc.minVer { + t.Skipf("requires systemd >= %d", tc.minVer) + } + props, err := unifiedResToSystemdProps(cm, tc.res) + if err != nil && !tc.expError { + t.Fatalf("expected no error, got: %v", err) + } + if err == nil && tc.expError { + t.Fatal("expected error, got nil") + } + if !reflect.DeepEqual(tc.expProps, props) { + t.Errorf("wrong properties (exp %+v, got %+v)", tc.expProps, props) + } + }) + } +} diff --git a/libcontainer/cgroups/systemd/v2.go b/libcontainer/cgroups/systemd/v2.go index 29d629afccf..362cdcf8e4a 100644 --- a/libcontainer/cgroups/systemd/v2.go +++ b/libcontainer/cgroups/systemd/v2.go @@ -20,6 +20,10 @@ import ( "github.com/opencontainers/runc/libcontainer/configs" ) +const ( + cpuIdleSupportedVersion = 252 +) + type UnifiedManager struct { mu sync.Mutex cgroups *configs.Cgroup @@ -48,6 +52,14 @@ func NewUnifiedManager(config *configs.Cgroup, path string) (*UnifiedManager, er return m, nil } +func shouldSetCPUIdle(cm *dbusConnManager, v string) bool { + // The only valid values for cpu.idle are 0 and 1. As it is + // not possible to directly set cpu.idle to 0 via systemd, + // ignore 0. Ignore other values as we'll error out later + // in Set() while calling fsMgr.Set(). + return v == "1" && systemdVersion(cm) >= cpuIdleSupportedVersion +} + // unifiedResToSystemdProps tries to convert from Cgroup.Resources.Unified // key/value map (where key is cgroupfs file name) to systemd unit properties. // This is on a best-effort basis, so the properties that are not known @@ -72,6 +84,14 @@ func unifiedResToSystemdProps(cm *dbusConnManager, res map[string]string) (props v = strings.TrimSpace(v) // Please keep cases in alphabetical order. switch k { + case "cpu.idle": + if shouldSetCPUIdle(cm, v) { + // Setting CPUWeight to 0 tells systemd + // to set cpu.idle to 1. + props = append(props, + newProp("CPUWeight", uint64(0))) + } + case "cpu.max": // value: quota [period] quota := int64(0) // 0 means "unlimited" for addCpuQuota, if period is set @@ -97,6 +117,12 @@ func unifiedResToSystemdProps(cm *dbusConnManager, res map[string]string) (props addCpuQuota(cm, &props, quota, period) case "cpu.weight": + if shouldSetCPUIdle(cm, strings.TrimSpace(res["cpu.idle"])) { + // Do not add duplicate CPUWeight property + // (see case "cpu.idle" above). + logrus.Warn("unable to apply both cpu.weight and cpu.idle to systemd, ignoring cpu.weight") + continue + } num, err := strconv.ParseUint(v, 10, 64) if err != nil { return nil, fmt.Errorf("unified resource %q value conversion error: %w", k, err) @@ -212,9 +238,21 @@ func genV2ResourcesProperties(dirPath string, r *configs.Resources, cm *dbusConn newProp("MemorySwapMax", uint64(swap))) } - if r.CpuWeight != 0 { + idleSet := false + // The logic here is the same as in shouldSetCPUIdle. + if r.CPUIdle != nil && *r.CPUIdle == 1 && systemdVersion(cm) >= cpuIdleSupportedVersion { properties = append(properties, - newProp("CPUWeight", r.CpuWeight)) + newProp("CPUWeight", uint64(0))) + idleSet = true + } + if r.CpuWeight != 0 { + if idleSet { + // Ignore CpuWeight if CPUIdle is already set. + logrus.Warn("unable to apply both CPUWeight and CpuIdle to systemd, ignoring CPUWeight") + } else { + properties = append(properties, + newProp("CPUWeight", r.CpuWeight)) + } } addCpuQuota(cm, &properties, r.CpuQuota, r.CpuPeriod) diff --git a/tests/integration/update.bats b/tests/integration/update.bats index 2e52af6fb54..56d9f98d43e 100644 --- a/tests/integration/update.bats +++ b/tests/integration/update.bats @@ -473,6 +473,51 @@ EOF check_cgroup_value "cpu.idle" "1" } +@test "update cgroup cpu.idle via systemd v252+" { + requires cgroups_v2 systemd cgroups_cpu_idle + [ $EUID -ne 0 ] && requires rootless_cgroup + if [ "$(systemd_version)" -lt 252 ]; then + skip "requires systemd >= v252" + fi + + runc run -d --console-socket "$CONSOLE_SOCKET" test_update + [ "$status" -eq 0 ] + check_cgroup_value "cpu.idle" "0" + + # If cpu-idle is set, cpu-share (converted to CPUWeight) can't be set via systemd. + runc update --cpu-share 200 --cpu-idle 1 test_update + [[ "$output" == *"unable to apply both"* ]] + check_cgroup_value "cpu.idle" "1" + + # Changing cpu-shares (converted to CPU weight) resets cpu.idle to 0. + runc update --cpu-share 200 test_update + check_cgroup_value "cpu.idle" "0" + + # Setting values via unified map. + + # If cpu.idle is set, cpu.weight is ignored. + runc update -r - test_update <