[WIP] add stream-timeout option to kubectl exec

[juanvallejo]

Release note:

adds a --stream-timeout option to "kubectl exec" in order to timeout once inside of a remote shell

This patch implements a timeout option for remote streams. This allows a
maximum time to be specified for a remote shell to remain open. If a
--stream-timeout value > 0 is provided, the stream executor will not
wait for the stdout and stderr streams of a remote shell to finish
copying, but will rather close the stream and exit with error Timeout exceeded for this operation.

Although kubectl exec already supports the global
request-timeout option as well as the local --timeout option, the
former is only for making ther inital request with a restclient, and
the latter is the amount of time to wait before a pod is retrieved.
Therefore, it was necessary to introduce a third timeout option for
specifically timing out the command once inside of a remote shell.

Example

$ kubectl exec -- /bin/sh -c "while true; do echo test; sleep 1s; done"
Defaulting container name to mypodcontainer.
Use 'kubectl describe pod/mypod' to see all of the containers in this pod.
test
test
test
...
^C

$ kubectl exec --stream-timeout=5s mypod -- /bin/sh -c "while true; do echo test; sleep 1s; done"
Defaulting container name to mypodcontainer.
Use 'kubectl describe pod/mypod' to see all of the containers in this pod.
test
test
test
test
test
error: Timeout exceeded for this operation.

@kubernetes/cli-review @fabianofranz

[k8s-ci-robot]

Hi @juanvallejo. 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.

If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-reviewable]

This change is 

[juanvallejo]

@smarterclayton or @fabianofranz would it make more sense from a user's perspective to just re-use the value from the global --request-timeout flag, or is it worth adding a third timeout option?

[fabianofranz]

The global --request-timeout makes sense to me.

[juanvallejo]

Related bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1314240

[smarterclayton]

I'm not sure I would call this stream-timeout in general. Is this an idle timeout, or a duration timeout? request-timeout is duration.

[juanvallejo]

@smarterclayton per this implementation at least, it is a duration timeout. Regardless of user input, it will exit after the specified time

[juanvallejo]

@smarterclayton @fabianofranz
The second commit 40f6ad4 turns this patch into an "idle timeout". This means that instead of the remote shell closing on a user after a specified timeout value x, it will only only close on that user after x seconds/units of inactivity.

Inactivity is defined as no bytes being copied from the remoteStdout reader to Stdout writer.
Example with oc rsh (also affected by this patch)

$ oc rsh --stream-timeout=5s idling-echo-5-972o2
sh-4.2$ while true; do echo test; sleep 1; done
test
test
test
...

The above example will keep running indefinitely, as there is going to be output being copied from remoteStdout to Stdout until the infinite loop is broken.

$ oc rsh --stream-timeout=5s idling-echo-5-972o2
sh-4.2$
# 5 seconds later
error: Timeout exceeded for this operation.

Since the remote shell was inactive for 5s, the timeout actually occurs.

$ oc rsh --stream-timeout=5s idling-echo-5-972o2
sh-4.2$
# 4 seconds later
sh-4.2$ ls
# timeout counter reset back to 5s

Because there was data to be copied (ls) a second before the specified timeout, there will have to be inactivity for another 5s before the timeout actually occurs.

Example with kubectl exec

$ kubectl exec --stream-timeout=1ms idling-echo-5-972o2 -- /bin/sh -c "echo test"
Defaulting container name to idling-tcp-echo.
Use 'kubectl describe pod/idling-echo-5-972o2' to see all of the containers in this pod.
error: Timeout exceeded for this operation.

kubectl exec --stream-timeout=5s idling-echo-5-972o2 -- /bin/sh -c "while true; do echo test; sleep 1s; done"
Defaulting container name to idling-tcp-echo.
Use 'kubectl describe pod/idling-echo-5-972o2' to see all of the containers in this pod.
test
test
test
test
test
test
...

The example above will also keep running indefinitely, as there will always be data to copy from remoteStdout to Stdout as long as the infinite loop continues running. This would be particularly useful for users who wish a specific task to keep running, but have the entire operation timeout if the task begins to hang.

Hopefully having a timeout on idle, rather than an overall duration timeout will provide a better user experience. I can re-use the value from --request-timeout instead of adding a new flag if that is still appropriate.

[smarterclayton]

Why would I want a duration timeout on exec?

[ncdc]

We already support idle timeouts for streaming operations (exec, attach, port forwarding), but the value is only configurable by the cluster administrator. If we decide we want the user to have some amount of control, I would prefer that we pass the user-supplied value through to the kubelet and then use min(user supplied value, admin supplied value). I don't think we need to add any code to implement idle timeout client-side.

[juanvallejo]

@smarterclayton

Why would I want a duration timeout on exec?

I convert it to an idle timeout here: 40f6ad4 (#38642 (comment))

@ncdc

We already support idle timeouts for streaming operations (exec, attach, port forwarding), but the value is only configurable by the cluster administrator. If we decide we want the user to have some amount of control, I would prefer that we pass the user-supplied value through to the kubelet and then use min(user supplied value, admin supplied value). I don't think we need to add any code to implement idle timeout client-side.

Sounds good, I could turn this PR into something that updates this. Would the value that is configurable by the cluster admin be handled in pkg/kubelet/server/remotecommand?

[ncdc]

The cluster admin configuration option is called StreamingConnectionIdleTimeout

[juanvallejo]

@ncdc

The cluster admin configuration option is called StreamingConnectionIdleTimeout

Thanks, I'll put something together

[juanvallejo]

@ncdc

If we decide we want the user to have some amount of control, I would prefer that we pass the user-supplied value through to the kubelet and then use min(user supplied value, admin supplied value)

Based on the original bugzilla for the issue addressed by this PR, the user requested a longer idle timeout, which means that this will still not be solved if we always choose the min value between user value and cluster config value. An option could be to still go ahead and implement this, but always have the user's value override the cluster config value. wdyt?

cc @fabianofranz

[ncdc]

I believe the report in the bz says that the rsh session died after 1 to 2 minutes. The default StreamingConnectionIdleTimeout value is 4 hours. It sounds like something else is going on with that user's environment.

I do NOT want to reimplement idle timeouts. I would be open to a discussion about whether or not the user should be allowed to override the cluster admin's setting, although my vote is "no".

[juanvallejo]

@ncdc @smarterclayton
closing this pr as the feature request in the bugzilla already exists in the cluster config as StreamingConnectionIdleTimeout and the root of the bugzilla issue is not directly caused by this option not existing in the client side.