Be able to specify the timeout to wait for pod for kubectl logs/attach #41813

shiywang · 2017-02-21T16:20:39Z

Fixes #41786
current flag is get-pod-timeout, we can have a discussion if you have better one, default unit is seconds, above 0

@soltysh @Kargakis ptal, thanks
@kubernetes/sig-cli-feature-requests

k8s-ci-robot · 2017-02-21T16:20:40Z

Hi @shiywang. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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-reviewable · 2017-02-21T16:20:47Z

This change is

0xmichalis · 2017-02-21T16:32:34Z

@kubernetes/sig-cli-api-reviews

fabianofranz · 2017-02-21T20:51:32Z

I'd suggest --pod-running-timeout, --pod-wait-timeout, --first-pod-timeout, --pod-readiness-timeout.

fabianofranz · 2017-02-21T20:52:22Z

pkg/kubectl/cmd/attach.go

@@ -77,6 +78,7 @@ func NewCmdAttach(f cmdutil.Factory, cmdIn io.Reader, cmdOut, cmdErr io.Writer)
 		},
 	}
 	// TODO support UID


This TODO is about the ContainerName flag.

fabianofranz · 2017-02-21T20:55:02Z

pkg/kubectl/cmd/attach.go

@@ -128,6 +131,9 @@ func (p *AttachOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, argsIn [
 		return cmdutil.UsageError(cmd, fmt.Sprintf("expected fewer than three arguments: POD or TYPE/NAME or TYPE NAME, saw %d: %s", len(argsIn), argsIn))
 	}

+	if p.GetPodTimeOut == 0 {


Make it check for > 0 and the message something like "... must be higher than zero".

fabianofranz · 2017-02-21T20:56:56Z

pkg/kubectl/cmd/attach.go

@@ -77,6 +78,7 @@ func NewCmdAttach(f cmdutil.Factory, cmdIn io.Reader, cmdOut, cmdErr io.Writer)
 		},
 	}
 	// TODO support UID
+	cmd.Flags().Int64Var(&options.GetPodTimeOut, "get-pod-timeout", 60, "Specify the timeout (in seconds) of get first pod")


"The length of time (in seconds) to wait until at least one pod is running"

shiywang · 2017-02-22T03:19:53Z

flag change to first one, all comments updated

I'd suggest --pod-running-timeout, --pod-wait-timeout, --first-pod-timeout, --pod-readiness-timeout.

fabianofranz · 2017-02-22T18:17:56Z

@k8s-bot ok to test

fabianofranz · 2017-02-22T18:45:13Z

@k8s-bot bazel test this

shiywang · 2017-02-23T06:08:21Z

@fabianofranz @Kargakis @soltysh update some unit tests, PTAL, thanks

soltysh · 2017-02-23T20:39:11Z

pkg/kubectl/cmd/attach.go

@@ -76,7 +77,7 @@ func NewCmdAttach(f cmdutil.Factory, cmdIn io.Reader, cmdOut, cmdErr io.Writer)
 			cmdutil.CheckErr(options.Run())
 		},
 	}
-	// TODO support UID


I'm not sure this TODO is not valid anymore, I'd leave it here.

see comments above

fabianofranz 2 days ago Contributor
This TODO is about the ContainerName flag.

Ok, sorry for the noise.

soltysh · 2017-02-23T20:42:42Z

pkg/kubectl/cmd/attach.go

@@ -76,7 +77,7 @@ func NewCmdAttach(f cmdutil.Factory, cmdIn io.Reader, cmdOut, cmdErr io.Writer)
 			cmdutil.CheckErr(options.Run())
 		},
 	}
-	// TODO support UID
+	cmd.Flags().Int64Var(&options.GetPodTimeOut, "pod-running-timeout", 60, "The length of time (in seconds) to wait until at least one pod is running")


Turn this into a helper, see: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/util/helpers.go

soltysh · 2017-02-23T20:43:07Z

pkg/kubectl/cmd/attach.go

