Merge pull request #4 from hshiina/cpu-quota-round

systemd: Write rounded CPU quota to cgroupfs

Author: AkihiroSuda

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