Allow passing request-timeout from NewRequest all the way down #51042

soltysh · 2017-08-21T19:17:07Z

What this PR does / why we need it:
Currently if you pass --request-timeout it's not passed all the way down to the actual request object. There's a separate field on the Request object that allows setting that timeout, but it's not taken from that flag.

@smarterclayton @deads2k ptal, this is coming from openshift/origin#13701

deads2k · 2017-08-23T13:59:11Z

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

@@ -130,6 +130,7 @@ func NewRequest(client HTTPClient, verb string, baseURL *url.URL, versionedAPIPa
 		serializers: serializers,
 		backoffMgr:  backoff,
 		throttle:    throttle,
+		timeout:     timeout,


Can you describe what you think this sets? It actually sets a query parameter on our request for the apiserver to consume, not a timeout on the client side. I doubt that's what most callers would expect.

That's exactly my intention with this change. The downstream problem (https://bugzilla.redhat.com/show_bug.cgi?id=1433244) was that --request-timeout wasn't passed to apiserver, which although I've allowed this functionality to be long-running request, is being cut off by the apiserver.

@deads2k the client-side timeout added by #33958 did not handles cases where the user requested a longer timeout, but the apiserver ended the request first. --request-timeout worked under the assumption that the client would be the one always terminating the connection

@deads2k the client-side timeout added by #33958 did not handles cases where the user requested a longer timeout, but the apiserver ended the request first. --request-timeout worked under the assumption that the client would be the one always terminating the connection

@juanvallejo more specifically. You think using the same value for both is a good idea?

From a user POV - I think the answer is yes. I'm not 100% sure about eventual downsides, though.

I'm fine with having both match if the CLI behavior matches expectations. @juanvallejo and @fabianofranz probably have more opinion there.

Is this set non-zero somewhere? The code you updated all seems to set zero

soltysh · 2017-08-29T13:04:14Z

/unassign @resouer @smarterclayton
/assign @deads2k

soltysh · 2017-10-11T13:26:44Z

@fabianofranz do you have any objections here, if not I'd like to rebase it and push it forward?

fabianofranz · 2017-10-18T20:46:39Z

@soltysh works for me.

deads2k · 2017-10-23T19:41:03Z

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

 	}
-	return NewRequest(c.Client, verb, c.base, c.versionedAPIPath, c.contentConfig, c.serializers, backoff, c.Throttle)
+	return NewRequest(c.Client, verb, c.base, c.versionedAPIPath, c.contentConfig, c.serializers, backoff, c.Throttle, 0)


This line is different in your patch in origin. Which is it supposed to be?

k8s-github-robot · 2017-11-22T23:35:52Z

This PR hasn't been active in 30 days. It will be closed in 59 days (Jan 21, 2018).

cc @deads2k @soltysh

You can add 'keep-open' label to prevent this from happening, or add a comment to keep it open another 90 days

soltysh · 2018-02-06T13:30:44Z

@juanvallejo @deads2k I've updated the PR ptal

deads2k · 2018-02-06T14:04:37Z

/lgtm

fejta-bot · 2018-02-06T16:22:31Z

/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.

fejta-bot · 2018-02-06T19:10:32Z

/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.

fejta-bot · 2018-02-06T21:58:31Z

/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.

fejta-bot · 2018-02-07T00:46:39Z

/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.

fejta-bot · 2018-02-07T03:34:39Z

/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.

fejta-bot · 2018-02-07T06:22:39Z

/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.

fejta-bot · 2018-02-07T09:10:41Z

/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.

fejta-bot · 2018-02-07T11:58:40Z

/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.

soltysh · 2018-02-07T13:24:23Z

/hold
failures are related

…ual request

k8s-ci-robot · 2018-02-07T15:02:54Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/kubectl/OWNERS~~ [deads2k]
~~staging/src/k8s.io/client-go/OWNERS~~ [deads2k]
~~test/OWNERS~~ [deads2k,soltysh]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

soltysh · 2018-02-07T15:04:44Z

/hold cancel
The tests should be fixed. I'm self applying the lgtm, since that only was updating tests with the new parameter that was added to NewRequest method.

fejta-bot · 2018-02-07T17:34:39Z

/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.

k8s-github-robot · 2018-02-07T18:43:36Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-ci-robot · 2018-02-07T19:12:09Z

