Enforce Node Allocatable via cgroups by vishh · Pull Request #41234 · kubernetes/kubernetes

vishh · 2017-02-10T05:22:31Z

This PR enforces node allocatable across all pods using a top level cgroup as described in kubernetes/community#348

This PR also provides an option to enforce kubeReserved and systemReserved on user specified cgroups.

This PR will by default make kubelet create top level cgroups even if kubeReserved and systemReserved is not specified and hence Allocatable = Capacity.

New Kubelet flag `--enforce-node-allocatable` with a default value of `pods` is added which will make kubelet create a top level cgroup for all pods to enforce Node Allocatable. Optionally, `system-reserved` & `kube-reserved` values can also be specified separated by comma to enforce node allocatable on cgroups specified via `--system-reserved-cgroup` & `--kube-reserved-cgroup` respectively. Note the default value of the latter flags are "".
This feature requires a **Node Drain** prior to upgrade failing which pods will be restarted if possible or terminated if they have a `RestartNever` policy.

cc @kubernetes/sig-node-pr-reviews @kubernetes/sig-node-feature-requests

TODO:

Adjust effective Node Allocatable to subtract hard eviction thresholds
Add unit tests
Complete pending e2e tests
Manual testing
Get the proposal merged

@dashpole is working on adding support for evictions for enforcing Node allocatable more gracefully. That work will show up in a subsequent PR for v1.6

k8s-reviewable · 2017-02-10T05:23:38Z

This change is

vishh · 2017-02-17T21:21:12Z

@derekwaynecarr @dashpole @sjenning Node e2e (finally) passes for this PR. I'd appreciate a quick review since @dashpole intends to add evictions on top of this.

derekwaynecarr

A number of comments and requests for changes throughout. I was confused on the getNodeAllocatable stuff, so if we can name or structure things a little differently, I will be less confused when we look again at the code.

derekwaynecarr · 2017-02-17T21:36:45Z

+	fs.Var(&s.SystemReserved, "system-reserved", "A set of ResourceName=ResourceQuantity (e.g. cpu=200m,memory=150G) pairs that describe resources reserved for non-kubernetes components. Currently only cpu and memory are supported. See http://kubernetes.io/docs/user-guide/compute-resources for more detail. [default=none]")
+	fs.Var(&s.KubeReserved, "kube-reserved", "A set of ResourceName=ResourceQuantity (e.g. cpu=200m,memory=150G) pairs that describe resources reserved for kubernetes system components. Currently only cpu and memory are supported. See http://kubernetes.io/docs/user-guide/compute-resources for more detail. [default=none]")
+
+	fs.StringSliceVar(&s.EnforceNodeAllocatable, "enforce-node-allocatable", s.EnforceNodeAllocatable, "A comma separated list of levels of node allocatable enforcement to be enforced by kubelet. Acceptible options are 'pods', 'system-reserved' & 'kube-reserved'. If the latter two options are specified, '--system-reserved-cgroup' & '--kube-reserved-cgroup' must also be set respectively. See https://github.com/kubernetes/community/blob/master/contributors/design-proposals/node-allocatable.md for more details. [default='']")


typo: Acceptable

i thought default was going to be 'pods'

also does the flag library not pull and display the default for us, or is there something about StringSliceVar that I am missing? I would need to verify to check, but for most other things, the default is rendered based on defaults.go value.

It should render the defaults, but I suspect the default string value is meant for automated docs.

derekwaynecarr · 2017-02-17T21:43:43Z

