Add support for remaining blkio settings #3950

swagatbora90 · 2025-02-28T06:53:15Z

Nerdctl today has support for --blkio-weight.

This PR adds support for the remaining blkio options (as supported by docker).

--blkio-weight-device    stringArray     Block IO weight (relative device weight) (default [])
--device-read-bps        stringArray     Limit read rate (bytes per second) from a device (default [])
--device-read-iops       stringArray     Limit read rate (IO per second) from a device (default [])
--device-write-bps       stringArray     Limit write rate (bytes per second) to a device (default [])
--device-write-iops      stringArray     Limit write rate (IO per second) to a device (default [])

Requires cgroup v2 and block device scheduler set to bfq

Testing

nerdctl run --rm --blkio-weight=1000 ubuntu dd bs=1M count=10024 if=/dev/zero of=/dump.dat oflag=direct status=progress

nerdctl run --rm --blkio-weight-device="/dev/xvda:1000" ubuntu dd bs=1M count=10024 if=/dev/zero of=/dump.dat oflag=direct status=progress

[root@ip-172-31-8-69 bin]# /home/ec2-user/nerdctl run --rm --device-write-bps="/dev/xvda:10mb" ubuntu dd bs=1M count=10024 if=/dev/zero of=/dump.dat oflag=direct status=progress
52428800 bytes (52 MB, 50 MiB) copied, 5 s, 10.5 MB/s

swagatbora90 · 2025-02-28T16:54:46Z

Test failures do not appear to be related to the specific change. Previous runs have already passed those checks.

djdongjin · 2025-03-01T15:58:43Z

cmd/nerdctl/container/container_run_cgroup_linux_test.go

