Add global timeout flag #33958

juanvallejo · 2016-10-03T19:16:32Z

Release note:

Add a new global option "--request-timeout" to the `kubectl` client

UPSTREAM: kubernetes/client-go#10

This patch adds a global timeout flag (viewable with kubectl -h) with
a default value of 0s (meaning no timeout).

The timeout value is added to the default http client, so that zero
values and default behavior are enforced by the client.

Adding a global timeout ensures that user-made scripts won't hang for an
indefinite amount of time while performing remote calls (right now, remote
calls are re-tried up to 10 times when each attempt fails, however, there is
no option to set a timeout in order to prevent any of these 10 attempts from
hanging indefinitely).

Example

$ kubectl get pods # no timeout flag set - default to 0s (which means no
timeout)
NAME                      READY     STATUS    RESTARTS   AGE
docker-registry-1-h7etw   1/1       Running   1          2h
router-1-uv0f9            1/1       Running   1          2h

$ kubectl get pods --request-timeout=0 # zero means no timeout no timeout flag set
NAME                      READY     STATUS    RESTARTS   AGE
docker-registry-1-h7etw   1/1       Running   1          2h
router-1-uv0f9            1/1       Running   1          2h

$kubectl get pods --request-timeout=1ms
Unable to connect to the server: net/http: request canceled while
waiting for connection (Client.Timeout exceeded while awaiting headers)

This change is

k8s-ci-robot · 2016-10-03T19:16:33Z

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot ok to test" on its own line.

Regular contributors should join the org to skip this step.

deads2k · 2016-10-03T19:18:57Z

pkg/kubectl/cmd/rollingupdate.go

@@ -91,7 +91,7 @@ func NewCmdRollingUpdate(f *cmdutil.Factory, out io.Writer) *cobra.Command {
 	}
 	cmd.Flags().Duration("update-period", updatePeriod, `Time to wait between updating pods. Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h".`)
 	cmd.Flags().Duration("poll-interval", pollInterval, `Time delay between polling for replication controller status after the update. Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h".`)
-	cmd.Flags().Duration("timeout", timeout, `Max time to wait for a replication controller to update before giving up. Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h".`)


You cannot change these flag names without breaking existing scripts.

reverted local flag names back to timeout. Updated global flag to --request-timeout 396551b

0xmichalis · 2016-10-05T15:44:43Z

cc: @kubernetes/kubectl

juanvallejo · 2016-10-05T18:26:04Z

@deads2k or @fabianofranz could you take a look please?

deads2k · 2016-10-05T19:34:57Z

pkg/client/unversioned/clientcmd/overrides.go

@@ -121,6 +138,7 @@ const (
 	FlagImpersonate  = "as"
 	FlagUsername     = "username"
 	FlagPassword     = "password"
+	FlagTimeout      = "request-timeout"


@kubernetes/kubectl Any comments on the flag name? --timeout was already take by commands that use reapers for a different meaning.

request-timeout sounds good to me.

deads2k · 2016-10-05T19:35:22Z

Squash and lgtm.

I'll leave it open a day for comments on the flag name.

deads2k · 2016-10-05T19:35:46Z

Oh, and test-cmd test. A kubectl get -w --request-timeout ought to return, right?

juanvallejo · 2016-10-05T19:53:51Z

@deads2k

A kubectl get -w --request-timeout ought to return, right?

Yes, when using the default 0 value, it behaves as usual (watches a resource until a user terminates the process).

When used with a non-zero request-timeout value, it does timeout after the time specified:

$ oc get pod zookeeper-1 -w --request-timeout=10s
NAME          READY     STATUS    RESTARTS   AGE
zookeeper-1   1/1       Running   2          6h

After 10s, the complete output from the command above is:

NAME          READY     STATUS    RESTARTS   AGE
zookeeper-1   1/1       Running   2          6h
E1005 15:50:25.125415   11319 streamwatcher.go:109] Unable to decode an event from the watch stream: net/http: request canceled (Client.Timeout exceeded while reading body)

