diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/drain/drain.go b/staging/src/k8s.io/kubectl/pkg/cmd/drain/drain.go index a4aa93fdb5c..865191cd2a7 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/drain/drain.go +++ b/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 diff --git a/staging/src/k8s.io/kubectl/pkg/drain/drain.go b/staging/src/k8s.io/kubectl/pkg/drain/drain.go index 348fd703fe7..b280d20ba96 100644 --- a/staging/src/k8s.io/kubectl/pkg/drain/drain.go +++ b/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, diff --git a/staging/src/k8s.io/kubectl/pkg/drain/drain_test.go b/staging/src/k8s.io/kubectl/pkg/drain/drain_test.go index 6acf2489ae3..373241054b6 100644 --- a/staging/src/k8s.io/kubectl/pkg/drain/drain_test.go +++ b/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) + } + }) + } +}