Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libcontainer/cgroups/fs/blkio: support BFQ weight[_device] #3010

Merged
merged 1 commit into from
Jun 11, 2021

Conversation

askervin
Copy link
Contributor

@askervin askervin commented Jun 8, 2021

  • Update the blkio cgroup to support the BFQ I/O Scheduler, that has
    replaced CFQ in the Linux kernel.
  • BFQ is controlled through blkio.bfq.weight[_device] instead of
    CFQ's blkio.weight[_device] in cgroups v1.
  • BFQ does not support blkio.leaf_weight[_device], so that behavior
    remains untouched.
  • Do not change behavior on legacy CFQ systems.
  • Enable using blkio weights on BFQ systems.

Signed-off-by: Antti Kervinen antti.kervinen@intel.com

@askervin
Copy link
Contributor Author

askervin commented Jun 9, 2021

Hi @AkihiroSuda,

There's something essential that I should have brought up earlier:

  1. CFQ was removed from Linux kernel (commit f382fb0bcef4, "block: remove legacy IO schedulers") 10/2018.
  2. BFQ replaced CFQ in RHEL 8, released 5/2019.
  3. BFQ replaced CFQ in Ubuntu 19.10, released 10/2019.

That is, this patch will fix runc on Linux running less than 3-years-old kernel. Therefore I'm hoping this patch would be considered to be a fix rather than an enhancement.

Without this patch adjusting weight/weight_device parameters in blockio PRs for CRI-O and containerd will prevent creating containers on any up-to-date Linux:

# kubectl describe pod pod1
Events:
  Type     Reason     Age               From               Message
  ----     ------     ----              ----               -------
   ...
  Normal   Created    61s               kubelet            Created container pod1c1
  Normal   Pulled     61s               kubelet            Container image "busybox" already present on machine                                                        
  Warning  Failed     60s               kubelet            Error: container create failed: time="2021-06-09T11:25:57Z" 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: open /sys/fs/cgroup/blkio/kubepods.slice/kubepodspodc83ca3da_5552_4fd0_8e17_e62c417b3018.slice/crio-f28b255e00529312c9bd177b3186778b2b8f1369533c9f80a8d48df1455ba7bf.scope/blkio.weight: no such file or directory"

@AkihiroSuda AkihiroSuda modified the milestones: post-1.0, 1.0.0 Jun 9, 2021
@AkihiroSuda AkihiroSuda closed this Jun 9, 2021
@AkihiroSuda AkihiroSuda reopened this Jun 9, 2021
@askervin askervin force-pushed the 5D6_blkio_bfq_weight branch from 71fb791 to cdfadb6 Compare June 9, 2021 13:00
AkihiroSuda
AkihiroSuda previously approved these changes Jun 9, 2021
@kolyshkin
Copy link
Contributor

Interesting... we support getting stats from BFQ since PR #2407, but never got to set the parameters for it :)

@kolyshkin
Copy link
Contributor

I am looking at a similar code for cgroup v2. In there, we also have a fallback (implemented in commit 8c7ece1). There's a weight conversion needed though. Do we need some kind of conversion here as well?

One other thing is, I think we need an integration test for this.

It seems that for BFQ we need kernel 5.x, but since this is cgroup v1, our Fedora is not good. Do we need to add a recent Debian or Ubuntu-based test? Or reboot the same Fedora image with cgroup v1 (modify kernel parameters with something like grubby --args="systemd.unified_cgroup_hierarchy=0" --update-kernel $(grubby --default-kernel), reboot, wait, repeat all the tests with cgroup v1).

@askervin
Copy link
Contributor Author

I am looking at a similar code for cgroup v2. In there, we also have a fallback (implemented in commit 8c7ece1). There's a weight conversion needed though. Do we need some kind of conversion here as well?

Good question. The situation is:

  • CFQ blkio.weight has range [10..1000], the default is 500.
  • BFQ blkio.bfq.weight has range [1..1000], the default is 100.
  • We do not have information on which I/O scheduler the user/admin thinks is under the hood.

