From dd49915c55bdb18f58d589549eea8e9465200fd1 Mon Sep 17 00:00:00 2001
From: Michael Gugino <mgugino@redhat.com>
Date: Tue, 2 Jun 2020 01:06:45 -0400
Subject: [PATCH] Eviction: ignore PDBs if pods with DeletionTimestamp

When using the eviction API, if a pod already has
a non-zero DeletionTimestamp, we don't need to check
PDBs as it has already been marked for deletion.
---
 pkg/registry/core/pod/storage/eviction.go     |  5 ++--
 .../core/pod/storage/eviction_test.go         | 23 +++++++++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/pkg/registry/core/pod/storage/eviction.go b/pkg/registry/core/pod/storage/eviction.go
index 7096e18c97d..7ed0875af02 100644
--- a/pkg/registry/core/pod/storage/eviction.go
+++ b/pkg/registry/core/pod/storage/eviction.go
@@ -247,7 +247,8 @@ func (r *EvictionREST) Create(ctx context.Context, name string, obj runtime.Obje
 // canIgnorePDB returns true for pod conditions that allow the pod to be deleted
 // without checking PDBs.
 func canIgnorePDB(pod *api.Pod) bool {
-	if pod.Status.Phase == api.PodSucceeded || pod.Status.Phase == api.PodFailed || pod.Status.Phase == api.PodPending {
+	if pod.Status.Phase == api.PodSucceeded || pod.Status.Phase == api.PodFailed ||
+		pod.Status.Phase == api.PodPending || !pod.ObjectMeta.DeletionTimestamp.IsZero() {
 		return true
 	}
 	return false
@@ -255,7 +256,7 @@ func canIgnorePDB(pod *api.Pod) bool {
 
 func shouldEnforceResourceVersion(pod *api.Pod) bool {
 	// We don't need to enforce ResourceVersion for terminal pods
-	if pod.Status.Phase == api.PodSucceeded || pod.Status.Phase == api.PodFailed {
+	if pod.Status.Phase == api.PodSucceeded || pod.Status.Phase == api.PodFailed || !pod.ObjectMeta.DeletionTimestamp.IsZero() {
 		return false
 	}
 	// Return true for all other pods to ensure we don't race against a pod becoming
diff --git a/pkg/registry/core/pod/storage/eviction_test.go b/pkg/registry/core/pod/storage/eviction_test.go
index d21d527179d..5ea203b3bc3 100644
--- a/pkg/registry/core/pod/storage/eviction_test.go
+++ b/pkg/registry/core/pod/storage/eviction_test.go
@@ -218,6 +218,7 @@ func TestEvictionIngorePDB(t *testing.T) {
 		podPhase            api.PodPhase
 		podName             string
 		expectedDeleteCount int
+		podTerminating      bool
 	}{
 		{
 			name: "pdbs No disruptions allowed, pod pending, first delete conflict, pod still pending, pod deleted successfully",
@@ -287,6 +288,19 @@ func TestEvictionIngorePDB(t *testing.T) {
 			podName:             "t5",
 			expectedDeleteCount: 1,
 		},
+		{
+			name: "matching pdbs with no disruptions allowed, pod terminating",
+			pdbs: []runtime.Object{&policyv1beta1.PodDisruptionBudget{
+				ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"},
+				Spec:       policyv1beta1.PodDisruptionBudgetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"a": "true"}}},
+				Status:     policyv1beta1.PodDisruptionBudgetStatus{DisruptionsAllowed: 0},
+			}},
+			eviction:            &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t6", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(300)},
+			expectError:         false,
+			podName:             "t6",
+			expectedDeleteCount: 1,
+			podTerminating:      true,
+		},
 	}
 
 	for _, tc := range testcases {
@@ -304,6 +318,11 @@ func TestEvictionIngorePDB(t *testing.T) {
 				pod.Status.Phase = tc.podPhase
 			}
 
+			if tc.podTerminating {
+				currentTime := metav1.Now()
+				pod.ObjectMeta.DeletionTimestamp = &currentTime
+			}
+
 			client := fake.NewSimpleClientset(tc.pdbs...)
 			evictionRest := newEvictionStorage(ms, client.PolicyV1beta1())
 
@@ -397,6 +416,10 @@ func (ms *mockStore) mutatorDeleteFunc(count int, options *metav1.DeleteOptions)
 		// Always return error for this pod
 		return nil, false, apierrors.NewConflict(resource("tests"), "2", errors.New("message"))
 	}
+	if ms.pod.Name == "t6" {
+		// This pod has a deletionTimestamp and should not raise conflict on delete
+		return nil, true, nil
+	}
 	if count == 1 {
 		// This is a hack to ensure that some test pods don't change phase
 		// but do change resource version