Repository: opencontainers/runc
Pull request #4186 (2024-02-02 22:03:22)

libct/cg: write unified resources line by line
It has been pointed out (in #4182) that some controllers can not accept multiple lines of output at once. In particular, io.max can only set one device at a time.

Practically, the only multi-line resource values we can get come from unified.* -- let's write those line by line.

Add a test case.

Reported-by: Tao Shen <shentaoskyking@gmail.com>

Closes: #4182 

kolyshkin: @smileusd does this fix your issue?
smileusd: -> @smileusd does this fix your issue?
yes could you merge this
kolyshkin: @AkihiroSuda @thaJeztah PTAL

Patch:
diff --git a/libcontainer/cgroups/file.go b/libcontainer/cgroups/file.go
index 16aae5a3..78c5bcf0 100644
--- a/libcontainer/cgroups/file.go
+++ b/libcontainer/cgroups/file.go
@@ -57,6 +57,40 @@ func WriteFile(dir, file, data string) error {
 	return nil
 }
 
+// WriteFileByLine is the same as WriteFile, except if data contains newlines,
+// it is written line by line.
+func WriteFileByLine(dir, file, data string) error {
+	i := strings.Index(data, "\n")
+	if i == -1 {
+		return WriteFile(dir, file, data)
+	}
+
+	fd, err := OpenFile(dir, file, unix.O_WRONLY)
+	if err != nil {
+		return err
+	}
+	defer fd.Close()
+	start := 0
+	for {
+		var line string
+		if i == -1 {
+			line = data[start:]
+		} else {
+			line = data[start : start+i+1]
+		}
+		_, err := fd.WriteString(line)
+		if err != nil {
+			return fmt.Errorf("failed to write %q: %w", line, err)
+		}
+		if i == -1 {
+			break
+		}
+		start += i + 1
+		i = strings.Index(data[start:], "\n")
+	}
+	return nil
+}
+
 const (
 	cgroupfsDir    = "/sys/fs/cgroup"
 	cgroupfsPrefix = cgroupfsDir + "/"
diff --git a/libcontainer/cgroups/fs2/fs2.go b/libcontainer/cgroups/fs2/fs2.go
index 0a579b23..87c89e8a 100644
--- a/libcontainer/cgroups/fs2/fs2.go
+++ b/libcontainer/cgroups/fs2/fs2.go
@@ -238,7 +238,7 @@ func (m *Manager) setUnified(res map[string]string) error {
 		if strings.Contains(k, "/") {
 			return fmt.Errorf("unified resource %q must be a file name (no slashes)", k)
 		}
-		if err := cgroups.WriteFile(m.dirPath, k, v); err != nil {
+		if err := cgroups.WriteFileByLine(m.dirPath, k, v); err != nil {
 			// Check for both EPERM and ENOENT since O_CREAT is used by WriteFile.
 			if errors.Is(err, os.ErrPermission) || errors.Is(err, os.ErrNotExist) {
 				// Check if a controller is available,


Test patch:
diff --git a/libcontainer/cgroups/file_test.go b/libcontainer/cgroups/file_test.go
index 6236bb9e..5ce43838 100644
--- a/libcontainer/cgroups/file_test.go
+++ b/libcontainer/cgroups/file_test.go
@@ -73,3 +73,25 @@ func TestOpenat2(t *testing.T) {
 		fd.Close()
 	}
 }
+
+func BenchmarkWriteFile(b *testing.B) {
+	TestMode = true
+	defer func() { TestMode = false }()
+
+	dir := b.TempDir()
+	tc := []string{
+		"one",
+		"one\ntwo\nthree",
+		"10:200 foo=bar boo=far\n300:1200 something=other\ndefault 45000\n",
+		"\n\n\n\n\n\n\n\n",
+	}
+
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		for _, val := range tc {
+			if err := WriteFileByLine(dir, "file", val); err != nil {
+				b.Fatal(err)
+			}
+		}
+	}
+}
diff --git a/tests/integration/cgroups.bats b/tests/integration/cgroups.bats
index d5ca1325..3a74eb26 100644
--- a/tests/integration/cgroups.bats
+++ b/tests/integration/cgroups.bats
@@ -187,6 +187,38 @@ function setup() {
 	[[ "$weights" == *"$major:$minor 444"* ]]
 }
 
+@test "runc run (per-device multiple iops via unified)" {
+	requires root cgroups_v2
+
+	dd if=/dev/zero of=backing1.img bs=4096 count=1
+	dev1=$(losetup --find --show backing1.img) || skip "unable to create a loop device"
+
+	# Second device.
+	dd if=/dev/zero of=backing2.img bs=4096 count=1
+	dev2=$(losetup --find --show backing2.img) || skip "unable to create a loop device"
+
+	set_cgroups_path
+
+	IFS=$' \t:' read -r major1 minor1 <<<"$(lsblk -nd -o MAJ:MIN "$dev1")"
+	IFS=$' \t:' read -r major2 minor2 <<<"$(lsblk -nd -o MAJ:MIN "$dev2")"
+	update_config '	  .linux.devices += [
+				{path: "'"$dev1"'", type: "b", major: '"$major1"', minor: '"$minor1"'},
+				{path: "'"$dev2"'", type: "b", major: '"$major2"', minor: '"$minor2"'}
+			   ]
+			| .linux.resources.unified |=
+				{"io.max": "'"$major1"':'"$minor1"' riops=333 wiops=444\n'"$major2"':'"$minor2"' riops=555 wiops=666\n"}'
+	runc run -d --console-socket "$CONSOLE_SOCKET" test_dev_weight
+	[ "$status" -eq 0 ]
+
+	# The loop devices are no longer needed.
+	losetup -d "$dev1"
+	losetup -d "$dev2"
+
+	weights=$(get_cgroup_value "io.max")
+	grep "^$major1:$minor1 .* riops=333 wiops=444$" <<<"$weights"
+	grep "^$major2:$minor2 .* riops=555 wiops=666$" <<<"$weights"
+}
+
 @test "runc run (cpu.idle)" {
 	requires cgroups_cpu_idle
 	[ $EUID -ne 0 ] && requires rootless_cgroup