Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Opt-out/Override client-side max-retry #89566

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 15 additions & 2 deletions staging/src/k8s.io/client-go/rest/request.go
Expand Up @@ -91,6 +91,7 @@ type Request struct {
rateLimiter flowcontrol.RateLimiter
backoff BackoffManager
timeout time.Duration
maxRetries int

// generic components accessible via method setters
verb string
Expand Down Expand Up @@ -139,6 +140,7 @@ func NewRequest(c *RESTClient) *Request {
backoff: backoff,
timeout: timeout,
pathPrefix: pathPrefix,
maxRetries: 10,
}

switch {
Expand Down Expand Up @@ -391,6 +393,18 @@ func (r *Request) Timeout(d time.Duration) *Request {
return r
}

// MaxRetries makes the request use the given integer as a ceiling of retrying upon receiving
// "Retry-After" headers and 429 status-code in the response. The default is 10 unless this
// function is specifically called with a different value.
// A zero maxRetries prevent it from doing retires and return an error immediately.
yue9944882 marked this conversation as resolved.
Show resolved Hide resolved
func (r *Request) MaxRetries(maxRetries int) *Request {
if maxRetries < 0 {
maxRetries = 0
}
r.maxRetries = maxRetries
return r
}

// Body makes the request use obj as the body. Optional.
// If obj is a string, try to read a file of that name.
// If obj is a []byte, send it directly.
Expand Down Expand Up @@ -831,7 +845,6 @@ func (r *Request) request(ctx context.Context, fn func(*http.Request, *http.Resp
}

// Right now we make about ten retry attempts if we get a Retry-After response.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update this comment

maxRetries := 10
retries := 0
for {

Expand Down Expand Up @@ -894,7 +907,7 @@ func (r *Request) request(ctx context.Context, fn func(*http.Request, *http.Resp
}()

retries++
if seconds, wait := checkWait(resp); wait && retries < maxRetries {
if seconds, wait := checkWait(resp); wait && retries <= r.maxRetries {
if seeker, ok := r.body.(io.Seeker); ok && r.body != nil {
_, err := seeker.Seek(0, 0)
if err != nil {
Expand Down
62 changes: 61 additions & 1 deletion staging/src/k8s.io/client-go/rest/request_test.go
Expand Up @@ -1403,7 +1403,8 @@ func TestConnectionResetByPeerIsRetried(t *testing.T) {
return nil, &net.OpError{Err: syscall.ECONNRESET}
}),
},
backoff: backoff,
backoff: backoff,
maxRetries: 10,
}
// We expect two retries of "connection reset by peer" and the success.
_, err := req.Do(context.Background()).Raw()
Expand Down Expand Up @@ -2218,3 +2219,62 @@ func TestThrottledLogger(t *testing.T) {
t.Fatalf("expected %v log messages, but got %v", e, a)
}
}

func TestRequestMaxRetries(t *testing.T) {
successAtNthCalls := 1
actualCalls := 0
retryOneTimeHandler := func(w http.ResponseWriter, req *http.Request) {
defer func() { actualCalls++ }()
if actualCalls >= successAtNthCalls {
w.WriteHeader(http.StatusOK)
return
}
w.Header().Set("Retry-After", "1")
w.WriteHeader(http.StatusTooManyRequests)
actualCalls++
}
testServer := httptest.NewServer(http.HandlerFunc(retryOneTimeHandler))
defer testServer.Close()

u, err := url.Parse(testServer.URL)
if err != nil {
t.Error(err)
}

testCases := []struct {
name string
maxRetries int
expectError bool
}{
{
name: "no retrying should fail",
maxRetries: 0,
expectError: true,
},
{
name: "1 max-retry should exactly work",
maxRetries: 1,
expectError: false,
},
{
name: "5 max-retry should work",
maxRetries: 5,
expectError: false,
},
}

for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
defer func() { actualCalls = 0 }()
_, err := NewRequestWithClient(u, "", defaultContentConfig(), testServer.Client()).
Verb("get").
MaxRetries(testCase.maxRetries).
AbsPath("/foo").
DoRaw(context.TODO())
hasError := err != nil
if testCase.expectError != hasError {
t.Error(" failed checking error")
}
})
}
}