Merge pull request kubernetes#91342 from mgugino-upstream-stage/evict…

…-deleted-ok Eviction: ignore PDBs if pods with DeletionTimestamp
answer1991 · Jul 9, 2020 · dd649bb · dd649bb
2 parents 82baa26 + dd49915
commit dd649bb
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 2 deletions.
diff --git a/pkg/registry/core/pod/storage/eviction.go b/pkg/registry/core/pod/storage/eviction.go
@@ -247,15 +247,16 @@ 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
 }
 
 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
@@ -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