Skip to content
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.

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

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

Merged
merged 1 commit into from
Mar 25, 2017

Conversation

shiywang
Copy link
Contributor

@shiywang shiywang commented Feb 21, 2017

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
Copy link
Contributor

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-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 21, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot 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
Copy link
Contributor

@kubernetes/sig-cli-api-reviews

@0xmichalis 0xmichalis assigned fabianofranz and unassigned rootfs Feb 21, 2017
@0xmichalis 0xmichalis added release-note-none Denotes a PR that doesn't merit a release note. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 21, 2017
@fabianofranz
Copy link
Contributor

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TODO is about the ContainerName flag.

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@shiywang
Copy link
Contributor Author

flag change to first one, all comments updated

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

@fabianofranz
Copy link
Contributor

@k8s-bot ok to test

@fabianofranz
Copy link
Contributor

@k8s-bot bazel test this

@k8s-github-robot 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 shiywang force-pushed the timeout_options branch 2 times, most recently from 60bbeee to 53bd752 Compare February 23, 2017 05:00
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2017
@shiywang
Copy link
Contributor Author

shiywang commented Feb 23, 2017

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments above

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sorry for the noise.

@@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, will do that

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

p.GetPodTimeOut, err = cmdutil.GetPodRunningTimoutFlag(cmd)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*Timeout

Config *restclient.Config
Attach RemoteAttach
PodClient coreclient.PodsGetter
GetPodTimeOut time.Duration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same camel case comment here.

@fabianofranz
Copy link
Contributor

nits then it's good to go.

@k8s-github-robot 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 shiywang force-pushed the timeout_options branch 2 times, most recently from d26dd44 to b6e4def Compare March 11, 2017 06:54
@k8s-github-robot 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
@shiywang
Copy link
Contributor Author

@fabianofranz updated, ptal, thanks

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

var err error
o.GetPodTimeout, err = cmdutil.GetPodRunningTimoutFlag(cmd)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

func GetPodRunningTimoutFlag(cmd *cobra.Command) (time.Duration, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

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

@k8s-bot kops aws e2e test this

@fabianofranz
Copy link
Contributor

/approve
/lgtm

@k8s-ci-robot 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 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
Copy link

[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:

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
Copy link

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

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

@shiywang PR needs rebase

@k8s-github-robot 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 shiywang deleted the timeout_options branch March 25, 2017 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants