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

Default cgroup driver to systemd from k8s 1.20 #10419

Merged
merged 1 commit into from Jan 12, 2021

Conversation

bharath-123
Copy link
Contributor

@bharath-123 bharath-123 commented Dec 13, 2020

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

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 13, 2020
@k8s-ci-robot
Copy link
Contributor

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 /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.

@k8s-ci-robot k8s-ci-robot added area/documentation size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/nodeup labels Dec 13, 2020
@bharath-123
Copy link
Contributor Author

cc: @hakman @bmelbourne @olemarkus

@bharath-123
Copy link
Contributor Author

as usual, i have forgotten to run make gofmt and static checks. Need to add them to my pre push commits :)

@bharath-123
Copy link
Contributor Author

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.

@rifelpet
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 17, 2020
@hakman
Copy link
Member

hakman commented Dec 17, 2020

@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:

  • config version 2
  • runtime version 2 (not sure when it was introduced, but was made default only in 1.4)

I am considering dropping support for 1.2, as it was never advertised as supported or used anywhere.

@bmelbourne
Copy link
Contributor

@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:

  • config version 2
  • runtime version 2 (not sure when it was introduced, but was made default only in 1.4)

I am considering dropping support for 1.2, as it was never advertised as supported or used anywhere.

@hakman @bharath-123
I've actually started working on a similar update for Kubespray where the containerd version is being bumped to 1.4.3, and could incorporate the containerd config v2 into PR #10370...just let me know your preference?

@bharath-123
Copy link
Contributor Author

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!

" [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]",
Copy link
Contributor

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

Copy link
Contributor Author

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 !

" [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]",
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 27, 2020
@hakman
Copy link
Member

hakman commented Jan 4, 2021

@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.
Also had some time to think about this and the settings. We should still keep io.containerd.runc.v2 to set this for 1.3.x.
@bharath-123 if you could update the PR, I can take a look at it and review it these days.

@bharath-123
Copy link
Contributor Author

Sure @hakman. I ll update this PR by tommorow for a review.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 9, 2021
@bharath-123 bharath-123 force-pushed the task/default-systemd branch 2 times, most recently from 585a6b5 to 5d30cbe Compare January 9, 2021 13:04
@bharath-123
Copy link
Contributor Author

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.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 12, 2021
@bharath-123
Copy link
Contributor Author

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

docs/cluster_spec.md Outdated Show resolved Hide resolved
docs/cluster_spec.md Outdated Show resolved Hide resolved
docs/cluster_spec.md Outdated Show resolved Hide resolved
docs/cluster_spec.md Outdated Show resolved Hide resolved
docs/cluster_spec.md Outdated Show resolved Hide resolved
pkg/model/components/containerd.go Show resolved Hide resolved
docs/cluster_spec.md Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 12, 2021
@hakman
Copy link
Member

hakman commented Jan 12, 2021

Time to see it in action. Thanks @bharath-123!
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2021
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2021
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/
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2021
@hakman
Copy link
Member

hakman commented Jan 12, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2021
@bharath-123
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit e4f4a20 into kubernetes:master Jan 12, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Jan 12, 2021
codablock added a commit to codablock/kops that referenced this pull request Feb 16, 2021
kubernetes#10419 forgot to actually enable the systemd cgroup for containerd.
codablock added a commit to codablock/kops that referenced this pull request Feb 16, 2021
kubernetes#10419 forgot to actually enable the systemd cgroup for containerd.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/documentation area/nodeup cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use systemd as the cgroup driver for kubelet and CRI
7 participants