+	if !s.CgroupsPerQOS && len(s.EnforceNodeAllocatable) > 0 {
+		return fmt.Errorf("Node Allocatable enforcement is not supported unless Cgroups Per QOS feature is turned on")
+	}
+	if s.SystemCgroups != "" && s.CgroupRoot == "" {


out of curiosity, are you making use of system cgroups flag? wondering if we can just deprecate it in the future and have the operator take over the responsibility of parenting daemons correctly from the start.

It is still being used in the old debian based distro that GKE uses. We can deprecate it once GKE stops using that distro.

sgtm - not needed by this pr, but we should probably put a big warning around those flags that you should not use them if your init system already places your stuff in a cgroup. maybe even error out completely if they are used and you run systemd.

derekwaynecarr · 2017-02-17T21:49:09Z

+				NodeAllocatableConfig: cm.NodeAllocatableConfig{
+					KubeReservedCgroupName:   s.KubeReservedCgroup,
+					SystemReservedCgroupName: s.SystemReservedCgroup,
+					EnforceNodeAllocatable:   sets.NewString(s.EnforceNodeAllocatable...),


its possible we validate this value later, but just recording here for reference that we don't validate the items in this list are actual valid options. i also wonder if we want to treat this is an opaque set of strings, or a more concrete type.

ack that this is now validated.

derekwaynecarr · 2017-02-17T21:52:33Z

+	zeroDuration = metav1.Duration{}
+	// Refer to [Node Allocatable](https://github.com/kubernetes/community/blob/master/contributors/design-proposals/node-allocatable.md) doc for more information.
+	// TODO: Set the default to "pods" once cgroups per qos is turned on by default.
+	defaultNodeAllocatableEnforcement = []string{}


ah, i see! ignore my earlier comment ;-)

derekwaynecarr · 2017-02-17T21:58:31Z

+	nodeInfo      *v1.Node
+	cgroupManager CgroupManager
+	capacity      v1.ResourceList
+	cgroupRoot    string


can you leave a comment just calling out that this is the literal cgroupfs value so its not forgotten?

derekwaynecarr · 2017-02-17T22:19:23Z

+	return &rc
+}
+
+func (cm *containerManagerImpl) getNodeAllocatableInternal(includeHardEviction bool) v1.ResourceList {


is there a test case for this? i need to pull and physically test your pr to make sure it does what i think it is doing.

i thought we need to handle two cases:

allocatable reported to scheduler which was capacity - reservations

allocatable enforced via cgroup which was min(capacity - reservations + reservation, capacity)

after more thought, i think this is right.

so for the 32Gi, 2 gi reserve, 100Mi threshold, this would given 29.9Gi allocatable, and 30Gi enforced. sorry, its late on a friday and this is complicated ;-)

i think the issue is its usage of includeHardEviction..

it would be clearer if called something like: getNodeAllocatableFor(scheduling | enforcement)

Renaming sounds useful. I will make the changes. Unit tests do exist.

derekwaynecarr · 2017-02-17T22:22:30Z


