Fix/5163 retry download upgrade verifiers #6276
Conversation
|
This pull request does not have a backport label. Could you fix it @kaanyalti? 🙏
|
|
|
mergify
bot
added
the
backport-8.x
| resp, err = btr.next.RoundTrip(req) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
This lint error should be fixed. Should probably call close no matter what.
There was a problem hiding this comment.
Looked into this with @belimawr . The linter is most probably complaining here because the response is used in a closure and not passed onto anything else. Closing the response body here causes an error when the caller tries to read from it. The response is closed after each unsuccessful attempt so that we don't leak resources. When an attempt is successful, the response is sent back up the call stack. This is similar to how it is implemented in FleetAuthRoundTripper. Another option was to read the whole response into a buffer, close the body, and then assign the bufffer back to the response body after wrapping it with io.NopCloser. This is how the DebugRoundTripper handles the request body. While it is a valid solution, it may cause issues in case there are other midleware in the rountripper chain where there may be a response body size limiter, and it also doesn't seem to be necessary here as the current implementation works as expected. To resolve the linting problem I added a nolint comment.
There was a problem hiding this comment.
Thanks for the detailed explanation. Glad to see it was also added to the code.
kaanyalti
force-pushed
the
fix/5163_retry_download_upgrade_verifiers
branch
from
7e387dc to
5ea64d3
Compare
| exp := backoff.NewExponentialBackOff() | ||
| boCtx := backoff.WithContext(exp, req.Context()) | ||
|
|
||
| opNotify := func(err error, retryAfter time.Duration) { |
internal/pkg/agent/application/upgrade/artifact/download/http/verify_backoff_rtt_test.go
Show resolved
Hide resolved
…lls, added backoff roundtripper
…roundtripper, added unit tests, updated comments
kaanyalti
force-pushed
the
fix/5163_retry_download_upgrade_verifiers
branch
from
b503a10 to
a391457
Compare
|
* fix(5163): added context to the verify interface, updated relevant calls, added backoff roundtripper * fix(5163): added backoff roundtripper, added tests for asc file download * fix(5163): remove log * fix(5163): ran mage fmt * fix(5163): removed unused roundtripper * fix(5163): closing response body on unsuccesful responses * fix(5163): added changelog * fix(5163): added comment explaining the backoff function, added nolint comment * fix(5163): added logic to handle resetting request body in the retry roundtripper, added unit tests, updated comments * fix(5163): fixed linting errors * fix(5164): ran mage check (cherry picked from commit a5eb77b)
* fix(5163): added context to the verify interface, updated relevant calls, added backoff roundtripper * fix(5163): added backoff roundtripper, added tests for asc file download * fix(5163): remove log * fix(5163): ran mage fmt * fix(5163): removed unused roundtripper * fix(5163): closing response body on unsuccesful responses * fix(5163): added changelog * fix(5163): added comment explaining the backoff function, added nolint comment * fix(5163): added logic to handle resetting request body in the retry roundtripper, added unit tests, updated comments * fix(5163): fixed linting errors * fix(5164): ran mage check (cherry picked from commit a5eb77b) Co-authored-by: Kaan Yalti <kaan.yalti@elastic.co> Co-authored-by: Julien Lind <julien.lind@elastic.co>
What does this PR do?
Adds backoff retry for verifier artifact requests
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files./changelog/fragmentsusing the changelog tool[ ] I have added an integration test or an E2E testHow to test this PR locally
Related issues