Do not convert blkio weight value using blkio->io conversion scheme #2786

dqminh · 2021-02-03T11:34:39Z

bfq weight controller (i.e. io.bfq.weight if present) is still using the
same bfq weight scheme (i.e 1->1000, see [1].) Unfortunaly the
documentation for this was wrong, and only fixed recently [2].

Therefore, if we map blkio weight to io.bfq.weight, there's no need to
do any conversion.

[1] https://github.com/torvalds/linux/blob/master/Documentation/block/bfq-iosched.rst
[2] torvalds/linux@65752ae

dqminh · 2021-02-03T11:35:37Z

This will also need to be fixed in crun and systemd among other things afair @giuseppe

More details here: opencontainers/runc#2786 Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe · 2021-02-03T13:20:54Z

This will also need to be fixed in crun and systemd among other things afair @giuseppe

thanks for letting me know! I've opened a PR for crun as well: containers/crun#584

giuseppe · 2021-02-03T13:21:18Z

@keszybz FYI

dqminh · 2021-02-03T14:59:12Z

Updated to add a test so we can try to catch this in the future. I'm not sure if there's movement to merge io.weight and io.bfq.weight at the moment, at which point we need to fix the inconsistency between 2 interface.

kolyshkin

LGTM

kolyshkin · 2021-02-03T18:58:11Z

CI failure on Fedora is a glitch (of download.fedoraproject.org) -- created #2787 to address.

CI restared.

AkihiroSuda · 2021-02-03T23:56:47Z

tests/integration/cgroups.bats

@@ -190,6 +190,22 @@ function setup() {
 	[[ "$output" == *'invalid configuration'* ]]
 }

