UPSTREAM: 33958: add global timeout flag #11104
Conversation
| @@ -95,7 +94,6 @@ 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".`) | |||
There was a problem hiding this comment.
@fabianofranz I am unsure how we would handle cases where timeout has a specific description (or default value) for a command. For example, newtoken.go has a timeout of 30s.
juanvallejo
force-pushed
the
jvallejo/add-global-timeout-flag
branch
3 times, most recently
from
4f6c234 to
c0b117b
Compare
juanvallejo
force-pushed
the
jvallejo/add-global-timeout-flag
branch
from
c0b117b to
3014209
Compare
Okay, this makes sense, I'll set the starting value at I was hoping I could get your feedback on how to approach the actual implementation of timing out requests. I'm looking at k8s.io/kubernetes/pkg/client/restclient/request.go#L797 where the current implementation does not currently have a time limit per request, but rather a fixed number of max retries (10 at the moment) and a small timeout in between each, set by the |
|
what does the timeout apply to? dialing? time to first byte? time without data being read? total time? |
I was originally thinking of it applying to total time. Would it make more sense for it to apply to something else? Maybe the timeout applying to dialing would work better, then once it successfully connects to the server, the timeout "timer" would no longer be in effect. |
juanvallejo
force-pushed
the
jvallejo/add-global-timeout-flag
branch
from
3014209 to
550e4f9
Compare
| r.backoffMgr.Sleep(time.Duration(seconds) * time.Second) | ||
| return false | ||
| // ignore timeout if timeout value is zero | ||
| if r.timeout == 0 { |
There was a problem hiding this comment.
PTAL at this implementation, it works as expected, however maybe there is a cleaner way to "time out" requests?
|
This timeout applies to total time, the same way the higher-level About defaulting, in case a timeout was not explicitly provided then you just don't set it (not even to |
|
Also make sure the timeouts work correctly when you are under a proxy with |
|
I'm really unsure about this... many command line commands are made up of multiple API requests, but this is globally hooking the timeout to the client |
|
Based on the implementation, the timeout is essentially from when the user hits |
|
Another idea would be to remove the |
juanvallejo
force-pushed
the
jvallejo/add-global-timeout-flag
branch
3 times, most recently
from
a1ae8f0 to
b9030fe
Compare
|
Current working example: |
| @@ -185,6 +189,9 @@ func RESTClientFor(config *Config) (*RESTClient, error) { | |||
| var httpClient *http.Client | |||
| if transport != http.DefaultTransport { | |||
| httpClient = &http.Client{Transport: transport} | |||
| if config.Timeout != 0 { | |||
There was a problem hiding this comment.
Why do you allow negatives?
There was a problem hiding this comment.
Updated this here: 4a7769f
| @@ -89,7 +89,6 @@ func NewCmdScale(f *cmdutil.Factory, out io.Writer) *cobra.Command { | |||
| cmd.Flags().Int("current-replicas", -1, "Precondition for current size. Requires that the current size of the resource match this value in order to scale.") | |||
| cmd.Flags().Int("replicas", -1, "The new desired number of replicas. Required.") | |||
| cmd.MarkFlagRequired("replicas") | |||
| cmd.Flags().Duration("timeout", 0, "The length of time to wait before giving up on a scale operation, zero means don't wait. Any other values should contain a corresponding time unit (e.g. 1s, 2m, 3h).") | |||
There was a problem hiding this comment.
This timeout means something different.
There was a problem hiding this comment.
Addressed in faf8b4a
| @@ -110,9 +110,9 @@ func NewCmdDelete(f *cmdutil.Factory, out io.Writer) *cobra.Command { | |||
| cmd.Flags().Bool("cascade", true, "If true, cascade the deletion of the resources managed by this resource (e.g. Pods created by a ReplicationController). Default true.") | |||
| cmd.Flags().Int("grace-period", -1, "Period of time in seconds given to the resource to terminate gracefully. Ignored if negative.") | |||
| cmd.Flags().Bool("now", false, "If true, resources are force terminated without graceful deletion (same as --grace-period=0).") | |||
| cmd.Flags().Duration("timeout", 0, "The length of time to wait before giving up on a delete, zero means determine a timeout from the size of the object") | |||
There was a problem hiding this comment.
This timeout means something different
There was a problem hiding this comment.
would it be okay to rename these timeouts to something like --delete-timeout and --scale-timeout, or would it make more sense to rename the global timeout to --global-timeout?
juanvallejo
changed the title
Renaming flags is a pretty big deal for scripts. That's why we have @openshift/api-review take a look at new flags. |
juanvallejo
changed the title
Thanks for the feedback, I have updated the global flag's name to |
juanvallejo
force-pushed
the
jvallejo/add-global-timeout-flag
branch
2 times, most recently
from
825b616 to
72c9b2a
Compare
juanvallejo
force-pushed
the
jvallejo/add-global-timeout-flag
branch
3 times, most recently
from
f731e36 to
4d84495
Compare
|
check, integration, conformance tests flaked on #11292 re[test] |
|
check, integration, conformance tests flaked on #11292 re[test] |
|
integration test flaked on #11240 re[test] |
|
integration flaked on #11240 re[test] |
|
#8571 re[test] |
juanvallejo
force-pushed
the
jvallejo/add-global-timeout-flag
branch
2 times, most recently
from
1b779cf to
19b76ba
Compare
|
#8571 re[test] |
|
integration flaked on #11292 re[test] |
|
@fabianofranz upstream merged! kubernetes/kubernetes#33958 PTAL |
|
LGTM. Hold on to your seats, [merge]. |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10202/) (Image: devenv-rhel7_5206) |
Related Trello card: https://trello.com/c/5YkspaNk/711-5-oc-global-timeout-ux-p4 This patch adds a global timeout flag (viewable with `oc options`) with a default value of `10s`. It removes all local `timeout` flags previously declared in specific commands, such as `oc delete` and `oc scale`.
|
@juanvallejo failed on outdated completions, please update and I'll tag again. |
juanvallejo
force-pushed
the
jvallejo/add-global-timeout-flag
branch
from
19b76ba to
ec6ef5f
Compare
|
@fabianofranz Done! rebased with latest master and updated docs / completions |
|
[merge] |
|
Evaluated for origin merge up to ec6ef5f |
|
Evaluated for origin test up to ec6ef5f |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10202/) (Base Commit: a941850) |
UPSTREAM: kubernetes/kubernetes#33958
Related Trello card:
https://trello.com/c/5YkspaNk/711-5-oc-global-timeout-ux-p4
Related Bugzilla RFE: https://bugzilla.redhat.com/show_bug.cgi?id=1358393
This patch adds a global timeout flag (viewable with
oc options) witha default value of
0s(meaning no timeout). It renames all localtimeoutflagspreviously declared in specific commands, such as
oc deleteandoc scale, to avoid conflict with the global timeout flag.TODO
To Resolve
--timeoutflag with custom values / descriptions.Example
cc @openshift/cli-review @smarterclayton @jwforres @deads2k @enj