deads2k · 2016-10-05T20:11:54Z

Yes, when using the default 0 value, it behaves as usual (watches a resource until a user terminates the process).

Right, just pointed that out for your test-cmd.sh test.

adohe-zz · 2016-10-06T00:11:40Z

@juanvallejo any priority between local timeout flag and global flag?

soltysh · 2016-10-06T09:20:17Z

@juanvallejo any priority between local timeout flag and global flag?

I guess, since request-timeout is applied at transport layer it'll have priority over local timeout, which is usually applied to a bigger chunk of operation (ie. for reapers it's applied while waiting for scale down operation).

The changes LGTM, but since this is a new feature, release not is needed @juanvallejo,

juanvallejo · 2016-10-06T14:13:57Z

Thanks for the feedback, release note added!

soltysh · 2016-10-06T16:34:46Z

If there are no objections I'm gonna tag it for merge later today.

soltysh · 2016-10-06T19:30:35Z

@k8s-bot ok to test

soltysh · 2016-10-06T19:48:18Z

Flake #32430

@k8s-bot node e2e test this

fabianofranz · 2016-10-06T20:13:30Z

pkg/client/unversioned/clientcmd/overrides.go

@@ -105,6 +108,20 @@ func (f FlagInfo) BindBoolFlag(flags *pflag.FlagSet, target *bool) {
 	}
 }

