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.

Sign up for GitHub

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

Jump to bottom

Support for cgroups blockio #5490

Merged
merged 1 commit into from
Apr 29, 2022
Merged

Support for cgroups blockio #5490

merged 1 commit into from
Apr 29, 2022

Conversation

askervin
Copy link
Contributor

This PR implements support for the cgroup blockio controller.

How it is used

  • User defines blockio classes, for example: ThrottledIO and LowLatency. Class names are not restricted, and the number of classes is not limited.
  • Each class contains sets of devices (list of filenames, wildcards allowed) and blockio parameters for each set. For example, /etc/containers/blockio.yaml contains: 
    Classes:
  ThrottledIO:
  - Devices:
      - /dev/vd[a-z]
    ThrottleReadBps: 50M
    ThrottleWriteBps: 10M
  • User launches containerd with the blockio plugin configured to read the blockio class configuration file. For example, /etc/containerd/config.toml contains: 
    [plugins]
  [plugins."io.containerd.internal.v1.blockio"]
    config_file = "/etc/containers/blockio.yaml"
  • User annotates container(s) in a pod with a blockio class name in the configuration. Example, a pod spec contains: 
    metadata:
  annotations:
    blockio.resources.beta.kubernetes.io/pod: ThrottledIO.

When a container in this pod is created, containerd assigns blockio parameters based its class configuration.

How it is implemented

The goresctrl library understands blockio class configurations, block devices in the system and the OCI BlockIO format. When containerd starts, the blockio plugin sets effective user-defined blockio class configuration to goresctrl.

containerd looks for container's blockio class from pod and container annotations. The annotation syntax is similar to container RDT class annotations in PR #5439.

If containerd finds a class, it uses goresctrl to get corresponding OCI BlockIO data and includes it in the OCI runtime-spec.

ctr is extended with --blockio-config-file and --blockio-class for direct command-line support and testing.

@k8s-ci-robot
Copy link

Hi @askervin. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@theopenlab-ci
Copy link

theopenlab-ci bot commented May 12, 2021

Build succeeded.

AkihiroSuda
AkihiroSuda reviewed May 12, 2021
View reviewed changes
go.mod Outdated Show resolved Hide resolved
@theopenlab-ci
Copy link

theopenlab-ci bot commented May 12, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented May 14, 2021

Build succeeded.

@askervin askervin mentioned this pull request Jun 9, 2021
Merged
@theopenlab-ci
Copy link

theopenlab-ci bot commented Jun 21, 2021

Build succeeded.

AkihiroSuda
AkihiroSuda reviewed Jun 21, 2021
View reviewed changes
pkg/blockio/blockio.go Outdated
Config: &Config{},
InitFn: initBlockio,
})
}
Copy link
Member

Choose a reason for hiding this comment

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

Rather than adding a new subsystem, can we use NRI? https://github.com/containerd/nri

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AkihiroSuda, happy that you asked! Actually I initially planned implementing blockio as an NRI plugin.

However, what changed my mind was the fact that OCI spec LinuxResources already had a blockio field, and that runc was already working fine with the CFQ I/O scheduler. Furthermore, Runc v1.0 (released today) supports both CFQ and BFQ schedulers in both cgroups v1 and v2. It obviously provides consistent blockio configuration behavior and low-level error messages to all container runtimes using it.

If NRI allowed a plugin to fill in OCI spec fields and thereby use runc, it would be a clear benefit in architectural perspective. In ideal case NRI would be supported by other container runtimes, too. Then NRI would definitely be the number one option for blockio: there would be a single plugin that you could install to any node running any container runtime that supports NRI. As this is not yet the case, we decided to go with runtime-internal blockio implementations but so that they are very light-weight in the runtime perspective: they use a common class-based blockio configuration and pretty much all the logic is in a library that is common to runtimes.

Related to this, you might be interested in our NRI prototype that enables OCI spec modifications from plugins - among other goodies. But that's a separate discussion.

Copy link
Member

Choose a reason for hiding this comment

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

Rather than adding a new subsystem, can we use NRI? https://github.com/containerd/nri

@AkihiroSuda Sorry if this is slightly off topic, but we have been playing around with a prototype of extending the scope of NRI to allow more versatile plugin functionality, for instance allowing us to implement container resource assignment algorithms as NRI plugins. AFAIK @askervin has taken a look at implementing functionality equivalent to this PR with that extended NRI scope, but we haven't taken a look at whether and how that would be possible with the currently available NRI plugin interface.

@AkihiroSuda AkihiroSuda added impact/changelog kind/feature status/needs-discussion Needs discussion and decision from maintainers labels Jun 21, 2021
@theopenlab-ci
Copy link