+	// Enforce Node Allocatable (if required)
+	if err := enforceNodeAllocatableCgroups(cm.NodeConfig.NodeAllocatableConfig,
+		cm.getNodeAllocatableInternal(false), // ignore eviction thresholds.


i thought kubepods needs to have allocatable = capacity - reserved + thresholds enforced. right now, this appears to not give the buffer discussed.

derekwaynecarr · 2017-02-17T22:27:38Z

+			NodeConfig: nc,
+			capacity:   tc.capacity,
+		}
+		for k, v := range cm.GetNodeAllocatable() {


this is confusing me. see my earlier comment on the internal version of the method, but this doesn't appear correct, and at minimum is not testing both patterns where we pass true or false to the operation.

after further reflection, i think we just need a test that tests the enfored values and a test that verifies the reported to scheduler value so its clearer. i think this is correct for the as reported to scheduler case.

Ack. I can split up the tests accordingly tomorrow.

derekwaynecarr · 2017-02-17T22:35:26Z

+		hardThreshold  evictionapi.ThresholdValue
+	}{
+		{
+			kubeReserved:   getResourceList("100m", "100Mi"),


add a case for the 0 expected resources scenario when thresholds cause us to exceed capacity.

Adding a check in code and a test case

derekwaynecarr · 2017-02-17T22:38:58Z

 var _ = framework.KubeDescribe("Kubelet Container Manager [Serial]", func() {
 	f := framework.NewDefaultFramework("kubelet-container-manager")

+	Describe("Validate Node Allocatable", func() {


@sjenning -- this seems like a pattern you could follow for your work in e2e

dashpole

Took me a while, but I think I understand the changes. Other than @derekwaynecarr's comments about validation, I found the allocatable code a bit difficult to read. I think we need to make sure that everything is either a "threshold", "reservation", "limit", or "resource", since an "allocatable" doesn't have a concrete meaning.

dashpole · 2017-02-17T21:23:16Z

 	fs.Int64Var(&s.MaxOpenFiles, "max-open-files", s.MaxOpenFiles, "Number of files that can be opened by Kubelet process. [default=1000000]")
-	fs.Var(&s.SystemReserved, "system-reserved", "A set of ResourceName=ResourceQuantity (e.g. cpu=200m,memory=150G) pairs that describe resources reserved for non-kubernetes components. Currently only cpu and memory are supported. See http://kubernetes.io/docs/user-guide/compute-resources for more detail. [default=none]")
-	fs.Var(&s.KubeReserved, "kube-reserved", "A set of ResourceName=ResourceQuantity (e.g. cpu=200m,memory=150G) pairs that describe resources reserved for kubernetes system components. Currently only cpu and memory are supported. See http://kubernetes.io/docs/user-guide/compute-resources for more detail. [default=none]")
+


nit: remove empty line

dashpole · 2017-02-17T21:29:18Z

+	// Node Allocatable Flags
+	fs.Var(&s.SystemReserved, "system-reserved", "A set of ResourceName=ResourceQuantity (e.g. cpu=200m,memory=150G) pairs that describe resources reserved for non-kubernetes components. Currently only cpu and memory are supported. See http://kubernetes.io/docs/user-guide/compute-resources for more detail. [default=none]")
+	fs.Var(&s.KubeReserved, "kube-reserved", "A set of ResourceName=ResourceQuantity (e.g. cpu=200m,memory=150G) pairs that describe resources reserved for kubernetes system components. Currently only cpu and memory are supported. See http://kubernetes.io/docs/user-guide/compute-resources for more detail. [default=none]")
+


nit: remove extra line

dashpole · 2017-02-18T00:23:29Z

+}
+
+// GetNodeAllocatable returns amount of compute resource available for pods.
+func (cm *containerManagerImpl) GetNodeAllocatable() v1.ResourceList {


Since this is an eviction threshold, I would like to see the word threshold in this function name

Or on second thought, GetNodeAllocatableResources() would also work.

This is the real NodeAllocatable from system and users standpoint.
The internal one is just meant for enforcing the real NodeAllocatable within kubelet.
If we were to rename, I'd rename the internal one.

dashpole · 2017-02-18T00:31:50Z

+	return &rc
+}
+
+func (cm *containerManagerImpl) getNodeAllocatableInternal(includeHardEviction bool) v1.ResourceList {


nit: I would love a comment explaining the purpose of this function.

dashpole · 2017-02-18T00:33:28Z

+	return nil
+}
+
+func enforceExistingCgroup(cgroupManager CgroupManager, cName string, rl v1.ResourceList) error {


nit: comment explaining function

vishh · 2017-02-28T05:21:43Z

Err. rebase :(

Signed-off-by: Vishnu Kannan <vishnuk@google.com>

Signed-off-by: Vishnu kannan <vishnuk@google.com>

vishh · 2017-02-28T05:26:10Z

Bumping the priority further up because there are about 4+ PRs that are built on top of this which needs to go into v1.6

vishh · 2017-02-28T05:42:09Z

adding LGTM label after rebasing.

k8s-github-robot · 2017-02-28T06:16:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

The following people have approved this PR: derekwaynecarr, k8s-merge-robot, vishh

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kubelet/app/OWNERS~~ [derekwaynecarr,vishh]
hack/OWNERS
pkg/OWNERS
~~pkg/apis/componentconfig/OWNERS~~ [vishh]
~~pkg/controller/node/OWNERS~~ [derekwaynecarr]
~~pkg/kubelet/OWNERS~~ [derekwaynecarr,vishh]
~~pkg/kubelet/eviction/OWNERS~~ [derekwaynecarr,vishh]
~~test/e2e_node/OWNERS~~ [vishh]

We suggest the following people:
cc @lavalamp
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-02-28T07:55:45Z

Automatic merge from submit-queue

@vish

Automatic merge from submit-queue Critial pod test uses allocatable instead of capacity This solves #42239. When this test was first introduced, pods could request up to the capacity of the node. With the addition of allocatable introduced in #41234, this is no longer the case, and pods can only use up to allocatable. This should be included in 1.6, as it is a bug related to a 1.6 feature. cc @vish @yujuhong

@vishh

Automatic merge from submit-queue Eviction Manager Enforces Allocatable Thresholds This PR modifies the eviction manager to enforce node allocatable thresholds for memory as described in kubernetes/community#348. This PR should be merged after #41234. cc @kubernetes/sig-node-pr-reviews @kubernetes/sig-node-feature-requests @vishh ** Why is this a bug/regression** Kubelet uses `oom_score_adj` to enforce QoS policies. But the `oom_score_adj` is based on overall memory requested, which means that a Burstable pod that requested a lot of memory can lead to OOM kills for Guaranteed pods, which violates QoS. Even worse, we have observed system daemons like kubelet or kube-proxy being killed by the OOM killer. Without this PR, v1.6 will have node stability issues and regressions in an existing GA feature `out of Resource` handling.

yshngg · 2025-12-07T03:29:07Z

+	// Setup event recorder if required.
+	makeEventRecorder(&s.KubeletConfiguration, kubeDeps, nodeName)


Hi @vishh , while reading the source code related EventRecorder in kubelet, I noticed that makeEventRecorder is called twice in the run function: one before ContainerManager initialization and once in the RunKubelet function. So i'm here to ask why makeEventRecorder is called before ContainerManager initialization. Thank you! 🙏

vishh assigned dashpole and derekwaynecarr Feb 10, 2017

vishh requested a review from dchen1107 February 10, 2017 05:22

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 10, 2017

vishh mentioned this pull request Feb 10, 2017

Phase 2 of node allocatable kubernetes/community#348

Merged

vishh force-pushed the nodeaphase2 branch from 6c310b8 to 348d474 Compare February 13, 2017 18:58

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 13, 2017

vishh force-pushed the nodeaphase2 branch from 348d474 to bb699ca Compare February 14, 2017 00:04

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2017

vishh force-pushed the nodeaphase2 branch 3 times, most recently from ba4acf0 to 8c7160c Compare February 15, 2017 20:52

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 15, 2017

vishh changed the title ~~[WIP] Enforce Node Allocatable via cgroups~~ Enforce Node Allocatable via cgroups Feb 17, 2017

vishh added this to the v1.6 milestone Feb 17, 2017

vishh force-pushed the nodeaphase2 branch 3 times, most recently from 6158204 to 703fb50 Compare February 17, 2017 21:20

derekwaynecarr suggested changes Feb 17, 2017

View reviewed changes

dashpole suggested changes Feb 18, 2017

View reviewed changes

vishh force-pushed the nodeaphase2 branch from c366879 to 7b1ee2b Compare February 21, 2017 00:11

vishh added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2017

vishh and others added 6 commits February 27, 2017 21:24

adding kubelet flags for node allocatable phase 2

70e340b

Signed-off-by: Vishnu Kannan <vishnuk@google.com>

add support for node allocatable phase 2 to kubelet

cc5f547

Signed-off-by: Vishnu Kannan <vishnuk@google.com>

fix go vet issues

9a65640

Signed-off-by: Vishnu kannan <vishnuk@google.com>

update flags script

b868829

Signed-off-by: Vishnu kannan <vishnuk@google.com>

Fix get all pods from cgroups logic

a768456

fix eviction helper function description

9b4a8f7

Signed-off-by: Vishnu kannan <vishnuk@google.com>

vishh force-pushed the nodeaphase2 branch from 3327294 to 9b4a8f7 Compare February 28, 2017 05:25

vishh removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2017

vishh added priority/P1 and removed priority/P2 labels Feb 28, 2017

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2017

vishh added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2017

k8s-github-robot merged commit fbe2d15 into kubernetes:master Feb 28, 2017

dashpole mentioned this pull request Mar 1, 2017

Critial pod test uses allocatable instead of capacity #42341

Merged

vishh mentioned this pull request Mar 1, 2017

Enforce node allocatable on the nodes #25429

Closed

ivan4th mentioned this pull request Mar 27, 2017

Kubelet fails when run inside container #43704

Closed

FengyunPan mentioned this pull request Aug 5, 2017

Log spam: Lower "allocatable:" log from error-level #50167

Closed

dashpole mentioned this pull request Nov 16, 2018

modify description for SystemReservedCgroup and KubeReservedCgroup #70857

Closed

SergeyKanzhelev mentioned this pull request May 12, 2021

code cleanup:remove dup event recorder add in kubelet #101472

Closed

yshngg reviewed Dec 7, 2025

View reviewed changes

yshngg mentioned this pull request Dec 7, 2025

fix(kubelet): remove redundant makeEventRecorder #135618

Closed

		// Setup event recorder if required.
		makeEventRecorder(&s.KubeletConfiguration, kubeDeps, nodeName)

Conversation

vishh commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-reviewable commented Feb 10, 2017

Uh oh!

vishh commented Feb 17, 2017

Uh oh!

derekwaynecarr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dashpole left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vishh commented Feb 10, 2017 •

edited

Loading

vishh commented Feb 28, 2017 •

edited

Loading