Merge pull request #39150 from kargakis/ignore-deleted-pods-in-rolling-updater

Automatic merge from submit-queue

kubectl: ignore deleted pods in the rolling updater

Fixes https://github.com/kubernetes/kubernetes/issues/37955

The Deployment controller already ignores deleted pods or more precisely the ReplicaSet controller ignores deleted pods ([pod classification](a30b6e2d16/pkg/controller/replicaset/replica_set.go (L600)) ignores [deleted pods](a30b6e2d16/pkg/controller/controller_ref_manager.go (L63))) and the Deployment controller reuses what's in the ReplicaSet status.

@kubernetes/sig-apps-misc @kubernetes/sig-cli-misc

pkg/kubectl/rolling_updater.go

@@ -425,6 +425,10 @@ func (r *RollingUpdater) readyPods(oldRc, newRc *api.ReplicationController, minR
 			if err := v1.Convert_api_Pod_To_v1_Pod(&pod, v1Pod, nil); err != nil {
 				return 0, 0, err
 			}
+			// Do not count deleted pods as ready
+			if v1Pod.DeletionTimestamp != nil {
+				continue
+			}
 			if !deploymentutil.IsPodAvailable(v1Pod, minReadySeconds, r.nowFn().Time) {
 				continue
 			}

pkg/kubectl/rolling_updater_test.go

@@ -1651,6 +1651,10 @@ func TestRollingUpdater_readyPods(t *testing.T) {
 		// pods owned by the rcs; indicate whether they're ready
 		oldPods []bool
 		newPods []bool
+		// deletions - should be less then the size of the respective slice above
+		// eg. len(oldPods) > oldPodDeletions && len(newPods) > newPodDeletions
+		oldPodDeletions int
+		newPodDeletions int
 		// specify additional time to wait for deployment to wait on top of the
 		// pod ready time
 		minReadySeconds int32
@@ -1728,6 +1732,18 @@ func TestRollingUpdater_readyPods(t *testing.T) {
 			nowFn:           func() metav1.Time { return metav1.Time{Time: now.Add(time.Duration(6 * time.Second))} },
 			podReadyTimeFn:  func() metav1.Time { return now },
 		},
+		{
+			oldRc:    oldRc(4, 4),
+			newRc:    newRc(4, 4),
+			oldReady: 2,
+			newReady: 0,
+			oldPods: []bool{
+				// All old pods are ready
+				true, true, true, true,
+			},
+			// Two of them have been marked for deletion though
+			oldPodDeletions: 2,
+		},
 	}
 
 	for i, test := range tests {
@@ -1741,10 +1757,22 @@ func TestRollingUpdater_readyPods(t *testing.T) {
 		// Populate the fake client with pods associated with their owners.
 		pods := []runtime.Object{}
 		for _, ready := range test.oldPods {
-			pods = append(pods, mkpod(test.oldRc, ready, test.podReadyTimeFn()))
+			pod := mkpod(test.oldRc, ready, test.podReadyTimeFn())
+			if test.oldPodDeletions > 0 {
+				now := metav1.Now()
+				pod.DeletionTimestamp = &now
+				test.oldPodDeletions--
+			}
+			pods = append(pods, pod)
 		}
 		for _, ready := range test.newPods {
-			pods = append(pods, mkpod(test.newRc, ready, test.podReadyTimeFn()))
+			pod := mkpod(test.newRc, ready, test.podReadyTimeFn())
+			if test.newPodDeletions > 0 {
+				now := metav1.Now()
+				pod.DeletionTimestamp = &now
+				test.newPodDeletions--
+			}
+			pods = append(pods, pod)
 		}
 		client := fake.NewSimpleClientset(pods...)