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

kubectl exec doesn't return command exit code #26424

Closed
sttts opened this issue May 27, 2016 · 13 comments
Closed

kubectl exec doesn't return command exit code #26424

sttts opened this issue May 27, 2016 · 13 comments
Assignees
Labels
area/kubectl priority/backlog Higher priority than priority/awaiting-more-evidence.

Comments

@sttts
Copy link
Contributor

sttts commented May 27, 2016

$ kubectl run --image=busybox sleep -- /bin/sh -c "sleep 1000"
$ kubectl exec -it sleep-548116132-nc8zh -- /bin/sh -c "exit 5"
$ echo $?
1

The last line should be 5.

@sttts sttts self-assigned this May 27, 2016
@dims
Copy link
Member

dims commented May 27, 2016

@sttts i am trying kubectl 1.2.4 and i see

$ ./kubectl exec -it sleep-548116132-yf6wd -- /bin/sh -c "exit 5"
error: error executing remote command: error executing command in container: Error executing in Docker Container: 5

@sttts
Copy link
Contributor Author

sttts commented May 27, 2016

@dims that's right. But the return code in the shell of the whole command is 1, compare $?.

@dims
Copy link
Member

dims commented May 27, 2016

@sttts ack, just making sure we are seeing the same thing :)

@sttts
Copy link
Contributor Author

sttts commented May 27, 2016

Haven't looked into the code yet. Not sure cobra supports that.

@dims
Copy link
Member

dims commented May 27, 2016

@sttts looks like kubelet itself is treating exit code and the error stream as one thing. This will take some non-trivial work i think

@sttts
Copy link
Contributor Author

sttts commented May 30, 2016

I don't want to introduce another stream for the return code only. I would propose to switch the error stream protocol to json, e.g. {message: "the old message string", code: 42}. Looking at #25273 for resize events, it seems to be a good moment to add this with protocol v3.

Opinions @dims @ncdc ?

@dims
Copy link
Member

dims commented May 30, 2016

@sttts - just for error stream. yes, i think that would be good!

@ncdc
Copy link
Member

ncdc commented May 30, 2016

Yes that's what I had been thinking as well.

On Monday, May 30, 2016, Dr. Stefan Schimanski notifications@github.com
wrote:

I don't want to introduce another stream for the return code only. I would
propose to switch the error stream protocol to json, e.g. {message: "the
old message string", code: 42}. Looking at #25273
#25273 for resize events,
it seems to be a good moment to add this with protocol v3.

Opinions @dims https://github.com/dims @ncdc https://github.com/ncdc ?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#26424 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAABYo4w4do_co2X0odis3W2ALRc-o83ks5qGshLgaJpZM4Iojdc
.

@ncdc
Copy link
Member

ncdc commented May 30, 2016

Do you want to bundle this in with my terminal resizing pull, or try to
maybe do it right set it merges?

On Monday, May 30, 2016, Andy Goldstein agoldste@redhat.com wrote:

Yes that's what I had been thinking as well.

On Monday, May 30, 2016, Dr. Stefan Schimanski <notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

I don't want to introduce another stream for the return code only. I
would propose to switch the error stream protocol to json, e.g. {message:
"the old message string", code: 42}. Looking at #25273
#25273 for resize events,
it seems to be a good moment to add this with protocol v3.

Opinions @dims https://github.com/dims @ncdc https://github.com/ncdc
?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#26424 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAABYo4w4do_co2X0odis3W2ALRc-o83ks5qGshLgaJpZM4Iojdc
.

@sttts
Copy link
Contributor Author

sttts commented May 30, 2016

@ncdc working on it right now. Makes sense to put that on-top of your PR.

@pwittrock pwittrock added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Jun 1, 2016
@dims
Copy link
Member

dims commented Jul 6, 2016

Looks like PR #26541 is close to merging.

@sttts
Copy link
Contributor Author

sttts commented Jul 6, 2016

Waiting for #25273 to go in.

k8s-github-robot pushed a commit that referenced this issue Aug 21, 2016
Automatic merge from submit-queue

Return container command exit codes in kubectl run/exec

Fixes #26424
Based on #25273.

TODO:
- [x] add e2e tests
- [x] investigate `kubectl run` exit code for `--restart=Never` (compare issue #24533 and PR #25253)
- [x] document exit codes
@VojtechVitek
Copy link
Contributor

--attach=true --restart=Never returns with the pod's exit code :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kubectl priority/backlog Higher priority than priority/awaiting-more-evidence.
Projects
None yet
Development

No branches or pull requests

6 participants