theopenlab-ci bot commented Jun 30, 2021

Build succeeded.

@askervin
Copy link
Contributor Author

  • Rebased.
  • Commits reordered / squashed so that every step compiles.
  • Tested on k8s v1.21.2 and runc v1.0. (everything works correctly)
  • Tested on k8s v1.21.2 and runc v1.0-rc93. (everything else works but setting weights, as runc starts supporting those from v1.0)

@dmcgowan dmcgowan added this to Needs Discussion in Code Review Jul 23, 2021
@theopenlab-ci
Copy link

theopenlab-ci bot commented Aug 9, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 14, 2021

Build succeeded.

@askervin askervin force-pushed the 5Bu_blockio branch 3 times, most recently from 1c55b4f to 38243f0 Compare September 14, 2021 12:21
@theopenlab-ci
Copy link

theopenlab-ci bot commented Sep 14, 2021

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 30, 2022

Build succeeded.

@askervin
Copy link
Contributor Author

Rebased.

@dmcgowan @AkihiroSuda @samuelkarp, would you have time to take a look, please?

@aledbf
Copy link

aledbf commented Apr 17, 2022 •
edited
Loading

@askervin quick question, does this work for hosts running cgroups v2 only? Thanks!

@kylos101 kylos101 mentioned this pull request Apr 19, 2022
Closed
@askervin
Copy link
Contributor Author

@askervin quick question, does this work for hosts running cgroups v2 only? Thanks!

@aledbf , sorry about slow answer on your quick question! This works on both cgroups v1 and v2. The blockio parameters are applied by runc that supports both versions.

fuweid
fuweid reviewed Apr 26, 2022
View reviewed changes
Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

We can update the BUILDING.md in follow-up

pkg/cri/server/blockio_linux.go Outdated Show resolved Hide resolved
@mikebrow
Copy link
Member

discussion in containerd community meeting.. leaned to not provide the extra no_blockio tags

@askervin
cri: support blockio class in pod and container annotations
10576c2 
This patch adds support for a container annotation and two separate
pod annotations for controlling the blockio class of containers.

The container annotation can be used by a CRI client:
  "io.kubernetes.cri.blockio-class"

Pod annotations specify the blockio class in the K8s pod spec level:
  "blockio.resources.beta.kubernetes.io/pod"
  (pod-wide default for all containers within)

  "blockio.resources.beta.kubernetes.io/container.<container_name>"
  (container-specific overrides)

Correspondingly, this patch adds support for --blockio-class and
--blockio-config-file to ctr, too.

This implementation follows the resource class annotation pattern
introduced in RDT and merged in commit 8937012.

Signed-off-by: Antti Kervinen <antti.kervinen@intel.com>
mikebrow
mikebrow approved these changes Apr 29, 2022
View reviewed changes
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM

@mikebrow mikebrow merged commit 6b35307 into containerd:main Apr 29, 2022
Code Review automation moved this from Needs Discussion to Done Apr 29, 2022
@pacoxu pacoxu mentioned this pull request May 5, 2022
Closed
@d-honeybadger
Copy link

This PR isn't included in any of any 1.6.x releases so far, right? Is there a plan to release it anytime soon?

@AkihiroSuda
Copy link
Member

This will be included in v1.7.
Basically we do not backport new features to v1.6.

@AkihiroSuda AkihiroSuda added this to the 1.7 milestone Aug 23, 2022
@pacoxu pacoxu mentioned this pull request Sep 16, 2022
Closed
4 tasks
@gjkim42
Copy link
Contributor

gjkim42 commented Sep 18, 2022

Do we have unit tests or e2e tests for this feature?

@llamerada-jp llamerada-jp mentioned this pull request Sep 20, 2022
Closed
@dmcgowan dmcgowan mentioned this pull request Dec 10, 2022
Merged
@dmcgowan dmcgowan mentioned this pull request Jan 3, 2023
Merged
@dmcgowan dmcgowan mentioned this pull request Jan 28, 2023
Merged
@dmcgowan dmcgowan mentioned this pull request Feb 15, 2023
Merged
This was referenced Feb 25, 2023
Merged
Merged
This was referenced Mar 7, 2023
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@klihub klihub klihub left review comments

@d-honeybadger d-honeybadger d-honeybadger left review comments

@fuweid fuweid fuweid left review comments

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@mikebrow mikebrow mikebrow approved these changes

Assignees
No one assigned
Labels
impact/changelog kind/feature ok-to-test status/needs-discussion Needs discussion and decision from maintainers
Projects
Code Review
Done
Milestone
1.7
Development

Successfully merging this pull request may close these issues.

None yet