From bfd4ea5998b715ed6f41bb364afca6dccbce1def Mon Sep 17 00:00:00 2001 From: Hironori Shiina Date: Sat, 22 Feb 2025 01:12:27 +0000 Subject: [PATCH] libct/cg/sd: 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 runc 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 by calculating the value from systemd properties. Signed-off-by: Hironori Shiina --- libcontainer/cgroups/systemd/common.go | 20 +++++++ libcontainer/cgroups/systemd/systemd_test.go | 61 ++++++++++++++++++++ libcontainer/cgroups/systemd/v1.go | 10 ++++ libcontainer/cgroups/systemd/v2.go | 10 ++++ tests/integration/update.bats | 32 +++++++++- 5 files changed, 130 insertions(+), 3 deletions(-) diff --git a/libcontainer/cgroups/systemd/common.go b/libcontainer/cgroups/systemd/common.go index bcd1f99eb0c..440a71d5e43 100644 --- a/libcontainer/cgroups/systemd/common.go +++ b/libcontainer/cgroups/systemd/common.go @@ -314,6 +314,26 @@ func addCpuQuota(cm *dbusConnManager, properties *[]systemdDbus.Property, quota } } +func getCPUQuotaFromProperties(properties []systemdDbus.Property, period uint64) (int64, error) { + var cpuQuotaPerSecUsec int64 + + for _, prop := range properties { + if prop.Name == "CPUQuotaPerSecUSec" { + if err := prop.Value.Store(&cpuQuotaPerSecUsec); err != nil { + return 0, fmt.Errorf("can't parse CPUQuotaPerSecUSec %v: %w", prop.Value, err) + } + break + } + } + if cpuQuotaPerSecUsec == 0 { + return 0, nil + } + if period == 0 { + period = defCPUQuotaPeriod + } + return cpuQuotaPerSecUsec * int64(period) / 1000000, nil +} + func addCpuset(cm *dbusConnManager, props *[]systemdDbus.Property, cpus, mems string) error { if cpus == "" && mems == "" { return nil diff --git a/libcontainer/cgroups/systemd/systemd_test.go b/libcontainer/cgroups/systemd/systemd_test.go index 9980e780df3..7940001e507 100644 --- a/libcontainer/cgroups/systemd/systemd_test.go +++ b/libcontainer/cgroups/systemd/systemd_test.go @@ -178,3 +178,64 @@ func TestUnifiedResToSystemdProps(t *testing.T) { }) } } + +func TestGetCPUQuotaFromProperties(t *testing.T) { + testCases := []struct { + name string + properties []systemdDbus.Property + cpuPeriod uint64 + expectedCPUQuota int64 + }{ + { + name: "Update only CPU quota", + properties: []systemdDbus.Property{ + newProp("CPUQuotaPerSecUSec", 1230000), + }, + cpuPeriod: 0, + expectedCPUQuota: 123000, + }, + { + name: "Update CPU Quota and CPU period", + properties: []systemdDbus.Property{ + newProp("CPUQuotaPerSecUSec", 560000), + newProp("CPUQuotaPeriodUSec", 900000), + }, + cpuPeriod: 900000, + expectedCPUQuota: 504000, + }, + { + name: "Update CPU Quota and CPU period without supporting CPUQuotaPeriodUSec", + properties: []systemdDbus.Property{ + newProp("CPUQuotaPerSecUSec", 560000), + }, + cpuPeriod: 900000, + expectedCPUQuota: 504000, + }, + { + name: "Update only CPU period", + properties: []systemdDbus.Property{ + newProp("CPUQuotaPeriodUSec", 900000), + }, + cpuPeriod: 900000, + expectedCPUQuota: 0, + }, + { + name: "Update neither CPU quota nor CPU period", + properties: []systemdDbus.Property{ + newProp("MemoryMax", 67108864), + }, + cpuPeriod: 0, + expectedCPUQuota: 0, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if cpuQuota, err := getCPUQuotaFromProperties(tc.properties, tc.cpuPeriod); err != nil { + t.Errorf("getCPUQuotaFromProperties(%v); unexpected error %v", tc.properties, err) + } else if cpuQuota != tc.expectedCPUQuota { + t.Errorf("getCPUQuotaFromProperties(%v); want %d; got %d", tc.properties, tc.expectedCPUQuota, cpuQuota) + } + }) + } +} diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go index cee76eb35cf..276a89746de 100644 --- a/libcontainer/cgroups/systemd/v1.go +++ b/libcontainer/cgroups/systemd/v1.go @@ -370,6 +370,16 @@ func (m *LegacyManager) Set(r *cgroups.Resources) error { return setErr } + if cpuQuota, err := getCPUQuotaFromProperties(properties, r.CpuPeriod); err != nil { + return err + } else if cpuQuota > 0 { + // CPU Quota is rounded up to the nearest 10ms in properties. + // Get the rounded value in order to write the same value to the file. + rCopy := *r + r = &rCopy + r.CpuQuota = cpuQuota + } + for _, sys := range legacySubsystems { // Get the subsystem path, but don't error out for not found cgroups. path, ok := m.paths[sys.Name()] diff --git a/libcontainer/cgroups/systemd/v2.go b/libcontainer/cgroups/systemd/v2.go index 7b16f528b9c..4d93fbdfa92 100644 --- a/libcontainer/cgroups/systemd/v2.go +++ b/libcontainer/cgroups/systemd/v2.go @@ -489,6 +489,16 @@ func (m *UnifiedManager) Set(r *cgroups.Resources) error { return fmt.Errorf("unable to set unit properties: %w", err) } + if cpuQuota, err := getCPUQuotaFromProperties(properties, r.CpuPeriod); err != nil { + return err + } else if cpuQuota > 0 { + // CPU Quota is rounded up to the nearest 10ms in properties. + // Get the rounded value in order to write the same value to the file. + rCopy := *r + r = &rCopy + r.CpuQuota = cpuQuota + } + return m.fsMgr.Set(r) } diff --git a/tests/integration/update.bats b/tests/integration/update.bats index 22a92e8bd69..6d698d4acec 100644 --- a/tests/integration/update.bats +++ b/tests/integration/update.bats @@ -267,15 +267,37 @@ EOF check_cpu_quota 500000 1000000 "500ms" check_cpu_shares 100 + # update cpu quota with fraction + runc update test_update --cpu-quota 123456 + [ "$status" -eq 0 ] + expected_quota=123456 + if [ -v RUNC_USE_SYSTEMD ]; then + expected_quota=130000 + fi + check_cpu_quota $expected_quota 1000000 "130ms" + + # update cpu quota to original value + runc update test_update --cpu-quota 500000 + [ "$status" -eq 0 ] + check_cpu_quota 500000 1000000 "500ms" + # update cpu period runc update test_update --cpu-period 900000 [ "$status" -eq 0 ] - check_cpu_quota 500000 900000 "560ms" + expected_quota=500000 + if [ -v RUNC_USE_SYSTEMD ]; then + expected_quota=504000 + fi + check_cpu_quota $expected_quota 900000 "560ms" # update cpu quota runc update test_update --cpu-quota 600000 [ "$status" -eq 0 ] - check_cpu_quota 600000 900000 "670ms" + expected_quota=600000 + if [ -v RUNC_USE_SYSTEMD ]; then + expected_quota=603000 + fi + check_cpu_quota $expected_quota 900000 "670ms" # remove cpu quota runc update test_update --cpu-quota -1 @@ -304,7 +326,11 @@ EOF runc update test_update \ --cpu-period 900000 --cpu-quota 600000 --cpu-share 200 [ "$status" -eq 0 ] - check_cpu_quota 600000 900000 "670ms" + expected_quota=600000 + if [ -v RUNC_USE_SYSTEMD ]; then + expected_quota=603000 + fi + check_cpu_quota $expected_quota 900000 "670ms" check_cpu_shares 200 # remove cpu quota and reset the period