Don't create a new sandbox for pod with RestartPolicyOnFailure if all containers succeeded #92614

tnqn · 2020-06-29T18:16:34Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
The kubelet would attempt to create a new sandbox for a pod whose RestartPolicy is OnFailure even after all container succeeded. It caused unnecessary CRI and CNI calls, confusing logs and conflicts between the routine that creates the new sandbox and the routine that kills the Pod.

This patch checks the containers to start and stops creating sandbox if no container is supposed to start.

Which issue(s) this PR fixes:

Fixes #92613

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fixed kubelet creating extra sandbox for pods with RestartPolicyOnFailure after all containers succeeded

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

k8s-ci-robot · 2020-06-29T18:16:42Z

Hi @tnqn. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

neolit123 · 2020-06-29T18:23:49Z

/ok-to-test

neolit123 · 2020-06-29T18:24:11Z

Does this PR introduce a user-facing change?:

^ please add a release note that explains the change in the kubelet

pkg/kubelet/kuberuntime/kuberuntime_manager.go

SergeyKanzhelev · 2020-06-29T20:38:10Z

pkg/kubelet/kuberuntime/kuberuntime_manager.go

-			}
-			changes.ContainersToStart = append(changes.ContainersToStart, idx)
-		}
+		changes.ContainersToStart = containersToStart


another nit: wouldn't it be more logical to move the loop back here - after the Init containers check.

I think it's important to change ordering back to make sure that any changes related to containers start/stop are only applied after all Init container complete. Just to avoid potential issues in a future if more logic will be added

containersToStart is also used to determine whether there's a need to create a sandbox. Wouldn't it cause repeated computation if moving the loop back here?

