Skip to content

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

Merged
kolyshkin merged 1 commit intoopencontainers:masterfrom
askervin:5D6_blkio_bfq_weight
Jun 11, 2021
Merged

libcontainer/cgroups/fs/blkio: support BFQ weight[_device]#3010
kolyshkin merged 1 commit intoopencontainers:masterfrom
askervin:5D6_blkio_bfq_weight

Conversation

@askervin
Copy link
Copy Markdown
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

@AkihiroSuda AkihiroSuda added this to the post-1.0 milestone Jun 8, 2021
@askervin askervin force-pushed the 5D6_blkio_bfq_weight branch from 669c539 to 71fb791 Compare June 9, 2021 10:47
@askervin
Copy link
Copy Markdown
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
AkihiroSuda
AkihiroSuda reviewed Jun 9, 2021
View reviewed changes
@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
View reviewed changes
@askervin askervin mentioned this pull request Jun 9, 2021
Open
kolyshkin
kolyshkin reviewed Jun 9, 2021
View reviewed changes
kolyshkin
kolyshkin reviewed Jun 9, 2021
View reviewed changes
@kolyshkin
Copy link
Copy Markdown
Contributor

kolyshkin commented Jun 9, 2021

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

@kolyshkin
Copy link
Copy Markdown
Contributor

kolyshkin commented Jun 9, 2021

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 askervin force-pushed the 5D6_blkio_bfq_weight branch from cdfadb6 to 9fd77d7 Compare June 10, 2021 10:31
@askervin
Copy link
Copy Markdown
Contributor Author

askervin commented Jun 10, 2021

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
Copy Markdown
Contributor

kolyshkin commented Jun 10, 2021

@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).

kolyshkin
kolyshkin reviewed Jun 10, 2021
View reviewed changes
kolyshkin
kolyshkin reviewed Jun 10, 2021
View reviewed changes
kolyshkin
kolyshkin reviewed Jun 10, 2021
View reviewed changes
@askervin
libcontainer/cgroups/fs/blkio: support BFQ weight[_device]
6339d8a 
- 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
Copy Markdown
Contributor Author

askervin commented Jun 11, 2021

@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
kolyshkin
kolyshkin approved these changes Jun 11, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

kolyshkin commented Jun 14, 2021

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
Copy Markdown
Contributor

kolyshkin commented Jun 14, 2021

The fix is part of #3022 00b8e31