+// BindDurationFlag binds the flag based on the provided info.  If LongName == "", nothing is registered
+func (f FlagInfo) BindDurationFlag(flags *pflag.FlagSet, target *time.Duration) {


I still believe --request-timeout should support values without units, which defaults to seconds like every other timeout flag out there. Like in ssh, timeout, scp, curl, and so on.

You'd probably have to use string and parse it by yourself like in here.

Like --request-timeout=30 is the first thing I'll try.

Done! Thanks for providing the example

fabianofranz · 2016-10-07T00:23:13Z

LGTM

soltysh · 2016-10-07T12:40:49Z

@juanvallejo can you look into those test failures and link to flakes?

caesarxuchao · 2016-10-15T05:25:51Z

LGTM. Thanks!

k8s-github-robot · 2016-10-15T05:49:39Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-10-15T06:29:58Z

Automatic merge from submit-queue

googlebot added the cla: yes label Oct 3, 2016

juanvallejo mentioned this pull request Oct 3, 2016

UPSTREAM: 33958: add global timeout flag openshift/origin#11104

Merged

2 tasks

k8s-github-robot assigned deads2k Oct 3, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Oct 3, 2016

deads2k reviewed Oct 3, 2016

View reviewed changes

deads2k reviewed Oct 5, 2016

View reviewed changes

juanvallejo force-pushed the jvallejo/add-global-timeout-flag branch from 396551b to 5933816 Compare October 5, 2016 20:27

juanvallejo force-pushed the jvallejo/add-global-timeout-flag branch from 5933816 to 778e57e Compare October 6, 2016 14:17

soltysh added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Oct 6, 2016

fabianofranz suggested changes Oct 6, 2016

View reviewed changes

juanvallejo force-pushed the jvallejo/add-global-timeout-flag branch from 778e57e to 1742dc5 Compare October 6, 2016 21:50

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

juanvallejo force-pushed the jvallejo/add-global-timeout-flag branch from 1742dc5 to 10571e3 Compare October 7, 2016 17:20

caesarxuchao self-assigned this Oct 15, 2016

caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 15, 2016

k8s-github-robot merged commit c0bd6e8 into kubernetes:master Oct 15, 2016

juanvallejo deleted the jvallejo/add-global-timeout-flag branch October 17, 2016 13:48

caesarxuchao mentioned this pull request Oct 17, 2016

remove 1.4 folder from 1.5 branch kubernetes/client-go#14

Merged

juanvallejo mentioned this pull request Oct 10, 2017

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

Merged

This was referenced Jun 6, 2021

fix(deps): update module k8s.io/client-go to v12 - autoclosed IPA-CyberLab/kmgm-issuer#24

Closed

Update module k8s.io/client-go to v12 utopia-planitia/k8s-gitlab-gc#7

Closed

This was referenced Jun 14, 2021

Update module k8s.io/client-go to v12 kedacore/keda#1891

Closed

fix(deps): update module k8s.io/client-go to v12 - autoclosed mmmake-gmbh/cert-manager-webhook-project-pinto#9

Closed

renovate bot mentioned this pull request Jul 16, 2021

Update module k8s.io/client-go to v12 angelnu/gateway-admision-controller#7

Closed

1 task

This was referenced Jul 29, 2021

fix(deps): update module k8s.io/client-go to v12 utkuozdemir/pv-migrate#66

Closed

Update module k8s.io/client-go to v12 - autoclosed vshn/statefulset-resize-controller#10

Closed

This was referenced Aug 11, 2021

Update module k8s.io/client-go to v1 - autoclosed giantswarm/cluster-apps-operator#67

Closed

Update module k8s.io/client-go to v1 - autoclosed giantswarm/app-operator#843

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add global timeout flag #33958

Add global timeout flag #33958

juanvallejo commented Oct 3, 2016 •

edited

Loading

k8s-ci-robot commented Oct 3, 2016

deads2k Oct 3, 2016

juanvallejo Oct 3, 2016

0xmichalis commented Oct 5, 2016

juanvallejo commented Oct 5, 2016

deads2k Oct 5, 2016

adohe-zz Oct 6, 2016

deads2k commented Oct 5, 2016

deads2k commented Oct 5, 2016

juanvallejo commented Oct 5, 2016

deads2k commented Oct 5, 2016

adohe-zz commented Oct 6, 2016

soltysh commented Oct 6, 2016

juanvallejo commented Oct 6, 2016

soltysh commented Oct 6, 2016

soltysh commented Oct 6, 2016

soltysh commented Oct 6, 2016

fabianofranz Oct 6, 2016 •

edited

Loading

fabianofranz Oct 6, 2016

juanvallejo Oct 6, 2016

fabianofranz commented Oct 7, 2016

soltysh commented Oct 7, 2016

caesarxuchao commented Oct 15, 2016

k8s-github-robot commented Oct 15, 2016

k8s-github-robot commented Oct 15, 2016

Add global timeout flag #33958

Add global timeout flag #33958

Conversation

juanvallejo commented Oct 3, 2016 • edited Loading

k8s-ci-robot commented Oct 3, 2016

deads2k Oct 3, 2016

Choose a reason for hiding this comment

juanvallejo Oct 3, 2016

Choose a reason for hiding this comment

0xmichalis commented Oct 5, 2016

juanvallejo commented Oct 5, 2016

deads2k Oct 5, 2016

Choose a reason for hiding this comment

adohe-zz Oct 6, 2016

Choose a reason for hiding this comment

deads2k commented Oct 5, 2016

deads2k commented Oct 5, 2016

juanvallejo commented Oct 5, 2016

deads2k commented Oct 5, 2016

adohe-zz commented Oct 6, 2016

soltysh commented Oct 6, 2016

juanvallejo commented Oct 6, 2016

soltysh commented Oct 6, 2016

soltysh commented Oct 6, 2016

soltysh commented Oct 6, 2016

fabianofranz Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

fabianofranz Oct 6, 2016

Choose a reason for hiding this comment

juanvallejo Oct 6, 2016

Choose a reason for hiding this comment

fabianofranz commented Oct 7, 2016

soltysh commented Oct 7, 2016

caesarxuchao commented Oct 15, 2016

k8s-github-robot commented Oct 15, 2016

k8s-github-robot commented Oct 15, 2016

juanvallejo commented Oct 3, 2016 •

edited

Loading

fabianofranz Oct 6, 2016 •

edited

Loading