Various exec fixes #13322

ncdc · 2015-08-28T19:58:11Z

If stdin is noninteractive, the io.Copy from stdin to remoteStdin will
unblock when it finishes reading from stdin. In this case, make sure to
close remoteStdin so the server knows the client won't be sending any
more data. This ensures that the remote process terminates. For example:

echo foo | kubectl exec -i <pod> -- cat

Without this change, the cat process never terminates and kubectl exec hangs.

Fix interactive exec sessions hanging after you type 'exit'.

Add e2e test to cover noninteractive stdin: echo a | kubectl exec -i <pod> cat

Add e2e test to cover psuedo-interactive stdin: kubectl exec -i <pod> bash

Prep for sending multiple data frames over multiple streams in remote command
test, which is more likely to find flakes (requires bump of spdystream
once an issue with the frame worker queues not being fully drained when
a goaway frame is received).

Fixes #13394
Fixes #13395

Release note info
Fixed hanging when using non-interactive stdin data, e.g. echo foo | kubectl exec -i <pod> -- cat
Fixed interactive kubectl exec sessions hanging after you type 'exit'

ncdc · 2015-08-28T19:58:29Z

@smarterclayton @pmorie @liggitt

liggitt · 2015-08-28T20:01:34Z

test/e2e/kubectl.go

+			By("executing a command in the container with noninteractive stdin")
+			execOutput = newKubectlCommand("exec", fmt.Sprintf("--namespace=%v", ns), "-i", simplePodName, "cat").
+				withStdinData("abcd1234").
+				exec()


+1 for tests for this scenario

actually, it would be easy to add a withStdin(io.Reader) helper so we could also test an interactive exec with a stdin that sends "echo hello\nexit\n", then blocks

Good idea. I'll definitely want to add that. Once I figure out why interactive is broken...

Blocks via select {} or something else?

Yeah, something like https://gist.github.com/liggitt/29077b2835004caab5c3

liggitt · 2015-08-28T20:18:42Z

pkg/client/unversioned/remotecommand/remotecommand.go