This was referenced Jun 14, 2021
Merged
Merged
Maks1mS pushed a commit to stplr-dev/stplr that referenced this pull request Mar 14, 2026
fix(deps): update module github.com/opencontainers/runc to v1.4.1 (#361)
2e27228 
This PR contains the following updates:

| Package | Type | Update | Change | OpenSSF |
|---|---|---|---|---|
| [github.com/opencontainers/runc](https://github.com/opencontainers/runc) | require | patch | `v1.4.0` → `v1.4.1` | [![OpenSSF Scorecard](https://api.securityscorecards.dev/projects/github.com/opencontainers/runc/badge)](https://securityscorecards.dev/viewer/?uri=github.com/opencontainers/runc) |

---

> ⚠️ **Warning**
>
> Some dependencies could not be looked up. Check the [Dependency Dashboard](issues/23) for more information.

---

### Release Notes

<details>
<summary>opencontainers/runc (github.com/opencontainers/runc)</summary>

### [`v1.4.1`](https://github.com/opencontainers/runc/blob/HEAD/CHANGELOG.md#100---2021-06-22)

[Compare Source](opencontainers/runc@v1.4.0...v1.4.1)

> A wizard is never late, nor is he early, he arrives precisely when he means
> to.

As runc follows Semantic Versioning, we will endeavour to not make any
breaking changes without bumping the major version number of runc.
However, it should be noted that Go API usage of runc's internal
implementation (libcontainer) is *not* covered by this policy.

##### Removed

- Removed libcontainer/configs.Device\* identifiers (deprecated since rc94,
  use libcontainer/devices). ([#&#8203;2999](opencontainers/runc#2999))
- Removed libcontainer/system.RunningInUserNS function (deprecated since
  rc94, use libcontainer/userns). ([#&#8203;2999](opencontainers/runc#2999))

##### Deprecated

- The usage of relative paths for mountpoints will now produce a warning
  (such configurations are outside of the spec, and in future runc will
  produce an error when given such configurations). ([#&#8203;2917](opencontainers/runc#2917), [#&#8203;3004](opencontainers/runc#3004))

##### Fixed

- cgroupv2: devices: rework the filter generation to produce consistent
  results with cgroupv1, and always clobber any existing eBPF
  program(s) to fix `runc update` and avoid leaking eBPF programs
  (resulting in errors when managing containers).  ([#&#8203;2951](opencontainers/runc#2951))
- cgroupv2: correctly convert "number of IOs" statistics in a
  cgroupv1-compatible way. ([#&#8203;2965](opencontainers/runc#2965), [#&#8203;2967](opencontainers/runc#2967), [#&#8203;2968](opencontainers/runc#2968), [#&#8203;2964](opencontainers/runc#2964))
- cgroupv2: support larger than 32-bit IO statistics on 32-bit architectures.
- cgroupv2: wait for freeze to finish before returning from the freezing
  code, optimize the method for checking whether a cgroup is frozen. ([#&#8203;2955](opencontainers/runc#2955))
- cgroups/systemd: fixed "retry on dbus disconnect" logic introduced in rc94
- cgroups/systemd: fixed returning "unit already exists" error from a systemd
  cgroup manager (regression in rc94). ([#&#8203;2997](opencontainers/runc#2997), [#&#8203;2996](opencontainers/runc#2996))

##### Added

- cgroupv2: support SkipDevices with systemd driver. ([#&#8203;2958](opencontainers/runc#2958), [#&#8203;3019](opencontainers/runc#3019))
- cgroup1: blkio: support BFQ weights. ([#&#8203;3010](opencontainers/runc#3010))
- cgroupv2: set per-device io weights if BFQ IO scheduler is available.
  ([#&#8203;3022](opencontainers/runc#3022))

##### Changed

- cgroup/systemd: return, not ignore, stop unit error from Destroy. ([#&#8203;2946](opencontainers/runc#2946))
- Fix all golangci-lint failures. ([#&#8203;2781](opencontainers/runc#2781), [#&#8203;2962](opencontainers/runc#2962))
- Make `runc --version` output sane even when built with `go get` or
  otherwise outside of our build scripts. ([#&#8203;2962](opencontainers/runc#2962))
- cgroups: set SkipDevices during runc update (so we don't modify
  cgroups at all during `runc update`). ([#&#8203;2994](opencontainers/runc#2994))

<!-- minor releases -->

[Unreleased]: opencontainers/runc@v1.3.0-rc.1...HEAD

[1.3.0]: opencontainers/runc@v1.3.0-rc.2...v1.3.0

[1.2.0]: opencontainers/runc@v1.2.0-rc.1...v1.2.0

[1.1.0]: opencontainers/runc@v1.1.0-rc.1...v1.1.0

[1.0.0]: https://github.com/opencontainers/runc/releases/tag/v1.0.0

<!-- 1.0.z patch releases -->

[Unreleased 1.0.z]: opencontainers/runc@v1.0.3...release-1.0

[1.0.3]: opencontainers/runc@v1.0.2...v1.0.3

[1.0.2]: opencontainers/runc@v1.0.1...v1.0.2

[1.0.1]: opencontainers/runc@v1.0.0...v1.0.1

<!-- 1.1.z patch releases -->

[Unreleased 1.1.z]: opencontainers/runc@v1.1.15...release-1.1

[1.1.15]: opencontainers/runc@v1.1.14...v1.1.15

[1.1.14]: opencontainers/runc@v1.1.13...v1.1.14

[1.1.13]: opencontainers/runc@v1.1.12...v1.1.13

[1.1.12]: opencontainers/runc@v1.1.11...v1.1.12

[1.1.11]: opencontainers/runc@v1.1.10...v1.1.11

[1.1.10]: opencontainers/runc@v1.1.9...v1.1.10

[1.1.9]: opencontainers/runc@v1.1.8...v1.1.9

[1.1.8]: opencontainers/runc@v1.1.7...v1.1.8

[1.1.7]: opencontainers/runc@v1.1.6...v1.1.7

[1.1.6]: opencontainers/runc@v1.1.5...v1.1.6

[1.1.5]: opencontainers/runc@v1.1.4...v1.1.5

[1.1.4]: opencontainers/runc@v1.1.3...v1.1.4

[1.1.3]: opencontainers/runc@v1.1.2...v1.1.3

[1.1.2]: opencontainers/runc@v1.1.1...v1.1.2

[1.1.1]: opencontainers/runc@v1.1.0...v1.1.1

[1.1.0-rc.1]: opencontainers/runc@v1.0.0...v1.1.0-rc.1

<!-- 1.2.z patch releases -->

[Unreleased 1.2.z]: opencontainers/runc@v1.2.9...release-1.2

[1.2.9]: opencontainers/runc@v1.2.8...v1.2.9

[1.2.8]: opencontainers/runc@v1.2.7...v1.2.8

[1.2.7]: opencontainers/runc@v1.2.6...v1.2.7

[1.2.6]: opencontainers/runc@v1.2.5...v1.2.6

[1.2.5]: opencontainers/runc@v1.2.4...v1.2.5

[1.2.4]: opencontainers/runc@v1.2.3...v1.2.4

[1.2.3]: opencontainers/runc@v1.2.2...v1.2.3

[1.2.2]: opencontainers/runc@v1.2.1...v1.2.2

[1.2.1]: opencontainers/runc@v1.2.0...v1.2.1

[1.2.0-rc.3]: opencontainers/runc@v1.2.0-rc.2...v1.2.0-rc.3

[1.2.0-rc.2]: opencontainers/runc@v1.2.0-rc.1...v1.2.0-rc.2

[1.2.0-rc.1]: opencontainers/runc@v1.1.0...v1.2.0-rc.1

<!-- 1.3.z patch releases -->

[Unreleased 1.3.z]: opencontainers/runc@v1.3.4...release-1.3

[1.3.4]: opencontainers/runc@v1.3.3...v1.3.4

[1.3.3]: opencontainers/runc@v1.3.2...v1.3.3

[1.3.2]: opencontainers/runc@v1.3.1...v1.3.2

[1.3.1]: opencontainers/runc@v1.3.0...v1.3.1

[1.3.0]: opencontainers/runc@v1.3.0-rc.2...v1.3.0

[1.3.0-rc.2]: opencontainers/runc@v1.3.0-rc.1...v1.3.0-rc.2

[1.3.0-rc.1]: opencontainers/runc@v1.2.0...v1.3.0-rc.1

<!-- 1.4.z patch releases -->

[Unreleased 1.4.z]: opencontainers/runc@v1.4.1...release-1.4

[1.4.1]: opencontainers/runc@v1.4.0...v1.4.1

[1.4.0]: opencontainers/runc@v1.4.0-rc.3...v1.4.0

[1.4.0-rc.3]: opencontainers/runc@v1.4.0-rc.2...v1.4.0-rc.3

[1.4.0-rc.2]: opencontainers/runc@v1.4.0-rc.1...v1.4.0-rc.2

[1.4.0-rc.1]: opencontainers/runc@v1.3.0...v1.4.0-rc.1

<!-- 1.5.z patch releases -->

[Unreleased 1.5.z]: opencontainers/runc@v1.5.0-rc.1...release-1.5

[1.5.0-rc.1]: opencontainers/runc@v1.4.0...v1.5.0-rc.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At 12:00 AM through 04:59 AM and 10:00 PM through 11:59 PM, Monday through Friday ( * 0-4,22-23 * * 1-5 ), Only on Sunday and Saturday ( * * * * 0,6 ) (UTC), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My41OS40IiwidXBkYXRlZEluVmVyIjoiNDMuNTkuNCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiS2luZC9EZXBlbmRlbmNpZXMiXX0=-->

Reviewed-on: https://altlinux.space/stapler/stplr/pulls/361
Co-authored-by: Renovate Bot <stapler-helper-bot@noreply.altlinux.space>
Co-committed-by: Renovate Bot <stapler-helper-bot@noreply.altlinux.space>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kolyshkin kolyshkin kolyshkin approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees

No one assigned

Labels

area/cgroupv1 kind/bug

Projects

None yet

Milestone

1.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@askervin @kolyshkin @AkihiroSuda