systemd: Write rounded CPU quota to cgroupfs

When CPU quota is updated, the value is converted to CPUQuotaPerSecUSec
property for passing to systemd. The value can be rounded in the
following cases:
- The value is rounded up to the nearest 10ms.
- Depending on CPU period, the value may be rounded during division.

Because of this rounding, systemd and systemd driver may write
different values to cgroupfs. In order to avoid this inconsistency,
this fix makes systemd driver write the same rounded value to cgroupfs.

Even if systemd writes CPU quota and CPU period to cgroupfs, systemd
driver still needs to write to cgroupfs for a case where CPU period
is updated to less than the minimum value. In this case, systemd accepts
this value by adjusting CPU period while direct update by systemd driver
fails. In order to keep this case invalid, systemd driver still needs to
update cgroupfs.

Signed-off-by: Hironori Shiina <[email protected]>

Author: hshiina

systemd/common.go

@@ -280,7 +280,9 @@ func systemdVersionAtoi(str string) (int, error) {
 	return ver, nil
 }
 
-func addCpuQuota(cm *dbusConnManager, properties *[]systemdDbus.Property, quota int64, period uint64) {
+// addCPUQuota adds CPUQuotaPeriodUSec and CPUQuotaPerSecUSec to the properties. The passed quota may be modified
+// along with round-up during calculation in order to write the same value to cgroupfs later.
+func addCPUQuota(cm *dbusConnManager, properties *[]systemdDbus.Property, quota *int64, period uint64) {
 	if period != 0 {
 		// systemd only supports CPUQuotaPeriodUSec since v242
 		sdVer := systemdVersion(cm)
@@ -292,10 +294,10 @@ func addCpuQuota(cm *dbusConnManager, properties *[]systemdDbus.Property, quota
 				" (setting will still be applied to cgroupfs)", sdVer)
 		}
 	}
-	if quota != 0 || period != 0 {
+	if *quota != 0 || period != 0 {
 		// corresponds to USEC_INFINITY in systemd
 		cpuQuotaPerSecUSec := uint64(math.MaxUint64)
-		if quota > 0 {
+		if *quota > 0 {
 			if period == 0 {
 				// assume the default
 				period = defCPUQuotaPeriod
@@ -304,9 +306,11 @@ func addCpuQuota(cm *dbusConnManager, properties *[]systemdDbus.Property, quota
 			// (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(quota*1000000) / period
+			cpuQuotaPerSecUSec = uint64(*quota*1000000) / period
 			if cpuQuotaPerSecUSec%10000 != 0 {
 				cpuQuotaPerSecUSec = ((cpuQuotaPerSecUSec / 10000) + 1) * 10000
+				// Update the requested quota along with the round-up in order to write the same value to cgroupfs.
+				*quota = int64(cpuQuotaPerSecUSec) * int64(period) / 1000000
 			}
 		}
 		*properties = append(*properties,

systemd/systemd_test.go

@@ -178,3 +178,61 @@ func TestUnifiedResToSystemdProps(t *testing.T) {
 		})
 	}
 }
+
+func TestAddCPUQuota(t *testing.T) {
+	if !IsRunningSystemd() {
+		t.Skip("Test requires systemd.")
+	}
+
+	cm := newDbusConnManager(os.Geteuid() != 0)
+
+	testCases := []struct {
+		name                       string
+		quota                      int64
+		period                     uint64
+		expectedCPUQuotaPerSecUSec uint64
+		expectedQuota              int64
+	}{
+		{
+			name:                       "No round up",
+			quota:                      500000,
+			period:                     1000000,
+			expectedCPUQuotaPerSecUSec: 500000,
+			expectedQuota:              500000,
+		},
+		{
+			name:                       "With fraction",
+			quota:                      123456,
+			expectedCPUQuotaPerSecUSec: 1240000,
+			expectedQuota:              124000,
+		},
+		{
+			name:                       "Round up at division",
+			quota:                      500000,
+			period:                     900000,
+			expectedCPUQuotaPerSecUSec: 560000,
+			expectedQuota:              504000,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			props := []systemdDbus.Property{}
+			addCPUQuota(cm, &props, &tc.quota, tc.period)
+			var cpuQuotaPerSecUSec uint64
+			for _, p := range props {
+				if p.Name == "CPUQuotaPerSecUSec" {
+					if err := p.Value.Store(&cpuQuotaPerSecUSec); err != nil {
+						t.Errorf("failed to parse CPUQuotaPerSecUSec: %v", err)
+					}
+				}
+			}
+			if cpuQuotaPerSecUSec != tc.expectedCPUQuotaPerSecUSec {
+				t.Errorf("CPUQuotaPerSecUSec is not set as expected (exp: %v, got: %v)", tc.expectedCPUQuotaPerSecUSec, cpuQuotaPerSecUSec)
+			}
+			if tc.quota != tc.expectedQuota {
+				t.Errorf("quota is not updated as expected (exp: %v, got: %v)", tc.expectedQuota, tc.quota)
+			}
+		})
+	}
+}

systemd/v1.go

@@ -90,7 +90,7 @@ func genV1ResourcesProperties(r *cgroups.Resources, cm *dbusConnManager) ([]syst
 			newProp("CPUShares", r.CpuShares))
 	}
 
-	addCpuQuota(cm, &properties, r.CpuQuota, r.CpuPeriod)
+	addCPUQuota(cm, &properties, &r.CpuQuota, r.CpuPeriod)
 
 	if r.BlkioWeight != 0 {
 		properties = append(properties,
@@ -334,6 +334,9 @@ func (m *LegacyManager) Set(r *cgroups.Resources) error {
 	if r.Unified != nil {
 		return cgroups.ErrV1NoUnified
 	}
+	// Use a copy since CpuQuota in r may be modified.
+	rCopy := *r
+	r = &rCopy
 	properties, err := genV1ResourcesProperties(r, m.dbus)
 	if err != nil {
 		return err

systemd/v2.go

@@ -113,7 +113,7 @@ func unifiedResToSystemdProps(cm *dbusConnManager, res map[string]string) (props
 					return nil, fmt.Errorf("unified resource %q quota value conversion error: %w", k, err)
 				}
 			}
-			addCpuQuota(cm, &props, quota, period)
+			addCPUQuota(cm, &props, &quota, period)
 
 		case "cpu.weight":
 			if shouldSetCPUIdle(cm, strings.TrimSpace(res["cpu.idle"])) {
@@ -254,7 +254,7 @@ func genV2ResourcesProperties(dirPath string, r *cgroups.Resources, cm *dbusConn
 		}
 	}
 
-	addCpuQuota(cm, &properties, r.CpuQuota, r.CpuPeriod)
+	addCPUQuota(cm, &properties, &r.CpuQuota, r.CpuPeriod)
 
 	if r.PidsLimit > 0 || r.PidsLimit == -1 {
 		properties = append(properties,
@@ -480,6 +480,9 @@ func (m *UnifiedManager) Set(r *cgroups.Resources) error {
 	if r == nil {
 		return nil
 	}
+	// Use a copy since CpuQuota in r may be modified.
+	rCopy := *r
+	r = &rCopy
 	properties, err := genV2ResourcesProperties(m.fsMgr.Path(""), r, m.dbus)
 	if err != nil {
 		return err