Add `request-timeout` val to `oc login` restclient #12062

juanvallejo · 2016-11-29T18:31:20Z

UPSTREAM: kubernetes/kubernetes#37708
Fixes: #12059

This patch adds a user-specified request-timeout flag value to the
restclient that is created by oc login. This allows oc login to
honor the --request-timeout flag.

$ oc login -u system:admin
Logged into "https://10.13.137.149:8443" as "system:admin" using
existing credentials.

You have access to the following projects and can switch between them
with 'oc project <projectname>':

  * default
  ...

$ oc login -u system:admin --request-timeout=1ms
Logged into "https://10.13.137.149:8443" as "system:admin" using
existing credentials.

Unable to connect to the server: net/http: request canceled
(Client.Timeout exceeded while awaiting headers)

cc @openshift/cli-review

ncdc · 2016-11-29T18:34:53Z

pkg/cmd/cli/cmd/login/loginoptions.go

@@ -99,6 +104,16 @@ func (o *LoginOptions) getClientConfig() (*restclient.Config, error) {

 	clientConfig := &restclient.Config{}

+	// set request timeout from --request-timeout option


Is there some way to avoid copying this block of code from k8s.io/kubernetes/pkg/client/unversioned/clientcmd/client_config.go?

deads2k · 2016-11-29T18:38:08Z

pkg/cmd/cli/cmd/login/loginoptions.go

@@ -99,6 +104,16 @@ func (o *LoginOptions) getClientConfig() (*restclient.Config, error) {

 	clientConfig := &restclient.Config{}

+	// set request timeout from --request-timeout option
+	timeoutValue := kcmdutil.GetFlagString(o.Cmd, "request-timeout")


Also, this will panic if you forget the flag on any caller

deads2k · 2016-11-29T18:38:37Z

pkg/cmd/cli/cmd/login/login.go

@@ -95,6 +95,8 @@ func NewCmdLogin(fullName string, f *osclientcmd.Factory, reader io.Reader, out
 }

 func (o *LoginOptions) Complete(f *osclientcmd.Factory, cmd *cobra.Command, args []string, commandName string) error {
+	o.Cmd = cmd


This worrysome. Why do we even have this value? Options objects shouldn't have any cobra dependencies.

deads2k · 2016-11-29T18:39:05Z

pkg/cmd/cli/cmd/login/loginoptions.go

@@ -66,6 +70,7 @@ type LoginOptions struct {
 	PathOptions *kclientcmd.PathOptions

 	CommandName string
+	Cmd         *cobra.Command


no cobra depedencies. Pass the values you want, not magic containers.

juanvallejo · 2016-11-29T19:25:32Z

@ncdc @deads2k Thanks for the review, will add upstream PR if new changes are okay

ncdc · 2016-11-29T19:26:55Z

@fabianofranz please weigh in on the severity of this issue (can we get the upstream changes in and then wait for the 1.6 rebase to fix this?)

juanvallejo · 2016-11-29T19:28:01Z

[test]

liggitt · 2016-11-29T20:00:52Z

pkg/cmd/cli/cmd/login/loginoptions.go

@@ -66,6 +67,7 @@ type LoginOptions struct {
 	PathOptions *kclientcmd.PathOptions

 	CommandName string
+	Timeout     string


pretty sure you need to honor this in dialToServer or it won't be effective

also, make this a time.Duration, not a string

and call it RequestTimeout to make it clear it is per-request, not for the whole operation

juanvallejo · 2016-11-29T22:59:41Z

@liggitt Thanks for the feedback, review comments addressed

fabianofranz · 2016-11-30T00:22:55Z

please weigh in on the severity of this issue (can we get the upstream changes in and then wait for the 1.6 rebase to fix this?)

Yes, can wait for the 1.6 rebase. I added the post-rebase label in the original issue and this PR, @juanvallejo please add the upstream PR link in a comment when opened. Tks!

juanvallejo · 2016-11-30T19:22:59Z

Related upstream PR: kubernetes/kubernetes#37708

juanvallejo · 2016-11-30T21:32:49Z

integration flaked on #9203
re[test]

juanvallejo · 2016-12-01T15:15:24Z

networking flake on #11452
conformance flake on #9203
re[test]

juanvallejo · 2016-12-01T17:17:08Z

conformance flaked on #9825 re[test]

juanvallejo · 2016-12-02T14:51:09Z

integration flake on #9379
re[test]

@fabianofranz

…nfig-helper-for-parsing-global-timeout Automatic merge from submit-queue (batch tested with PRs 37708, 34410) Add restclientconfig helper fn for parsing timeout Related downstream PR: openshift/origin#12062 (example of use-case for this patch) **Release note**: ```release-note release-note-none ``` This patch adds a package `pkg/client/unversioned/clientcmd/util` and defines a `ParseTimeout` helper function for parsing time from a user-defined string. This allows code re-use in other packages that require the creation of a new restclient (and therefore must set the `--global-timeout` flag value manually). @fabianofranz @kubernetes/cli-review

xingxingxia · 2017-05-23T07:23:48Z

Reminder if this PR is missed: now many 3.6 features are tested, hope this is merged/rebased when it is proper, thanks

This patch adds a user-specified `request-timeout` flag value to the restclient that is created by `oc login`. This allows `oc login` to honor the `--request-timeout` flag. ``` $ oc login -u system:admin Logged into "https://10.13.137.149:8443" as "system:admin" using existing credentials. You have access to the following projects and can switch between them with 'oc project <projectname>': * default ... $ oc login -u system:admin --request-timeout=1ms Logged into "https://10.13.137.149:8443" as "system:admin" using existing credentials. Unable to connect to the server: net/http: request canceled (Client.Timeout exceeded while awaiting headers) ```

openshift-bot · 2017-05-23T16:29:28Z

Evaluated for origin test up to a684fe8

openshift-bot · 2017-05-23T18:07:26Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1642/) (Base Commit: 48e5e40)

fabianofranz · 2017-05-26T20:37:47Z

[merge]

fabianofranz · 2017-05-26T20:40:33Z

Actually
[merge][severity: bug]

openshift-bot · 2017-05-26T20:45:44Z

Evaluated for origin merge up to a684fe8

openshift-bot · 2017-05-26T20:45:56Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/813/) (Base Commit: 97f1407) (Extended Tests: bug) (Image: devenv-rhel7_6277)

ncdc reviewed Nov 29, 2016

View reviewed changes

deads2k reviewed Nov 29, 2016

View reviewed changes

liggitt reviewed Nov 29, 2016

View reviewed changes

fabianofranz added component/cli kind/bug Categorizes issue or PR as related to a bug. kind/post-rebase labels Nov 30, 2016

juanvallejo mentioned this pull request Nov 30, 2016

Add restclientconfig helper fn for parsing timeout kubernetes/kubernetes#37708

Merged

juanvallejo force-pushed the jvallejo/add-request-timeout-val-to-oc-login-restclient branch from 75d769d to d5acf72 Compare November 30, 2016 19:25

juanvallejo force-pushed the jvallejo/add-request-timeout-val-to-oc-login-restclient branch 2 times, most recently from b691301 to a9480a4 Compare December 1, 2016 20:54

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 1, 2017

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 16, 2017

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 23, 2017

juanvallejo force-pushed the jvallejo/add-request-timeout-val-to-oc-login-restclient branch from a9480a4 to a684fe8 Compare May 23, 2017 16:26

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2017

juanvallejo requested a review from fabianofranz May 24, 2017 18:59

openshift-bot merged commit f703da4 into openshift:master May 26, 2017

juanvallejo deleted the jvallejo/add-request-timeout-val-to-oc-login-restclient branch May 31, 2017 20:00

juanvallejo mentioned this pull request Jan 19, 2018

oc login does not honor --request-timeout option #18159

Closed

		@@ -99,6 +104,16 @@ func (o LoginOptions) getClientConfig() (restclient.Config, error) {

		clientConfig := &restclient.Config{}

		// set request timeout from --request-timeout option

Add request-timeout val to oc login restclient #12062

Add request-timeout val to oc login restclient #12062

Uh oh!

Conversation

juanvallejo commented Nov 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ncdc Nov 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k Nov 29, 2016

Choose a reason for hiding this comment

Uh oh!

deads2k Nov 29, 2016

Choose a reason for hiding this comment

Uh oh!

deads2k Nov 29, 2016

Choose a reason for hiding this comment

Uh oh!

juanvallejo commented Nov 29, 2016

Uh oh!

ncdc commented Nov 29, 2016

Uh oh!

juanvallejo commented Nov 29, 2016

Uh oh!

liggitt Nov 29, 2016

Choose a reason for hiding this comment

Uh oh!

liggitt Nov 29, 2016

Choose a reason for hiding this comment

Uh oh!

liggitt Nov 29, 2016

Choose a reason for hiding this comment

Uh oh!

juanvallejo commented Nov 29, 2016

Uh oh!

fabianofranz commented Nov 30, 2016

Uh oh!

juanvallejo commented Nov 30, 2016

Uh oh!

juanvallejo commented Nov 30, 2016

Uh oh!

juanvallejo commented Dec 1, 2016

Uh oh!

juanvallejo commented Dec 1, 2016

Uh oh!

juanvallejo commented Dec 2, 2016

Uh oh!

xingxingxia commented May 23, 2017

Uh oh!

openshift-bot commented May 23, 2017

Uh oh!

openshift-bot commented May 23, 2017

Uh oh!

fabianofranz commented May 26, 2017

Uh oh!

fabianofranz commented May 26, 2017

Uh oh!

openshift-bot commented May 26, 2017

Uh oh!

openshift-bot commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Add `request-timeout` val to `oc login` restclient #12062

Add `request-timeout` val to `oc login` restclient #12062

juanvallejo commented Nov 29, 2016 •

edited

Loading

ncdc Nov 29, 2016 •

edited

Loading

openshift-bot commented May 26, 2017 •

edited

Loading