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: include init containers when determining pod QoS #75223

Merged

Conversation

@sjenning
Copy link
Contributor

commented Mar 8, 2019

fixes #75212

@derekwaynecarr @dashpole

Init container resource requests now impact pod QoS class

/sig node

@dashpole

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

This looks correct.
/lgtm

@dims

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

/assign @liggitt @smarterclayton

Looks like something we need fixed in 1.14 right? please apply the relevant labels please

@@ -39,7 +39,7 @@ func GetPodQOS(pod *core.Pod) core.PodQOSClass {
limits := core.ResourceList{}
zeroQuantity := resource.MustParse("0")
isGuaranteed := true
for _, container := range pod.Spec.Containers {
for _, container := range append(pod.Spec.Containers, pod.Spec.InitContainers...) {

This comment has been minimized.

Copy link
@liggitt

liggitt Mar 11, 2019

Member

appending to a slice we don't own can end up sharing memory in unpleasant ways. prefer double iterating or appending to a slice we allocated (didn't check how frequently this is called to know which is better)

This comment has been minimized.

Copy link
@sjenning

sjenning Mar 11, 2019

Author Contributor

ok, changed pushed. thank for the review 👍

@sjenning

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

/kind bug
/priority important-soon

@sjenning sjenning force-pushed the sjenning:fix-pod-qos-init-containers branch from 190fcee to 370a993 Mar 11, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 11, 2019

@derekwaynecarr

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

/lgtm
/approve

@sjenning

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

/assign @liggitt

@liggitt

This comment has been minimized.

Copy link
Member

commented May 16, 2019

#75223 (comment) still applies... the changes in this PR are still appending to pod.Spec.Containers

@sjenning sjenning force-pushed the sjenning:fix-pod-qos-init-containers branch from 370a993 to 7b9ee59 May 17, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 17, 2019

@sjenning sjenning force-pushed the sjenning:fix-pod-qos-init-containers branch from 7b9ee59 to bcfa60d May 17, 2019

@dashpole

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 29, 2019

@sjenning

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

@liggitt can I get apis/core approval on this?

@liggitt

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

This looks reasonable. Do we know what will happen if an existing pod changes QOS class on upgrade as a result of this change? Is this used in the api server or kubelet or both?

@liggitt

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

/approve

/hold
For question on effect on existing pods on upgrade

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, liggitt, sjenning

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

@sjenning

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

The kubelet computes the QoS class whenever it need to know since it already contains the logic.

Some history #33255 #37968 #38989

This change might cause the QOSClass field in the Pod Status to change. This does not impact Kubernetes, however, custom clients looking at this field might not expect the change. Status for a resources is understood to be mutable though so... hard to say. @derekwaynecarr ?

@liggitt

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

if other places computing this were already doing so considering initContainers, then making this consistent seems reasonable

/hold cancel

@fejta-bot

This comment has been minimized.

Copy link

commented Jun 25, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit cff6e42 into kubernetes:master Jun 25, 2019

20 of 23 checks passed

pull-kubernetes-bazel-build Job triggered.
Details
pull-kubernetes-node-e2e-containerd Job triggered.
Details
tide Not mergeable. Job pull-kubernetes-bazel-build has not succeeded.
Details
cla/linuxfoundation sjenning authorized
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-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
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.