kubelet: Admission must exclude completed pods and avoid races #104577

smarterclayton · 2021-08-25T15:49:54Z

Fixes two issues with how the pod worker refactor calculated the
pods that admission could see (GetActivePods() and
filterOutTerminatedPods())

First, completed pods must be filtered from the "desired" state
for admission, which arguably should be happening earlier in
config. Exclude the two terminal pods states from GetActivePods()

Second, the previous check introduced with the pod worker lifecycle
ownership changes was subtly wrong for the admission use case.
Admission has to include pods that haven't yet hit the pod worker,
which CouldHaveRunningContainers was filtering out (because the
pod worker hasn't seen them). Introduce a weaker check -
IsPodKnownTerminated() - that returns true only if the pod is in
a known terminated state (no running containers AND known to pod
worker). This weaker check may only be called from components that
need admitted pods, not other kubelet subsystems.

This commit does not fix the long standing bug that force deleted
pods are omitted from admission checks, which must be fixed by
having GetActivePods() also include pods "still terminating".

/kind bug
/kind regression

Fixes #104560

Fix a 1.22 regression where the Kubelet failed to exclude already completed pods from calculations about how many resources it was currently using when deciding whether to allow more pods.

Needs additional e2es to prevent future regressions.

k8s-ci-robot · 2021-08-25T15:51:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton

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/kubelet/OWNERS~~ [smarterclayton]

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

rphillips · 2021-08-25T15:53:06Z

/triage accepted
/priority critical-urgent

smarterclayton · 2021-08-25T16:09:01Z

Still needs a unit test to exercise the admission filter here. Can add the node e2e here or separately for completed pod (although for regression purposes I'd be ok with that separate)

rphillips · 2021-08-25T16:19:15Z

/hold for the test or top level approver

Fixes two issues with how the pod worker refactor calculated the pods that admission could see (GetActivePods() and filterOutTerminatedPods()) First, completed pods must be filtered from the "desired" state for admission, which arguably should be happening earlier in config. Exclude the two terminal pods states from GetActivePods() Second, the previous check introduced with the pod worker lifecycle ownership changes was subtly wrong for the admission use case. Admission has to include pods that haven't yet hit the pod worker, which CouldHaveRunningContainers was filtering out (because the pod worker hasn't seen them). Introduce a weaker check - IsPodKnownTerminated() - that returns true only if the pod is in a known terminated state (no running containers AND known to pod worker). This weaker check may only be called from components that need admitted pods, not other kubelet subsystems. This commit does not fix the long standing bug that force deleted pods are omitted from admission checks, which must be fixed by having GetActivePods() also include pods "still terminating".

bobbypage · 2021-08-25T20:46:42Z

Thanks @smarterclayton for the quick investigation and proposed fix.

I validated your PR against the minimal repro test created in #104560 (comment) and can confirm the PR here results in pods no longer being rejected by kubelet.

$ gh pr checkout 104577
# cherry-pick e2e test (https://github.com/bobbypage/kubernetes/blob/b8e2c5bec8f12d866493ecfccede3669377a7f0c/test/e2e/scheduling/job-completions.go)
$ git cherry-pick b8e2c5bec8f12d866493ecfccede3669377a7f0c
$ kind build node-image
$ kind create cluster --config ${HOME}/dev_scripts/kind-config-e2e.yaml --image kindest/node:latest
$ ./_output/bin/e2e.test -kubeconfig ${HOME}/.kube/kind-test-config -ginkgo.focus="\[Feature:JobWithCompletions\]" --num-nodes=1 2>&1 | tee -i "/tmp/build-log.txt"

Test passes - build log - https://gist.github.com/c59e092d6befdb74fab35caeed2571c1

bobbypage · 2021-08-25T21:07:58Z

pkg/kubelet/kubelet_pods.go

+		if kl.podWorkers.IsPodKnownTerminated(p.UID) {
+			continue
+		}
+		if p.Status.Phase == v1.PodSucceeded || p.Status.Phase == v1.PodFailed {


curious for my own understanding, why is it necessary to also check the pod phase in addition to IsPodKnownTerminated?

Is it not guaranteed that if status.IsTerminated() for the pod worker then the pod is in succeeded or failed phase?

The pod worker is not guaranteed to track terminal pods - once a pod is terminal, the worker shuts down and then the metadata is eventually removed.

rphillips · 2021-08-26T19:44:46Z

@bobbypage I ported your test case over into #104611. Any other changes you would like to see?

harche · 2021-08-30T11:22:55Z

@smarterclayton @bobbypage @rphillips I was able to write a node e2e to test this PR - #104658

mrunalp · 2021-08-30T18:46:02Z

/lgtm

Tagging this and then we can pick one of the e2e PRs on top.

bobbypage · 2021-08-30T18:47:13Z

Thanks @harche for creating node e2e test.

I suggest we proceed with merging this PR as is and then either pick up @harche 's node e2e test as followup or the original one I made as @rphillips suggested in #104611.

bobbypage · 2021-08-30T18:47:19Z

/lgtm

rphillips · 2021-08-30T18:48:17Z

/hold cancel

…04577-upstream-release-1.22 Automated cherry pick of #104577: kubelet: Admission must exclude completed pods and avoid

harche · 2021-08-31T14:19:42Z

Thanks @harche for creating node e2e test.

I suggest we proceed with merging this PR as is and then either pick up @harche 's node e2e test as followup or the original one I made as @rphillips suggested in #104611.

@bobbypage I closed the with node e2e because it wasn't really reproducing the scenario correctly, I would vote for your original test case which went through scheduler which is now part of this PR, #104611

smarterclayton · 2021-09-07T16:32:06Z

Found another gap, #104817 is also needed when a pod is rejected. The test we add also needs to test that rejected pods (not just completed pods) are part of admission failures.

Once we verified #104817 it would also need to be backported.

k8s-ci-robot requested review from andrewsykim and yujuhong August 25, 2021 15:50

smarterclayton mentioned this pull request Aug 25, 2021

DO NOT MERGE: kubelet: Include not yet admitted pods in admission checks #104576

Closed

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

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 25, 2021

smarterclayton changed the title ~~kubelet: Admission must exclude completed tests and avoid races~~ kubelet: Admission must exclude completed pods and avoid races Aug 25, 2021

smarterclayton force-pushed the smaller_filter_master branch from 9ed9b5f to a2ca66d Compare August 25, 2021 17:31

bobbypage reviewed Aug 25, 2021

View reviewed changes

rphillips mentioned this pull request Aug 26, 2021

WIP: kubelet: Admission must exclude completed pods and avoid races e2e test #104611

Closed

harche mentioned this pull request Aug 30, 2021

WIP: Node e2e test for completed pods #104658

Closed

openshift-ci-robot mentioned this pull request Aug 30, 2021

Bug 1997657: UPSTREAM: 104577: kubelet: Admission must exclude completed pods and avoid races openshift/kubernetes#920

Merged

k8s-ci-robot assigned mrunalp Aug 30, 2021

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

k8s-ci-robot assigned bobbypage Aug 30, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 30, 2021

bobbypage mentioned this pull request Aug 30, 2021

Automated cherry pick of #104577: kubelet: Admission must exclude completed pods and avoid #104665

Merged

k8s-ci-robot merged commit bbbeceb into kubernetes:master Aug 30, 2021

k8s-ci-robot added this to the v1.23 milestone Aug 30, 2021

k8s-ci-robot added a commit that referenced this pull request Aug 31, 2021

Merge pull request #104665 from bobbypage/automated-cherry-pick-of-#1…

33b12ea

…04577-upstream-release-1.22 Automated cherry pick of #104577: kubelet: Admission must exclude completed pods and avoid

smarterclayton mentioned this pull request Sep 7, 2021

kubelet: Rejected pods should be filtered from admission #104817

Merged

bobbypage mentioned this pull request Feb 25, 2022

Status of pods can become "OutOfCpu" when many pods are created and completed in a short time on the same node. #106884

Closed

luozhiwenn mentioned this pull request Jan 28, 2023

kubelet: fix exclusive CPUs not removed from deleted Pod #107732

Closed

likakuli mentioned this pull request Sep 14, 2023

fix: exclusive CPUs removed from deleted Pod and put back in the defaultCPUSet #120661

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubelet: Admission must exclude completed pods and avoid races #104577

kubelet: Admission must exclude completed pods and avoid races #104577

smarterclayton commented Aug 25, 2021 •

edited by liggitt

k8s-ci-robot commented Aug 25, 2021

rphillips commented Aug 25, 2021

smarterclayton commented Aug 25, 2021

rphillips commented Aug 25, 2021 •

edited

bobbypage commented Aug 25, 2021 •

edited

bobbypage Aug 25, 2021

smarterclayton Aug 30, 2021

rphillips commented Aug 26, 2021

harche commented Aug 30, 2021

mrunalp commented Aug 30, 2021

bobbypage commented Aug 30, 2021 •

edited

bobbypage commented Aug 30, 2021

rphillips commented Aug 30, 2021

harche commented Aug 31, 2021 •

edited

smarterclayton commented Sep 7, 2021

kubelet: Admission must exclude completed pods and avoid races #104577

kubelet: Admission must exclude completed pods and avoid races #104577

Conversation

smarterclayton commented Aug 25, 2021 • edited by liggitt

k8s-ci-robot commented Aug 25, 2021

rphillips commented Aug 25, 2021

smarterclayton commented Aug 25, 2021

rphillips commented Aug 25, 2021 • edited

bobbypage commented Aug 25, 2021 • edited

bobbypage Aug 25, 2021

Choose a reason for hiding this comment

smarterclayton Aug 30, 2021

Choose a reason for hiding this comment

rphillips commented Aug 26, 2021

harche commented Aug 30, 2021

mrunalp commented Aug 30, 2021

bobbypage commented Aug 30, 2021 • edited

bobbypage commented Aug 30, 2021

rphillips commented Aug 30, 2021

harche commented Aug 31, 2021 • edited

smarterclayton commented Sep 7, 2021

smarterclayton commented Aug 25, 2021 •

edited by liggitt

rphillips commented Aug 25, 2021 •

edited

bobbypage commented Aug 25, 2021 •

edited

bobbypage commented Aug 30, 2021 •

edited

harche commented Aug 31, 2021 •

edited