@@ -194,12 +194,19 @@ func (e *Streamer) doStream() error {
 		if err != nil {
 			return err
 		}
-		defer remoteStdin.Reset()


don't you still need the deferred remoteStdin.Reset() in the case where the remoteStdin cp never completes, or does the conn.Close() cover that?

I'll check and see, but it's broken until #13324 lands. I can test w/that locally.

actually, I'm surprised the remote connection was being held open with the deferred Reset() in place previously

It's a bit of a chicken and egg game. With this scenario:

echo foo | kubectl exec -i <pod> cat

The cat command won't terminate until it gets EOF from its stdin. Its stdin comes from the spdy stream. The spdy stream gets its stdin from the non interactive input (echo foo). While foo is fully read and closed, doStream won't exit until cat is done, but we aren't resetting remoteStdin until doStream exits, etc etc etc. This makes sure that remoteStdin gets closed/reset as soon as the copy unblocks. Make sense?

interesting, the cp essentially "intercepts" the EOF, so the remote command never sees it.

Hmm, assuming I merged and rebuilt everything locally correctly, trying to exit an interactive shell is hanging. Grrr....

the deferred remoteStdin.Close() in the goroutine makes sense, but I would keep the deferred Reset() in place

Ok, added it back.

k8s-bot · 2015-08-28T20:21:31Z

GCE e2e build/test passed for commit 969e3dc30b424732e00ff500e045a02d24f1ca9c.

k8s-bot · 2015-08-28T21:41:49Z

GCE e2e build/test failed for commit 51681c18fd7e08d6b27dc848ca97b87a9d7a28fa.

k8s-github-robot · 2015-09-02T00:01:50Z

Labelling this PR as size/M

ncdc · 2015-09-02T16:25:01Z

@liggitt @smarterclayton PR updated. I think I've finally gotten there...

k8s-github-robot · 2015-09-02T16:26:46Z

Labelling this PR as size/L

liggitt · 2015-09-02T16:37:03Z

LGTM... the additional e2e tests give me a lot more confidence

k8s-bot · 2015-09-02T16:37:05Z

GCE e2e build/test passed for commit bfab798919aa7db3b0603be882dc077cb0d4f47c.

ncdc · 2015-09-02T16:37:40Z

And e2e passed, woot!

ncdc · 2015-09-02T16:39:31Z

... oh, for a previous commit. Guess we'll wait a bit longer

k8s-bot · 2015-09-02T16:50:28Z

GCE e2e build/test passed for commit 57d4df45975382f754102f5e194e787b2c0c06da.

k8s-bot · 2015-09-02T20:09:53Z

GCE e2e build/test failed for commit addece6f95d290f0d62f32591db6a73b683d0dc1.

ncdc · 2015-09-02T20:13:31Z

e2e failure looks like flakes:

[Fail] Services [It] should serve multiport endpoints from pods 
/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/service.go:1147

[Fail] Services [It] should serve a basic endpoint from pods 
/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/service.go:1147

[Fail] Services [It] should be able to up and down services 
/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/service.go:247

@k8s-bot test this please

k8s-bot · 2015-09-02T20:42:31Z

GCE e2e build/test passed for commit addece6f95d290f0d62f32591db6a73b683d0dc1.

If stdin is noninteractive, the io.Copy from stdin to remoteStdin will unblock when it finishes reading from stdin. In this case, make sure to close remoteStdin so the server knows the client won't be sending any more data. This ensures that the remote process terminates. For example: echo foo | kubectl exec -i <pod> -- cat Without this change, the `cat` process never terminates and `kubectl exec` hangs. Fix interactive exec sessions hanging after you type 'exit'. Add e2e test to cover noninteractive stdin: `echo a | kubectl exec -i <pod> cat` Add e2e test to cover psuedo-interactive stdin: `kubectl exec -i <pod> bash` Prep for sending multiple data frames over multiple streams in remote command test, which is more likely to find flakes (requires bump of spdystream once an issue with the frame worker queues not being fully drained when a goaway frame is received).

k8s-bot · 2015-09-04T14:45:56Z

GCE e2e build/test failed for commit c837869.

ncdc · 2015-09-04T14:48:12Z

e2e flake

Fetching upstream changes from https://github.com/kubernetes/kubernetes
 > git --version # timeout=10
 > git -c core.askpass=true fetch --tags --progress https://github.com/kubernetes/kubernetes +refs/pull/*:refs/remotes/origin/pr/* --prune
ERROR: Error fetching remote repo 'origin'
hudson.plugins.git.GitException: Failed to fetch from https://github.com/kubernetes/kubernetes
    at hudson.plugins.git.GitSCM.fetchFrom(GitSCM.java:763)
    at hudson.plugins.git.GitSCM.retrieveChanges(GitSCM.java:1012)
    at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1043)
    at hudson.scm.SCM.checkout(SCM.java:485)
    at hudson.model.AbstractProject.checkout(AbstractProject.java:1277)
    at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:610)
    at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
    at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:532)
    at hudson.model.Run.execute(Run.java:1741)
    at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
    at hudson.model.ResourceController.execute(ResourceController.java:98)
    at hudson.model.Executor.run(Executor.java:381)
Caused by: hudson.plugins.git.GitException: Command "git -c core.askpass=true fetch --tags --progress https://github.com/kubernetes/kubernetes +refs/pull/*:refs/remotes/origin/pr/* --prune" returned status code 128:
stdout: 
stderr: error: The requested URL returned error: 403 while accessing https://github.com/kubernetes/kubernetes/info/refs
fatal: HTTP request failed

    at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1600)
    at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandWithCredentials(CliGitAPIImpl.java:1363)
    at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.access$300(CliGitAPIImpl.java:61)
    at org.jenkinsci.plugins.gitclient.CliGitAPIImpl$1.execute(CliGitAPIImpl.java:299)
    at hudson.plugins.git.GitSCM.fetchFrom(GitSCM.java:761)
    ... 11 more

@k8s-bot test this please

liggitt · 2015-09-04T17:26:58Z

wondering how to kick what appears to be a hung e2e... @k8s-bot test this again please?

k8s-bot · 2015-09-04T17:50:04Z

GCE e2e build/test passed for commit c837869.

ncdc · 2015-09-04T17:50:14Z

Woot :-)

Various exec fixes

googlebot added the cla: yes label Aug 28, 2015

liggitt reviewed Aug 28, 2015
View reviewed changes

ncdc mentioned this pull request Aug 28, 2015

Add http proxy support for exec/port-forward #11694

Merged

liggitt reviewed Aug 28, 2015
View reviewed changes

ncdc force-pushed the fix-noninteractive-stdin-exec branch from 969e3dc to 51681c1 Compare August 28, 2015 21:05

k8s-github-robot assigned brendandburns Sep 2, 2015

k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 2, 2015

