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.

Sign up for GitHub

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

Jump to bottom

Fix client-go request retry race #113933

Merged
merged 1 commit into from Dec 10, 2022
Merged

Fix client-go request retry race #113933

merged 1 commit into from Dec 10, 2022

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Nov 15, 2022 •
edited

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.

client-go: fixes potential data races retrying requests using a custom io.Reader body; with this fix, only requests with no body or with string / []byte / runtime.Object bodies can be retried

/cc @aojea

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 15, 2022
@alexzielenski
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 15, 2022
@aojea
Copy link
Member

aojea commented Nov 15, 2022

Does it pass.?

This was not closed golang/go#51907

@liggitt
Copy link
Member Author

liggitt commented Nov 15, 2022 •
edited

Does it pass.?

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

@liggitt liggitt changed the title Expand client-go request retry unit test Fix client-go request retry race Nov 15, 2022
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 15, 2022
@liggitt
Copy link
Member Author

liggitt commented Nov 15, 2022

/assign @smarterclayton @aojea
as reviewers on the 1.24 fix

/cc @MadhavJivrajani @tkashem
as folks with context on the 1.24 fix

@liggitt liggitt mentioned this pull request Nov 15, 2022
Closed
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Nov 15, 2022
@deads2k
Copy link
Contributor

deads2k commented Nov 16, 2022

I think this bugfix should be backported to all supported branches to resolve the latent data race panic we have with the .Seek() approach. I'm on the fence whether we should merge this for 1.26.0 and backport for December releases of 1.23-1.25, or hold it for 1.26.1 and January releases.

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.

@dims
Copy link
Member

dims commented Nov 16, 2022

This is somewhat of a prereq for efforts to get release-1.23 building/passing tests on go1.18, so I would like to see it happen now if possible, but would like to hear perspectives of @lavalamp / @deads2k, as well as the 1.26 release folks.

Let me assign the milestone and release folks can remove /hold if they agree.

/milestone v1.26

cc @kubernetes/release-team-leads

@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Nov 16, 2022
@liggitt
Copy link
Member Author

liggitt commented Nov 16, 2022 •
edited

I'm ok with 1.26.1, will queue this up once master reopens

/milestone clear

@liggitt
Copy link
Member Author

liggitt commented Nov 16, 2022 •
edited

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.

@liggitt liggitt removed this from the v1.26 milestone Nov 16, 2022
This was referenced Nov 17, 2022
Merged
Merged
Merged
Merged
Merged
@tkashem
Copy link
Contributor

tkashem commented Nov 18, 2022

This looks good, thanks!

/lgtm

@wojtek-t
Copy link
Member

Thanks for fixing this! LGTM too.

lavalamp
lavalamp reviewed Nov 29, 2022
View reviewed changes
staging/src/k8s.io/client-go/rest/request.go
body io.Reader
err error

// only one of body / bodyBytes may be set. requests using body are not retriable.
Copy link
Member

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)

Copy link
Member Author

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)

Copy link
Member

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" :)

@lavalamp
Copy link
Member

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@liggitt liggitt mentioned this pull request Dec 6, 2022
Merged
@liggitt
Copy link
Member Author

liggitt commented Dec 10, 2022

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 10, 2022
@k8s-ci-robot k8s-ci-robot merged commit 8d2fbfc into kubernetes:master Dec 10, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Dec 10, 2022
k8s-ci-robot added a commit that referenced this pull request Dec 12, 2022
@k8s-ci-robot
Merge pull request #113988 from liggitt/automated-cherry-pick-of-#113…
7c0eb3b 
…933-upstream-release-1.26

[1.26.1] Automated cherry pick of #113933: Limit request retrying to []byte request bodies
k8s-ci-robot added a commit that referenced this pull request Dec 12, 2022
@k8s-ci-robot
Merge pull request #113990 from liggitt/automated-cherry-pick-of-#113…
e6d249f 
…933-upstream-release-1.25

Automated cherry pick of #113933: Limit request retrying to []byte request bodies
k8s-ci-robot added a commit that referenced this pull request Dec 12, 2022
@k8s-ci-robot
Merge pull request #113993 from liggitt/automated-cherry-pick-of-#113…
22b81eb 
…933-upstream-release-1.23

Automated cherry pick of #113933: Limit request retrying to []byte request bodies
k8s-ci-robot added a commit that referenced this pull request Dec 12, 2022
@k8s-ci-robot
Merge pull request #113991 from liggitt/automated-cherry-pick-of-#113…
d0ea49e 
…933-upstream-release-1.24

Automated cherry pick of #113933: Limit request retrying to []byte request bodies
@liggitt liggitt modified the milestones: v1.26, v1.27 Dec 13, 2022
@liggitt liggitt deleted the retry-race branch May 9, 2023 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lavalamp lavalamp lavalamp left review comments

@aojea aojea aojea left review comments

@MadhavJivrajani MadhavJivrajani MadhavJivrajani left review comments

@tkashem tkashem Awaiting requested review from tkashem

Assignees

@lavalamp lavalamp

@smarterclayton smarterclayton

@aojea aojea

@tkashem tkashem

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.27
Development

Successfully merging this pull request may close these issues.

None yet