I considered two options:

  1. A full-range conversion (linear, or non-linear due to different defaults). I believe that converting values on our code only adds complexity and is likely to result in bug reports like I tried to set weight "100", but got "500" or "101".

  2. Convert values < 10 to 10 if running CFQ. After all, even in this case I'd prefer Numerical result out of range error. It reveals that the system is running an unexpected I/O scheduler. Either the system needs reconfiguration or weights that the user/admin is using should be recalibrated. I think errors like this should not be hidden.

It seems that for BFQ we need kernel 5.x, but since this is cgroup v1, our Fedora is not good. Do we need to add a recent Debian or Ubuntu-based test? Or reboot the same Fedora image with cgroup v1 (modify kernel parameters with something like grubby --args="systemd.unified_cgroup_hierarchy=0" --update-kernel $(grubby --default-kernel), reboot, wait, repeat all the tests with cgroup v1).

Maybe adding a new platform to run all tests would be a good topic for another PR?

@askervin askervin force-pushed the 5D6_blkio_bfq_weight branch from 9fd77d7 to feceacc Compare June 10, 2021 11:31
@askervin askervin requested a review from kolyshkin June 10, 2021 11:33
@kolyshkin
Copy link
Contributor

@askervin thank you for a detailed reply! I agree that in this case no conversion is probably the best thing to do.

I also agree that testing can be discussed separately -- but without the test we can't be sure if the whole thing works or not (even if there is not too much code).

- Update the blkio cgroup to support the BFQ I/O Scheduler, that has
  replaced CFQ in the Linux kernel.
- BFQ is controlled through blkio.bfq.weight[_device] instead of
  CFQ's blkio.weight[_device] in cgroups v1.
- BFQ does not support blkio.leaf_weight[_device], so that behavior
  remains untouched.
- Do not change behavior on legacy CFQ systems.
- Enable using blkio weights on BFQ systems.

Signed-off-by: Antti Kervinen <antti.kervinen@intel.com>
@askervin askervin force-pushed the 5D6_blkio_bfq_weight branch from feceacc to 6339d8a Compare June 11, 2021 08:18
@askervin
Copy link
Contributor Author

@kolyshkin , Thanks for your feedback on unit tests, too!

I updated these tests so that they cover

  1. detecting the weight file with BFQ and CFQ cases (legacyIOScheduler false and true, respectively).
  2. detecting the weight_device file with BFQ and CFQ cases
  3. detecting files when blkio controller is disabled: no weight files in the cgroup. Should try to use (and in real case fail using) BFQ files in this case.

@askervin askervin requested a review from AkihiroSuda June 11, 2021 08:29
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and thanks!

@kolyshkin kolyshkin merged commit 0ca4cda into opencontainers:master Jun 11, 2021
@kolyshkin
Copy link
Contributor

Ah, this is why I wanted a test case:

not ok 9 runc run (per-device io weight)
# (in test file tests/integration/cgroups.bats, line 173)
#   `[ "$status" -eq 0 ]' failed
# runc spec (status=0):
# 
# 16+0 records in
# 16+0 records out
# 65536 bytes (66 kB, 64 KiB) copied, 0.000114909 s, 570 MB/s
# runc run -d --console-socket /tmp/bats-run-14673/runc.axD6tf/tty/sock test_dev_weight (status=1):
# time="2021-06-14T00:54:57Z" level=error msg="container_linux.go:380: starting container process caused: process_linux.go:545: container init caused: process_linux.go:508: setting cgroup config for procHooks process caused: openat2 /sys/fs/cgroup/blkio/system.slice/runner-provisioner.service/test_dev_weight/blkio.leaf_weight_device: no such file or directory"

Fix is coming

@kolyshkin
Copy link
Contributor

The fix is part of #3022 00b8e31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants