Timeout and Max-in-flight don't report non-resource URLs correctly. #49678
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
the
cncf-cla: yes
k8s-github-robot
added
size/L
smarterclayton
force-pushed
the
429_metric
branch
from
694d535 to
67ac843
Compare
smarterclayton
added
release-note
smarterclayton
force-pushed
the
429_metric
branch
2 times, most recently
from
85a0ecf to
d88f091
Compare
deads2k
reviewed
Jul 27, 2017
| func NewTooManyRequests(message string, retryAfterSeconds int) *StatusError { | ||
| return &StatusError{metav1.Status{ | ||
| Status: metav1.StatusFailure, | ||
| Code: http.StatusServiceUnavailable, |
There was a problem hiding this comment.
I don't keep them all in cache, but isn't this a 503, not a 429?
There was a problem hiding this comment.
Um, yes, I don't know how this didn't get copied. I must have autocompleted the wrong field. Terrifying.
|
double check the status code I pointed out. otherwise lgtm. |
smarterclayton
force-pushed
the
429_metric
branch
from
d88f091 to
d89508e
Compare
All of these errors are now part of the standard HTTP method. Formalize those into our error types and remove duplication and unclear separation.
This potentially has high cardinality, however we can rate limit based on queries to these endpoints as well.
Our rules are that code of the error must match code of the response. We were also not setting the correct reason. This updates the timeout filter to be consistent with other clients, without changing the error code (504 is correct). The new message properly indicates the request may still be running, which the old message did not do.
Also uses the standard error constructor for TooManyRequests and clarifies *why* a disruption is rejected.
SuggestClientDelay is returning whether the server has requested that the client delay their next action. It is *not* about whether the client should retry the action. Webhook was using it incorrectly, and the method is now up to date.
smarterclayton
force-pushed
the
429_metric
branch
3 times, most recently
from
124d536 to
8c18db0
Compare
smarterclayton
force-pushed
the
429_metric
branch
from
8c18db0 to
ddbc2ad
Compare
|
@deads2k see the SuggestClientDelay bit |
deads2k
reviewed
Aug 1, 2017
| @@ -90,6 +90,11 @@ func WithExponentialBackoff(initialBackoff time.Duration, webhookFn func() error | |||
| var err error | |||
| wait.ExponentialBackoff(backoff, func() (bool, error) { | |||
| err = webhookFn() | |||
| // these errors indicate a need to retry an authentication check | |||
| if apierrors.IsServerTimeout(err) || apierrors.IsTimeout(err) || apierrors.IsTooManyRequests(err) { | |||
There was a problem hiding this comment.
do these not trigger the lower check on a specified client delay?
There was a problem hiding this comment.
Not necessarily. We don't actually honor the delay right now, but we have two classes of things:
- errors that mean retry
- an error or response (that doesn't automatically mean retry) that still asks you to retry for some other reason
|
lgtm |
|
/approve no-issue |
smarterclayton
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smarterclayton Associated issue requirement bypassed by: smarterclayton The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
the
approved
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Unify error reporting for 429 and 504 to be correct for timeout and max in flight and eviction. Add better messages to eviction (removing a todo). Return the correct body content for timeouts (reason and code should be correct).
This potentially increases cardinality of 429, but because non-api urls may be under the max-inflight budget we need to report them somewhere (if something breaks and starts fetching API versions endlessly).