-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Default cgroup driver to systemd from k8s 1.20 #10419
Default cgroup driver to systemd from k8s 1.20 #10419
Conversation
Hi @bharath-123. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
as usual, i have forgotten to run make gofmt and static checks. Need to add them to my pre push commits :) |
b3f036f
to
35bcf65
Compare
I think this can reviewed. Would love to get some feedback on this. Made this a WIP mainly so that we can test this out since this can break kOps clusters if not done right. |
/ok-to-test |
@bharath-123 Thanks for digging into this. It looks mostly good at first sight. Will need some more time to review it and not sure if I can get to it until next week. One thought is that kOps can use at the moment containerd 1.2+ and this PR introduces 2 things that may be incompatible with older containerd versions:
I am considering dropping support for 1.2, as it was never advertised as supported or used anywhere. |
@hakman @bharath-123 |
Seems I don't get emails anymore regarding github! Missed these convos! @hakman I think we can drop support for containerd 1.2. It has reached EOL https://containerd.io/releases/ @bmelbourne will have a look at your PR! |
pkg/model/components/containerd.go
Outdated
" [plugins.\"io.containerd.grpc.v1.cri\".containerd.runtimes]", | ||
" [plugins.\"io.containerd.grpc.v1.cri\".containerd.runtimes.runc]", | ||
" runtime_type = \"io.containerd.runc.v2\"", | ||
" [plugins.\"io.containerd.grpc.v1.cri\".containerd.runtimes.runc.options]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you're switching to the default CRI plugin runtime io.containerd.runc.v2
for Containerd 1.4, the SystemdCgroup
option can be removed as it's only relevant for runtime io.containerd.runtime.v1.linux
.
https://github.com/containerd/containerd/blob/master/pkg/cri/config/config.go#L213-L216
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it! Thanks for this @bmelbourne !
pkg/model/components/containerd.go
Outdated
" [plugins.\"io.containerd.grpc.v1.cri\".containerd.runtimes]", | ||
" [plugins.\"io.containerd.grpc.v1.cri\".containerd.runtimes.runc]", | ||
" runtime_type = \"io.containerd.runc.v2\"", | ||
" [plugins.\"io.containerd.grpc.v1.cri\".containerd.runtimes.runc.options]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you're switching to the default CRI plugin runtime io.containerd.runc.v2
for Containerd 1.4, the SystemdCgroup
option can be removed as it's only relevant for runtime io.containerd.runtime.v1.linux
.
https://github.com/containerd/containerd/blob/master/pkg/cri/config/config.go#L213-L216
@bharath-123 @bmelbourne sorry for not answering sooner. I spent some time during the holidays to simplify the code and remove the unsupported parts like kubenet and 1.2.x. |
Sure @hakman. I ll update this PR by tommorow for a review. |
35bcf65
to
7cc3331
Compare
585a6b5
to
5d30cbe
Compare
I am not sure if this would break existing clusters running on runc runtime type v1 since containerd v < 1.4 run on runtime v1? I can't confirm that kubelet decides which cgroup-driver is used tbh. I have put a message in #sig-node and #containerd to confirm from the cri and containerd maintainers. |
5d30cbe
to
f99c616
Compare
f99c616
to
dbf9ad1
Compare
Based on discussions on slack, we will be defaulting to runtime v2 on containerd. Have pushed the latest code. Would love a round of review :) @bmelbourne @hakman @olemarkus |
8b8a200
to
a9d9f89
Compare
a9d9f89
to
ede3ec4
Compare
Time to see it in action. Thanks @bharath-123! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bharath-123, hakman The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Currently, kOps uses cgroupfs cgroup driver for the kubelet and CRIs. This PR defaults the cgroup driver to systemd for clusters created with k8s versions >= 1.20. Using systemd as the cgroup-driver is the recommended way as per https://kubernetes.io/docs/setup/production-environment/container-runtimes/
ede3ec4
to
a8d709a
Compare
/lgtm |
/retest |
kubernetes#10419 forgot to actually enable the systemd cgroup for containerd.
kubernetes#10419 forgot to actually enable the systemd cgroup for containerd.
Currently, kOps uses cgroupfs cgroup driver for the kubelet and CRIs. This PR defaults
the cgroup driver to systemd for clusters created with k8s versions >= 1.20.
Using systemd as the cgroup-driver is the recommended way as per
https://kubernetes.io/docs/setup/production-environment/container-runtimes/
It would be really great if this can be tested by others :) !! I have tested this by bring up and tearing down a whole bunch of clusters :) .
I verified that systemd was being used by checking the memory cgroup hierarchy. I was able to see the top level kubepod.slice hierarchy. If cgroupfs driver is used then the directory would be named kubepod instead of kubepod.slice.
Please do let me know if I have missed anything major here. Getting containerd working was very tricky since we don't have a TOML parser in kOps.
Fixes: #10372