Count pod overhead against an entity's ResourceQuota #99600

gjkim42 · 2021-03-01T16:26:54Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

According to https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/688-pod-overhead, the ResourceQuota controller should account for pod.Spec.Overhead

Which issue(s) this PR fixes:

Fixes #99577

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Count pod overhead against an entity's ResourceQuota

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

gjkim42 · 2021-03-01T16:30:09Z

/cc @egernst
Could you review this?

gjkim42 · 2021-03-01T16:32:09Z

/sig node

gjkim42 · 2021-03-01T16:43:55Z

/kind bug

troy0820 · 2021-03-01T21:25:19Z

/priority important-soon
/triage accepted

gjkim42 · 2021-03-01T23:00:50Z

Need ok-to-test!

gjkim42 · 2021-03-01T23:04:36Z

/auto-cc

troy0820 · 2021-03-01T23:05:38Z

/ok-to-test

gjkim42 · 2021-03-01T23:34:00Z

/retest

troy0820 · 2021-03-02T00:19:47Z

/assign

troy0820 · 2021-03-02T00:21:51Z

/assign @dims

chestack · 2021-03-02T03:50:45Z

@gjkim42 thanks for your quick fix.

About PodOverhead, I have something else to discuss.

Currently, PodOverhead is defined as a fixed resource. this value looks like the average usage value and being added into requests resource during scheduling.

type Overhead struct {
  PodFixed *ResourceList
}

My use case is kata_container, the VMM related processes consume much overhead resource when pod running with high workload. A small overhead(small cgroup limits) will trigger oom but a big value result that big overhead requests take into account during scheduling.

So how about define PodOverhead including limits and requests like that in Pod resources definition. It could be helpful

    resources:
      limits:
        memory: 1Gi
      requests:
        memory: 200M

gjkim42 · 2021-03-02T11:26:59Z

So how about define PodOverhead including limits and requests like that in Pod resources definition. It could be helpful
    resources:
      limits:
        memory: 1Gi
      requests:
        memory: 200M

@chestack
In my opinion, there seem to be some places to be used. Maybe, you can make an issue to request api-change. However, I am not familiar with this domain.

@egernst need your opinion!

gjkim42 · 2021-03-02T12:03:54Z

/retest

gjkim42 · 2021-03-04T22:28:33Z

Rebased. PTAL
/assign @vishh (for approval)
ping @derekwaynecarr

gjkim42 · 2021-03-04T22:33:24Z

/assign @vishh

SergeyKanzhelev · 2021-03-05T21:51:00Z

pkg/quota/v1/evaluator/core/pods_test.go

-		usage corev1.ResourceList
+		pod                *api.Pod
+		usage              corev1.ResourceList
+		podOverheadEnabled bool


so default is false? And all tests that do not set this field will execute with the pod overhead disabled? What am I missing?

Yes, I would say that there is no need to explicitly enable the pod overhead for the other cases. The other cases are irrelevant.

got it. So there is no intention to explicitly disable it for other cases. It is just a side effect of the need to enable it for one case. Since it's in beta and about to get GA, does this logic even needed?

If you think it is needed, maybe just do nothing when it's false. See comment in file below

oh, I see the new test case. it's ok than. Hopefully will GA PodOverhead in 1.22

SergeyKanzhelev · 2021-03-05T21:54:38Z

So this KEP paragraph wasn't implemented? This comment still lists it as a "(placeholder)".

KEP xref: #kubernetes/enhancements#688

pkg/quota/v1/evaluator/core/pods_test.go

SergeyKanzhelev · 2021-03-05T22:09:26Z

pkg/quota/v1/evaluator/core/pods_test.go