+@test "runc run (blkio weight)" {
+	requires root cgroups_v2


Shouldn't require root?

@AkihiroSuda once #2818 is merged we will be able to enable it (rm root, add [[ "$ROOTLESS" -ne 0 ]] && requires rootless_cgroup)

...because currently runc exec test_cgroups_unified sh -c 'cat /sys/fs/cgroup/io.bfq.weight' is not working

cyphar · 2021-02-04T03:03:29Z

tests/integration/cgroups.bats

+	set_cgroups_path "$BUSYBOX_BUNDLE"
+	update_config '.linux.resources.blockIO |= {"weight": 750}' "${BUSYBOX_BUNDLE}"
+
+	runc run -d --console-socket "$CONSOLE_SOCKET" test_cgroups_v2_blkio


If you add a new container name you need to add it to the teardown() hook at the start -- teardown_running_container.

I noticed it, too, but did not comment because I am fixing all this soon (after #2757 is merged).

cyphar · 2021-02-04T03:05:12Z

tests/integration/cgroups.bats

+
+	runc exec test_cgroups_v2_blkio sh -c 'cat /sys/fs/cgroup/io.bfq.weight'
+	[ "$status" -eq 0 ]
+	echo "$output"


This shouldn't be necessary? runc is a function which prints the output to the bats log (I'm also pretty sure a naked echo doesn't work which is why runc redirects to stderr):

# Wrapper for runc. function runc() { run __runc "$@" # Some debug information to make life easier. bats will only print it if the # test failed, in which case the output is useful. echo "runc $@ (status=$status):" >&2 echo "$output" >&2 } # Raw wrapper for runc. function __runc() { "$RUNC" ${RUNC_USE_SYSTEMD+--systemd-cgroup} --root "$ROOT" "$@" }

cyphar

Test needs to be cleaned up.

keszybz · 2021-02-04T08:27:57Z

@giuseppe thanks for the ping.

This will also need to be fixed in crun and systemd among other things afair

Do I get this right that io.bfq.weight wants a value that is in the range 1..1000, while io.weight wants a value in the range 1..10000? systemd treats the cgroupv2 range as the input value, so we'd need to add a conversion io.bfq.weight = (io.weight + 9) / 10.

keszybz · 2021-02-04T08:34:17Z

^ @paolo-github

keszybz · 2021-02-04T08:52:04Z

systemd/systemd#18467

dqminh · 2021-02-04T14:14:30Z

@keszybz yes, i think that is correct.

Which then also raises more questions:

Do we want to set both interface ( like what systemd is doing ) ? In our case, since the spec is saying that the allowed range is 0-1000, and i assume that we don't want breaking changes, we still have to do the conversion to write to io.weight
I don't think runc's systemd v2 is setting IOWeight, is that right ? I tried to do a quick search and can't find any.

In general, io.weight iocost interface is also more likely to be available everywhere (is it ?, see containers/podman#8737), so maybe doing 1) makes sense.

kolyshkin

Indeed, the io.bfq.weight range is 1-1000, but io.weight range is 1-10000. runc is not currently setting io.weight.

paolo-github · 2021-02-05T07:59:15Z

If useful for you, I'll be glad to change the range for BFQ too. The current BFQ range is due just to legacy.

…

Il giorno 5 feb 2021, alle ore 02:36, Kir Kolyshkin ***@***.***> ha scritto: @kolyshkin commented on this pull request. Indeed, the io.bfq.weight range is 1-1000, but io.weight range is 1-10000. runc is not currently setting io.weight. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

kolyshkin · 2021-02-23T19:47:40Z

Took a liberty to

remove root requirement from the integration test case (as per Do not convert blkio weight value using blkio->io conversion scheme #2786 (comment));
fix a typo in the commit message
rebase.

kolyshkin

LGTM

kolyshkin · 2021-02-23T20:09:22Z

@AkihiroSuda PTAL

kolyshkin · 2021-02-23T22:20:11Z

The test case is not working under rootless on Fedora:

not ok 10 runc run (blkio weight)
# (in test file /vagrant/tests/integration/cgroups.bats, line 203)
#   `[ "$status" -eq 0 ]' failed
# runc spec --rootless (status=0):
# 
# runc run -d --console-socket /tmp/console.sock test_cgroups_unified (status=0):
# 
# runc exec test_cgroups_unified sh -c cat /sys/fs/cgroup/io.bfq.weight (status=1):
# cat: can't open '/sys/fs/cgroup/io.bfq.weight': No such file or directory

Apparently the problem is, rootless containers somehow do no honor cgroupns. Instead of "just this container" we see everything in container's view of /sys/fs/cgroup.

Looking...

kolyshkin · 2021-02-23T22:34:53Z

Normal container's cgroup2 mount:

[kir@kir-rhat runc]$ cat /proc/2474364/mountinfo | grep cgroup
1956 1955 0:27 /user.slice/user-1000.slice/[email protected]/x453 /sys/fs/cgroup ro,nosuid,nodev,noexec,relatime - cgroup2 cgroup rw,seclabel

rootless container cgroup2 mount:

[kir@kir-rhat runc]$ cat /proc/2475909/mountinfo | grep cgroup
30 23 0:27 / /sys/fs/cgroup rw,nosuid,nodev,noexec,relatime shared:4 - cgroup2 cgroup2 rw,seclabel

Ah, so it is caused by #2158. Hmmm.

dqminh · 2021-02-23T22:58:14Z

@kolyshkin sorry for the delay. yes i indeed hit this along the removal of root for the test so it wasn't done, and i forgot to put a comment on that.

kolyshkin · 2021-02-23T23:31:10Z

@dqminh your patch is fine (note that I pushed into your branch though), the rootless cgroup mount is a separate issue I'm going to address.

bfq weight controller (i.e. io.bfq.weight if present) is still using the same bfq weight scheme (i.e 1->1000, see [1].) Unfortunately the documentation for this was wrong, and only fixed recently [2]. Therefore, if we map blkio weight to io.bfq.weight, there's no need to do any conversion. Otherwise, we will try to write invalid value which results in error such as: ``` time="2021-02-03T14:55:30Z" level=error msg="container_linux.go:367: starting container process caused: process_linux.go:495: container init caused: process_linux.go:458: setting cgroup config for procHooks process caused: failed to write \"7475\": write /sys/fs/cgroup/runc-cgroups-integration-test/test-cgroup/io.bfq.weight: numerical result out of range" ``` [1] https://github.com/torvalds/linux/blob/master/Documentation/block/bfq-iosched.rst [2] torvalds/linux@65752ae Signed-off-by: Daniel Dao <[email protected]>

kolyshkin

LGTM

kolyshkin · 2021-02-24T03:48:59Z

@AkihiroSuda PTAL

kolyshkin · 2021-02-24T04:48:54Z

@cyphar PTAL (you requested changes and thus merging is blocked until you approve). Seems your review comments are now addressed.

cyphar

LGTM. As an aside, feel free to dismiss my old reviews if they are clearly no longer applicable. (I wish GitHub did this already, because it treats approvals as something that can go stale but "request changes" reviews aren't treated as stale.)

BFQ weight controller is using the same BFQ weight scheme (i.e 1->1000). Therefore, there is no need to do the conversion. More details here: opencontainers/runc#2786 Fixes: kata-containers#1440 Signed-off-by: Manabu Sugimoto <[email protected]>

The way libcontainer and lots of other runtime deals with blockio setting in cgroupv2 is buggy, see [1] For now, we disable BlockIO until a fix can land in libcontainer. [1] opencontainers/runc#2786 Signed-off-by: Daniel Dao <[email protected]>

dqminh requested review from AkihiroSuda and kolyshkin February 3, 2021 11:34

AkihiroSuda added this to the 1.0.0-rc94 milestone Feb 3, 2021

AkihiroSuda added area/cgroupv2 kind/bug impact/changelog labels Feb 3, 2021

giuseppe added a commit to giuseppe/crun that referenced this pull request Feb 3, 2021

cgroup: fix conversion from blkio to io

de6082f

More details here: opencontainers/runc#2786 Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe mentioned this pull request Feb 3, 2021

cgroup: fix conversion from blkio to io containers/crun#584

Merged

dqminh force-pushed the fix-blkio-weight branch from 29e782f to 7b13d84 Compare February 3, 2021 14:57

kolyshkin previously approved these changes Feb 3, 2021

View reviewed changes

AkihiroSuda reviewed Feb 3, 2021

View reviewed changes

cyphar reviewed Feb 4, 2021

View reviewed changes

cyphar requested changes Feb 4, 2021

View reviewed changes

cevich mentioned this pull request Feb 4, 2021

Error: opening file io.bfq.weight for writing: Permission denied containers/podman#8737

Closed

kolyshkin reviewed Feb 5, 2021

View reviewed changes

kolyshkin previously approved these changes Feb 23, 2021

View reviewed changes

kolyshkin requested review from AkihiroSuda and cyphar February 23, 2021 20:09

kolyshkin mentioned this pull request Feb 24, 2021

Fix cgroup2 mount for rootless case #2818

Merged

kolyshkin dismissed their stale review via c3ffd2e February 24, 2021 03:46

kolyshkin force-pushed the fix-blkio-weight branch from b9eed6c to c3ffd2e Compare February 24, 2021 03:46

kolyshkin approved these changes Feb 24, 2021

View reviewed changes

AkihiroSuda approved these changes Feb 24, 2021

View reviewed changes

cyphar approved these changes Feb 24, 2021

View reviewed changes

cyphar closed this in c153261 Feb 24, 2021

cyphar merged commit c153261 into opencontainers:master Feb 24, 2021

kolyshkin mentioned this pull request May 3, 2021

rc94 discussion (mid-April 2021?) #2790

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not convert blkio weight value using blkio->io conversion scheme #2786

Do not convert blkio weight value using blkio->io conversion scheme #2786

dqminh commented Feb 3, 2021

dqminh commented Feb 3, 2021

giuseppe commented Feb 3, 2021

giuseppe commented Feb 3, 2021

dqminh commented Feb 3, 2021

kolyshkin left a comment

kolyshkin commented Feb 3, 2021

AkihiroSuda Feb 3, 2021

kolyshkin Feb 24, 2021

kolyshkin Feb 24, 2021

cyphar Feb 4, 2021

kolyshkin Feb 4, 2021

cyphar Feb 4, 2021

cyphar left a comment

keszybz commented Feb 4, 2021

keszybz commented Feb 4, 2021

keszybz commented Feb 4, 2021

dqminh commented Feb 4, 2021

kolyshkin left a comment •

edited

Loading

paolo-github commented Feb 5, 2021 via email

kolyshkin commented Feb 23, 2021 •

edited

Loading

kolyshkin left a comment

kolyshkin commented Feb 23, 2021

kolyshkin commented Feb 23, 2021

kolyshkin commented Feb 23, 2021

dqminh commented Feb 23, 2021

kolyshkin commented Feb 23, 2021

kolyshkin left a comment

kolyshkin commented Feb 24, 2021

kolyshkin commented Feb 24, 2021

cyphar left a comment •

edited

Loading

Do not convert blkio weight value using blkio->io conversion scheme #2786

Do not convert blkio weight value using blkio->io conversion scheme #2786

Conversation

dqminh commented Feb 3, 2021

dqminh commented Feb 3, 2021

giuseppe commented Feb 3, 2021

giuseppe commented Feb 3, 2021

dqminh commented Feb 3, 2021

kolyshkin left a comment

Choose a reason for hiding this comment

kolyshkin commented Feb 3, 2021

AkihiroSuda Feb 3, 2021

Choose a reason for hiding this comment

kolyshkin Feb 24, 2021

Choose a reason for hiding this comment

kolyshkin Feb 24, 2021

Choose a reason for hiding this comment

cyphar Feb 4, 2021

Choose a reason for hiding this comment

kolyshkin Feb 4, 2021

Choose a reason for hiding this comment

cyphar Feb 4, 2021

Choose a reason for hiding this comment

cyphar left a comment

Choose a reason for hiding this comment

keszybz commented Feb 4, 2021

keszybz commented Feb 4, 2021

keszybz commented Feb 4, 2021

dqminh commented Feb 4, 2021

kolyshkin left a comment • edited Loading

Choose a reason for hiding this comment

paolo-github commented Feb 5, 2021 via email

kolyshkin commented Feb 23, 2021 • edited Loading

kolyshkin left a comment

Choose a reason for hiding this comment

kolyshkin commented Feb 23, 2021

kolyshkin commented Feb 23, 2021

kolyshkin commented Feb 23, 2021

dqminh commented Feb 23, 2021

kolyshkin commented Feb 23, 2021

kolyshkin left a comment

Choose a reason for hiding this comment

kolyshkin commented Feb 24, 2021

kolyshkin commented Feb 24, 2021

cyphar left a comment • edited Loading

Choose a reason for hiding this comment

kolyshkin left a comment •

edited

Loading

kolyshkin commented Feb 23, 2021 •

edited

Loading

cyphar left a comment •

edited

Loading