[apiserver] Add retry and timeout to apiserver V2 #3869
Conversation
Signed-off-by: Cheng-Yeh Chung <[email protected]>
|
@dentiny PTAL, thanks! |
Signed-off-by: Cheng-Yeh Chung <[email protected]>
apiserversdk/proxy.go
Outdated
| return nil, err | ||
| } | ||
| proxy.Transport = newRetryRoundTripper(baseTransport, 3) |
There was a problem hiding this comment.
make retry count a constant, and make sure it's compatible with v1.
https://github.com/machichima/kuberay/blob/a321fbc81220c75c0d52b4ebab337810f7cd4f50/apiserver/pkg/util/config.go#L28-L37
we should at least followup a PR to unify these retry configs between v1 and v2
Currently my implementation logs the error message to I test with the following steps And what I got is: |
|
However, this may not work for the case that we wrapped error message into error |
…iserver V1 Signed-off-by: Cheng-Yeh Chung <[email protected]>
kenchung285
changed the title
|
@dentiny PTAL, thanks! |
Signed-off-by: Cheng-Yeh Chung <[email protected]>
| for attempt := 0; attempt <= rrt.retries; attempt++ { | ||
| if attempt > 0 && req.GetBody != nil { | ||
| var bodyCopy io.ReadCloser | ||
| bodyCopy, err = req.GetBody() |
There was a problem hiding this comment.
Can we merge bodyPreserveMiddleware into this function? At least merge it into retryRoundTripper.
| req.Body = bodyCopy | ||
| } | ||
|
|
||
| resp, err = rrt.base.RoundTrip(req) |
There was a problem hiding this comment.
Should we drain the old response before the next retry?
|
|
||
| import "time" | ||
|
|
||
| // Compatible with apiserver V1 |
There was a problem hiding this comment.
no need to be in this PR, could you please leave a TODO, that we will merge v1/v2 retry timeout constants and logic into one?
| sleepDuration = HTTPClientDefaultMaxBackoff | ||
| } | ||
|
|
||
| if deadline, ok := ctx.Deadline(); ok { |
There was a problem hiding this comment.
No need to be in this PR, let's extract into a util sleep function
https://github.com/machichima/kuberay/blob/a321fbc81220c75c0d52b4ebab337810f7cd4f50/apiserver/pkg/http/client.go#L733-L744
| return 200 <= statusCode && statusCode < 300 | ||
| } | ||
|
|
||
| func retryableHTTPStatusCodes(statusCode int) bool { |
There was a problem hiding this comment.
same here, worth a TODO to merge common utils
https://github.com/machichima/kuberay/blob/a321fbc81220c75c0d52b4ebab337810f7cd4f50/apiserver/pkg/http/client.go#L651-L658
There was a problem hiding this comment.
And it's better to use array instead of map for small number of lookups
| return | ||
| } | ||
| err = r.Body.Close() | ||
| if err != nil { |
There was a problem hiding this comment.
Should we contain the err in the error message?
| retries int | ||
| } | ||
|
|
||
| func newRetryRoundTripper(base http.RoundTripper, retries int) http.RoundTripper { |
There was a problem hiding this comment.
On testing, RoundTripper is an interface, which means you can have your mock implementation to mimic error; I think we should be able to test retry and timeout with unit test?
Why are these changes needed?
Add retry logic and timeout to apiserver v2:
Wrap errors with more informations
Related issue number
Closes #3606
Checks