From the logic here - if the if len(pod.Spec.InitContainers) != 0 { than we need to start an Init container. So this condition check should go before any container-related ones. I was imagining situation when len(pod.Spec.Containers) is 0, i.e. pod has no containers, but has Init containers. In this case if order of checks will be as it's written, Init containers wouldn't run. The situation when there are no containers in a pod seems to be impossible today, thus it's just a recommendation to swap these checks. In anticipation that one day more conditions may be added above which semantically needs to run after all Init containers logic complete.

Thanks for the explanation. I understand the concern now, but swapping them back could cause sandbox still being recreated if any initContainer is present. The current workflow of a Pod with initContainer is and would be kept as below:

create sandbox, create init container, create container (expected actions)

all containers succeeded, delete sandbox (expected actions)

create sandbox, create init container (unexpected actions)

If taking potential no container case into consideration, could we add a condition to ensure this is not the first time to create sandbox like

kubernetes/pkg/kubelet/kuberuntime/kuberuntime_manager.go

Line 490 in e7ca64f

if !shouldRestartOnFailure(pod) && attempt != 0 && len(podStatus.ContainerStatuses) != 0 {

w.r.t. Sergey's comment, is it possible that a pod has:

some init containers (> 0)
no container
some Ephemeral Containers (> 0)

To ensure all initContainers can run once successfully regardless of there are containers to start, I added one more condition for not creating sandbox. It should address the potential case "init containers (> 0), no container".

For the case of Ephemeral Containers (> 0), I think it's not affected by this PR. It was never created along with (re-)creating sandbox, and was handled when createPodSandbox is false after initContainer or normal container became running. The behavior is kept as it is.

I made a comment above. Suggestion is to minimize the introduced changes.

Also, maybe introducing a test with the situaiton @tedyu outlined will be beneficial. So future changes will take it into account.

There are already some tests to cover a Pod in TestComputePodActionsWithInitAndEphemeralContainers for example:

kubernetes/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go

Line 1265 in e7ca64f

"Kill pod and do not restart ephemeral container if the pod sandbox is dead": {

And I have added Create a new pod sandbox if the pod sandbox is dead, init container failed and RestartPolicy == OnFailure to it to ensure sandbox can be recreated as long as initialization is not done regardless of regular containers and ephemeral containers' status.
I didn't really delete regular containers from the pod spec of the tests, thinking it sounds strange to have a case with illegal input, but I think they should cover the situation @tedyu outlined implicitly

tnqn · 2020-06-30T02:18:28Z

Does this PR introduce a user-facing change?:

^ please add a release note that explains the change in the kubelet

Done, thanks.

tedyu · 2020-06-30T04:24:10Z

Is it possible to add an e2e node test (under test/e2e_node) that covers the scenario ?

Thanks

tnqn · 2020-07-08T06:37:14Z

/retest

tnqn · 2020-07-08T14:06:40Z

/retest

tnqn · 2020-07-10T16:38:16Z

/retest

SergeyKanzhelev · 2020-07-15T21:16:55Z

I don't have permissions to lgtm, for what it worth:

/lgtm

SergeyKanzhelev · 2020-08-16T05:53:42Z

/test pull-kubernetes-e2e-kind-ipv6

tnqn · 2020-09-02T03:22:19Z

@SergeyKanzhelev @dashpole @dims could this PR get into 1.20 milestone?

dims · 2020-09-03T18:17:26Z

/milestone v1.20

alex-vmw · 2020-09-03T18:24:24Z

@tnqn @dims Would it be possible to also backport this into 1.18/1.19? Thanks.

SergeyKanzhelev · 2020-09-03T18:37:34Z

If you want 1.19.1 cutoff is Friday I think. So need to do it today

SergeyKanzhelev · 2020-09-03T20:20:19Z

failure doesn't seem to be related

/retest

tnqn · 2020-09-11T16:28:09Z

@alex-vmw @SergeyKanzhelev I just created #94725 to backport to 1.19, hope it can catch 1.19.2.

alex-vmw · 2020-09-21T20:24:59Z

@tnqn How about for 1.18.x? Any plans to backport there?

cpanato · 2020-10-06T11:57:37Z

@tnqn @SergeyKanzhelev this only need for 1.18 and 1.19, not for 1.17?

tnqn · 2020-10-13T06:34:58Z

@tnqn @SergeyKanzhelev this only need for 1.18 and 1.19, not for 1.17?

@cpanato I just checked the issue can be reproduced on 1.17.12 as well, created #95508 for 1.17.

…pstream-release-1.19 Automated cherry pick of #92614: Don't create a new sandbox for pod with

…pstream-release-1.18 Automated cherry pick of #92614: Don't create a new sandbox for pod with

…pstream-release-1.17 Automated cherry pick of #92614: Don't create a new sandbox for pod with

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 29, 2020

tnqn mentioned this pull request Jun 29, 2020

kubelet always creates sandbox twice for Pods with RestartPolicyOnFailure #92613

Closed

k8s-ci-robot added the area/kubelet label Jun 29, 2020

k8s-ci-robot requested review from dashpole and dims June 29, 2020 18:19

k8s-ci-robot added 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 Jun 29, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 29, 2020

SergeyKanzhelev reviewed Jun 29, 2020

View reviewed changes

pkg/kubelet/kuberuntime/kuberuntime_manager.go Outdated Show resolved Hide resolved

SergeyKanzhelev reviewed Jun 29, 2020

View reviewed changes

tnqn force-pushed the onfailure-recreate branch from 743e396 to bf4e43c Compare June 30, 2020 02:12

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 30, 2020

tnqn force-pushed the onfailure-recreate branch from bf4e43c to 1e66062 Compare June 30, 2020 13:14

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 30, 2020

tnqn force-pushed the onfailure-recreate branch from fc3ef9e to b2b082f Compare July 7, 2020 14:51

k8s-ci-robot assigned SergeyKanzhelev Jul 15, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2020

dims approved these changes Jul 26, 2020

View reviewed changes

k8s-ci-robot added this to the v1.20 milestone Sep 3, 2020

k8s-ci-robot merged commit 48d5d20 into kubernetes:master Sep 3, 2020

tnqn mentioned this pull request Sep 11, 2020

Automated cherry pick of #92614: Don't create a new sandbox for pod with #94725

Merged

tnqn mentioned this pull request Sep 22, 2020

Automated cherry pick of #92614: Don't create a new sandbox for pod with #94959

Merged

tnqn mentioned this pull request Oct 13, 2020

Automated cherry pick of #92614: Don't create a new sandbox for pod with #95508

Merged

k8s-ci-robot added a commit that referenced this pull request Oct 23, 2020

Merge pull request #94725 from tnqn/automated-cherry-pick-of-#92614-u…

94cbbc0

…pstream-release-1.19 Automated cherry pick of #92614: Don't create a new sandbox for pod with

SergeyKanzhelev mentioned this pull request Nov 18, 2020

kubelet: do not rerun init containers if any main containers have status #96572

Merged

k8s-ci-robot added a commit that referenced this pull request Nov 27, 2020

Merge pull request #94959 from tnqn/automated-cherry-pick-of-#92614-u…

fcb54b8

…pstream-release-1.18 Automated cherry pick of #92614: Don't create a new sandbox for pod with

k8s-ci-robot added a commit that referenced this pull request Nov 27, 2020

Merge pull request #95508 from tnqn/automated-cherry-pick-of-#92614-u…

64a9540

…pstream-release-1.17 Automated cherry pick of #92614: Don't create a new sandbox for pod with

aojea mentioned this pull request Dec 30, 2020

kuberuntime: clean up container and suppress sandbox creation if pod is done #90727

Closed

liuxu623 mentioned this pull request Feb 24, 2021

fix recreate sandbox when a job is done #99343

Closed

SergeyKanzhelev mentioned this pull request Aug 5, 2024

adding SergeyKanzhelev as SIG Node approver #126551

Merged

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't create a new sandbox for pod with RestartPolicyOnFailure if all containers succeeded #92614

Don't create a new sandbox for pod with RestartPolicyOnFailure if all containers succeeded #92614

tnqn commented Jun 29, 2020 •

edited

Loading

k8s-ci-robot commented Jun 29, 2020

neolit123 commented Jun 29, 2020

neolit123 commented Jun 29, 2020

SergeyKanzhelev Jun 29, 2020

SergeyKanzhelev Jun 29, 2020

tnqn Jun 30, 2020

SergeyKanzhelev Jun 30, 2020

tnqn Jun 30, 2020

tedyu Jun 30, 2020

tnqn Jun 30, 2020

SergeyKanzhelev Jun 30, 2020

SergeyKanzhelev Jun 30, 2020

tnqn Jul 1, 2020

tnqn commented Jun 30, 2020

tedyu commented Jun 30, 2020

tnqn commented Jul 8, 2020

tnqn commented Jul 8, 2020

tnqn commented Jul 10, 2020

SergeyKanzhelev commented Jul 15, 2020

SergeyKanzhelev commented Aug 16, 2020

tnqn commented Sep 2, 2020

dims commented Sep 3, 2020

alex-vmw commented Sep 3, 2020

SergeyKanzhelev commented Sep 3, 2020

SergeyKanzhelev commented Sep 3, 2020

tnqn commented Sep 11, 2020

alex-vmw commented Sep 21, 2020

cpanato commented Oct 6, 2020

tnqn commented Oct 13, 2020

Don't create a new sandbox for pod with RestartPolicyOnFailure if all containers succeeded #92614

Don't create a new sandbox for pod with RestartPolicyOnFailure if all containers succeeded #92614

Conversation

tnqn commented Jun 29, 2020 • edited Loading

k8s-ci-robot commented Jun 29, 2020

neolit123 commented Jun 29, 2020

neolit123 commented Jun 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn commented Jun 30, 2020

tedyu commented Jun 30, 2020

tnqn commented Jul 8, 2020

tnqn commented Jul 8, 2020

tnqn commented Jul 10, 2020

SergeyKanzhelev commented Jul 15, 2020

SergeyKanzhelev commented Aug 16, 2020

tnqn commented Sep 2, 2020

dims commented Sep 3, 2020

alex-vmw commented Sep 3, 2020

SergeyKanzhelev commented Sep 3, 2020

SergeyKanzhelev commented Sep 3, 2020

tnqn commented Sep 11, 2020

alex-vmw commented Sep 21, 2020

cpanato commented Oct 6, 2020

tnqn commented Oct 13, 2020

tnqn commented Jun 29, 2020 •

edited

Loading