client-go: add unit test to verify order of calls with retry #108262
Merged
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
release-note-none
tkashem
force-pushed
the
retry-unit-test
branch
from
396317f
to
166c7d2
Compare
k8s-ci-robot
added
sig/api-machinery
tkashem
force-pushed
the
retry-unit-test
branch
from
166c7d2
to
6f878de
Compare
|
@aojea that's great, i will review the retry metric PR you are working on. In the meantime, this unit test PR can still merge, it asserts on the expected behavior with rate limiter, backoff and the existing metric call. Can you review this PR please? :) |
|
I think we should test more permutations,
something like (it can be simplified.) 😄 diff --git a/staging/src/k8s.io/client-go/rest/request_test.go b/staging/src/k8s.io/client-go/rest/request_test.go
index 49689119223..8ac86355183 100644
--- a/staging/src/k8s.io/client-go/rest/request_test.go
+++ b/staging/src/k8s.io/client-go/rest/request_test.go
@@ -3000,8 +3000,10 @@ func (lb *withRateLimiterBackoffManagerAndMetrics) Do() {
func testRetryWithRateLimiterBackoffAndMetrics(t *testing.T, key string, doFunc func(ctx context.Context, r *Request)) {
type expected struct {
- attempts int
- order []string
+ attempts int
+ order []string
+ sleeps []string
+ statusCodes []string
}
// we define the expected order of how the client invokes the
@@ -3010,47 +3012,6 @@ func testRetryWithRateLimiterBackoffAndMetrics(t *testing.T, key string, doFunc
// - A: original request fails with a retryable response: (500, 'Retry-After: 1')
// - B: retry 1: successful with a status code 200
// so we have a total of 2 attempts
- callOrderExpected := []string{
- // before we send the request to the server:
- // - we wait as dictated by the client rate lmiter
- // - we wait, as dictated by the backoff manager
- "RateLimiter.Wait",
- "BackoffManager.CalculateBackoff",
- "BackoffManager.Sleep",
-
- // A: first attempt for which the server sends a retryable response
- "Client.Do",
-
- // we got a response object, status code: 500, Retry-Afer: 1
- // - call metrics method with appropriate status code
- // - update backoff parameters with the status code returned
- // - sleep for N seconds from 'Retry-After: N' response header
- "RequestResult.Increment",
- "BackoffManager.UpdateBackoff",
- "BackoffManager.Sleep",
- // sleep for delay dictated by backoff parameters
- "BackoffManager.CalculateBackoff",
- "BackoffManager.Sleep",
- // wait as dictated by the client rate lmiter
- "RateLimiter.Wait",
-
- // B: 2nd attempt: retry, and this should return a status code=200
- "Client.Do",
-
- // it's a success, so do the following:
- // - call metrics and update backoff parameters
- "RequestResult.Increment",
- "BackoffManager.UpdateBackoff",
- }
- sleepExpected := []string{
- "0s", // initial backoff.Sleep before we send the request to the server for the first time
- (1 * time.Second).String(), // from 'Retry-After: 1' response header
- (2 * time.Minute).String(), // backoff.Sleep before retry 1 (B)
- }
- statusCodesExpected := []string{
- "500",
- "200",
- }
tests := []struct {
name string
@@ -3060,7 +3021,76 @@ func testRetryWithRateLimiterBackoffAndMetrics(t *testing.T, key string, doFunc
expectations map[string]expected
}{
{
- name: "success after two retries",
+ name: "success",
+ maxRetries: 2,
+ serverReturns: []responseErr{
+ {response: &http.Response{StatusCode: http.StatusOK}, err: nil},
+ },
+ expectations: map[string]expected{
+ "Do": {
+ attempts: 1,
+ order: []string{
+ // before we send the request to the server:
+ // - we wait as dictated by the client rate limiter
+ // - we wait, as dictated by the backoff manager
+ "RateLimiter.Wait",
+ "BackoffManager.CalculateBackoff",
+ "BackoffManager.Sleep",
+
+ // A: first attempt for which the server sends a retryable response
+ "Client.Do",
+
+ // it's a success, so do the following:
+ // - call metrics and update backoff parameters
+ "RequestResult.Increment",
+ "BackoffManager.UpdateBackoff",
+ },
+ sleeps: []string{
+ "0s", // initial backoff.Sleep before we send the request to the server for the first time
+ },
+ statusCodes: []string{
+ "200",
+ },
+ },
+ "Watch": {
+ attempts: 1,
+ // Watch does not do 'RateLimiter.Wait' before initially sending the request to the server
+ order: []string{
+ // before we send the request to the server:
+ // - we wait, as dictated by the backoff manager
+ "BackoffManager.CalculateBackoff",
+ "BackoffManager.Sleep",
+ // A: first attempt for which the server sends a retryable response
+ "Client.Do",
+ // it's a success, so do the following:
+ // - call metrics and update backoff parameters
+ "RequestResult.Increment",
+ "BackoffManager.UpdateBackoff",
+ },
+ },
+ "Stream": {
+ attempts: 1,
+ order: []string{
+ // before we send the request to the server:
+ // - we wait as dictated by the client rate limiter
+ // - we wait, as dictated by the backoff manager
+ "RateLimiter.Wait",
+ "BackoffManager.CalculateBackoff",
+ "BackoffManager.Sleep",
+
+ // A: first attempt for which the server sends a retryable response
+ "Client.Do",
+
+ // it's a success, so do the following:
+ // - call metrics and update backoff parameters
+ "RequestResult.Increment",
+ "BackoffManager.UpdateBackoff",
+ },
+ },
+ },
+ },
+ {
+ name: "success after 2 retries",
maxRetries: 2,
serverReturns: []responseErr{
{response: retryAfterResponse(), err: nil},
@@ -3069,19 +3099,358 @@ func testRetryWithRateLimiterBackoffAndMetrics(t *testing.T, key string, doFunc
expectations: map[string]expected{
"Do": {
attempts: 2,
- order: callOrderExpected,
+ order: []string{
+ // before we send the request to the server:
+ // - we wait as dictated by the client rate lmiter
+ // - we wait, as dictated by the backoff manager
+ "RateLimiter.Wait",
+ "BackoffManager.CalculateBackoff",
+ "BackoffManager.Sleep",
+
+ // A: first attempt for which the server sends a retryable response
+ "Client.Do",
+
+ // we got a response object, status code: 500, Retry-Afer: 1
+ // - call metrics method with appropriate status code
+ // - update backoff parameters with the status code returned
+ // - sleep for N seconds from 'Retry-After: N' response header
+ "RequestResult.Increment",
+ "BackoffManager.UpdateBackoff",
+ "BackoffManager.Sleep",
+ // sleep for delay dictated by backoff parameters
+ "BackoffManager.CalculateBackoff",
+ "BackoffManager.Sleep",
+ // wait as dictated by the client rate lmiter
+ "RateLimiter.Wait",
+
+ // B: 2nd attempt: retry, and this should return a status code=200
+ "Client.Do",
+
+ // it's a success, so do the following:
+ // - call metrics and update backoff parameters
+ "RequestResult.Increment",
+ "BackoffManager.UpdateBackoff",
+ },
+ sleeps: []string{
+ "0s", // initial backoff.Sleep before we send the request to the server for the first time
+ (1 * time.Second).String(), // from 'Retry-After: 1' response header
+ (2 * time.Minute).String(), // backoff.Sleep before retry 1 (B)
+ },
+ statusCodes: []string{
+ "500",
+ "200",
+ },
},
"Watch": {
attempts: 2,
- // Watch does not do 'RateLimiter.Wait' before initially sending the request to the server
- order: callOrderExpected[1:],
+ order: []string{
+ // before we send the request to the server:
+ // - we wait, as dictated by the backoff manager
+ "BackoffManager.CalculateBackoff",
+ "BackoffManager.Sleep",
+
+ // A: first attempt for which the server sends a retryable response
+ "Client.Do",
+
+ // we got a response object, status code: 500, Retry-Afer: 1
+ // - call metrics method with appropriate status code
+ // - update backoff parameters with the status code returned
+ // - sleep for N seconds from 'Retry-After: N' response header
+ "RequestResult.Increment",
+ "BackoffManager.UpdateBackoff",
+ "BackoffManager.Sleep",
+ // sleep for delay dictated by backoff parameters
+ "BackoffManager.CalculateBackoff",
+ "BackoffManager.Sleep",
+ // wait as dictated by the client rate lmiter
+ "RateLimiter.Wait",
+
+ // B: 2nd attempt: retry, and this should return a status code=200
+ "Client.Do",
+
+ // it's a success, so do the following:
+ // - call metrics and update backoff parameters
+ "RequestResult.Increment",
+ "BackoffManager.UpdateBackoff",
+ },
+ sleeps: []string{
+ "0s", // initial backoff.Sleep before we send the request to the server for the first time
+ (1 * time.Second).String(), // from 'Retry-After: 1' response header
+ (2 * time.Minute).String(), // backoff.Sleep before retry 1 (B)
+ },
+ statusCodes: []string{
+ "500",
+ "200",
+ },
},
"Stream": {
attempts: 2,
- order: callOrderExpected,
+ order: []string{
+ // before we send the request to the server:
+ // - we wait as dictated by the client rate lmiter
+ // - we wait, as dictated by the backoff manager
+ "RateLimiter.Wait",
+ "BackoffManager.CalculateBackoff",
+ "BackoffManager.Sleep",
+
+ // A: first attempt for which the server sends a retryable response
+ "Client.Do",
+
+ // we got a response object, status code: 500, Retry-Afer: 1
+ // - call metrics method with appropriate status code
+ // - update backoff parameters with the status code returned
+ // - sleep for N seconds from 'Retry-After: N' response header
+ "RequestResult.Increment",
+ "BackoffManager.UpdateBackoff",
+ "BackoffManager.Sleep",
+ // sleep for delay dictated by backoff parameters
+ "BackoffManager.CalculateBackoff",
+ "BackoffManager.Sleep",
+ // wait as dictated by the client rate lmiter
+ "RateLimiter.Wait",
+
+ // B: 2nd attempt: retry, and this should return a status code=200
+ "Client.Do",
+
+ // it's a success, so do the following:
+ // - call metrics and update backoff parameters
+ "RequestResult.Increment",
+ "BackoffManager.UpdateBackoff",
+ },
+ sleeps: []string{
+ "0s", // initial backoff.Sleep before we send the request to the server for the first time
+ (1 * time.Second).String(), // from 'Retry-After: 1' response header
+ (2 * time.Minute).String(), // backoff.Sleep before retry 1 (B)
+ },
+ statusCodes: []string{
+ "500",
+ "200",
+ },
},
},
},
+ {
+ name: "failure after 2 retries",
+ maxRetries: 2,
+ serverReturns: []responseErr{
+ {response: retryAfterResponse(), err: nil},
+ {response: retryAfterResponse(), err: nil},
+ {response: retryAfterResponse(), err: nil},
+ },
+
+ expectations: map[string]expected{
+ "Do": {
+ attempts: 3,
+ order: []string{
+ // before we send the request to the server:
+ // - we wait as dictated by the client rate lmiter
+ // - we wait, as dictated by the backoff manager
+ "RateLimiter.Wait",
+ "BackoffManager.CalculateBackoff",
+ "BackoffManager.Sleep",
+ "Client.Do",
+ "RequestResult.Increment",
+ "BackoffManager.UpdateBackoff",
+ "BackoffManager.Sleep",
+ "BackoffManager.CalculateBackoff",
+ "BackoffManager.Sleep",
+ "RateLimiter.Wait",
+ // A: first attempt for which the server sends a retryable response
+ "Client.Do",
+
+ // we got a response object, status code: 500, Retry-Afer: 1
+ // - call metrics method with appropriate status code
+ // - update backoff parameters with the status code returned
+ // - sleep for N seconds from 'Retry-After: N' response header
+ "RequestResult.Increment",
+ "BackoffManager.UpdateBackoff",
+ "BackoffManager.Sleep",
+ // sleep for delay dictated by backoff parameters
+ "BackoffManager.CalculateBackoff",
+ "BackoffManager.Sleep",
+ // wait as dictated by the client rate lmiter
+ "RateLimiter.Wait",
+
+ // B: 2nd attempt: retry, and this should return a status code=200
+ "Client.Do",
+
+ // it's a success, so do the following:
+ // - call metrics and update backoff parameters
+ "RequestResult.Increment",
+ "BackoffManager.UpdateBackoff",
+ },
+ sleeps: []string{
+ "0s", // initial backoff.Sleep before we send the request to the server for the first time
+ (1 * time.Second).String(), // from 'Retry-After: 1' response header
+ (2 * time.Minute).String(), // backoff.Sleep before retry 1 (B)
+ (1 * time.Second).String(), // from 'Retry-After: 1' response header
+ (4 * time.Minute).String(), // backoff.Sleep before retry 1 (B)
+ },
+ statusCodes: []string{
+ "500",
+ "500",
+ "500",
+ },
+ },
+ "Watch": {
+ attempts: 3,
+ order: []string{
+ // before we send the request to the server:
+ // - we wait, as dictated by the backoff manager
+ "BackoffManager.CalculateBackoff",
+ "BackoffManager.Sleep",
+
+ // A: first attempt for which the server sends a retryable response
+ "Client.Do",
+
+ // we got a response object, status code: 500, Retry-Afer: 1
+ // - call metrics method with appropriate status code
+ // - update backoff parameters with the status code returned
+ // - sleep for N seconds from 'Retry-After: N' response header
+ "RequestResult.Increment",
+ "BackoffManager.UpdateBackoff",
+ "BackoffManager.Sleep",
+ // sleep for delay dictated by backoff parameters
+ "BackoffManager.CalculateBackoff",
+ "BackoffManager.Sleep",
+ // wait as dictated by the client rate lmiter
+ "RateLimiter.Wait",
+
+ // B: 2nd attempt: retry, and this should return a status code=200
+ "Client.Do",
+
+ // it's a success, so do the following:
+ // - call metrics and update backoff parameters
+ "RequestResult.Increment",
+ "BackoffManager.UpdateBackoff",
+ },
+ sleeps: []string{
+ "0s", // initial backoff.Sleep before we send the request to the server for the first time
+ (1 * time.Second).String(), // from 'Retry-After: 1' response header
+ (2 * time.Minute).String(), // backoff.Sleep before retry 1 (B)
+ },
+ statusCodes: []string{
+ "500",
+ "500",
+ },
+ },
+ "Stream": {
+ attempts: 3,
+ order: []string{
+ // before we send the request to the server:
+ // - we wait as dictated by the client rate lmiter
+ // - we wait, as dictated by the backoff manager
+ "RateLimiter.Wait",
+ "BackoffManager.CalculateBackoff",
+ "BackoffManager.Sleep",
+
+ // A: first attempt for which the server sends a retryable response
+ "Client.Do",
+
+ // we got a response object, status code: 500, Retry-Afer: 1
+ // - call metrics method with appropriate status code
+ // - update backoff parameters with the status code returned
+ // - sleep for N seconds from 'Retry-After: N' response header
+ "RequestResult.Increment",
+ "BackoffManager.UpdateBackoff",
+ "BackoffManager.Sleep",
+ // sleep for delay dictated by backoff parameters
+ "BackoffManager.CalculateBackoff",
+ "BackoffManager.Sleep",
+ // wait as dictated by the client rate lmiter
+ "RateLimiter.Wait",
+
+ // B: 2nd attempt: retry, and this should return a status code=200
+ "Client.Do",
+
+ // it's a success, so do the following:
+ // - call metrics and update backoff parameters
+ "RequestResult.Increment",
+ "BackoffManager.UpdateBackoff",
+ },
+ sleeps: []string{
+ "0s", // initial backoff.Sleep before we send the request to the server for the first time
+ (1 * time.Second).String(), // from 'Retry-After: 1' response header
+ (2 * time.Minute).String(), // backoff.Sleep before retry 1 (B)
+ },
+ statusCodes: []string{
+ "500",
+ "500",
+ },
+ },
+ },
+ },
+ {
+ name: "do not retry on network errors",
+ maxRetries: 2,
+ serverReturns: []responseErr{
+ {response: nil, err: fmt.Errorf("network error")},
+ },
+ expectations: map[string]expected{
+ "Do": {
+ attempts: 1,
+ order: []string{
+ // before we send the request to the server:
+ // - we wait as dictated by the client rate lmiter
+ // - we wait, as dictated by the backoff manager
+ "RateLimiter.Wait",
+ "BackoffManager.CalculateBackoff",
+ "BackoffManager.Sleep",
+
+ // A: first attempt for which the server sends a retryable response
+ "Client.Do",
+
+ // ???
+ "RequestResult.Increment",
+ "BackoffManager.UpdateBackoff",
+ },
+ sleeps: []string{
+ "0s", // initial backoff.Sleep before we send the request to the server for the first time
+ },
+ statusCodes: []string{"<error>"},
+ },
+ "Watch": {
+ attempts: 1,
+ order: []string{
+ // before we send the request to the server:
+ // - we wait, as dictated by the backoff manager
+ "BackoffManager.CalculateBackoff",
+ "BackoffManager.Sleep",
+
+ // A: first attempt for which the server sends a retryable response
+ "Client.Do",
+
+ // ???
+ "RequestResult.Increment",
+ "BackoffManager.UpdateBackoff",
+ },
+ sleeps: []string{
+ "0s", // initial backoff.Sleep before we send the request to the server for the first time
+ },
+ statusCodes: []string{"<error>"}},
+ "Stream": {
+ attempts: 1,
+ order: []string{
+ // before we send the request to the server:
+ // - we wait as dictated by the client rate lmiter
+ // - we wait, as dictated by the backoff manager
+ "RateLimiter.Wait",
+ "BackoffManager.CalculateBackoff",
+ "BackoffManager.Sleep",
+
+ // A: first attempt for which the server sends a retryable response
+ "Client.Do",
+
+ // ???
+ "RequestResult.Increment",
+ "BackoffManager.UpdateBackoff",
+ },
+ sleeps: []string{
+ "0s", // initial backoff.Sleep before we send the request to the server for the first time
+ },
+ statusCodes: []string{"<error>"}},
+ },
+ },
}
for _, test := range tests {
@@ -3149,11 +3518,11 @@ func testRetryWithRateLimiterBackoffAndMetrics(t *testing.T, key string, doFunc
if !cmp.Equal(expected.order, interceptor.order) {
t.Errorf("%s: Expected order of calls to match, diff: %s", key, cmp.Diff(expected.order, interceptor.order))
}
- if !cmp.Equal(sleepExpected, interceptor.sleeps) {
- t.Errorf("%s: Expected order of calls to match, diff: %s", key, cmp.Diff(sleepExpected, interceptor.sleeps))
+ if !cmp.Equal(expected.sleeps, interceptor.sleeps) {
+ t.Errorf("%s: Expected order of calls to match, diff: %s", key, cmp.Diff(expected.sleeps, interceptor.sleeps))
}
- if !cmp.Equal(statusCodesExpected, interceptor.statusCodes) {
- t.Errorf("%s: Expected status codes to match, diff: %s", key, cmp.Diff(statusCodesExpected, interceptor.statusCodes))
+ if !cmp.Equal(expected.statusCodes, interceptor.statusCodes) {
+ t.Errorf("%s: Expected status codes to match, diff: %s", key, cmp.Diff(expected.statusCodes, interceptor.statusCodes))
}
})
}is the behavior of "failure after exhausting the retries" correct? |
@aojea we have dedicated tests for these above scenarios, this PR adds a new test only to validate on backoff, rate limiter and metric call invocation. |
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
ah ok, I though it would be good to validate that combo on the different scenarios If there is no need it LGTM then |
tkashem
force-pushed
the
retry-unit-test
branch
from
6f878de
to
f6a66bb
Compare
|
/test pull-kubernetes-e2e-kind |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
wojtek-t
reviewed
Feb 23, 2022
| "BackoffManager.Sleep", | ||
| // sleep for delay dictated by backoff parameters | ||
| "BackoffManager.CalculateBackoff", | ||
| "BackoffManager.Sleep", |
There was a problem hiding this comment.
I was surprised too, but I think that one sleep is the "retry-after" sleep and the other the actual backoff
(1 * time.Second).String(),
// backoff.Sleep before retry 1 (B)
(2 * time.Minute).String(),
Line 3045
There was a problem hiding this comment.
ok, I've found it
kubernetes/staging/src/k8s.io/client-go/rest/with_retry.go
Lines 171 to 175 in 296bf4f
why do we have to sleep on the retryAfter only if backoff exists?
are not both Sleeps independent?
There was a problem hiding this comment.
@wojtek-t today, with a default client-go configuration of a noBackoff, it's always BackoffManager.Sleep(0). I think when #106272 merges we can remove the sleep from Retry-After response.
I am working on a refactor PR where I am trying to put the backoff, rate limiter, and metric calls in a common site so Do, Watch, and Stream can use re-use them. I have added a TODO to remove it once #106272 merges.
There was a problem hiding this comment.
#106272 merges we can remove the sleep from Retry-After response.
I don't think we can just simply remove it - we should respect what kube-apiserver is returning to us (it's always returning 1 now, but we should address that too at some point).
So I think it's not abot removing - it's rather about unifying them and making something like:
BackOffManager.Sleep(max(backoffManager.CalculateBackoff(), retryAfter))
There was a problem hiding this comment.
yes, this is more forward looking, i will bake that in my next PR
| }, | ||
| "Watch": { | ||
| attempts: 2, | ||
| // Watch does not do 'RateLimiter.Wait' before initially sending the request to the server |
There was a problem hiding this comment.
This is another thing that seems strange to me - if it's not throttling before the first request, why it is throttling before retries?
@tkashem - can you please maybe open an issue with those inonsistencies so that we can discuss (and fix assuming I'm not missing something subtle) those things there?
There was a problem hiding this comment.
yes, I will open an issue with this, thanks!
k8s-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tkashem, wojtek-t 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
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.
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This unit test verifies that
Do,DoRaw,Stream, andWatchinvoke theflowcontrol.RateLimiter,BackoffManager, andmetrics.ResultMetricappropriately and in right order for retry.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: