Skip to content

update the client generator to set a client-side timeout #70998

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
Merged
k8s-ci-robot merged 3 commits into from
Nov 16, 2018
Merged

update the client generator to set a client-side timeout #70998

k8s-ci-robot merged 3 commits into from
Nov 16, 2018

Conversation

@deads2k
Copy link
Contributor

@deads2k deads2k commented Nov 13, 2018

accidentally closed #68895

This is that same pull with the test added.

Our client-go/client has a way to set a context deadline based on a timeout. However, the ListOptions timeoutSeconds didn't trigger this flow since the parameters were built differently. This updates the generated client to set the local timeout as well. This helps in cases where the server hangs unexpectedly or the network fails in some way that prevent the server from closing the request.

@kubernetes/sig-api-machinery-bugs
@sttts @mfojtik @knobunc @ironcladlou @caesarxuchao fyi

timeouts set in ListOptions for clients will also be respected locally

@k8s-ci-robot k8s-ci-robot added 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. kind/bug Categorizes issue or PR as related to a bug. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 13, 2018
@deads2k deads2k changed the title Client 07 listwatchtimeout update the client generator to set a client-side timeout Nov 13, 2018
@deads2k deads2k added this to the v1.13 milestone Nov 13, 2018
@k8s-ci-robot k8s-ci-robot added area/code-generation sig/auth Categorizes an issue or PR as relevant to SIG Auth. labels Nov 13, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Nov 13, 2018

/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 13, 2018
@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 13, 2018
@smarterclayton
Copy link
Contributor

smarterclayton commented Nov 13, 2018

Actually, why is its only for LIST, DELETE COLLECTION, and WATCH? Why not others?

@smarterclayton smarterclayton removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Nov 13, 2018

Actually, why is its only for LIST, DELETE COLLECTION, and WATCH? Why not others?

The others don't have timeouts set on their options structs. I'm not trying to change the API, just make what we have work properly

@deads2k deads2k force-pushed the client-07-listwatchtimeout branch from ee10ea0 to 0dc40dd Compare November 13, 2018 21:30
@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2018
@deads2k deads2k force-pushed the client-07-listwatchtimeout branch from 0dc40dd to 1e20b1d Compare November 13, 2018 22:12
@deads2k
Copy link
Contributor Author

deads2k commented Nov 13, 2018

/retest

@lavalamp
Copy link
Contributor

lavalamp commented Nov 14, 2018

This timeout sets the query parameter, but the PR description sounds like it sets the transport layer option--does it do both?

@liggitt
Copy link
Member

liggitt commented Nov 14, 2018

This timeout sets the query parameter, but the PR description sounds like it sets the transport layer option--does it do both?

It sets it on the Request, which sets both a query param:

kubernetes/staging/src/k8s.io/client-go/rest/request.go

Lines 448 to 451 in d00cb23

// timeout is handled specially here.
if r.timeout != 0 {
query.Set("timeout", r.timeout.String())
}

and a context deadline honored by the transport:

kubernetes/staging/src/k8s.io/client-go/rest/request.go

Lines 712 to 719 in d00cb23

if r.timeout > 0 {
if r.ctx == nil {
r.ctx = context.Background()
}
var cancelFn context.CancelFunc
r.ctx, cancelFn = context.WithTimeout(r.ctx, r.timeout)
defer cancelFn()
}

@deads2k
Copy link
Contributor Author

deads2k commented Nov 14, 2018

both failures on audit, something about resourceversion (not touched here)

/retest

@k8s-ci-robot k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Nov 14, 2018
@deads2k deads2k force-pushed the client-07-listwatchtimeout branch from 11f9056 to c2d1af1 Compare November 14, 2018 14:42
@deads2k deads2k force-pushed the client-07-listwatchtimeout branch 2 times, most recently from 49571a8 to ebfd494 Compare November 14, 2018 20:26
@deads2k
Copy link
Contributor Author

deads2k commented Nov 15, 2018

/retest

@smarterclayton
Copy link
Contributor

smarterclayton commented Nov 15, 2018

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 15, 2018
@fejta-bot
Copy link

fejta-bot commented Nov 16, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@deads2k deads2k force-pushed the client-07-listwatchtimeout branch from ebfd494 to 81240dc Compare November 16, 2018 13:39
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 16, 2018

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 16, 2018
@deads2k deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Nov 16, 2018

rebased and regenerated

@deads2k deads2k force-pushed the client-07-listwatchtimeout branch from 81240dc to cdb0f0e Compare November 16, 2018 14:38
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 16, 2018

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2018
@deads2k deads2k force-pushed the client-07-listwatchtimeout branch 2 times, most recently from a74eb4b to 2e7e8ca Compare November 16, 2018 17:40
@deads2k deads2k force-pushed the client-07-listwatchtimeout branch from 2e7e8ca to 8f7edec Compare November 16, 2018 17:41
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 16, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, smarterclayton

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@deads2k deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Nov 16, 2018

/retest

@AishSundar
Copy link
Contributor

AishSundar commented Nov 16, 2018

/priority critical-urgent
/remove-priority important-soon

Setting priority to critical-urgent to prevent it from getting kicked out of merge queue in case Tide doesnt get to it by Code freeze time.

@k8s-ci-robot k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Nov 16, 2018
@k8s-ci-robot k8s-ci-robot merged commit 9878253 into kubernetes:master Nov 16, 2018
@s-urbaniak s-urbaniak mentioned this pull request Nov 22, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@caesarxuchao caesarxuchao Awaiting requested review from caesarxuchao

@dims dims Awaiting requested review from dims

Assignees

@smarterclayton smarterclayton

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/code-generation 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/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. 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. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

v1.13

Development

Successfully merging this pull request may close these issues.

7 participants

@deads2k @smarterclayton @lavalamp @liggitt @fejta-bot @k8s-ci-robot @AishSundar