Confusing use of TooManyRequests error for eviction

[theunrepentantgeek]

What happened?

When the Kubernetes API server returns a 429 (throttled), the response may include misleading cause information.

We observed that a pod eviction API call received a response that included both of the following fragments:

"responseStatus": {
  "metadata": {},
  "status": "Failure",
  "reason": "TooManyRequests",
  "code": 429
},

and

"responseObject": {
  "kind": "Status",
  "apiVersion": "v1",
  "metadata": {},
  "status": "Failure",
  "message": "Cannot evict pod as it would violate the pod's disruption budget.",
  "reason": "TooManyRequests",
  "details": {
    "causes": [
      {
        "reason": "DisruptionBudget",
        "message": "The disruption budget [elided] needs 11 healthy pods and has 12 currently"
      }
    ]
  },
  "code": 429
},

Note that the message given above is confusing, as there are more currently healthy pods than the required number.

What did you expect to happen?

When a request is rejected due to throttling (429 HTTP response), either

• no other error information should be included; or
• the included error information should indicate throttling

How can we reproduce it (as minimally and precisely as possible)?

Provoke a 429 HTTP response and inspect the response.

Anything else we need to know?

Issue #88535 seems to be similar - it also describes a 429 response where inclusion of a cause resulted in confusion.

Kubernetes version

Version: 1.20.9

Cloud provider

Azure Kubernetes Service

OS version

# On Linux:
$ cat /etc/os-release
# paste output here
$ uname -a
# paste output here

# On Windows:
C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture
# paste output here

Install tools

Container runtime (CRI) and and version (if applicable)

Related plugins (CNI, CSI, ...) and versions (if applicable)

[theunrepentantgeek]

/sig api-machinery

[liggitt]

/remove-sig api-machinery
/sig apps
for PDB

[theunrepentantgeek]

@liggitt , are you sure?

As far as I've been able to work out, the 429 HTTP response is correct and the PDB error is a red herring.

The PDB error we observed doesn't even make sense, given the number of healthy pods (12) was more than the needed (11):

The disruption budget [elided] needs 11 healthy pods and has 12 currently

(My experience is that I spent a bunch of time digging into possible causes of the PDB error before realizing that I'd been ignoring the HTTP response code - and once I started paying attention to that, everything started to make sense. Of course, you understand this area far better, I just don't want you to fall into the same trap that I did!)

[liggitt]

any message coming back with PDB language in it belongs to sig-apps

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[theunrepentantgeek]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[theunrepentantgeek]

/remove-lifecycle stale

[theunrepentantgeek]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[theunrepentantgeek]

/remove-lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[theunrepentantgeek]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[theunrepentantgeek]

/remove-lifecycle stale

[tallclair]

/triage accepted
/priority important-longterm

It looks like eviction has some creative uses of TooManyRequests errors. Why is this conflict error converted to TooManyRequests?

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

Lines 294 to 305 in 786316f

	// Try the delete
	err = addConditionAndDeletePod(r, ctx, eviction.Name, rest.ValidateAllObjectFunc, deleteOptions)
	if err != nil {
	if errors.IsConflict(err) && updateDeletionOptions &&
	(originalDeleteOptions.Preconditions == nil || originalDeleteOptions.Preconditions.ResourceVersion == nil) {
	// If we encounter a resource conflict error, we updated the deletion options to include them,
	// and the original deletion options did not specify ResourceVersion, we send back
	// TooManyRequests so clients will retry.
	return nil, createTooManyRequestsError(pdbName)
	}
	return nil, err
	}

Also this observed generation mismatch (stale cache?):

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

Lines 419 to 422 in 786316f

	if pdb.Status.ObservedGeneration < pdb.Generation {
	return createTooManyRequestsError(pdb.Name)
	}

Violating the disruption budget is more debatable, but I still don't think a 429 error is right:

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

Lines 429 to 433 in 786316f

	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)})
	return err
	}

[liggitt]

It looks like eviction has some creative uses of TooManyRequests errors. Why is this conflict error converted to TooManyRequests?

that was added in https://github.com/kubernetes/kubernetes/pull/94381/files#r507982781 with some discussion there

I think the short answer is we wanted to drive an automatic retry (which the server returning TooManyRequests with a RetryAfter does), and returning a conflict when the user did not set a precondition on the request did not seem correct

[k8s-triage-robot]

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

[flavianmissi]

If we put aside the 429 for a moment and focus on the error's cause in question:

  "details": {
    "causes": [
      {
        "reason": "DisruptionBudget",
        "message": "The disruption budget [elided] needs 11 healthy pods and has 12 currently"
      }
    ]
  },

source:

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

Lines 429 to 433 in 786316f

	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)})
	return err
	}

Isn't it weird that this state (needs 11 healthy pods and has 12 currently) is an error?
The code returns this error cause when pdb.Status.DisruptionsAllowed == 0. Is this value wrong in this case, should it have been 1? I couldn't find how this is calculated but I don't know the code so well.

[liggitt]

pdb.Status.DisruptionsAllowed = 0 is set as a fail safe when the DesiredHealthy / CurrentHealthy pods can't be calculated so those numbers are not trustworthy:

kubernetes/pkg/controller/disruption/disruption.go

Lines 944 to 962 in bb838fd

	// failSafe is an attempt to at least update the DisruptionsAllowed field to
	// 0 if everything else has failed. This is one place we
	// implement the "fail open" part of the design since if we manage to update
	// this field correctly, we will prevent the /evict handler from approving an
	// eviction when it may be unsafe to do so.
	func (dc *DisruptionController) failSafe(ctx context.Context, pdb *policy.PodDisruptionBudget, err error) error {
	newPdb := pdb.DeepCopy()
	newPdb.Status.DisruptionsAllowed = 0
	if newPdb.Status.Conditions == nil {
	newPdb.Status.Conditions = make([]metav1.Condition, 0)
	}
	apimeta.SetStatusCondition(&newPdb.Status.Conditions, metav1.Condition{
	Type: policy.DisruptionAllowedCondition,
	Status: metav1.ConditionFalse,
	Reason: policy.SyncFailedReason,
	Message: err.Error(),
	ObservedGeneration: newPdb.Status.ObservedGeneration,
	})

If the type=DisruptionAllowedCondition, status=False condition is present, we should probably use the message from that condition rather than the pdb.Status.DesiredHealthy and pdb.Status.CurrentHealthy fields

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[theunrepentantgeek]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[theunrepentantgeek]

/lifecycle stale

[c-goes]

/remove-lifecycle stale