@@ -128,6 +130,9 @@ func (p *AttachOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, argsIn [
 		return cmdutil.UsageError(cmd, fmt.Sprintf("expected fewer than three arguments: POD or TYPE/NAME or TYPE NAME, saw %d: %s", len(argsIn), argsIn))
 	}

+	if p.GetPodTimeOut <= 0 {


Again, helper, which would save changing this, again see: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/util/helpers.go

ok, will do that

fabianofranz · 2017-03-08T19:32:47Z

pkg/kubectl/cmd/attach.go

@@ -133,6 +140,11 @@ func (p *AttachOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, argsIn [
 		return err
 	}

+	p.GetPodTimeOut, err = cmdutil.GetPodRunningTimoutFlag(cmd)


fabianofranz · 2017-03-08T19:34:28Z

pkg/kubectl/cmd/attach.go

-	Config    *restclient.Config
+	Attach        RemoteAttach
+	PodClient     coreclient.PodsGetter
+	GetPodTimeOut time.Duration


Make camel case concise with GetPodRunningTimeoutFlag, e.g. change it here to GetPodTimeout.

fabianofranz · 2017-03-08T19:35:21Z

pkg/kubectl/cmd/util/helpers.go

@@ -403,6 +411,10 @@ func AddDryRunFlag(cmd *cobra.Command) {
 	cmd.Flags().Bool("dry-run", false, "If true, only print the object that would be sent, without sending it.")
 }

+func AddPodRunningTimeOutFlag(cmd *cobra.Command, defaultTimeout time.Duration) {


Same camel case comment here.

fabianofranz · 2017-03-08T19:36:15Z

nits then it's good to go.

shiywang · 2017-03-11T08:07:11Z

@fabianofranz updated, ptal, thanks

fabianofranz · 2017-03-13T19:44:39Z

pkg/kubectl/cmd/logs.go

@@ -224,6 +225,11 @@ func (o LogsOptions) Validate() error {
 		return errs.ToAggregate()
 	}

+	var err error
+	o.GetPodTimeout, err = cmdutil.GetPodRunningTimoutFlag(cmd)


This fits better in Complete instead of here in Validate. In which case you'll not need to pass cmd to the Validate method.

fabianofranz · 2017-03-13T19:45:08Z

pkg/kubectl/cmd/logs.go

@@ -224,6 +225,11 @@ func (o LogsOptions) Validate() error {
 		return errs.ToAggregate()
 	}

+	var err error
+	o.GetPodTimeout, err = cmdutil.GetPodRunningTimoutFlag(cmd)
+	if err != nil {


Since this is the last thing you do you don't need this check, just return err.

fabianofranz · 2017-03-13T19:45:38Z

pkg/kubectl/cmd/util/helpers.go

@@ -387,6 +387,14 @@ func GetFlagDuration(cmd *cobra.Command, flag string) time.Duration {
 	return d
 }

+func GetPodRunningTimoutFlag(cmd *cobra.Command) (time.Duration, error) {


fabianofranz · 2017-03-13T19:46:05Z

pkg/kubectl/cmd/attach_test.go

+			p:           &AttachOptions{},
+			args:        []string{"pod/foo"},
+			expectedPod: "foo",
+			name:        "invaild get pod timeout value",


shiywang · 2017-03-14T15:50:43Z

@k8s-bot kops aws e2e test this

fabianofranz · 2017-03-14T17:33:52Z

/approve
/lgtm

k8s-github-robot · 2017-03-14T18:32:36Z

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: fabianofranz, shiywang, soltysh

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

~~hack/verify-flags/OWNERS~~ [fabianofranz]
~~pkg/kubectl/OWNERS~~ [fabianofranz]

We suggest the following people:
cc @brendandburns
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-03-25T02:04:25Z

Automatic merge from submit-queue (batch tested with PRs 43642, 43170, 41813, 42170, 41581)

k8s-github-robot · 2017-03-25T02:06:31Z

@shiywang PR needs rebase

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

k8s-github-robot assigned rootfs Feb 21, 2017

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

0xmichalis assigned fabianofranz and unassigned rootfs Feb 21, 2017

fabianofranz reviewed Feb 21, 2017

View reviewed changes

shiywang force-pushed the timeout_options branch from fe9beb3 to 5c9601c Compare February 22, 2017 02:57

shiywang force-pushed the timeout_options branch from 5c9601c to 08cb820 Compare February 23, 2017 02:30

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 23, 2017

shiywang force-pushed the timeout_options branch 2 times, most recently from 60bbeee to 53bd752 Compare February 23, 2017 05:00

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2017

soltysh requested changes Feb 23, 2017

View reviewed changes

fabianofranz reviewed Mar 8, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2017

shiywang force-pushed the timeout_options branch 2 times, most recently from d26dd44 to b6e4def Compare March 11, 2017 06:54

k8s-github-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 11, 2017

fabianofranz reviewed Mar 13, 2017

View reviewed changes

shiywang force-pushed the timeout_options branch 2 times, most recently from ae6a4fa to 6c63d86 Compare March 14, 2017 15:00

Be able to specify the timeout to wait for pod for kubectl logs/attach

52e4be2

shiywang force-pushed the timeout_options branch from 6c63d86 to 52e4be2 Compare March 14, 2017 15:00

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 14, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2017

k8s-github-robot merged commit 20b01be into kubernetes:master Mar 25, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2017

shiywang deleted the timeout_options branch March 25, 2017 12:12

0xmichalis mentioned this pull request Mar 27, 2017

Many commands do poorly when pods take ~30s to go to pending openshift/origin#9572

Closed

idvoretskyi mentioned this pull request Jun 30, 2017

Fix broken markdown format in v1.7 CHANGELOG #48328

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Be able to specify the timeout to wait for pod for kubectl logs/attach #41813

Be able to specify the timeout to wait for pod for kubectl logs/attach #41813

shiywang commented Feb 21, 2017 •

edited

Loading

k8s-ci-robot commented Feb 21, 2017

k8s-reviewable commented Feb 21, 2017

0xmichalis commented Feb 21, 2017

fabianofranz commented Feb 21, 2017

fabianofranz Feb 21, 2017

fabianofranz Feb 21, 2017

fabianofranz Feb 21, 2017

shiywang commented Feb 22, 2017

fabianofranz commented Feb 22, 2017

fabianofranz commented Feb 22, 2017

shiywang commented Feb 23, 2017 •

edited

Loading

soltysh Feb 23, 2017

shiywang Feb 24, 2017

soltysh Feb 24, 2017

soltysh Feb 23, 2017

soltysh Feb 23, 2017

shiywang Feb 24, 2017

fabianofranz Mar 8, 2017

fabianofranz Mar 8, 2017

fabianofranz Mar 8, 2017

fabianofranz commented Mar 8, 2017

shiywang commented Mar 11, 2017

fabianofranz Mar 13, 2017

fabianofranz Mar 13, 2017

fabianofranz Mar 13, 2017

fabianofranz Mar 13, 2017

shiywang commented Mar 14, 2017

fabianofranz commented Mar 14, 2017

k8s-github-robot commented Mar 14, 2017

k8s-github-robot commented Mar 25, 2017

k8s-github-robot commented Mar 25, 2017

Be able to specify the timeout to wait for pod for kubectl logs/attach #41813

Be able to specify the timeout to wait for pod for kubectl logs/attach #41813

Conversation

shiywang commented Feb 21, 2017 • edited Loading

k8s-ci-robot commented Feb 21, 2017

k8s-reviewable commented Feb 21, 2017

0xmichalis commented Feb 21, 2017

fabianofranz commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiywang commented Feb 22, 2017

fabianofranz commented Feb 22, 2017

fabianofranz commented Feb 22, 2017

shiywang commented Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianofranz commented Mar 8, 2017

shiywang commented Mar 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiywang commented Mar 14, 2017

fabianofranz commented Mar 14, 2017

k8s-github-robot commented Mar 14, 2017

k8s-github-robot commented Mar 25, 2017

k8s-github-robot commented Mar 25, 2017

shiywang commented Feb 21, 2017 •

edited

Loading

shiywang commented Feb 23, 2017 •

edited

Loading