Resolve confusing use of TooManyRequests error for eviction #133097

kei01234kei · 2025-07-21T08:21:52Z

What type of PR is this?

/kind bug

Does this PR introduce a user-facing change?

Added the correct error when eviction is blocked due to the failSafe mechanism of the DisruptionController.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2025-07-21T08:22:01Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot · 2025-07-21T08:22:02Z

Hi @kei01234kei. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

kei01234kei · 2025-07-21T08:23:14Z

/sig apps

kei01234kei · 2025-07-22T09:52:57Z

Let me also assign you as reviewers because I saw you in the issue discussion.
/cc @tallclair @liggitt @atiratree

janetkuo · 2025-07-30T16:45:26Z

/ok-to-test

liggitt · 2025-07-30T19:19:55Z

pkg/registry/core/pod/storage/eviction.go

 		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)
+		if condition.Status == metav1.ConditionFalse {


condition.Status will panic if condition is nil, which FindStatusCondition will return if the condition is not present

I'd suggest customizing how we construct the message based on CurrentHealthy / DesiredHealthy / presence of a SyncFailedReason or other False condition, with a sensible generic fallback to avoid being confusing. I'd also suggest keeping the existing message as-is if CurrentHealthy <= DesiredHealthy since that is not confusing.

condition := meta.FindStatusCondition(pdb.Status.Conditions, policyv1.DisruptionAllowedCondition) var msg string switch { 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 && condition.Reason == policy.SyncFailedReason: msg = fmt.Sprintf("The disruption budget %s does not allow evicting pods currently because it failed sync: %v", pdb.Name, condition.Message) case condition != nil && condition.Status == metav1.ConditionFalse && len(condition.Message) > 0: msg = fmt.Sprintf("The disruption budget %s does not allow evicting pods currently: %v", pdb.Name, 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})

+1 for the above flow

Perhaps, this part could also output conditions without a condition.Message and print also the condition.Reason

switch { ... case condition != nil && condition.Status == metav1.ConditionFalse && len(condition.Message) > 0: msg = fmt.Sprintf("The disruption budget %s does not allow evicting pods currently: %v", pdb.Name, condition.Message) ...

@liggitt
Thank you for the good advice. I modified the code following your advice.

@atiratree
I changed the code to output the condition.Reason too, in condition != nil && condition.Status == metav1.ConditionFalse && len(condition.Message) > 0 case.

Resolve confusing use of TooManyRequests error for eviction

atiratree

@kei01234kei It would be great if we could test the new errors. I think we can add a new cases to this unit test

kubernetes/pkg/registry/core/pod/storage/eviction_test.go

Line 251 in ee065df

func TestEvictionIgnorePDB(t *testing.T) {

Btw, the name TestEvictionIgnorePDB does not describe all of its test cases well anymore. Because not all the cases ignore PDBs. The easiest way to fix this is as follows, IMO:

s/TestEviction/TestEvictionWithETCD
s/TestEvictionIgnorePDB/TestEviction

atiratree · 2025-08-07T19:41:02Z

pkg/registry/core/pod/storage/eviction.go

 		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)
+		if condition.Status == metav1.ConditionFalse {


+1 for the above flow

Perhaps, this part could also output conditions without a condition.Message and print also the condition.Reason

switch { ... case condition != nil && condition.Status == metav1.ConditionFalse && len(condition.Message) > 0: msg = fmt.Sprintf("The disruption budget %s does not allow evicting pods currently: %v", pdb.Name, condition.Message) ...

k8s-triage-robot · 2025-09-04T00:56:35Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

kei01234kei · 2025-09-04T01:17:55Z

/hold

kei01234kei · 2025-09-04T04:46:35Z

/label tide/merge-method-squash
/unhold

kei01234kei · 2025-09-04T05:58:59Z

unit test looks related

Yes. I modified the test.
modify test "the error includes the reason when the condition.Status …

liggitt · 2025-09-04T13:35:10Z

thanks, please squash to a single commit

modify test "the error includes the reason when the condition.Status is False"

kei01234kei · 2025-09-04T14:44:45Z

thanks, please squash to a single commit

Done.
(Would it be better for me to squash the commits even if the tide/merge-method-squash label is added?)

kei01234kei · 2025-09-04T15:08:49Z

/retest

liggitt · 2025-09-04T16:48:51Z

/lgtm
/approve

k8s-ci-robot · 2025-09-04T16:48:58Z

LGTM label has been added.

Git tree hash: 7c5e5d13cb5cebc0e93e7dba9e956eaec5e9b5a9

k8s-ci-robot · 2025-09-04T16:49:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kei01234kei, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/registry/core/pod/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kei01234kei · 2025-09-04T18:06:22Z

/retest

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 21, 2025

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jul 21, 2025

k8s-ci-robot requested review from derekwaynecarr and feiskyer July 21, 2025 08:22

k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 21, 2025

github-project-automation bot added this to SIG Node: code and documentation PRs Jul 21, 2025

github-project-automation bot moved this to Triage in SIG Node: code and documentation PRs Jul 21, 2025

k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Jul 21, 2025

github-project-automation bot added this to SIG Apps Jul 21, 2025

github-project-automation bot moved this to Needs Triage in SIG Apps Jul 21, 2025

k8s-ci-robot requested review from atiratree, liggitt and tallclair July 22, 2025 09:52

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 30, 2025

liggitt reviewed Aug 6, 2025

View reviewed changes

atiratree reviewed Aug 7, 2025

View reviewed changes

kei01234kei force-pushed the resolve_confusiong_use_of_toomanerequests_error_for_eviction branch from ef05923 to ef94aed Compare August 30, 2025 06:33

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 4, 2025

k8s-ci-robot requested a review from liggitt September 4, 2025 04:45

k8s-ci-robot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 4, 2025

kei01234kei force-pushed the resolve_confusiong_use_of_toomanerequests_error_for_eviction branch from ac72fae to aff1940 Compare September 4, 2025 05:07

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 4, 2025

kei01234kei force-pushed the resolve_confusiong_use_of_toomanerequests_error_for_eviction branch from e9085b3 to aff1940 Compare September 4, 2025 07:55

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 4, 2025

Resolve confusing use of TooManyRequests error for eviction

d014398

modify test "the error includes the reason when the condition.Status is False"

kei01234kei force-pushed the resolve_confusiong_use_of_toomanerequests_error_for_eviction branch from aff1940 to d014398 Compare September 4, 2025 14:43

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 4, 2025

k8s-ci-robot merged commit ddb015f into kubernetes:master Sep 4, 2025
13 checks passed

k8s-ci-robot added this to the v1.35 milestone Sep 4, 2025

github-project-automation bot moved this from Triage to Done in SIG Node: code and documentation PRs Sep 4, 2025

github-project-automation bot moved this from Needs Triage to Done in SIG Apps Sep 4, 2025

sbueringer mentioned this pull request Aug 28, 2025

Tasks to bump to Kubernetes v1.35 kubernetes-sigs/cluster-api#12700

Open

23 tasks

Resolve confusing use of TooManyRequests error for eviction #133097

Resolve confusing use of TooManyRequests error for eviction #133097

Uh oh!

Conversation

kei01234kei commented Jul 21, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR is related to:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented Jul 21, 2025

Uh oh!

k8s-ci-robot commented Jul 21, 2025

Uh oh!

kei01234kei commented Jul 21, 2025

Uh oh!

kei01234kei commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janetkuo commented Jul 30, 2025

Uh oh!

liggitt Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

atiratree Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

kei01234kei Aug 30, 2025

Choose a reason for hiding this comment

Uh oh!

atiratree left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atiratree Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

k8s-triage-robot commented Sep 4, 2025

Uh oh!

kei01234kei commented Sep 4, 2025

Uh oh!

kei01234kei commented Sep 4, 2025

Uh oh!

kei01234kei commented Sep 4, 2025

Uh oh!

liggitt commented Sep 4, 2025

Uh oh!

kei01234kei commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kei01234kei commented Sep 4, 2025

Uh oh!

liggitt commented Sep 4, 2025

Uh oh!

k8s-ci-robot commented Sep 4, 2025

Uh oh!

k8s-ci-robot commented Sep 4, 2025

Uh oh!

kei01234kei commented Sep 4, 2025

Uh oh!

Uh oh!

Uh oh!

kei01234kei commented Jul 22, 2025 •

edited

Loading

atiratree left a comment •

edited

Loading

kei01234kei commented Sep 4, 2025 •

edited

Loading