Repository: opencontainers/runc
Pull request #3749 (2023-02-23 10:39:14)

[Carry #3205] libct/cg: add CFS bandwidth burst for CPU
Carry #3205 

Zheaoli: Seems the systemd is not support the CPU burst feature yet. https://github.com/systemd/systemd/issues/26658

So should we keep pushing this PR or hold on it utill the feature has been supported in systemd @AkihiroSuda 
AkihiroSuda: > Seems the systemd is not support the CPU burst feature yet. [systemd/systemd#26658](https://github.com/systemd/systemd/issues/26658)
> 
> So should we keep pushing this PR or hold on it utill the feature has been supported in systemd @AkihiroSuda

You can just let fs2 handle it
https://github.com/opencontainers/runc/blob/a187c84e42015402263167a2c1007b5c62d494f3/libcontainer/cgroups/systemd/v2.go#L168-L170
Zheaoli: > > Seems the systemd is not support the CPU burst feature yet. [systemd/systemd#26658](https://github.com/systemd/systemd/issues/26658)
> > So should we keep pushing this PR or hold on it utill the feature has been supported in systemd @AkihiroSuda
> 
> You can just let fs2 handle it
> 
> https://github.com/opencontainers/runc/blob/a187c84e42015402263167a2c1007b5c62d494f3/libcontainer/cgroups/systemd/v2.go#L168-L170

Got it
Zheaoli: > > Seems the systemd is not support the CPU burst feature yet. [systemd/systemd#26658](https://github.com/systemd/systemd/issues/26658)
> > So should we keep pushing this PR or hold on it utill the feature has been supported in systemd @AkihiroSuda
> 
> You can just let fs2 handle it
> 
> https://github.com/opencontainers/runc/blob/a187c84e42015402263167a2c1007b5c62d494f3/libcontainer/cgroups/systemd/v2.go#L168-L170

But the burst feature supports both cv1 and cv2. hmmm I think maybe I can limit it should be only being used in non-systemd driver?
Zheaoli: @AkihiroSuda Hi, I have written the test, but the rootless test failed. I have no idea what happened. would you mind take a look at for me ?
Zheaoli: https://github.com/opencontainers/runc/actions/runs/4462135582/jobs/7836459757?pr=3749
https://github.com/opencontainers/runc/actions/runs/4462008272/jobs/7836249615?pr=3749

It seems strange why `no directory specified for cpu.cfs_burst_us` has been raised but not for other values like period and quota(both of them shared the same path 
Zheaoli: fine, I figure out the reason, all ci has been passed
Zheaoli: https://cirrus-ci.com/task/5913443228712960
https://cirrus-ci.com/task/4887318499164160
https://cirrus-ci.com/task/4605843522453504

Should I skip the systemd test for burst? cc @AkihiroSuda 
Zheaoli: @AkihiroSuda ping(would you mind to give me some advise about test failed in Centos?
AkihiroSuda: CI failing
Zheaoli: @AkihiroSuda It seems like there may be some compatibility issues between CPU Burst and Centos7 and Centos8, which could be causing the CI to fail.  Would your mind to give me some suggestion about how to pass all the CI?
AkihiroSuda: > It seems like there may be some compatibility issues between CPU Burst and Centos7 and Centos8, which could be causing the CI to fail. Would your mind to give me some suggestion about how to pass all the CI?

I guess you can just check existence of `cpu.cfs_burst_us` and skip the test when it does not exist?


Zheaoli: > > It seems like there may be some compatibility issues between CPU Burst and Centos7 and Centos8, which could be causing the CI to fail. Would your mind to give me some suggestion about how to pass all the CI?
> 
> I guess you can just check existence of `cpu.cfs_burst_us` and skip the test when it does not exist?

Seems good, I'll make a try! Thanks a lot!
Zheaoli: @AkihiroSuda Seems all CI has been passed except lint-extra test.

Some warning message has been raised from this step:" var-naming: struct field CpuBurst should be CPUBurst". But I think `CpuBurst` is better than `CPUBurst` because we should keep consistency with other fields?
Zheaoli: @kolyshkin Hello. Would you mind looking at this PR if you have time? Thanks!
AkihiroSuda: Fedora CI is failing, rebasing may solve the issue.

AkihiroSuda: Please update https://github.com/opencontainers/runc/blob/main/docs/spec-conformance.md too
Zheaoli: @AkihiroSuda Thanks for your tips, and sorry for replying late. 

I have merged the newest code from the main branch and updated the docs. But there is still some issues:

1. the CI on Centos-Stream-9 has failed. The message is `Command "grep -q oom events.log" failed 30 times. Output:`. I think this issue is not relevant to this PR. Would you mind rerunning this step for me?
2. The spell check has failed. I'm sure if I can skip the check or not.
Zheaoli: @kolyshkin ping~
lifubang: @Zheaoli I think you should use `git rebase main` instead of `git merge main`.
Zheaoli: > @Zheaoli I think you should use `git rebase main` instead of `git merge main`.

copy that, I will squash commit later
lifubang: 👍 Please use ‘git rebase main’ to catch up with the latest change, we should not have a commit named ‘merge main’. Thanks.
Zheaoli: @lifubang Thanks for the review, all things done
lifubang: I'm so sorry to let you spend more time on this. We need to cover as many test cases as possible. ☕ 
Zheaoli: > We need to cover as many test cases as possible. ☕

I think we already have enough test cases here. May I ask what's kind of the corner case in your thought not covered by current test code?
lifubang: > May I ask what's kind of the corner case in your thought not covered by current test code?

For example, how to clear `cpu-burst` value in `update` after the container started with a `cpu-burst` value?
lifubang: Left some further works:
1. No unit test for cpu controller in cgroupv2;
2. There are some different results for cgroup v1 and v2, for example:
```
runc update --cpu-burst 30001 --cpu-quota 30000 test
```
v1: fail, v2: succ
Because for v2, the kernel write seconds to `cpu.max.burst`, but not milliseconds.
3. There is no integration test for `cpu-burst` when create/run a container.

We can do these works in further PRs.
lifubang: @AkihiroSuda PTAL. I think this could be merged.

Patch:
diff --git a/contrib/completions/bash/runc b/contrib/completions/bash/runc
index 69a3b953..782234e8 100644
--- a/contrib/completions/bash/runc
+++ b/contrib/completions/bash/runc
@@ -735,6 +735,7 @@ _runc_update() {
 	   --blkio-weight
 	   --cpu-period
 	   --cpu-quota
+	   --cpu-burst
 	   --cpu-rt-period
 	   --cpu-rt-runtime
 	   --cpu-share
diff --git a/docs/spec-conformance.md b/docs/spec-conformance.md
index b512e5f5..4ec89dca 100644
--- a/docs/spec-conformance.md
+++ b/docs/spec-conformance.md
@@ -10,7 +10,6 @@ Spec version | Feature                                  | PR
 v1.0.0       | `SCMP_ARCH_PARISC`                       | Unplanned, due to lack of users
 v1.0.0       | `SCMP_ARCH_PARISC64`                     | Unplanned, due to lack of users
 v1.0.2       | `.linux.personality`                     | [#3126](https://github.com/opencontainers/runc/pull/3126)
-v1.1.0       | `.linux.resources.cpu.burst`             | [#3749](https://github.com/opencontainers/runc/pull/3749)
 v1.1.0       | `SECCOMP_FILTER_FLAG_WAIT_KILLABLE_RECV` | [#3862](https://github.com/opencontainers/runc/pull/3862)
 v1.1.0       | time namespaces                          | [#3876](https://github.com/opencontainers/runc/pull/3876)
 v1.1.0       | rsvd hugetlb cgroup                      | TODO ([#3859](https://github.com/opencontainers/runc/issues/3859))
diff --git a/libcontainer/cgroups/fs/cpu.go b/libcontainer/cgroups/fs/cpu.go
index 26a9d7c0..727f7f91 100644
--- a/libcontainer/cgroups/fs/cpu.go
+++ b/libcontainer/cgroups/fs/cpu.go
@@ -84,6 +84,28 @@ func (s *CpuGroup) Set(path string, r *configs.Resources) error {
 			period = ""
 		}
 	}
+
+	var burst string
+	if r.CpuBurst != nil {
+		burst = strconv.FormatUint(*r.CpuBurst, 10)
+		if err := cgroups.WriteFile(path, "cpu.cfs_burst_us", burst); err != nil {
+			// this is a special trick for burst feature, the current systemd and low version of kernel will not support it.
+			// So, an `no such file or directory` error would be raised, and we can ignore it .
+			if !errors.Is(err, unix.ENOENT) {
+				// Sometimes when the burst to be set is larger
+				// than the current one, it is rejected by the kernel
+				// (EINVAL) as old_quota/new_burst exceeds the parent
+				// cgroup quota limit. If this happens and the quota is
+				// going to be set, ignore the error for now and retry
+				// after setting the quota.
+				if !errors.Is(err, unix.EINVAL) || r.CpuQuota == 0 {
+					return err
+				}
+			}
+		} else {
+			burst = ""
+		}
+	}
 	if r.CpuQuota != 0 {
 		if err := cgroups.WriteFile(path, "cpu.cfs_quota_us", strconv.FormatInt(r.CpuQuota, 10)); err != nil {
 			return err
@@ -93,6 +115,13 @@ func (s *CpuGroup) Set(path string, r *configs.Resources) error {
 				return err
 			}
 		}
+		if burst != "" {
+			if err := cgroups.WriteFile(path, "cpu.cfs_burst_us", burst); err != nil {
+				if !errors.Is(err, unix.ENOENT) {
+					return err
+				}
+			}
+		}
 	}
 
 	if r.CPUIdle != nil {
diff --git a/libcontainer/cgroups/fs/fs.go b/libcontainer/cgroups/fs/fs.go
index d2decb12..e2c425d0 100644
--- a/libcontainer/cgroups/fs/fs.go
+++ b/libcontainer/cgroups/fs/fs.go
@@ -191,7 +191,7 @@ func (m *Manager) Set(r *configs.Resources) error {
 			if path == "" {
 				// We never created a path for this cgroup, so we cannot set
 				// limits for it (though we have already tried at this point).
-				return fmt.Errorf("cannot set %s limit: container could not join or create cgroup", sys.Name())
+				return fmt.Errorf("cannot set %s limit: container could not join or create cgroup, and the error is %w", sys.Name(), err)
 			}
 			return err
 		}
diff --git a/libcontainer/cgroups/fs2/cpu.go b/libcontainer/cgroups/fs2/cpu.go
index 3f8deb0a..8ee49d49 100644
--- a/libcontainer/cgroups/fs2/cpu.go
+++ b/libcontainer/cgroups/fs2/cpu.go
@@ -2,16 +2,19 @@ package fs2
 
 import (
 	"bufio"
+	"errors"
 	"os"
 	"strconv"
 
+	"golang.org/x/sys/unix"
+
 	"github.com/opencontainers/runc/libcontainer/cgroups"
 	"github.com/opencontainers/runc/libcontainer/cgroups/fscommon"
 	"github.com/opencontainers/runc/libcontainer/configs"
 )
 
 func isCpuSet(r *configs.Resources) bool {
-	return r.CpuWeight != 0 || r.CpuQuota != 0 || r.CpuPeriod != 0 || r.CPUIdle != nil
+	return r.CpuWeight != 0 || r.CpuQuota != 0 || r.CpuPeriod != 0 || r.CPUIdle != nil || r.CpuBurst != nil
 }
 
 func setCpu(dirPath string, r *configs.Resources) error {
@@ -32,6 +35,23 @@ func setCpu(dirPath string, r *configs.Resources) error {
 		}
 	}
 
+	var burst string
+	if r.CpuBurst != nil {
+		burst = strconv.FormatUint(*r.CpuBurst, 10)
+		if err := cgroups.WriteFile(dirPath, "cpu.max.burst", burst); err != nil {
+			// Sometimes when the burst to be set is larger
+			// than the current one, it is rejected by the kernel
+			// (EINVAL) as old_quota/new_burst exceeds the parent
+			// cgroup quota limit. If this happens and the quota is
+			// going to be set, ignore the error for now and retry
+			// after setting the quota.
+			if !errors.Is(err, unix.EINVAL) || r.CpuQuota == 0 {
+				return err
+			}
+		} else {
+			burst = ""
+		}
+	}
 	if r.CpuQuota != 0 || r.CpuPeriod != 0 {
 		str := "max"
 		if r.CpuQuota > 0 {
@@ -47,6 +67,11 @@ func setCpu(dirPath string, r *configs.Resources) error {
 		if err := cgroups.WriteFile(dirPath, "cpu.max", str); err != nil {
 			return err
 		}
+		if burst != "" {
+			if err := cgroups.WriteFile(dirPath, "cpu.max.burst", burst); err != nil {
+				return err
+			}
+		}
 	}
 
 	return nil
diff --git a/libcontainer/configs/cgroup_linux.go b/libcontainer/configs/cgroup_linux.go
index 1664304b..4a34cf76 100644
--- a/libcontainer/configs/cgroup_linux.go
+++ b/libcontainer/configs/cgroup_linux.go
@@ -69,6 +69,9 @@ type Resources struct {
 	// CPU hardcap limit (in usecs). Allowed cpu time in a given period.
 	CpuQuota int64 `json:"cpu_quota"`
 
+	// CPU hardcap burst limit (in usecs). Allowed accumulated cpu time additionally for burst in a given period.
+	CpuBurst *uint64 `json:"cpu_burst"` //nolint:revive
+
 	// CPU period to be used for hardcapping (in usecs). 0 to use system default.
 	CpuPeriod uint64 `json:"cpu_period"`
 
diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go
index 88f9c677..818afcff 100644
--- a/libcontainer/specconv/spec_linux.go
+++ b/libcontainer/specconv/spec_linux.go
@@ -755,6 +755,9 @@ func CreateCgroupConfig(opts *CreateOpts, defaultDevs []*devices.Device) (*confi
 				if r.CPU.Quota != nil {
 					c.Resources.CpuQuota = *r.CPU.Quota
 				}
+				if r.CPU.Burst != nil {
+					c.Resources.CpuBurst = r.CPU.Burst
+				}
 				if r.CPU.Period != nil {
 					c.Resources.CpuPeriod = *r.CPU.Period
 				}
diff --git a/man/runc-update.8.md b/man/runc-update.8.md
index 8ceaa386..8ca69451 100644
--- a/man/runc-update.8.md
+++ b/man/runc-update.8.md
@@ -28,6 +28,7 @@ In case **-r** is used, the JSON format is like this:
 			"cpu": {
 				"shares": 0,
 				"quota": 0,
+				"burst": 0,
 				"period": 0,
 				"realtimeRuntime": 0,
 				"realtimePeriod": 0,
@@ -53,6 +54,9 @@ stdin. If this option is used, all other options are ignored.
 **--cpu-quota** _num_
 : Set CPU usage limit within a given period (in microseconds).
 
+**--cpu-burst** _num_
+: Set CPU burst limit within a given period (in microseconds).
+
 **--cpu-rt-period** _num_
 : Set CPU realtime period to be used for hardcapping (in microseconds).
 
diff --git a/update.go b/update.go
index e5de1a77..fc2d656a 100644
--- a/update.go
+++ b/update.go
@@ -44,6 +44,7 @@ The accepted format is as follow (unchanged values can be omitted):
   "cpu": {
     "shares": 0,
     "quota": 0,
+    "burst": 0,
     "period": 0,
     "realtimeRuntime": 0,
     "realtimePeriod": 0,
@@ -73,6 +74,10 @@ other options are ignored.
 			Name:  "cpu-quota",
 			Usage: "CPU CFS hardcap limit (in usecs). Allowed cpu time in a given period",
 		},
+		cli.StringFlag{
+			Name:  "cpu-burst",
+			Usage: "CPU CFS hardcap burst limit (in usecs). Allowed accumulated cpu time additionally for burst a given period",
+		},
 		cli.StringFlag{
 			Name:  "cpu-share",
 			Usage: "CPU shares (relative weight vs. other containers)",
@@ -153,6 +158,7 @@ other options are ignored.
 			CPU: &specs.LinuxCPU{
 				Shares:          u64Ptr(0),
 				Quota:           i64Ptr(0),
+				Burst:           u64Ptr(0),
 				Period:          u64Ptr(0),
 				RealtimeRuntime: i64Ptr(0),
 				RealtimePeriod:  u64Ptr(0),
@@ -210,6 +216,7 @@ other options are ignored.
 				opt  string
 				dest *uint64
 			}{
+				{"cpu-burst", r.CPU.Burst},
 				{"cpu-period", r.CPU.Period},
 				{"cpu-rt-period", r.CPU.RealtimePeriod},
 				{"cpu-share", r.CPU.Shares},
@@ -299,6 +306,7 @@ other options are ignored.
 			}
 		}
 
+		config.Cgroups.Resources.CpuBurst = r.CPU.Burst
 		config.Cgroups.Resources.CpuShares = *r.CPU.Shares
 		// CpuWeight is used for cgroupv2 and should be converted
 		config.Cgroups.Resources.CpuWeight = cgroups.ConvertCPUSharesToCgroupV2Value(*r.CPU.Shares)


Test patch:
diff --git a/libcontainer/cgroups/fs/cpu_test.go b/libcontainer/cgroups/fs/cpu_test.go
index bbdd45a7..4848b56a 100644
--- a/libcontainer/cgroups/fs/cpu_test.go
+++ b/libcontainer/cgroups/fs/cpu_test.go
@@ -45,6 +45,7 @@ func TestCpuSetBandWidth(t *testing.T) {
 	const (
 		quotaBefore     = 8000
 		quotaAfter      = 5000
+		burstBefore     = 2000
 		periodBefore    = 10000
 		periodAfter     = 7000
 		rtRuntimeBefore = 8000
@@ -52,9 +53,11 @@ func TestCpuSetBandWidth(t *testing.T) {
 		rtPeriodBefore  = 10000
 		rtPeriodAfter   = 7000
 	)
+	burstAfter := uint64(1000)
 
 	writeFileContents(t, path, map[string]string{
 		"cpu.cfs_quota_us":  strconv.Itoa(quotaBefore),
+		"cpu.cfs_burst_us":  strconv.Itoa(burstBefore),
 		"cpu.cfs_period_us": strconv.Itoa(periodBefore),
 		"cpu.rt_runtime_us": strconv.Itoa(rtRuntimeBefore),
 		"cpu.rt_period_us":  strconv.Itoa(rtPeriodBefore),
@@ -62,6 +65,7 @@ func TestCpuSetBandWidth(t *testing.T) {
 
 	r := &configs.Resources{
 		CpuQuota:     quotaAfter,
+		CpuBurst:     &burstAfter,
 		CpuPeriod:    periodAfter,
 		CpuRtRuntime: rtRuntimeAfter,
 		CpuRtPeriod:  rtPeriodAfter,
@@ -79,6 +83,14 @@ func TestCpuSetBandWidth(t *testing.T) {
 		t.Fatal("Got the wrong value, set cpu.cfs_quota_us failed.")
 	}
 
+	burst, err := fscommon.GetCgroupParamUint(path, "cpu.cfs_burst_us")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if burst != burstAfter {
+		t.Fatal("Got the wrong value, set cpu.cfs_burst_us failed.")
+	}
+
 	period, err := fscommon.GetCgroupParamUint(path, "cpu.cfs_period_us")
 	if err != nil {
 		t.Fatal(err)
diff --git a/tests/integration/helpers.bash b/tests/integration/helpers.bash
index 65912951..cd08fb24 100755
--- a/tests/integration/helpers.bash
+++ b/tests/integration/helpers.bash
@@ -260,11 +260,16 @@ function get_cgroup_value() {
 # Helper to check a if value in a cgroup file matches the expected one.
 function check_cgroup_value() {
 	local current
+	local cgroup
+	cgroup="$(get_cgroup_path "$1")"
+	if [ ! -f "$cgroup/$1" ]; then
+		skip "$cgroup/$1 does not exist"
+	fi
 	current="$(get_cgroup_value "$1")"
 	local expected=$2
 
 	echo "current $current !? $expected"
-	[ "$current" = "$expected" ]
+	[ "$current" = "$expected" ] || [ "$current" = "$((expected / 1000))" ]
 }
 
 # Helper to check a value in systemd.
@@ -310,6 +315,15 @@ function check_cpu_quota() {
 	check_systemd_value "CPUQuotaPeriodUSec" $sd_period $sd_infinity
 }
 
+function check_cpu_burst() {
+	local burst=$1
+	if [ -v CGROUP_V2 ]; then
+		check_cgroup_value "cpu.max.burst" "$burst"
+	else
+		check_cgroup_value "cpu.cfs_burst_us" "$burst"
+	fi
+}
+
 # Works for cgroup v1 and v2, accepts v1 shares as an argument.
 function check_cpu_shares() {
 	local shares=$1
diff --git a/tests/integration/update.bats b/tests/integration/update.bats
index 5a87fa15..616fe809 100644
--- a/tests/integration/update.bats
+++ b/tests/integration/update.bats
@@ -288,6 +288,12 @@ EOF
 	runc update test_update --cpu-share 200
 	[ "$status" -eq 0 ]
 	check_cpu_shares 200
+	runc update test_update --cpu-period 900000 --cpu-burst 500000
+	[ "$status" -eq 0 ]
+	check_cpu_burst 500000
+	runc update test_update --cpu-period 900000 --cpu-burst 0
+	[ "$status" -eq 0 ]
+	check_cpu_burst 0
 
 	# Revert to the test initial value via json on stding
 	runc update -r - test_update <<EOF