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.

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

Kubelet enabling to support pod-overhead #79247

Merged
merged 4 commits into from Aug 20, 2019

Conversation

@egernst
Copy link
Contributor

commented Jun 21, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

/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?

@k8s-ci-robot k8s-ci-robot requested review from dashpole and tallclair Jun 21, 2019

@egernst egernst force-pushed the egernst:kubelet-PodOverhead branch from 6e67a56 to 68cfa0b Jun 21, 2019

@egernst egernst force-pushed the egernst:kubelet-PodOverhead branch from 68cfa0b to 6837f7b Jun 21, 2019

@dashpole

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

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

@egernst

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

@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?

liwei added a commit to liwei/kata-runtime that referenced this pull request Jun 22, 2019
config: add an option to control memory size calculation
The sandbox memory overhead was not take into account when
calculating sandbox memory quota size, that cause the following
issues:

- scheduler can't get an accurate memory usage information
- memory size info in container not match user specified memory limit,
  that would confuse the user

Before the pod overhead patch[1] got merged, let's add an option
to control memory size calculation to workaround these issues.

[1]: kubernetes/kubernetes#79247

Signed-off-by: Li Wei <wei@hyper.sh>
liwei added a commit to liwei/kata-runtime that referenced this pull request Jun 22, 2019
config: add an option to control memory size calculation
The sandbox memory overhead was not take into account when
calculating sandbox memory quota size, that cause the following
issues:

- scheduler can't get an accurate memory usage information
- memory size info in container not match user specified memory limit,
  that would confuse the user

Before the pod overhead patch[1] got merged, let's add an option
to control memory size calculation to workaround these issues.

[1]: kubernetes/kubernetes#79247

Fixes: kata-containers#1822
Signed-off-by: Li Wei <wei@hyper.sh>
liwei added a commit to liwei/kata-runtime that referenced this pull request Jun 24, 2019
config: add an option to control vcpu/memory resource calculation
The vcpu/memory overhead of sandbox are not take into account when
calculating container resource quota, that cause the following
issues:

- scheduler can't get an accurate resource usage information
- cpu/memory size in container do not match user specified,
  that would confuse users

Before the pod overhead patch[1] got merged, let's add an option
to control resource calculation to workaround these issues.

[1]: kubernetes/kubernetes#79247

Fixes: kata-containers#1822
Signed-off-by: Li Wei <wei@hyper.sh>

@egernst egernst force-pushed the egernst:kubelet-PodOverhead branch 2 times, most recently from 2163ce4 to 751e2d9 Aug 2, 2019

@egernst 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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

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

@egernst egernst force-pushed the egernst:kubelet-PodOverhead branch from 751e2d9 to 87d1f3f Aug 2, 2019

@egernst

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

@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.

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 egernst changed the title pod-overhead: utilize pod overhead for cgroup sizing in kubelet Kubelet enabling to support pod-overhead Aug 2, 2019

@egernst egernst force-pushed the egernst:kubelet-PodOverhead branch from 87d1f3f to 08f189a Aug 2, 2019

egernst added 3 commits Aug 2, 2019
kube-eviction: use common resource summation functions
Utilize resource helpers' GetResourceRequestQuantity instead of
duplicating the logic here.

Signed-off-by: Eric Ernst <eric.ernst@intel.com>
resource: modify resource helpers for better reuse
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>
resource: cleanup helpers_test.go
No test content changes - just improvements for readability

Signed-off-by: Eric Ernst <eric.ernst@intel.com>
@tallclair
Copy link
Member

left a comment

Mostly nits, sorry to complain about the newlines.

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

type podResources struct {

This comment has been minimized.

Copy link
@tallclair

tallclair Aug 15, 2019

Member

thanks, so much more readable this way.

@egernst egernst force-pushed the egernst:kubelet-PodOverhead branch from 0b515bc to 05b9ff4 Aug 15, 2019

@egernst

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

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

@tallclair

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

Thanks!

/lgtm
/approve

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

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 16, 2019

@egernst egernst force-pushed the egernst:kubelet-PodOverhead branch from 05b9ff4 to b813515 Aug 19, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 19, 2019

@tallclair

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 19, 2019

@tallclair

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

/assign @thockin
For /pkg/api approval.

@thockin
Copy link
Member

left a comment

Thanks!

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

[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

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

@egernst

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

/test pull-kubernetes-e2e-gce

totalResources = rQuantity.Value()
if rQuantity, ok := container.Resources.Requests[resourceName]; ok {
if requestQuantity.Cmp(rQuantity) < 0 {
requestQuantity = *rQuantity.Copy()

This comment has been minimized.

Copy link
@tallclair

tallclair Aug 20, 2019

Member

nit: use DeepCopy (#81627)

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

This comment has been minimized.

Copy link
@egernst

egernst Aug 20, 2019

Author Contributor

done

@tallclair

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

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.

@egernst egernst force-pushed the egernst:kubelet-PodOverhead branch from b813515 to 80ee072 Aug 20, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 20, 2019

pod-overhead: utilize pod overhead for cgroup sizing, eviction handling
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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

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

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 20, 2019

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

23 checks passed

cla/linuxfoundation egernst authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

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

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

This comment has been minimized.

Copy link
@tedyu

tedyu Aug 20, 2019

Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.