Merge pull request #3788 from kolyshkin/systemd-cpu-idle

cg/sd: support CPUWeight=idle (aka cpu.idle=1)

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            |                     |

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)
+			}
+		})
+	}
+}

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,15 +76,22 @@ 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
 		// around the value, and so should we.
 		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)

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