Repository: opencontainers/runc
Pull request #3788 (2023-03-27 18:23:00)

cg/sd: support CPUWeight=idle (aka cpu.idle=1)
Systemd v252 added support for setting cgroup v2 cpu.idle in https://github.com/systemd/systemd/pull/23299

The way it works is
 - if CPUWeight == 0, cpu.idle is set to 1;
 - if CPUWeight != 0, cpu.idle is set to 0.

This PR implements support for setting `cpu.idle` via systemd cgroup
v2 driver. In case CPUIdle is set to non-zero value, the driver sets
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.
Modify docs to show the newly supported property.

Fixes: #3786

AkihiroSuda: > currently only CentOS 9

And Fedora?

kolyshkin: > > currently only CentOS 9
> 
> And Fedora?

F37 on my laptop has systemd v231
AkihiroSuda: > > > currently only CentOS 9
> > 
> > 
> > And Fedora?
> 
> F37 on my laptop has systemd v231

👀 

I was expecting CentOS 9 Stream to be more conservative than Fedora.
Was there any specific reason that CentOS 9 Stream adopted the new systemd version ahead of Fedora?

kolyshkin: It is really hard to say how to handle this properly. The kernel allows to set `cpu.weight` and `cpu.idle` at the same time (and if `cpu.idle` is enabled, `cpu.weight` is ignored). Systemd does not allow that, overwriting `cpu.idle` in many scenarios (including, but apparently not limited to setting cpu-shares which is translated to CPUWeight property).

That means, for example, that running `runc update --cpu-shares 200 --cpu.idle 1` will cause systemd to set `cpu.idle` to `0`, and then runc will set it to `1` (because we back up everything that systemd does by the cgroup fs driver). Some other update, even unrelated, may trigger systemd to set cpu.idle back to 0 again.

For example, this is what we see when changing cpu-period (see #3786), which appears to be unrelated to cpu weight and cpu idle.

So, maybe we should make systemd cgroup v2 driver  to deny setting some CPU-related parameters if cpu.idle was/is set to 1. 
kolyshkin: Oh my, finally the CI is green again. If we merge this PR this week, we will be able to brag that it only took us a month to fix the CI (in main branch, that is).

Patch:
diff --git a/docs/systemd.md b/docs/systemd.md
index c74e2e2f..a119d935 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/v2.go b/libcontainer/cgroups/systemd/v2.go
index 3d66ed40..c5576441 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
@@ -64,8 +76,7 @@ func unifiedResToSystemdProps(cm *dbusConnManager, res map[string]string) (props
 		if strings.Contains(k, "/") {
 			return nil, fmt.Errorf("unified resource %q must be a file name (no slashes)", k)
 		}
-		sk := strings.SplitN(k, ".", 2)
-		if len(sk) != 2 {
+		if strings.IndexByte(k, '.') <= 0 {
 			return nil, fmt.Errorf("unified resource %q must be in the form CONTROLLER.PARAMETER", k)
 		}
 		// Kernel is quite forgiving to extra whitespace
@@ -73,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
@@ -98,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)
@@ -213,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)


Test patch:
diff --git a/libcontainer/cgroups/systemd/systemd_test.go b/libcontainer/cgroups/systemd/systemd_test.go
index 40584f78..8e333a00 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/tests/integration/update.bats b/tests/integration/update.bats
index 09949a1e..56d9f98d 100644
--- a/tests/integration/update.bats
+++ b/tests/integration/update.bats
@@ -454,12 +454,70 @@ EOF
 		check_cgroup_value "cpu.idle" "$val"
 	done
 
+	# Values other than 1 or 0 are ignored by the kernel, see
+	# sched_group_set_idle() in kernel/sched/fair.c.
+	#
+	# If this ever fails, it means that the kernel now accepts values
+	# other than 0 or 1, and runc needs to adopt.
+	for val in -1 2 3; do
+		runc update --cpu-idle "$val" test_update
+		[ "$status" -ne 0 ]
+		check_cgroup_value "cpu.idle" "1"
+	done
+
+	# https://github.com/opencontainers/runc/issues/3786
+	[ "$(systemd_version)" -ge 252 ] && return
 	# test update other option won't impact on cpu.idle
 	runc update --cpu-period 10000 test_update
 	[ "$status" -eq 0 ]
 	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 <<EOF
+{
+  "unified": {
+    "cpu.idle": "1",
+    "cpu.weight": "8"
+  }
+}
+EOF
+	[[ "$output" == *"unable to apply both"* ]]
+	check_cgroup_value "cpu.idle" "1"
+
+	# Setting any cpu.weight should reset cpu.idle to 0.
+	runc update -r - test_update <<EOF
+{
+  "unified": {
+    "cpu.weight": "8"
+  }
+}
+EOF
+	check_cgroup_value "cpu.idle" "0"
+}
+
 @test "update cgroup v2 resources via unified map" {
 	[ $EUID -ne 0 ] && requires rootless_cgroup
 	requires cgroups_v2