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

Use systemd cgroup driver for v1.24.0+ #2737

Merged
merged 9 commits into from
May 10, 2022
Merged

Use systemd cgroup driver for v1.24.0+ #2737

merged 9 commits into from
May 10, 2022

Conversation

BenTheElder
Copy link
Member

see: #1726

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 3, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder

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 requested review from munnerz and neolit123 May 3, 2022 18:23
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 3, 2022
@BenTheElder BenTheElder mentioned this pull request May 3, 2022
Closed
@BenTheElder BenTheElder force-pushed the systemd-driver branch 2 times, most recently from 1c3c1c0 to a5cf544 Compare May 3, 2022 19:31
@BenTheElder
Copy link
Member Author

hmm 1.24 kubelet unhealthy
https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kind/2737/pull-kind-e2e-kubernetes/1521573229309726720

https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kind/2737/pull-kind-e2e-kubernetes/1521573229309726720/artifacts/logs/kind-control-plane/kubelet.log

May 03 19:52:45 kind-control-plane kubelet[1749]: Error: failed to run Kubelet: invalid configuration: cgroup ["kubelet"] has some missing paths: /sys/fs/cgroup/pids/kubelet.slice, /sys/fs/cgroup/cpu,cpuacct/kubelet.slice, /sys/fs/cgroup/cpu,cpuacct/kubelet.slice, /sys/fs/cgroup/cpuset/kubelet.slice, /sys/fs/cgroup/memory/kubelet.slice, /sys/fs/cgroup/systemd/kubelet.slice, /sys/fs/cgroup/hugetlb/kubelet.slice

@BenTheElder BenTheElder changed the title use systemd cgroup driver for v1.24.0+ [WIP] use systemd cgroup driver for v1.24.0+ May 3, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 3, 2022
@BenTheElder
Copy link
Member Author

right ... all the entrypoint tricks are aimed at cgroupfs.

@BenTheElder
Copy link
Member Author

So, when using the systemd cgroup driver kubelet will automatically append .slice to --cgroup-root if missing => /kubelet.slice (under /sys/fs/cgroup).

But we setup /kubelet and we're doing so on our own, so /kubelet.slice doesn't exist ...

We'll need to handle this differently.

@BenTheElder
Copy link
Member Author

https://github.com/opencontainers/runc/blob/main/docs/systemd.md

@BenTheElder
Copy link
Member Author

Got back to this a bit today:

  • kubelet appends .slice internally and you should not add it yourself
  • ... but the cgroup --cgroup-root + .slice must exist
  • if we use the current /kubelet cgroup root approach but populate /kubelet.slice instead, everything seems fine

Debating if we should plumb through a switch or just always setup both, leaning towards the latter so it's simpler to ensure either can be used.

@BenTheElder BenTheElder force-pushed the systemd-driver branch 2 times, most recently from d1c5998 to 9b0db6c Compare May 6, 2022 01:12
@BenTheElder
Copy link
Member Author

s390x build flake https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kind/2737/pull-kind-build/1522383838079094784

@BenTheElder BenTheElder force-pushed the systemd-driver branch 2 times, most recently from 18834fa to 5e58731 Compare May 6, 2022 02:34
@BenTheElder
Copy link
Member Author

s390x flake again
https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kind/2737/pull-kind-build/1522405658077433856
/retest

@BenTheElder
Copy link
Member Author

BenTheElder commented May 6, 2022 •
edited
Loading

/retest

1.21 flake https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kind/2737/pull-kind-e2e-kubernetes-1-21/1522405665769787392

@BenTheElder
Copy link
Member Author

logs still show failing to remove rmda cgroup but that's not new to this PR / driver kubernetes/kubernetes#109182

@BenTheElder
Copy link
Member Author

We need to test this in more environments before moving forward, just to be on the safe side that we haven't missed a quirk.

I'm pushing a node image from this, will update the PR with it so that github actions environments pick it up, and then see about getting it tested in some additional environments.

@BenTheElder
Copy link
Member Author

podman the node only got this far:

INFO: ensuring we can execute mount/umount even with userns-remap
INFO: remounting /sys read-only
INFO: making mounts shared
INFO: detected cgroup v1
WARN: cgroupns not enabled! Please use cgroup v2, or cgroup v1 with cgroupns enabled.
INFO: fix cgroup mounts for all subsystems

exit status 1 https://github.com/kubernetes-sigs/kind/actions/runs/2296780194

@BenTheElder BenTheElder changed the title [WIP] use systemd cgroup driver for v1.24.0+ Use systemd cgroup driver for v1.24.0+ May 10, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 10, 2022
@BenTheElder
Copy link
Member Author

May 10 02:34:53.338: INFO: externalsvc-72bcl[services-5001].container[externalsvc]=failed to create containerd task: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: unable to apply cgroup configuration: failed to write 84042: write /sys/fs/cgroup/rdma/kubelet.slice/kubelet-kubepods.slice/kubelet-kubepods-besteffort.slice/kubelet-kubepods-besteffort-podbd0b7b96_aa32_4e00_8323_6f20dd1b85a4.slice/cri-containerd-6e1a4e9c43aba1945e9a28e410939e0801c180fa69bb1850018c9c0b2956a91a.scope/cgroup.procs: no such device: unknown

we still have the rdma issue https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kind/2737/pull-kind-conformance-parallel-ipv6/1523846989832261632

but that's not new
/retest

@BenTheElder BenTheElder removed the area/provider/podman Issues or PRs related to podman label May 10, 2022
aojea
aojea reviewed May 10, 2022
View reviewed changes
pkg/build/nodeimage/containerd.go
SystemdCgroup = false
`

func configureContainerdSystemdCgroupFalse(containerCmdr exec.Cmder, config string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

can this fail against an image that was built for 1.24.x with an old version of kind, without the containerd change?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, users that upgrade to kind v0.13 will need to use an image built with a newer version
we have no official images that way.

for custom built images that are 1.24+ with kind < v0.13, users will need to build new images. we'll have a release note.

the opposite is also true, 1.24+ official images / images built going forward will require v0.13.

this already would have happened in 1.21 with kubeadm defaulting to systemd then, we just shifted it a few releases.

@BenTheElder
Copy link
Member Author

/retest

we have report that macOS / docker desktop works fine https://kubernetes.slack.com/archives/CEKK1KTN2/p1652192992627339?thread_ts=1652149847.653599&cid=CEKK1KTN2
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 10, 2022
@BenTheElder BenTheElder mentioned this pull request May 10, 2022
Merged
@aojea
Copy link
Contributor

aojea commented May 10, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2022
@k8s-ci-robot k8s-ci-robot merged commit 181dc20 into kubernetes-sigs:main May 10, 2022
@BenTheElder BenTheElder deleted the systemd-driver branch May 10, 2022 17:50
@BenTheElder
Copy link
Member Author

Working on v0.13.0 based on this, pushing images. Wrote release notes.

@BenTheElder BenTheElder mentioned this pull request May 10, 2022
Merged
@BenTheElder
Copy link
Member Author

https://github.com/kubernetes-sigs/kind/releases/tag/v0.13.0

@aojea aojea mentioned this pull request May 13, 2022
Closed
@orelmisan orelmisan mentioned this pull request Nov 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aojea aojea aojea left review comments

@munnerz munnerz Awaiting requested review from munnerz

@neolit123 neolit123 Awaiting requested review from neolit123

Assignees

@aojea aojea

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BenTheElder @k8s-ci-robot @aojea