fix using stale pod when evict failed and retry (#133461)

* fix using stale pod when evict failed and retry

* simplify pod refresh process

* use activePod at getPodFn

* fix lint check

* add ut

* introduce EvictErrorRetryDelay

staging/src/k8s.io/kubectl/pkg/cmd/drain/drain.go

@@ -19,6 +19,7 @@ package drain
 import (
 	"errors"
 	"fmt"
+	"time"
 
 	"github.com/spf13/cobra"
 
@@ -151,10 +152,11 @@ func NewDrainCmdOptions(f cmdutil.Factory, ioStreams genericiooptions.IOStreams)
 		PrintFlags: genericclioptions.NewPrintFlags("drained").WithTypeSetter(scheme.Scheme),
 		IOStreams:  ioStreams,
 		drainer: &drain.Helper{
-			GracePeriodSeconds: -1,
-			Out:                ioStreams.Out,
-			ErrOut:             ioStreams.ErrOut,
-			ChunkSize:          cmdutil.DefaultChunkSize,
+			GracePeriodSeconds:   -1,
+			EvictErrorRetryDelay: 5 * time.Second,
+			Out:                  ioStreams.Out,
+			ErrOut:               ioStreams.ErrOut,
+			ChunkSize:            cmdutil.DefaultChunkSize,
 		},
 	}
 	o.drainer.OnPodDeletionOrEvictionFinished = o.onPodDeletionOrEvictionFinished

staging/src/k8s.io/kubectl/pkg/drain/drain.go

@@ -74,6 +74,9 @@ type Helper struct {
 	// won't drain otherwise
 	SkipWaitForDeleteTimeoutSeconds int
 
+	// EvictErrorRetryDelay is used to control the retry delay after a pod eviction error
+	EvictErrorRetryDelay time.Duration
+
 	// AdditionalFilters are applied sequentially after base drain filters to
 	// exclude pods using custom logic.  Any filter that returns PodDeleteStatus
 	// with Delete == false will immediately stop execution of further filters.
@@ -278,37 +281,27 @@ func (d *Helper) evictPods(pods []corev1.Pod, evictionGroupVersion schema.GroupV
 	defer cancel()
 	for _, pod := range pods {
 		go func(pod corev1.Pod, returnCh chan error) {
-			refreshPod := false
+			activePod := pod
 			for {
 				switch d.DryRunStrategy {
 				case cmdutil.DryRunServer:
-					fmt.Fprintf(d.Out, "evicting pod %s/%s (server dry run)\n", pod.Namespace, pod.Name)
+					//nolint:errcheck
+					fmt.Fprintf(d.Out, "evicting pod %s/%s (server dry run)\n", activePod.Namespace, activePod.Name)
 				default:
 					if d.OnPodDeletionOrEvictionStarted != nil {
-						d.OnPodDeletionOrEvictionStarted(&pod, true)
+						d.OnPodDeletionOrEvictionStarted(&activePod, true)
 					}
-					fmt.Fprintf(d.Out, "evicting pod %s/%s\n", pod.Namespace, pod.Name)
+					//nolint:errcheck
+					fmt.Fprintf(d.Out, "evicting pod %s/%s\n", activePod.Namespace, activePod.Name)
 				}
 				select {
 				case <-ctx.Done():
 					// return here or we'll leak a goroutine.
-					returnCh <- fmt.Errorf("error when evicting pods/%q -n %q: global timeout reached: %v", pod.Name, pod.Namespace, globalTimeout)
+					returnCh <- fmt.Errorf("error when evicting pods/%q -n %q: global timeout reached: %v", activePod.Name, activePod.Namespace, globalTimeout)
 					return
 				default:
 				}
 
-				// Create a temporary pod so we don't mutate the pod in the loop.
-				activePod := pod
-				if refreshPod {
-					freshPod, err := getPodFn(pod.Namespace, pod.Name)
-					// We ignore errors and let eviction sort it out with
-					// the original pod.
-					if err == nil {
-						activePod = *freshPod
-					}
-					refreshPod = false
-				}
-
 				err := d.EvictPod(activePod, evictionGroupVersion)
 				if err == nil {
 					break
@@ -316,8 +309,9 @@ func (d *Helper) evictPods(pods []corev1.Pod, evictionGroupVersion schema.GroupV
 					returnCh <- nil
 					return
 				} else if apierrors.IsTooManyRequests(err) {
-					fmt.Fprintf(d.ErrOut, "error when evicting pods/%q -n %q (will retry after 5s): %v\n", activePod.Name, activePod.Namespace, err)
-					time.Sleep(5 * time.Second)
+					//nolint:errcheck
+					fmt.Fprintf(d.ErrOut, "error when evicting pods/%q -n %q (will retry after %v): %v\n", activePod.Name, activePod.Namespace, d.EvictErrorRetryDelay, err)
+					time.Sleep(d.EvictErrorRetryDelay)
 				} else if !activePod.ObjectMeta.DeletionTimestamp.IsZero() && apierrors.IsForbidden(err) && apierrors.HasStatusCause(err, corev1.NamespaceTerminatingCause) {
 					// an eviction request in a deleting namespace will throw a forbidden error,
 					// if the pod is already marked deleted, we can ignore this error, an eviction
@@ -326,12 +320,19 @@ func (d *Helper) evictPods(pods []corev1.Pod, evictionGroupVersion schema.GroupV
 				} else if apierrors.IsForbidden(err) && apierrors.HasStatusCause(err, corev1.NamespaceTerminatingCause) {
 					// an eviction request in a deleting namespace will throw a forbidden error,
 					// if the pod is not marked deleted, we retry until it is.
-					fmt.Fprintf(d.ErrOut, "error when evicting pod %q from terminating namespace %q (will retry after 5s): %v\n", activePod.Name, activePod.Namespace, err)
-					time.Sleep(5 * time.Second)
+					//nolint:errcheck
+					fmt.Fprintf(d.ErrOut, "error when evicting pod %q from terminating namespace %q (will retry after %v): %v\n", activePod.Name, activePod.Namespace, d.EvictErrorRetryDelay, err)
+					time.Sleep(d.EvictErrorRetryDelay)
 				} else {
 					returnCh <- fmt.Errorf("error when evicting pods/%q -n %q: %v", activePod.Name, activePod.Namespace, err)
 					return
 				}
+
+				freshPod, err := getPodFn(activePod.Namespace, activePod.Name)
+				// we ignore errors and let eviction sort it out with the original pod.
+				if err == nil {
+					activePod = *freshPod
+				}
 			}
 			if d.DryRunStrategy == cmdutil.DryRunServer {
 				returnCh <- nil
@@ -339,7 +340,7 @@ func (d *Helper) evictPods(pods []corev1.Pod, evictionGroupVersion schema.GroupV
 			}
 			params := waitForDeleteParams{
 				ctx:                             ctx,
-				pods:                            []corev1.Pod{pod},
+				pods:                            []corev1.Pod{activePod},
 				interval:                        1 * time.Second,
 				timeout:                         time.Duration(math.MaxInt64),
 				usingEviction:                   true,

staging/src/k8s.io/kubectl/pkg/drain/drain_test.go

@@ -529,3 +529,110 @@ func TestFilterPods(t *testing.T) {
 		})
 	}
 }
+
+func TestEvictDuringNamespaceTerminating(t *testing.T) {
+	testPodUID := types.UID("test-uid")
+	testPodName := "test-pod"
+	testNamespace := "default"
+
+	retryDelay := 5 * time.Millisecond
+	globalTimeout := 2 * retryDelay
+
+	tests := []struct {
+		description string
+		refresh     bool
+		err         error
+	}{
+		{
+			description: "Pod refreshed after NamespaceTerminating error",
+			refresh:     true,
+			err:         nil,
+		},
+		{
+			description: "Pod not refreshed after NamespaceTerminating error",
+			refresh:     false,
+			err:         fmt.Errorf("error when evicting pods/%q -n %q: global timeout reached: %v", testPodName, testNamespace, globalTimeout),
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.description, func(t *testing.T) {
+			var retry bool
+
+			initialPod := &corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      testPodName,
+					Namespace: testNamespace,
+					UID:       testPodUID,
+				},
+			}
+
+			// pod with DeletionTimestamp, indicating deletion in progress
+			deletedPod := &corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:              testPodName,
+					Namespace:         testNamespace,
+					UID:               testPodUID,
+					DeletionTimestamp: &metav1.Time{Time: time.Now()},
+				},
+			}
+
+			evictPods := []corev1.Pod{*initialPod}
+
+			k := fake.NewClientset(initialPod)
+			addEvictionSupport(t, k, "v1")
+
+			// mock eviction to return NamespaceTerminating error
+			k.PrependReactor("create", "pods", func(action ktest.Action) (bool, runtime.Object, error) {
+				if action.GetSubresource() != "eviction" {
+					return false, nil, nil
+				}
+
+				err := apierrors.NewForbidden(
+					schema.GroupResource{Resource: "pods"},
+					testPodName,
+					errors.New("namespace is terminating"),
+				)
+
+				err.ErrStatus.Details.Causes = append(err.ErrStatus.Details.Causes, metav1.StatusCause{
+					Type: corev1.NamespaceTerminatingCause,
+				})
+
+				return true, nil, err
+			})
+
+			k.PrependReactor("get", "pods", func(action ktest.Action) (bool, runtime.Object, error) {
+				if !test.refresh {
+					// for non-refresh test, always return the initial pod
+					return true, initialPod, nil
+				}
+
+				if retry {
+					// second call, pod is deleted
+					return true, nil, apierrors.NewNotFound(schema.GroupResource{Resource: "pods"}, testPodName)
+				}
+
+				// first call, pod is being deleted
+				retry = true
+
+				return true, deletedPod, nil
+			})
+
+			h := &Helper{
+				Client:               k,
+				DisableEviction:      false,
+				Out:                  os.Stdout,
+				ErrOut:               os.Stderr,
+				Timeout:              globalTimeout,
+				EvictErrorRetryDelay: retryDelay,
+			}
+
+			err := h.DeleteOrEvictPods(evictPods)
+			if test.err == nil && err != nil {
+				t.Errorf("expected no error, got: %v", err)
+			} else if test.err != nil && (err == nil || err.Error() != test.err.Error()) {
+				t.Errorf("%s: unexpected eviction; actual %v; expected %v", test.description, err, test.err)
+			}
+		})
+	}
+}