optimize cgroups settings for node reserved #9209

shelmingsong · 2022-08-23T02:58:46Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

If cgroups driver is systemd and kube_reserved or system_reserved is set

Specify kubeReservedCgroup or systemReservedCgroup in kubelet-config.v1beta1.yaml.j2
Pre-create the cgroups folder before the Kubelet service starts
Specify the cgroups slice in the Containerd Service

The cgroups setup and hierarchy refer to this article

After the setup, the cgroups hierarchy is as follows:

/ (Cgroups Root)
├── kubepods.slice
│   ├── ...
│   ├── kubepods-besteffort.slice
│   ├── kubepods-burstable.slice
│   └── ...
├── kube.slice
│   ├── ...
│   ├── containerd.service
│   ├── kubelet.service
│   └── ...
├── system.slice
│   └── ...
└── ...

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

optimize cgroups settings for node reserved (using new `kube_reserved`, see docs for more information)

k8s-ci-robot · 2022-08-23T02:58:54Z

Hi @shelmingsong. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

cristicalin · 2022-08-23T06:20:04Z

Hi @shelmingsong , thank you for submitting this.

Before we accept the change, could you please explain how is this an optimisation? The design document is from the 1.7 days (2018) and the kubelet cgroup driver already takes care of setting the hierarchy, adding extra logic to the systemd units seems counter-intuitive and prone to clash with future versions of kubernetes so I would personally like to understand what is the benefit we get by adding this.

/hold

shelmingsong · 2022-08-24T02:49:05Z

Hi @cristicalin , thank you for your reply.

Let me describe the reasons for these changes in detail.

Specify kubeReservedCgroup or systemReservedCgroup in kubelet-config.v1beta1.yaml.j2

When enforceNodeAllocatable in kubelet-config.v1beta1.yaml contains kube-reserved or system-reserved, If kubeReservedCgroup or systemReservedCgroup is not specified, enforceNodeAllocatable will not take effect.