@soltysh: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-bazel	`f881544`	link	`/test pull-kubernetes-bazel`
pull-kubernetes-bazel-test	`7da1002`	link	`/test pull-kubernetes-bazel-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-github-robot · 2018-02-07T20:00:42Z

Automatic merge from submit-queue (batch tested with PRs 59276, 51042, 58973, 59377, 59472). If you want to cherry-pick this change to another branch, please follow the instructions here.

soltysh added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 21, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 21, 2017

k8s-github-robot assigned resouer and smarterclayton Aug 21, 2017

k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 21, 2017

soltysh mentioned this pull request Aug 23, 2017

UPSTREAM: 51042: Allow passing request-timeout from NewRequest all the way down openshift/origin#13701

Merged

deads2k reviewed Aug 23, 2017

View reviewed changes

k8s-ci-robot assigned deads2k and unassigned smarterclayton and resouer Aug 29, 2017

deads2k reviewed Oct 23, 2017

View reviewed changes

soltysh force-pushed the request_timeout branch from f881544 to ce1c7bc Compare February 6, 2018 13:27

kubernetes deleted a comment from k8s-github-robot Feb 6, 2018

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 Feb 6, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 7, 2018

Allow passing request-timeout from NewRequest all the way down to act…

7da1002

…ual request

soltysh force-pushed the request_timeout branch from ce1c7bc to 7da1002 Compare February 7, 2018 15:02

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 7, 2018

soltysh added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2018

k8s-github-robot merged commit 1f62514 into kubernetes:master Feb 7, 2018

soltysh deleted the request_timeout branch February 7, 2018 20:03

shyamjvs mentioned this pull request Mar 5, 2018

[test flakes] master-scalability suites #60589

Closed

3 tasks

aojea mentioned this pull request Sep 14, 2021

CloseIdleConnections of wrapped Transport RoundTrippers #104844

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow passing request-timeout from NewRequest all the way down #51042

Allow passing request-timeout from NewRequest all the way down #51042

soltysh commented Aug 21, 2017

deads2k Aug 23, 2017

soltysh Aug 29, 2017

juanvallejo Oct 10, 2017

deads2k Oct 10, 2017

soltysh Oct 10, 2017

deads2k Oct 11, 2017

soltysh commented Aug 29, 2017

soltysh commented Oct 11, 2017

fabianofranz commented Oct 18, 2017

deads2k Oct 23, 2017

k8s-github-robot commented Nov 22, 2017

soltysh commented Feb 6, 2018

deads2k commented Feb 6, 2018

fejta-bot commented Feb 6, 2018

fejta-bot commented Feb 6, 2018

fejta-bot commented Feb 6, 2018

fejta-bot commented Feb 7, 2018

fejta-bot commented Feb 7, 2018

fejta-bot commented Feb 7, 2018

fejta-bot commented Feb 7, 2018

fejta-bot commented Feb 7, 2018

soltysh commented Feb 7, 2018

k8s-ci-robot commented Feb 7, 2018

soltysh commented Feb 7, 2018

fejta-bot commented Feb 7, 2018

k8s-github-robot commented Feb 7, 2018

k8s-ci-robot commented Feb 7, 2018 •

edited

k8s-github-robot commented Feb 7, 2018

Allow passing request-timeout from NewRequest all the way down #51042

Allow passing request-timeout from NewRequest all the way down #51042

Conversation

soltysh commented Aug 21, 2017

deads2k Aug 23, 2017

Choose a reason for hiding this comment

soltysh Aug 29, 2017

Choose a reason for hiding this comment

juanvallejo Oct 10, 2017

Choose a reason for hiding this comment

deads2k Oct 10, 2017

Choose a reason for hiding this comment

soltysh Oct 10, 2017

Choose a reason for hiding this comment

deads2k Oct 11, 2017

Choose a reason for hiding this comment

soltysh commented Aug 29, 2017

soltysh commented Oct 11, 2017

fabianofranz commented Oct 18, 2017

deads2k Oct 23, 2017

Choose a reason for hiding this comment

k8s-github-robot commented Nov 22, 2017

soltysh commented Feb 6, 2018

deads2k commented Feb 6, 2018

fejta-bot commented Feb 6, 2018

fejta-bot commented Feb 6, 2018

fejta-bot commented Feb 6, 2018

fejta-bot commented Feb 7, 2018

fejta-bot commented Feb 7, 2018

fejta-bot commented Feb 7, 2018

fejta-bot commented Feb 7, 2018

fejta-bot commented Feb 7, 2018

soltysh commented Feb 7, 2018

k8s-ci-robot commented Feb 7, 2018

soltysh commented Feb 7, 2018

fejta-bot commented Feb 7, 2018

k8s-github-robot commented Feb 7, 2018

k8s-ci-robot commented Feb 7, 2018 • edited

k8s-github-robot commented Feb 7, 2018

k8s-ci-robot commented Feb 7, 2018 •

edited