Kubelet enabling to support pod-overhead by egernst · Pull Request #79247 · kubernetes/kubernetes

egernst · 2019-06-21T00:51:07Z

Pod and burstable QoS cgroups should take overhead of running a sandbox
into account if the PodOverhead feature is enabled. Update helper
functions which are utilized by Kubelet for sizing the pod and burstable QoS
cgroups as well as for eviction and preemption handling.

While enabling PodOverhead support, refactored a couple of functions to remove duplicated logic and improve test coverage (ie, have eviction handling use the resources package).

What type of PR is this?

/kind feature

What this PR does / why we need it:
Pod and burstable QoS cgroups should take overhead of running a sandbox
into account if the PodOverhead feature is enabled. Similarly, pod overhead should be considered within the eviction handling in Kubernetes.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Introduce support for applying pod overhead to pod cgroups, if the PodOverhead feature is enabled.

egernst · 2019-06-21T00:52:54Z

/cc @tallclair @dashpole

The changes are pretty basic, but I was having issues getting a basic unit test working for PodRequestsAndLimits (there wasn't a test for this function before). AFAICT, when you add two resourceLists, the string associated with each quantity is dropped. Is this just an optimization (creating string is a costly operation?)?

ie:

--- FAIL: TestPodRequestsAndLimits (0.00s)
    helpers_test.go:264: test case failure: [2]
        expected:       map[cpu:{{6 0} {<nil>} 6 DecimalSI} memory:{{15 0} {<nil>} 15 DecimalSI}]
        got             map[cpu:{{6 0} {<nil>}  DecimalSI} memory:{{15 0} {<nil>}  DecimalSI}]

I think this is why the Deep.Equal is failing. Is this the expected behavior? Or, any suggestions?

pkg/api/v1/resource/helpers_test.go

dashpole · 2019-06-21T21:26:26Z

Can we use quantity's Equal method to test equality of quantities instead of DeepEqual?

egernst · 2019-06-21T21:40:01Z

@dashpole - yes, but that'll require looping through each resource in the list, verifying each entry exists, and that for each quantity, quantity.Equal(expected). A bit painful; I'd at least like to understand if its expected behavior or not. Perhaps there should be a "ResourceList" Equal function?

egernst · 2019-08-02T17:33:17Z

Updated the test per @dashpole's recommendation, and improved limits handling

egernst · 2019-08-02T17:36:39Z

/cc @mcastelino @jcvenegas

k8s-ci-robot · 2019-08-02T17:36:40Z

@egernst: GitHub didn't allow me to request PR reviews from the following users: mcastelino, jcvenegas.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @mcastelino @jcvenegas

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.

egernst

Updated based on feedback - PTAL @tallclair @dashpole

pkg/api/v1/resource/helpers.go

Update GetResoureqRequest function to utilize a new helper, GetResourceRequestQuantity. This logic was duplicated in a couple of areas in the K/K codebase, so consolidating for better test coverage. Signed-off-by: Eric Ernst <eric.ernst@intel.com>

Utilize resource helpers' GetResourceRequestQuantity instead of duplicating the logic here. Signed-off-by: Eric Ernst <eric.ernst@intel.com>

No test content changes - just improvements for readability Signed-off-by: Eric Ernst <eric.ernst@intel.com>

tallclair

Mostly nits, sorry to complain about the newlines.

pkg/api/v1/resource/helpers.go

pkg/api/v1/resource/helpers_test.go

tallclair · 2019-08-15T22:30:31Z

pkg/api/v1/resource/helpers_test.go

thanks, so much more readable this way.

egernst · 2019-08-15T22:56:48Z

Thanks @tallclair - will keep that in mind for next PR. Addressed feedback, including utilizing .Copy() instead of assigning potential cached value.

tallclair · 2019-08-16T20:29:58Z

Thanks!

/lgtm
/approve

Need to fix up ssome of those CI failures. rerun hack/update-bazel.sh, not sure if there are other issues.

tallclair · 2019-08-19T21:13:45Z

/lgtm

tallclair · 2019-08-19T21:26:09Z

/assign @thockin
For /pkg/api approval.

thockin

Thanks!

/approve

k8s-ci-robot · 2019-08-19T23:08:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: egernst, tallclair, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/api/OWNERS~~ [thockin]
~~pkg/kubelet/eviction/OWNERS~~ [tallclair,thockin]

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

egernst · 2019-08-20T00:23:51Z

/test pull-kubernetes-e2e-gce

tallclair · 2019-08-20T00:27:18Z

pkg/api/v1/resource/helpers.go

nit: use DeepCopy (#81627)

Suggested change

requestQuantity = *rQuantity.Copy()

requestQuantity = rQuantity.DeepCopy()

tallclair · 2019-08-20T00:33:48Z

Hmm, the test failures are somewhat suspicious since it's the density & loader tests failing, which would be affected by PodOverhead. Except that overhead on those should be zero, and I think the feature gate is disabled, so probably a flake. Worth taking a look at though.

Pod and burstable QoS cgroups should take overhead of running a sandbox into account if the PodOverhead feature is enabled. These helper functions are utilized by Kubelet for sizing the pod and burstable QoS cgroups. Pod overhead is added to resource requests, regardless of the initial request values. A particular resource pod overhead is only added to a resource limit if a non-zero limit already existed. This commit updates eviction handling to also take Pod Overhead into account (if the feature is enabled). Signed-off-by: Eric Ernst <eric.ernst@intel.com>

egernst · 2019-08-20T00:43:45Z

Hmm, the test failures are somewhat suspicious since it's the density & loader tests failing, which would be affected by PodOverhead. Except that overhead on those should be zero, and I think the feature gate is disabled, so probably a flake. Worth taking a look at though.

Thanks. It is at the very least a reminder of e2e tests which should be added for PodOverhead.

tallclair · 2019-08-20T17:30:55Z

/lgtm

tedyu · 2019-08-20T20:51:02Z

pkg/api/v1/resource/helpers.go

-				totalResources += rQuantity.Value()
-			}
+		if rQuantity, ok := container.Resources.Requests[resourceName]; ok {
+			requestQuantity.Add(rQuantity)


Should rQuantity.MilliValue() be used if resourceName is v1.ResourceCPU ?

k8s-ci-robot requested review from deads2k and madhusudancs June 21, 2019 00:52

k8s-ci-robot requested review from dashpole and tallclair June 21, 2019 00:52

egernst force-pushed the kubelet-PodOverhead branch from 6e67a56 to 68cfa0b Compare June 21, 2019 01:12

egernst commented Jun 21, 2019

View reviewed changes

pkg/api/v1/resource/helpers_test.go Outdated Show resolved Hide resolved

egernst force-pushed the kubelet-PodOverhead branch from 68cfa0b to 6837f7b Compare June 21, 2019 17:03

liwei mentioned this pull request Jun 22, 2019

config: add an option to control memory size calculation kata-containers/runtime#1823

Closed

egernst mentioned this pull request Jul 9, 2019

Pod Overhead: account resources tied to the pod sandbox, but not specific containers kubernetes/enhancements#688

Closed

egernst force-pushed the kubelet-PodOverhead branch 2 times, most recently from 2163ce4 to 751e2d9 Compare August 2, 2019 17:31

egernst changed the title ~~pod-overhead: add pod overhead to resouce requests~~ pod-overhead: utilize pod overhead for cgroup sizing in kubelet Aug 2, 2019

egernst force-pushed the kubelet-PodOverhead branch from 751e2d9 to 87d1f3f Compare August 2, 2019 17:36

egernst changed the title ~~pod-overhead: utilize pod overhead for cgroup sizing in kubelet~~ Kubelet enabling to support pod-overhead Aug 2, 2019

egernst force-pushed the kubelet-PodOverhead branch from 87d1f3f to 08f189a Compare August 2, 2019 22:14

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 2, 2019

egernst commented Aug 13, 2019

View reviewed changes

pkg/api/v1/resource/helpers.go Outdated Show resolved Hide resolved

pkg/api/v1/resource/helpers.go Outdated Show resolved Hide resolved

Eric Ernst added 3 commits August 13, 2019 16:23

kube-eviction: use common resource summation functions

476c1c7

Utilize resource helpers' GetResourceRequestQuantity instead of duplicating the logic here. Signed-off-by: Eric Ernst <eric.ernst@intel.com>

resource: cleanup helpers_test.go

f137a9c

No test content changes - just improvements for readability Signed-off-by: Eric Ernst <eric.ernst@intel.com>

tallclair reviewed Aug 15, 2019

View reviewed changes

egernst force-pushed the kubelet-PodOverhead branch from 0b515bc to 05b9ff4 Compare August 15, 2019 22:55

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 16, 2019

egernst force-pushed the kubelet-PodOverhead branch from 05b9ff4 to b813515 Compare August 19, 2019 18:34

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 19, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 19, 2019

k8s-ci-robot assigned thockin Aug 19, 2019

thockin reviewed Aug 19, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 19, 2019

tallclair reviewed Aug 20, 2019

View reviewed changes

egernst force-pushed the kubelet-PodOverhead branch from b813515 to 80ee072 Compare August 20, 2019 00:33

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 20, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 20, 2019

k8s-ci-robot merged commit 8cf05f5 into kubernetes:master Aug 20, 2019

k8s-ci-robot added this to the v1.16 milestone Aug 20, 2019

tedyu reviewed Aug 20, 2019

View reviewed changes

	requestQuantity = *rQuantity.Copy()
	requestQuantity = rQuantity.DeepCopy()

Conversation

egernst commented Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

egernst commented Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dashpole commented Jun 21, 2019

Uh oh!

egernst commented Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

egernst commented Aug 2, 2019

Uh oh!

egernst commented Aug 2, 2019

Uh oh!

k8s-ci-robot commented Aug 2, 2019

Uh oh!

egernst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tallclair left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tallclair Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

egernst commented Aug 15, 2019

Uh oh!

tallclair commented Aug 16, 2019

Uh oh!

tallclair commented Aug 19, 2019

Uh oh!

tallclair commented Aug 19, 2019

Uh oh!

thockin left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Aug 19, 2019

Uh oh!

egernst commented Aug 20, 2019

Uh oh!

tallclair Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

egernst Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

tallclair commented Aug 20, 2019

Uh oh!

egernst commented Aug 20, 2019

Uh oh!

tallclair commented Aug 20, 2019

Uh oh!

tedyu Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

egernst commented Jun 21, 2019 •

edited

Loading

egernst commented Jun 21, 2019 •

edited

Loading

egernst commented Jun 21, 2019 •

edited

Loading