This can be seen in this official Kubernetes document: [enforcing-node-allocatable](https://kubernetes.io/docs/tasks/administer-cluster/reserve-compute-resources/#enforcing-node-allocatable)

Optionally, kubelet can be made to enforce kube-reserved and system-reserved by specifying kube-reserved & system-reserved values in the same flag. Note that to enforce kube-reserved or system-reserved, --kube-reserved-cgroup or --system-reserved-cgroup needs to be specified respectively.

Pre-create the cgroups folder before the Kubelet service starts

kubeReservedCgroup and systemReservedCgroup will not take effect if we do not create the cgroups folder required by Kubelet in advance.
This can be seen in the following two official Kubernetes documents:

[kube-reserved](https://kubernetes.io/docs/tasks/administer-cluster/reserve-compute-resources/#kube-reserved)

Note that Kubelet does not create --kube-reserved-cgroup if it doesn't exist. Kubelet will fail if an invalid cgroup is specified. With systemd cgroup driver, you should follow a specific pattern for the name of the cgroup you define: the name should be the value you set for --kube-reserved-cgroup, with .slice appended.

[system-reserved](https://kubernetes.io/docs/tasks/administer-cluster/reserve-compute-resources/#system-reserved)

Note that kubelet does not create --system-reserved-cgroup if it doesn't exist. kubelet will fail if an invalid cgroup is specified. With systemd cgroup driver, you should follow a specific pattern for the name of the cgroup you define: the name should be the value you set for --system-reserved-cgroup, with .slice appended.

Specify the cgroups slice in the Containerd Service

If we do not specify cgroup slice in the Containerd service, then Containerd cgroup is created under systemd.slice by default, which is not as expected of kube-reserved.

cristicalin

Thank you @shelmingsong for the explanation, it makes sense to me.

/hold cancel

Please also address the additional comments regarding documenting this feature and support for the other CRIs

cristicalin · 2022-08-24T07:09:37Z

roles/kubernetes/node/defaults/main.yml

@@ -23,6 +23,10 @@ kubelet_kubelet_cgroups_cgroupfs: "/system.slice/kubelet.service"
 kubelet_fail_swap_on: true

 # Reserve this space for kube resources
+# Set to true to reserve resources for kube daemons


Please add these variables to the sample files as well as they serve a documentation role.

It would also be useful to add some more extensive documentation for these variables in the docs/ folder.

OK, I have added these variables to the sample file.

https://github.com/kubernetes-sigs/kubespray/pull/9209/files#diff-4c8783fcb045b2d16d9bc33735499d4a80e491dc206a1b87cbbb371fb96094e4

In addition, I added a document:

https://github.com/kubernetes-sigs/kubespray/pull/9209/files#diff-14e9e88531913c6d713377788ee34c81e92555ab4af9c980affcd06b1b13d3bf

I'm sorry, I misunderstood the yellow "Pending" decorator on my comments, so the above comment was just sent (it was actually commented many days ago).

cristicalin · 2022-08-24T07:12:03Z

roles/container-engine/containerd/templates/containerd.service.j2

@@ -35,6 +35,9 @@ LimitNOFILE=infinity
 # Only systemd 226 and above support this version.
 TasksMax=infinity
 OOMScoreAdjust=-999
+{% if kube_reserved is defined and kube_reserved|bool %}
+Slice={{ kube_reserved_cgroups_for_service_slice }}


Is there an equivalent for cri-o and docker (cri-dockerd) for these options? If not then the documentation should explicitly state that this capability is only available with containerd.

Is there an equivalent for cri-o and docker (cri-dockerd) for these options?

Yes, I have added these options to each of these container manager configuration files. You can see the changes in the following files:

https://github.com/kubernetes-sigs/kubespray/pull/9209/files#diff-32032985c3f2d4c78c99cfe13e3db695bf4f766e7718b45889c1b8db527105a3

https://github.com/kubernetes-sigs/kubespray/pull/9209/files#diff-2ea93b939c50e808ef0af1426d7bbb5ad2b243db9bf5106c4050fe1d110e4c26

https://github.com/kubernetes-sigs/kubespray/pull/9209/files#diff-510a4a526ce7fbd9000f27bbafc2403ea9bccf5cee16643a51e533ec910b3332

https://github.com/kubernetes-sigs/kubespray/pull/9209/files#diff-2627b48dffe270eb911768d8b3c9d3fb287ad2c91130280729d4dae1462cf333

I'm sorry, I misunderstood the yellow "Pending" decorator on my comments, so the above comment was just sent (it was actually commented many days ago).

cristicalin · 2022-08-30T05:45:42Z

Hi @shelmingsong , please rebase this PR on latest state of the master branch to be able to pass the CI tests.

shelmingsong · 2022-08-31T02:26:12Z

Hi @cristicalin , thank you for reminding me.
I rebased, and the CI tests have now passed

cristicalin · 2022-08-31T13:15:29Z

Great work @shelmingsong, thank you!

/approve
/ok-to-test

yankay · 2022-09-05T04:04:30Z

roles/kubernetes/node/defaults/main.yml

-kubelet_runtime_cgroups: "/systemd/system.slice"
-kubelet_kubelet_cgroups: "/systemd/system.slice"
+kubelet_runtime_cgroups: "{{ kube_reserved_cgroups }}/{{ container_manager }}.service"
+kubelet_kubelet_cgroups: "{{ kube_reserved_cgroups }}/kubelet.service"


HI @shelmingsong , Thank you very much.

After the PR , the default kubelet_kubelet_cgroups, would change from the /systemd/system.slice to the /kube.slice/kubelet.service. And not managed by the systemd.

is the default kubelet config changed after the PR?

@yankay Sorry for taking so long to reply.

Yes, the default kubelet configuration will be changed.

I'm sorry, I misunderstood the yellow "Pending" decorator on my comments, so the above comment was just sent (it was actually commented many days ago).

k8s-triage-robot · 2022-12-25T07:07:23Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

shelmingsong · 2022-12-28T09:48:34Z

/remove-lifecycle stale

floryut · 2022-12-29T09:33:55Z

/remove-lifecycle stale

@shelmingsong Wow sorry I guess we missed this one, I'll do my best to take the time and review it, big one 😅

shelmingsong · 2022-12-30T15:51:46Z

> /remove-lifecycle stale @shelmingsong Wow sorry I guess we missed this one, I'll do my best to take the time and review it, big one 😅

it's okay (*^▽^*)

floryut

/lgtm
@shelmingsong Thank you for this feature, nice one 🙇

🚀

k8s-ci-robot · 2022-12-30T16:03:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cristicalin, floryut, shelmingsong

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:

~~OWNERS~~ [cristicalin,floryut]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* optimize cgroups settings for node reserved * fix * set cgroup slice for multi container engine * set cgroup slice for crio * add reserved cgroups variables to sample files * Compatible with cgroup path for different container managers * add cgroups doc * fix markdown

mirex05 · 2023-02-10T05:28:27Z

Hello everyone, isn't this changes added some misconfiguration in kubelet.env?
Because if kube_reserved set to false (which is default), kubelet starts with flag --runtime-cgroups=/kube.slice/{{ container_manager }}.service, but runtime, for example, docker used with cri-dockerd still points out to system.slice. Maybe i'm wrong and missed something, so just want to specify this out.

* optimize cgroups settings for node reserved * fix * set cgroup slice for multi container engine * set cgroup slice for crio * add reserved cgroups variables to sample files * Compatible with cgroup path for different container managers * add cgroups doc * fix markdown

rptaylor · 2024-07-09T06:24:42Z

I think there are two separate functions at play here:

Resource reservation configured by KubeReserved and SystemReserved. This is like a k8s resource request. It is very important for cluster stability, to ensure that pods do not consume all node resources, to leave enough resources for daemons running on the node. For scheduling purposes the total resources on the node are simply reduced by the kube reserved and system reserved amounts, and the remaining node allocatable amount can be used by pods.
Resource enforcement configured by kubeReservedCgroup and systemReservedCgroup. This is like a k8s resource limit. Applying this can be dangerous because it would kill the critical daemons if they exceed the specified amount of resources (though that may also depend on the value of enforceNodeAllocatable ?)

Note from the docs, the kubeReservedCgroup setting is optional. It is not required for reservation; it is for enforcement. You can (and should) set KubeReserved without kubeReservedCgroup.

To optionally enforce kubeReserved on kubernetes system daemons, specify the parent control group for kube daemons as the value for kubeReservedCgroup setting, and add kube-reserved to enforceNodeAllocatable

The behaviour before this MR was that by default, resource reservation was applied (via KubeReserved), and enforcement was not (the optional parameter kubeReservedCgroup was not applied). This is the best/safest/most resilient default behaviour. However after the MR, the resource reservation disappeared by default since kube_reserved is false by default, so pods could fully consume node resources, causing important node daemons to die.

If we do not specify cgroup slice in the Containerd service, then Containerd cgroup is created under systemd.slice by default, which is not as expected of kube-reserved.

I don't think that is true, because the safe standard way to reserve resources for kubernetes daemons should be to define KubeReserved resources, without specifying the optional and unnecessary kubeReservedCgroup setting. After this MR that is no longer possible, and there are no kube-reserved resources by default.

I spent many hours trying to figure this out, if I misunderstood or made an incorrect assumption I would be happy for a clarification.

Anyway can someone please re-open #9692 to address this? @cristicalin @yankay ? Thanks!

rptaylor · 2024-07-09T06:56:15Z

I have tried #11367 to fix this.

k8s-ci-robot added 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 Aug 23, 2022

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 23, 2022

k8s-ci-robot requested review from EppO and liupeng0518 August 23, 2022 02:59

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 23, 2022

cristicalin reviewed Aug 24, 2022

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2022

shelmingsong added 6 commits August 30, 2022 13:52

optimize cgroups settings for node reserved

dec11c7

fix

e122e36

set cgroup slice for multi container engine

d8e0905

set cgroup slice for crio

9d70779

add reserved cgroups variables to sample files

0be76d7

Compatible with cgroup path for different container managers

044b631

shelmingsong force-pushed the ftr/cgroups branch from de87322 to 044b631 Compare August 30, 2022 05:54

add cgroups doc

60cff5b

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 30, 2022

fix markdown

f54394b

yankay reviewed Sep 5, 2022

View reviewed changes

yuha0 mentioned this pull request Dec 8, 2022

Conflicting comment and implementation for kubelet_enforce_node_allocatable #9552

Closed

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 25, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 28, 2022

floryut approved these changes Dec 30, 2022

View reviewed changes

k8s-ci-robot assigned floryut Dec 30, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 30, 2022

k8s-ci-robot merged commit 1c4db61 into kubernetes-sigs:master Dec 30, 2022

floryut mentioned this pull request Jan 4, 2023

Release Proposal v2.21 #9638

Closed

Tristan971 mentioned this pull request Jan 20, 2023

(Re-)Enable resource reservation by default for core components and host container runtime #9692

Closed

MrFreezeex mentioned this pull request Feb 27, 2023

node: fix default kubelet/runtime cgroups when kube_reserved is false #9834

Merged

VannTen mentioned this pull request Dec 12, 2023

Refactor cgroup and kubelet enforceNodeAllocatable #10714

Open

rptaylor mentioned this pull request Jul 9, 2024

fix kube_reserved so it only controls kubeReservedCgroup #11367

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

optimize cgroups settings for node reserved #9209

optimize cgroups settings for node reserved #9209

shelmingsong commented Aug 23, 2022 •

edited by floryut

Loading

k8s-ci-robot commented Aug 23, 2022

cristicalin commented Aug 23, 2022

shelmingsong commented Aug 24, 2022

cristicalin left a comment

cristicalin Aug 24, 2022

shelmingsong Aug 30, 2022

shelmingsong Sep 26, 2022

cristicalin Aug 24, 2022

shelmingsong Aug 30, 2022

shelmingsong Sep 26, 2022

cristicalin commented Aug 30, 2022

shelmingsong commented Aug 31, 2022

cristicalin commented Aug 31, 2022

yankay Sep 5, 2022 •

edited

Loading

shelmingsong Sep 22, 2022

shelmingsong Sep 26, 2022

k8s-triage-robot commented Dec 25, 2022

shelmingsong commented Dec 28, 2022

floryut commented Dec 29, 2022

shelmingsong commented Dec 30, 2022 via email •

edited

Loading

floryut left a comment

k8s-ci-robot commented Dec 30, 2022

mirex05 commented Feb 10, 2023 •

edited

Loading

rptaylor commented Jul 9, 2024

rptaylor commented Jul 9, 2024

optimize cgroups settings for node reserved #9209

optimize cgroups settings for node reserved #9209

Conversation

shelmingsong commented Aug 23, 2022 • edited by floryut Loading

k8s-ci-robot commented Aug 23, 2022

cristicalin commented Aug 23, 2022

shelmingsong commented Aug 24, 2022

cristicalin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristicalin commented Aug 30, 2022

shelmingsong commented Aug 31, 2022

cristicalin commented Aug 31, 2022

yankay Sep 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-triage-robot commented Dec 25, 2022

shelmingsong commented Dec 28, 2022

floryut commented Dec 29, 2022

shelmingsong commented Dec 30, 2022 via email • edited Loading

floryut left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 30, 2022

mirex05 commented Feb 10, 2023 • edited Loading

rptaylor commented Jul 9, 2024

rptaylor commented Jul 9, 2024

shelmingsong commented Aug 23, 2022 •

edited by floryut

Loading

yankay Sep 5, 2022 •

edited

Loading

shelmingsong commented Dec 30, 2022 via email •

edited

Loading

mirex05 commented Feb 10, 2023 •

edited

Loading