+
+func TestRunBlkioSettingCgroupV2(t *testing.T) {
+	t.Parallel()
+	testutil.DockerIncompatible(t)


why is this docker imcompatible?

The current test validation uses the created container's runtime spec to check for the specified values to be present. Calling containerd's socket is not support when running with Docker.

I think I can achieve the same by using container inspect instead, but need to populate the inspect response. I can add that in the next revision.

I have made the change to use inspect so the test can work with both nerdctl and docker. However, while investigating the docker failure, I found another issue related to a bug in docker cli docker/cli#5321. Some of the blkio options are broken in docker v26.0.1 <-> v27.1.1. The fix has been merged and backported to all branches, however the docker cli version available in the runner does not have the fix yet.

I have decided to temporarily disable the specific options test for docker. We can re-enable them once we have a newer docker cli version available. On another note, we can decide to use a specific(newer) version of docker clifor the integration tests. However, I think that is beyond the scope of this PR.

djdongjin · 2025-03-01T15:59:27Z

cmd/nerdctl/container/container_run_cgroup_linux_test.go

+	switch info.CgroupDriver {
+	case "none", "":
+		t.Skip("test requires cgroup driver")
+	}


duplicated with line 429-431

djdongjin · 2025-03-01T16:00:22Z

cmd/nerdctl/container/container_run_cgroup_linux_test.go

+	}
+
+	for _, tt := range tests {
+		tt := tt // capture range variable


this is no longer needed after go 1.22 (or 1.21? not sure the exact version)

djdongjin · 2025-03-01T16:03:45Z

docs/command-reference.md

@@ -414,7 +414,7 @@ IPFS flags:
 - :nerd_face: `--ipfs-address`: Multiaddr of IPFS API (default uses `$IPFS_PATH` env variable if defined or local directory `~/.ipfs`)

 Unimplemented `docker run` flags:
-    `--blkio-weight-device`, `--cpu-rt-*`, `--device-*`,
+    `--cpu-rt-*`, `--device-cgroup-rule`,


can you also add these new flags to the this command doc?

djdongjin · 2025-03-01T16:06:59Z

pkg/api/types/container_types.go

+	BlkioWeight uint16
+	// BlkioWeightDevice specifies the Block IO weight (relative device weight)
+	BlkioWeightDevice []string
+	// BlkioDeviceReadBps specifies the Block IO read rate limit(bytes per second) of a device
+	BlkioDeviceReadBps []string
+	// BlkioDeviceWriteBps specifies the Block IO write rate limit(bytes per second) of a device
+	BlkioDeviceWriteBps []string
+	// BlkioDeviceReadIOps specifies the Block IO read rate limit(IO per second) of a device
+	BlkioDeviceReadIOps []string
+	// BlkioDeviceWriteIOps specifies the Block IO read rate limit(IO per second) of a device
+	BlkioDeviceWriteIOps []string


Do we need to support these flags in other commands? If so I suggest we create a separate struct, e.g., Blkio.

These flags are only used in run and create. Besides that update only support blkioweight.

swagatbora90 · 2025-03-11T22:25:23Z

The failing tests appear to be unrelated to the changes.

swagatbora90 · 2025-03-11T22:25:51Z

@djdongjin I have addressed most of your comments. PTAL.

djdongjin · 2025-03-11T22:42:53Z

@swagatbora90 is this CI failure related?

=== Failed
=== FAIL: cmd/nerdctl/container TestContainerInspectHostConfig (0.37s)
    container_inspect_linux_test.go:269: assertion failed: 500 (uint16) != 0 (inspect.HostConfig.BlkioWeight uint16)

=== FAIL: cmd/nerdctl/container TestContainerInspectHostConfig (re-run 1) (0.57s)
    container_inspect_linux_test.go:269: assertion failed: 500 (uint16) != 0 (inspect.HostConfig.BlkioWeight uint16)

=== FAIL: cmd/nerdctl/container TestContainerInspectHostConfig (re-run 2) (0.52s)
    container_inspect_linux_test.go:269: assertion failed: 500 (uint16) != 0 (inspect.HostConfig.BlkioWeight uint16)

DONE 3 runs, 825 tests, 120 skipped, 3 failures in 1580.158s
Sending SIGTERM to remaining processes...

https://github.com/containerd/nerdctl/actions/runs/13775727448/job/38524458510?pr=3950

swagatbora90 · 2025-03-11T23:43:43Z

@swagatbora90 is this CI failure related?

=== Failed
=== FAIL: cmd/nerdctl/container TestContainerInspectHostConfig (0.37s)
    container_inspect_linux_test.go:269: assertion failed: 500 (uint16) != 0 (inspect.HostConfig.BlkioWeight uint16)

=== FAIL: cmd/nerdctl/container TestContainerInspectHostConfig (re-run 1) (0.57s)
    container_inspect_linux_test.go:269: assertion failed: 500 (uint16) != 0 (inspect.HostConfig.BlkioWeight uint16)

=== FAIL: cmd/nerdctl/container TestContainerInspectHostConfig (re-run 2) (0.52s)
    container_inspect_linux_test.go:269: assertion failed: 500 (uint16) != 0 (inspect.HostConfig.BlkioWeight uint16)

DONE 3 runs, 825 tests, 120 skipped, 3 failures in 1580.158s
Sending SIGTERM to remaining processes...

https://github.com/containerd/nerdctl/actions/runs/13775727448/job/38524458510?pr=3950

Ah I missed that. This is most probably related to my change. Taking a look.

swagatbora90 · 2025-03-12T17:33:34Z

Actually the mainline test should also fail, but previous container inspectwas simply relying on the container labels to retrieve the blkio-weight and not the actual container spec. I had changed that behavior in the current PR, so now it retrieves the blkio values from the container spec directly.

I ran the same test on mainline with some debug turned on, and I see this:

    container_inspect_linux_test.go:282: Container run output: 22eb56684dbf77c2cea9de88c8f3cd392523920820a6fa8843822d1365841ff7
        time="2025-03-12T17:16:52Z" level=warning msg="kernel support for cgroup blkio weight missing, weight discarded"

So looks like the blkio-weight was never getting applied. Also I know that blkio-weight has been deprecated in cgroupv1 since kernel 5.0 docker/cli#2908.

Also adds HostConfig inspect tests and skip unsupported docker tests Signed-off-by: Swagat Bora <[email protected]>

manugupt1 · 2025-03-14T00:44:04Z

pkg/cmd/container/run_blkio_linux.go

+func withBlkioReadIOPSDevice(devices []specs.LinuxThrottleDevice) oci.SpecOpts {
+	return func(_ context.Context, _ oci.Client, _ *containers.Container, s *specs.Spec) error {
+		if s.Linux.Resources.BlockIO == nil {
+			s.Linux.Resources.BlockIO = &specs.LinuxBlockIO{}


setting zero values has unintended consequences. They can over-write system settings provided by OS. Let's keep nil as nil

manugupt1 · 2025-03-14T00:44:09Z

pkg/cmd/container/run_blkio_linux.go

+
+func withBlkioWriteIOPSDevice(devices []specs.LinuxThrottleDevice) oci.SpecOpts {
+	return func(_ context.Context, _ oci.Client, _ *containers.Container, s *specs.Spec) error {
+		if s.Linux.Resources.BlockIO == nil {


setting zero values has unintended consequences. They can over-write system settings provided by OS. Let's keep nil as nil

manugupt1 · 2025-03-14T00:52:35Z

cmd/nerdctl/container/container_inspect_linux_test.go

+
+	// ensure the file will be removed in case of failed in the test
+	defer func() {
+		exec.Command("rm", devPath).Run()


nit: should be assertOk()

manugupt1 · 2025-03-14T00:52:52Z

cmd/nerdctl/container/container_inspect_linux_test.go

+	}()
+
+	base := testutil.NewBase(t)
+	defer base.Cmd("rm", "-f", testContainer).Run()


nit: should be assertOk() to prevent leaks

swagatbora90 force-pushed the blkio-options branch 4 times, most recently from 45a457d to ef71f08 Compare February 28, 2025 16:18

swagatbora90 marked this pull request as ready for review February 28, 2025 16:51

swagatbora90 force-pushed the blkio-options branch from ef71f08 to 39f89b3 Compare February 28, 2025 17:16

djdongjin reviewed Mar 1, 2025

View reviewed changes

swagatbora90 force-pushed the blkio-options branch 3 times, most recently from d7d06b4 to 5ebc362 Compare March 10, 2025 22:08

swagatbora90 force-pushed the blkio-options branch 2 times, most recently from b1c4eff to 7ecd71e Compare March 13, 2025 00:03

Add support for remaining blkio settings

cfb1704

Also adds HostConfig inspect tests and skip unsupported docker tests Signed-off-by: Swagat Bora <[email protected]>

swagatbora90 force-pushed the blkio-options branch from 7ecd71e to cfb1704 Compare March 13, 2025 22:03

manugupt1 reviewed Mar 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for remaining blkio settings #3950

Add support for remaining blkio settings #3950

swagatbora90 commented Feb 28, 2025 •

edited

Loading

swagatbora90 commented Feb 28, 2025 •

edited

Loading

djdongjin Mar 1, 2025

swagatbora90 Mar 3, 2025

swagatbora90 Mar 10, 2025

djdongjin Mar 1, 2025

djdongjin Mar 1, 2025

djdongjin Mar 1, 2025

djdongjin Mar 1, 2025

swagatbora90 Mar 3, 2025

swagatbora90 commented Mar 11, 2025

swagatbora90 commented Mar 11, 2025

djdongjin commented Mar 11, 2025

swagatbora90 commented Mar 11, 2025

swagatbora90 commented Mar 12, 2025

manugupt1 Mar 14, 2025

manugupt1 Mar 14, 2025

manugupt1 Mar 14, 2025

manugupt1 Mar 14, 2025

Add support for remaining blkio settings #3950

Are you sure you want to change the base?

Add support for remaining blkio settings #3950

Conversation

swagatbora90 commented Feb 28, 2025 • edited Loading

Testing

swagatbora90 commented Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swagatbora90 commented Mar 11, 2025

swagatbora90 commented Mar 11, 2025

djdongjin commented Mar 11, 2025

swagatbora90 commented Mar 11, 2025

swagatbora90 commented Mar 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swagatbora90 commented Feb 28, 2025 •

edited

Loading

swagatbora90 commented Feb 28, 2025 •

edited

Loading