ncdc force-pushed the fix-noninteractive-stdin-exec branch 2 times, most recently from bfab798 to 57d4df4 Compare September 2, 2015 16:20

ncdc mentioned this pull request Sep 2, 2015

[do not merge] bump(fsouza/go-dockerclient): c0187cbf588c59709e8881fd2bba082b02231356 #13324

Closed

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 2, 2015

ncdc changed the title ~~Ensure remote stdin stream is closed~~ Various exec fixes Sep 2, 2015

This was referenced Sep 2, 2015

Fix remotecommand races #13144

Closed

UPSTREAM: 13144: Deflake TestRequestExecuteRemoteCommand openshift/origin#4353

Closed

brendandburns removed their assignment Sep 3, 2015

brendandburns assigned smarterclayton Sep 3, 2015

Andy Goldstein added 2 commits September 4, 2015 10:40

bump(fsouza/go-dockerclient): 76fd6c6

2bf984a

ncdc force-pushed the fix-noninteractive-stdin-exec branch from addece6 to c837869 Compare September 4, 2015 14:41

smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 4, 2015

liggitt added the ok-to-merge label Sep 4, 2015

liggitt mentioned this pull request Sep 4, 2015

UPSTREAM: 13322: Various exec fixes openshift/origin#4535

Merged

ghost pushed a commit that referenced this pull request Sep 4, 2015

Merge pull request #13322 from ncdc/fix-noninteractive-stdin-exec

c324fdd

Various exec fixes

ghost merged commit c324fdd into kubernetes:master Sep 4, 2015

ncdc mentioned this pull request Sep 10, 2015

createStreams returns a reset errorStream #13137

Closed

ncdc added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 11, 2015

This was referenced Oct 15, 2015

kubectl exec -i exits prematurely #15702

Closed

Add protocol versions & negotiation for exec, attach, port forward #15726

Closed

ncdc mentioned this pull request Oct 22, 2015

Add integration test with mock kubelet 1.0 backend to verify exec subprotocol fallback #16119

Closed

ncdc deleted the fix-noninteractive-stdin-exec branch February 13, 2017 17:25

seans3 mentioned this pull request Mar 21, 2023

[WIP] kubectl exec/cp/attach over WebSocket #116778

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various exec fixes #13322

Various exec fixes #13322

ncdc commented Aug 28, 2015

ncdc commented Aug 28, 2015

liggitt Aug 28, 2015

liggitt Aug 28, 2015

ncdc Aug 28, 2015

ncdc Aug 28, 2015

liggitt Aug 28, 2015

liggitt Aug 28, 2015

ncdc Aug 28, 2015

liggitt Aug 28, 2015

ncdc Aug 28, 2015

liggitt Aug 28, 2015

ncdc Aug 28, 2015

liggitt Aug 28, 2015

ncdc Aug 28, 2015

k8s-bot commented Aug 28, 2015

k8s-bot commented Aug 28, 2015

k8s-github-robot commented Sep 2, 2015

ncdc commented Sep 2, 2015

k8s-github-robot commented Sep 2, 2015

liggitt commented Sep 2, 2015

k8s-bot commented Sep 2, 2015

ncdc commented Sep 2, 2015

ncdc commented Sep 2, 2015

k8s-bot commented Sep 2, 2015

k8s-bot commented Sep 2, 2015

ncdc commented Sep 2, 2015

k8s-bot commented Sep 2, 2015

k8s-bot commented Sep 4, 2015

ncdc commented Sep 4, 2015

liggitt commented Sep 4, 2015

k8s-bot commented Sep 4, 2015

ncdc commented Sep 4, 2015

Various exec fixes #13322

Various exec fixes #13322

Conversation

ncdc commented Aug 28, 2015

ncdc commented Aug 28, 2015

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

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

k8s-bot commented Aug 28, 2015

k8s-bot commented Aug 28, 2015

k8s-github-robot commented Sep 2, 2015

ncdc commented Sep 2, 2015

k8s-github-robot commented Sep 2, 2015

liggitt commented Sep 2, 2015

k8s-bot commented Sep 2, 2015

ncdc commented Sep 2, 2015

ncdc commented Sep 2, 2015

k8s-bot commented Sep 2, 2015

k8s-bot commented Sep 2, 2015

ncdc commented Sep 2, 2015

k8s-bot commented Sep 2, 2015

k8s-bot commented Sep 4, 2015

ncdc commented Sep 4, 2015

liggitt commented Sep 4, 2015

k8s-bot commented Sep 4, 2015

ncdc commented Sep 4, 2015