From d014398e6819ec412396057da1cb75d765f5b6aa Mon Sep 17 00:00:00 2001 From: Keisuke Ishigami Date: Mon, 21 Jul 2025 17:08:45 +0900 Subject: [PATCH] Resolve confusing use of TooManyRequests error for eviction modify test "the error includes the reason when the condition.Status is False" --- pkg/registry/core/pod/storage/eviction.go | 21 ++++++- .../core/pod/storage/eviction_test.go | 62 ++++++++++++++++++- 2 files changed, 80 insertions(+), 3 deletions(-) diff --git a/pkg/registry/core/pod/storage/eviction.go b/pkg/registry/core/pod/storage/eviction.go index f088cf37de365..c1819b5b8d387 100644 --- a/pkg/registry/core/pod/storage/eviction.go +++ b/pkg/registry/core/pod/storage/eviction.go @@ -434,7 +434,26 @@ func (r *EvictionREST) checkAndDecrement(namespace string, podName string, pdb p } if pdb.Status.DisruptionsAllowed == 0 { err := errors.NewTooManyRequests("Cannot evict pod as it would violate the pod's disruption budget.", 0) - err.ErrStatus.Details.Causes = append(err.ErrStatus.Details.Causes, metav1.StatusCause{Type: policyv1.DisruptionBudgetCause, Message: fmt.Sprintf("The disruption budget %s needs %d healthy pods and has %d currently", pdb.Name, pdb.Status.DesiredHealthy, pdb.Status.CurrentHealthy)}) + condition := meta.FindStatusCondition(pdb.Status.Conditions, policyv1.DisruptionAllowedCondition) + var msg string + switch { + // check whether sync is failed first because DesiredHealthy and CurrentHealthy are not trustworthy when the sync is failed + case condition != nil && condition.Status == metav1.ConditionFalse && len(condition.Message) > 0 && condition.Reason == policyv1.SyncFailedReason: + msg = fmt.Sprintf("The disruption budget %s does not allow evicting pods currently because it failed sync: %s", pdb.Name, condition.Message) + case pdb.Status.CurrentHealthy <= pdb.Status.DesiredHealthy: + msg = fmt.Sprintf("The disruption budget %s needs %d healthy pods and has %d currently", pdb.Name, pdb.Status.DesiredHealthy, pdb.Status.CurrentHealthy) + case condition != nil && condition.Status == metav1.ConditionFalse && len(condition.Message) > 0: + msg = fmt.Sprintf("The disruption budget %s does not allow evicting pods currently (%s): %s", pdb.Name, condition.Reason, condition.Message) + default: + msg = fmt.Sprintf("The disruption budget %s does not allow evicting pods currently", pdb.Name) + } + err.ErrStatus.Details.Causes = append( + err.ErrStatus.Details.Causes, + metav1.StatusCause{ + Type: policyv1.DisruptionBudgetCause, + Message: msg, + }, + ) return err } diff --git a/pkg/registry/core/pod/storage/eviction_test.go b/pkg/registry/core/pod/storage/eviction_test.go index a9d398ff7cd22..4cbdcfede7905 100644 --- a/pkg/registry/core/pod/storage/eviction_test.go +++ b/pkg/registry/core/pod/storage/eviction_test.go @@ -45,7 +45,7 @@ import ( "k8s.io/utils/ptr" ) -func TestEviction(t *testing.T) { +func TestEvictionWithETCD(t *testing.T) { testcases := []struct { name string pdbs []runtime.Object @@ -248,7 +248,7 @@ func TestEviction(t *testing.T) { } } -func TestEvictionIgnorePDB(t *testing.T) { +func TestEviction(t *testing.T) { testcases := []struct { name string pdbs []runtime.Object @@ -521,6 +521,64 @@ func TestEvictionIgnorePDB(t *testing.T) { Status: api.ConditionTrue, }, }, + { + name: "the error is about sync failure when the condition reason is SyncFailedReason", + pdbs: []runtime.Object{&policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, + Spec: policyv1.PodDisruptionBudgetSpec{Selector: &metav1.LabelSelector{}}, + Status: policyv1.PodDisruptionBudgetStatus{ + DisruptionsAllowed: 0, + Conditions: []metav1.Condition{ + { + Type: policyv1.DisruptionAllowedCondition, + Status: metav1.ConditionFalse, + Reason: policyv1.SyncFailedReason, + Message: "hoge", + }, + }, + }, + }}, + eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t12", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, + expectError: "Cannot evict pod as it would violate the pod's disruption budget.: TooManyRequests: The disruption budget foo does not allow evicting pods currently because it failed sync: hoge", + podName: "t12", + expectedDeleteCount: 0, + podTerminating: false, + podPhase: api.PodRunning, + prc: &api.PodCondition{ + Type: api.PodReady, + Status: api.ConditionTrue, + }, + }, + { + name: "the error includes the reason when the condition.Status is False", + pdbs: []runtime.Object{&policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, + Spec: policyv1.PodDisruptionBudgetSpec{Selector: &metav1.LabelSelector{}}, + Status: policyv1.PodDisruptionBudgetStatus{ + DisruptionsAllowed: 0, + CurrentHealthy: 4, + DesiredHealthy: 3, + Conditions: []metav1.Condition{ + { + Type: policyv1.DisruptionAllowedCondition, + Status: metav1.ConditionFalse, + Reason: "bar-reason", + Message: "hoge", + }, + }, + }, + }}, + eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "t12", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, + expectError: "Cannot evict pod as it would violate the pod's disruption budget.: TooManyRequests: The disruption budget foo does not allow evicting pods currently (bar-reason): hoge", + podName: "t12", + expectedDeleteCount: 0, + podTerminating: false, + podPhase: api.PodRunning, + prc: &api.PodCondition{ + Type: api.PodReady, + Status: api.ConditionTrue, + }, + }, } for _, unhealthyPodEvictionPolicy := range []*policyv1.UnhealthyPodEvictionPolicyType{unhealthyPolicyPtr(policyv1.AlwaysAllow), nil, unhealthyPolicyPtr(policyv1.IfHealthyBudget)} {