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
Fix client-go request retry race #113933
Fix client-go request retry race #113933
Conversation
/triage accepted |
Does it pass.? This was not closed golang/go#51907 |
nope, demonstrating we still have this issue. looking at how we actually use request/body, I think I'm going to rip out seeking entirely, preserve the original []byte data, and construct a new bytes.Reader for each request. That would mean requests setting custom io.Reader bodies would not be retriable, but I don't see how we can guarantee correctness for them anyway |
/assign @smarterclayton @aojea /cc @MadhavJivrajani @tkashem |
This is a non-regression of an edge case (dependent on size of input) in a failure case (first request failed). I'm biased towards 1.26.1. |
Let me assign the milestone and release folks can remove /milestone v1.26 cc @kubernetes/release-team-leads |
I'm ok with 1.26.1, will queue this up once master reopens /milestone clear |
I'll start working on the cherry-picks for review now though, since all the refactorings in the 1.24/1.25 timeframe mean this likely won't pick cleanly. |
This looks good, thanks! /lgtm |
Thanks for fixing this! LGTM too. |
body io.Reader | ||
err error | ||
|
||
// only one of body / bodyBytes may be set. requests using body are not retriable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make them retriable by using e.g. a TeeReader to store what we read and send.
(I don't think we should do this but it's not impossible)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, I replied to that suggestion at #113933 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to stop getting this comment, you could make the comment say something like "this could be fixed with TeeReader but we don't have any evidence it's needed" :)
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lavalamp, liggitt 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 |
/hold cancel |
…933-upstream-release-1.26 [1.26.1] Automated cherry pick of #113933: Limit request retrying to []byte request bodies
…933-upstream-release-1.25 Automated cherry pick of #113933: Limit request retrying to []byte request bodies
…933-upstream-release-1.23 Automated cherry pick of #113933: Limit request retrying to []byte request bodies
…933-upstream-release-1.24 Automated cherry pick of #113933: Limit request retrying to []byte request bodies
What type of PR is this?
/kind bug
/area test
What this PR does / why we need it:
Drops .Seek() behavior on retry to avoid data races still present (xref golang/go#51907 (comment)). There is no way to safely interrupt the previous background read/copy of the request body from the previous request write.
This limits retries to rest.Requests with no body or with a []byte body. Requests setting custom io.Reader bodies will not be retriable because we cannot seek/reset them safely.
In practice, no in-tree uses set io.Reader bodies, so this has no impact on kubectl, controllers, or client-go generated clients.
/cc @aojea