-
Notifications
You must be signed in to change notification settings - Fork 39.8k
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
Conversation
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 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. |
@kubernetes/sig-cli-api-reviews |
I'd suggest |
pkg/kubectl/cmd/attach.go
Outdated
@@ -77,6 +78,7 @@ func NewCmdAttach(f cmdutil.Factory, cmdIn io.Reader, cmdOut, cmdErr io.Writer) | |||
}, | |||
} | |||
// TODO support UID |
There was a problem hiding this comment.
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.
pkg/kubectl/cmd/attach.go
Outdated
@@ -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 { |
There was a problem hiding this comment.
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".
pkg/kubectl/cmd/attach.go
Outdated
@@ -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") |
There was a problem hiding this comment.
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"
fe9beb3
to
5c9601c
Compare
flag change to first one, all comments updated
|
@k8s-bot ok to test |
@k8s-bot bazel test this |
5c9601c
to
08cb820
Compare
60bbeee
to
53bd752
Compare
@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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
pkg/kubectl/cmd/attach.go
Outdated
@@ -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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turn this into a helper, see: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/util/helpers.go
pkg/kubectl/cmd/attach.go
Outdated
@@ -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 { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, will do that
pkg/kubectl/cmd/attach.go
Outdated
@@ -133,6 +140,11 @@ func (p *AttachOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, argsIn [ | |||
return err | |||
} | |||
|
|||
p.GetPodTimeOut, err = cmdutil.GetPodRunningTimoutFlag(cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*Timeout
pkg/kubectl/cmd/attach.go
Outdated
Config *restclient.Config | ||
Attach RemoteAttach | ||
PodClient coreclient.PodsGetter | ||
GetPodTimeOut time.Duration |
There was a problem hiding this comment.
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
.
pkg/kubectl/cmd/util/helpers.go
Outdated
@@ -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) { |
There was a problem hiding this comment.
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.
nits then it's good to go. |
d26dd44
to
b6e4def
Compare
@fabianofranz updated, ptal, thanks |
pkg/kubectl/cmd/logs.go
Outdated
@@ -224,6 +225,11 @@ func (o LogsOptions) Validate() error { | |||
return errs.ToAggregate() | |||
} | |||
|
|||
var err error | |||
o.GetPodTimeout, err = cmdutil.GetPodRunningTimoutFlag(cmd) |
There was a problem hiding this comment.
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.
pkg/kubectl/cmd/logs.go
Outdated
@@ -224,6 +225,11 @@ func (o LogsOptions) Validate() error { | |||
return errs.ToAggregate() | |||
} | |||
|
|||
var err error | |||
o.GetPodTimeout, err = cmdutil.GetPodRunningTimoutFlag(cmd) | |||
if err != nil { |
There was a problem hiding this comment.
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
.
pkg/kubectl/cmd/util/helpers.go
Outdated
@@ -387,6 +387,14 @@ func GetFlagDuration(cmd *cobra.Command, flag string) time.Duration { | |||
return d | |||
} | |||
|
|||
func GetPodRunningTimoutFlag(cmd *cobra.Command) (time.Duration, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo
pkg/kubectl/cmd/attach_test.go
Outdated
p: &AttachOptions{}, | ||
args: []string{"pod/foo"}, | ||
expectedPod: "foo", | ||
name: "invaild get pod timeout value", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
ae6a4fa
to
6c63d86
Compare
6c63d86
to
52e4be2
Compare
@k8s-bot kops aws e2e test this |
/approve |
[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: |
Automatic merge from submit-queue (batch tested with PRs 43642, 43170, 41813, 42170, 41581) |
@shiywang PR needs rebase |
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