+					},
+				},
+			},
+			usage: corev1.ResourceList{


this test case tests for usage, not for quota, correct? Sorry I might be missing something

Yes, the test is for usage, and resourcequota controller ensures the usage to not exceed the quota

According to the KEP (The controller will be updated to add the pod Overhead to the container resource request summation.), I would say that the container resource request summation means the usage. And #99577 requested the same thing.

gjkim42

@SergeyKanzhelev
Done.

gjkim42 · 2021-03-05T22:31:18Z

pkg/quota/v1/evaluator/core/pods_test.go

-		usage corev1.ResourceList
+		pod                *api.Pod
+		usage              corev1.ResourceList
+		podOverheadEnabled bool


Yes, I would say that there is no need to explicitly enable the pod overhead for the other cases. The other cases are irrelevant.

gjkim42 · 2021-03-05T23:04:33Z

pkg/quota/v1/evaluator/core/pods_test.go

+					},
+				},
+			},
+			usage: corev1.ResourceList{


Yes, the test is for usage, and resourcequota controller ensures the usage to not exceed the quota

According to the KEP (The controller will be updated to add the pod Overhead to the container resource request summation.), I would say that the container resource request summation means the usage. And #99577 requested the same thing.

gjkim42 · 2021-03-05T23:20:01Z

pkg/quota/v1/evaluator/core/pods_test.go

+		"do not count pod overhead as usage with pod overhead disabled": {
+			pod: &api.Pod{
+				Spec: api.PodSpec{
+					Overhead: api.ResourceList{
+						api.ResourceCPU: resource.MustParse("1"),
+					},
+					Containers: []api.Container{
+						{
+							Resources: api.ResourceRequirements{
+								Requests: api.ResourceList{
+									api.ResourceCPU: resource.MustParse("1"),
+								},
+								Limits: api.ResourceList{
+									api.ResourceCPU: resource.MustParse("2"),
+								},
+							},
+						},
+					},
+				},
+			},
+			usage: corev1.ResourceList{
+				corev1.ResourceRequestsCPU: resource.MustParse("1"),
+				corev1.ResourceLimitsCPU:   resource.MustParse("2"),
+				corev1.ResourcePods:        resource.MustParse("1"),
+				corev1.ResourceCPU:         resource.MustParse("1"),
+				generic.ObjectCountQuotaResourceNameFor(schema.GroupResource{Resource: "pods"}): resource.MustParse("1"),
+			},
+			podOverheadEnabled: false,
+		},


added the case containing pod overhead with the pod overhead disabled

gjkim42 · 2021-03-05T23:50:42Z

So this KEP paragraph wasn't implemented?

I don't know the full history, but I think it wasn't.

SergeyKanzhelev · 2021-03-06T00:23:03Z

/lgtm

gjkim42 · 2021-03-08T07:33:32Z

@derekwaynecarr
Need your approval! Thanks.

derekwaynecarr · 2021-03-09T22:22:07Z

pkg/quota/v1/evaluator/core/pods.go

@@ -354,6 +354,10 @@ func PodUsageFunc(obj runtime.Object, clock clock.Clock) (corev1.ResourceList, e
 		limits = quota.Max(limits, pod.Spec.InitContainers[i].Resources.Limits)
 	}

+	if feature.DefaultFeatureGate.Enabled(features.PodOverhead) {


i wish we had this in beta as intended; happy we caught it prior to moving to GA.

derekwaynecarr · 2021-03-09T22:23:24Z

/approve
/lgtm

this should probably warrant cherry-picks to past releases.

k8s-ci-robot · 2021-03-09T22:23:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, dims, gjkim42

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:

~~pkg/quota/v1/OWNERS~~ [derekwaynecarr]

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

…00-upstream-release-1.19 Automated cherry pick of #99600: Count pod overhead as an entity's resource usage

…00-upstream-release-1.18 Automated cherry pick of #99600: Count pod overhead as an entity's resource usage

…00-upstream-release-1.20 Automated cherry pick of #99600: Count pod overhead as an entity's resource usage

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 1, 2021

k8s-ci-robot requested a review from egernst March 1, 2021 16:30

k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Mar 1, 2021

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 1, 2021

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Mar 1, 2021

troy0820 added this to Triage in SIG Node PR Triage Mar 1, 2021

k8s-ci-robot requested review from deads2k and vishh March 1, 2021 23:05

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Mar 1, 2021

k8s-ci-robot assigned troy0820 Mar 2, 2021

troy0820 removed their assignment Mar 2, 2021

k8s-ci-robot assigned dims Mar 2, 2021

gjkim42 force-pushed the count-pod-overhead-against-rsquota branch from c783d35 to 53ab29b Compare March 2, 2021 11:56

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Mar 2, 2021

k8s-ci-robot assigned vishh Mar 4, 2021

SergeyKanzhelev reviewed Mar 5, 2021

View reviewed changes

pkg/quota/v1/evaluator/core/pods_test.go Outdated Show resolved Hide resolved

SergeyKanzhelev reviewed Mar 5, 2021

View reviewed changes

pkg/quota/v1/evaluator/core/pods_test.go Outdated Show resolved Hide resolved

SergeyKanzhelev reviewed Mar 5, 2021

View reviewed changes

Count pod overhead as an entity's resource usage

df48ee4

gjkim42 force-pushed the count-pod-overhead-against-rsquota branch from 0c65219 to df48ee4 Compare March 5, 2021 23:00

gjkim42 commented Mar 5, 2021

View reviewed changes

k8s-ci-robot assigned SergeyKanzhelev Mar 6, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 6, 2021

derekwaynecarr reviewed Mar 9, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2021

k8s-ci-robot merged commit 03ae13a into kubernetes:master Mar 10, 2021

SIG Node PR Triage automation moved this from Needs Approver to Done Mar 10, 2021

k8s-ci-robot added this to the v1.21 milestone Mar 10, 2021

gjkim42 deleted the count-pod-overhead-against-rsquota branch March 10, 2021 01:01

k8s-ci-robot added a commit that referenced this pull request Mar 15, 2021

Merge pull request #100038 from gjkim42/automated-cherry-pick-of-#996…

b6fa3b5

…00-upstream-release-1.19 Automated cherry pick of #99600: Count pod overhead as an entity's resource usage

k8s-ci-robot added a commit that referenced this pull request Mar 15, 2021

Merge pull request #100039 from gjkim42/automated-cherry-pick-of-#996…

f0884a3

…00-upstream-release-1.18 Automated cherry pick of #99600: Count pod overhead as an entity's resource usage

k8s-ci-robot added a commit that referenced this pull request Mar 15, 2021

Merge pull request #100037 from gjkim42/automated-cherry-pick-of-#996…

7e48dab

…00-upstream-release-1.20 Automated cherry pick of #99600: Count pod overhead as an entity's resource usage

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Count pod overhead against an entity's ResourceQuota #99600

Count pod overhead against an entity's ResourceQuota #99600

gjkim42 commented Mar 1, 2021

gjkim42 commented Mar 1, 2021

gjkim42 commented Mar 1, 2021

gjkim42 commented Mar 1, 2021

troy0820 commented Mar 1, 2021

gjkim42 commented Mar 1, 2021

gjkim42 commented Mar 1, 2021

troy0820 commented Mar 1, 2021

gjkim42 commented Mar 1, 2021

troy0820 commented Mar 2, 2021

troy0820 commented Mar 2, 2021

chestack commented Mar 2, 2021 •

edited

gjkim42 commented Mar 2, 2021

gjkim42 commented Mar 2, 2021

gjkim42 commented Mar 4, 2021

gjkim42 commented Mar 4, 2021 •

edited

SergeyKanzhelev Mar 5, 2021

gjkim42 Mar 5, 2021

SergeyKanzhelev Mar 5, 2021

SergeyKanzhelev Mar 6, 2021

SergeyKanzhelev commented Mar 5, 2021 •

edited

SergeyKanzhelev Mar 5, 2021

gjkim42 Mar 5, 2021 •

edited

gjkim42 left a comment

gjkim42 Mar 5, 2021

gjkim42 Mar 5, 2021 •

edited

gjkim42 Mar 5, 2021

gjkim42 commented Mar 5, 2021

SergeyKanzhelev commented Mar 6, 2021

gjkim42 commented Mar 8, 2021 •

edited

derekwaynecarr Mar 9, 2021 •

edited

derekwaynecarr commented Mar 9, 2021

k8s-ci-robot commented Mar 9, 2021

Count pod overhead against an entity's ResourceQuota #99600

Count pod overhead against an entity's ResourceQuota #99600

Conversation

gjkim42 commented Mar 1, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

gjkim42 commented Mar 1, 2021

gjkim42 commented Mar 1, 2021

gjkim42 commented Mar 1, 2021

troy0820 commented Mar 1, 2021

gjkim42 commented Mar 1, 2021

gjkim42 commented Mar 1, 2021

troy0820 commented Mar 1, 2021

gjkim42 commented Mar 1, 2021

troy0820 commented Mar 2, 2021

troy0820 commented Mar 2, 2021

chestack commented Mar 2, 2021 • edited

gjkim42 commented Mar 2, 2021

gjkim42 commented Mar 2, 2021

gjkim42 commented Mar 4, 2021

gjkim42 commented Mar 4, 2021 • edited

SergeyKanzhelev Mar 5, 2021

Choose a reason for hiding this comment

gjkim42 Mar 5, 2021

Choose a reason for hiding this comment

SergeyKanzhelev Mar 5, 2021

Choose a reason for hiding this comment

SergeyKanzhelev Mar 6, 2021

Choose a reason for hiding this comment

SergeyKanzhelev commented Mar 5, 2021 • edited

SergeyKanzhelev Mar 5, 2021

Choose a reason for hiding this comment

gjkim42 Mar 5, 2021 • edited

Choose a reason for hiding this comment

gjkim42 left a comment

Choose a reason for hiding this comment

gjkim42 Mar 5, 2021

Choose a reason for hiding this comment

gjkim42 Mar 5, 2021 • edited

Choose a reason for hiding this comment

gjkim42 Mar 5, 2021

Choose a reason for hiding this comment

gjkim42 commented Mar 5, 2021

SergeyKanzhelev commented Mar 6, 2021

gjkim42 commented Mar 8, 2021 • edited

derekwaynecarr Mar 9, 2021 • edited

Choose a reason for hiding this comment

derekwaynecarr commented Mar 9, 2021

k8s-ci-robot commented Mar 9, 2021

chestack commented Mar 2, 2021 •

edited

gjkim42 commented Mar 4, 2021 •

edited

SergeyKanzhelev commented Mar 5, 2021 •

edited

gjkim42 Mar 5, 2021 •

edited

gjkim42 Mar 5, 2021 •

edited

gjkim42 commented Mar 8, 2021 •

edited

derekwaynecarr Mar 9, 2021 •

edited