Fix Deployment with Recreate strategy not to wait on Pods in terminal phase #60301

tnozicka · 2018-02-23T13:19:07Z

What this PR does / why we need it:
Fixes Deployment with Recreate strategy not to wait on Pods in terminal phase. It can happen after eviction or failing to match selector and RS leaves such pod around right now. (Hopefully RC gets fixed separately.)

Which issue(s) this PR fixes *:
Fixes #60162

Special notes for your reviewer:

Release note:

Fixes a case when Deployment with recreate strategy could get stuck on old failed Pod.

/cc @janetkuo

tnozicka · 2018-02-23T13:25:26Z

Per @jsafrane: "volumes are unmounted for failed or succeeded pods" - so we should be safe in that regard

tnozicka · 2018-02-23T17:33:17Z

/kind bug
/sig apps
/priority P1

/cc @kow3ns @kubernetes/sig-apps-bugs

tnozicka · 2018-02-23T17:58:00Z

/test pull-kubernetes-unit

tnozicka · 2018-02-23T18:55:27Z

tests are green :)

janetkuo · 2018-02-23T19:06:29Z

pkg/controller/deployment/recreate_test.go

+		},
+		{
+			name: "old RSs with zero status replicas but pods in terminal state are present",
+			oldRSs: []*extensions.ReplicaSet{


Can use the same format as above test cases: []*extensions.ReplicaSet{newRSWithStatus("rs-blabla", 0, 0, nil)},

I prefer to state the tested value more verbosely in contrast to argument to a function that let's you look somewhere else for what the value means, but I have fixed it for consistency and your request.

janetkuo · 2018-02-23T19:11:07Z

pkg/controller/deployment/recreate.go

-			return true
+		for _, pod := range podList.Items {
+			switch pod.Status.Phase {
+			case v1.PodFailed, v1.PodSucceeded:


It should be impossible for RS to have succeeded pods, but there's no harm to add this check.

It should be impossible, but a prefer to handle even such case explicitly.

janetkuo · 2018-02-23T19:12:25Z

pkg/controller/deployment/recreate.go

+			case v1.PodFailed, v1.PodSucceeded:
+				continue
+			case v1.PodUnknown:
+				glog.Warning("Encountered Pod %s/%s in unknown state", pod.Namespace, pod.Name)


IMO I don't think we need to check unknown pods and print a warning here. Just return true in default case should be enough.

Agreed. That doesn't conform to the logging standards for controller_manager and could get spammy.

you are right - fixed, thx

janetkuo · 2018-02-23T19:18:07Z

Because ReplicaSets (and Deployments) only allow restartPolicy:Always, the only case their pods would end up in a terminated state is when 1) kubelet evicts a pod (causing the pod in a Failed state), or 2) kubelet rejects a pod when scheduling (also Failed pod). I confirmed with node team and it should be safe to assume that containers have terminated and resources have been released in both cases.

janetkuo · 2018-02-23T19:21:34Z

We should cherrypick this to 1.9.

kow3ns · 2018-02-24T01:16:45Z

@tnozicka @janetkuo I'm assuming we want to target 1.10.0 with this and not cherry pick it back into 1.10.1 and 1.9.x ?

k8s-cherrypick-bot · 2018-02-24T01:18:24Z

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

… phase

…rminal state present

tnozicka · 2018-02-24T13:39:37Z

Comments addressed.
I'll cherrypick it back to 1.9 once it merges.

janetkuo · 2018-02-26T21:05:54Z

/lgtm

k8s-ci-robot · 2018-02-26T21:06:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: janetkuo, tnozicka

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/controller/deployment/OWNERS~~ [janetkuo]

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

janetkuo · 2018-02-26T21:07:55Z

I'll cherrypick it back to 1.9 once it merges.

@tnozicka Thanks! Would you cherrypick it to 1.8 also?

k8s-github-robot · 2018-02-26T22:11:14Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-02-26T23:00:49Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@janetkuo

Automatic merge from submit-queue. [1.8] - Fix Deployment with Recreate strategy not to wait on Pods in terminal phase Backport of #60301 ```release-note Fixes a case when Deployment with recreate strategy could get stuck on old failed Pod. ``` /assign @janetkuo @kow3ns

@janetkuo

Automatic merge from submit-queue. [1.9] - Fix Deployment with Recreate strategy not to wait on Pods in terminal phase Backport of #60301 ```release-note Fixes a case when Deployment with recreate strategy could get stuck on old failed Pod. ``` /assign @janetkuo @kow3ns

k8s-ci-robot requested a review from janetkuo February 23, 2018 13:19

tnozicka force-pushed the fix-recreate branch from eef75a3 to 6428b9f Compare February 23, 2018 14:56

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 23, 2018

tnozicka changed the title ~~[WIP] - Fix Deployment with Recreate strategy not to wait on Pods in terminal phase~~ Fix Deployment with Recreate strategy not to wait on Pods in terminal phase Feb 23, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 23, 2018

tnozicka force-pushed the fix-recreate branch from aff1792 to f467a66 Compare February 23, 2018 17:26

k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. kind/bug Categorizes issue or PR as related to a bug. priority/P1 labels Feb 23, 2018

tnozicka force-pushed the fix-recreate branch from f467a66 to ac1c98c Compare February 23, 2018 18:03

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 23, 2018

tnozicka mentioned this pull request Feb 23, 2018

Deployment controller fails to create new RS when old RS has an evicted pod #60162

Closed

janetkuo reviewed Feb 23, 2018

View reviewed changes

janetkuo added this to the v1.9 milestone Feb 23, 2018

janetkuo added the cherrypick-candidate label Feb 23, 2018

k8s-github-robot added the milestone/incomplete-labels label Feb 23, 2018

kow3ns modified the milestones: v1.9, v1.10 Feb 24, 2018

k8s-cherrypick-bot removed the cherrypick-candidate label Feb 24, 2018

tnozicka added 2 commits February 24, 2018 14:29

Fix Deployment with Recreate strategy not to wait on Pods in terminal…

952e6c6

… phase

Add tests for Deployments Recreate strategy when there are pods in te…

ffdd3b5

…rminal state present

tnozicka force-pushed the fix-recreate branch from ac1c98c to ffdd3b5 Compare February 24, 2018 13:31

kow3ns added status/approved-for-milestone and removed milestone/needs-approval labels Feb 26, 2018

janetkuo approved these changes Feb 26, 2018

View reviewed changes

k8s-ci-robot assigned janetkuo Feb 26, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2018

janetkuo added the queue/fix label Feb 26, 2018

k8s-github-robot merged commit e491689 into kubernetes:master Feb 26, 2018

tnozicka deleted the fix-recreate branch February 27, 2018 10:05

This was referenced Feb 27, 2018

[1.9] - Fix Deployment with Recreate strategy not to wait on Pods in terminal phase #60493

Merged

[1.8] - Fix Deployment with Recreate strategy not to wait on Pods in terminal phase #60496

Merged

Shuanglu mentioned this pull request Dec 15, 2021

Whether pod deletion should enqueue the deployment again when there is only evicted pod left #106410

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Deployment with Recreate strategy not to wait on Pods in terminal phase #60301

Fix Deployment with Recreate strategy not to wait on Pods in terminal phase #60301

tnozicka commented Feb 23, 2018 •

edited

Loading

tnozicka commented Feb 23, 2018 •

edited

Loading

tnozicka commented Feb 23, 2018

tnozicka commented Feb 23, 2018

tnozicka commented Feb 23, 2018

janetkuo Feb 23, 2018

tnozicka Feb 24, 2018

janetkuo Feb 23, 2018

tnozicka Feb 24, 2018

janetkuo Feb 23, 2018

kow3ns Feb 24, 2018

tnozicka Feb 24, 2018

janetkuo commented Feb 23, 2018 •

edited

Loading

janetkuo commented Feb 23, 2018

kow3ns commented Feb 24, 2018

k8s-cherrypick-bot commented Feb 24, 2018

tnozicka commented Feb 24, 2018

janetkuo commented Feb 26, 2018

k8s-ci-robot commented Feb 26, 2018

janetkuo commented Feb 26, 2018

k8s-github-robot commented Feb 26, 2018

k8s-github-robot commented Feb 26, 2018

Fix Deployment with Recreate strategy not to wait on Pods in terminal phase #60301

Fix Deployment with Recreate strategy not to wait on Pods in terminal phase #60301

Conversation

tnozicka commented Feb 23, 2018 • edited Loading

tnozicka commented Feb 23, 2018 • edited Loading

tnozicka commented Feb 23, 2018

tnozicka commented Feb 23, 2018

tnozicka commented Feb 23, 2018

janetkuo Feb 23, 2018

Choose a reason for hiding this comment

tnozicka Feb 24, 2018

Choose a reason for hiding this comment

janetkuo Feb 23, 2018

Choose a reason for hiding this comment

tnozicka Feb 24, 2018

Choose a reason for hiding this comment

janetkuo Feb 23, 2018

Choose a reason for hiding this comment

kow3ns Feb 24, 2018

Choose a reason for hiding this comment

tnozicka Feb 24, 2018

Choose a reason for hiding this comment

janetkuo commented Feb 23, 2018 • edited Loading

janetkuo commented Feb 23, 2018

kow3ns commented Feb 24, 2018

k8s-cherrypick-bot commented Feb 24, 2018

tnozicka commented Feb 24, 2018

janetkuo commented Feb 26, 2018

k8s-ci-robot commented Feb 26, 2018

janetkuo commented Feb 26, 2018

k8s-github-robot commented Feb 26, 2018

k8s-github-robot commented Feb 26, 2018

tnozicka commented Feb 23, 2018 •

edited

Loading

tnozicka commented Feb 23, 2018 •

edited

Loading

janetkuo commented Feb 23, 2018 •

edited

Loading