Change 'docker run' exit codes to distinguish docker/contained errors - #14012
Conversation
|
We are basing this off of the "defacto" chroot standard. Bash has some rules around this also. |
|
looks ok for me. |
f6b3e71 to
52aabf2
Compare
|
Any word on this? |
6459251 to
9267e33
Compare
|
I think this is a good idea. Do you think we would break anyone relying on the existing error codes or do you think they are just checking for a non-zero return? |
|
I would doubt anyone would check on this, but if they were that would be a bug in their code. Currently you can not differentiate between docker errors and errors in apps in containers, so looking for exit code of 1 makes little sense. |
5c38be1 to
0698333
Compare
|
will this be considered before Docker 1.8? |
|
+1 makes sense. Moving to code review since @crosbymichael seemed to be ok with it as well. |
There was a problem hiding this comment.
Can you explain this code here? If we have to check the output error message to "calculate" the exit code, then isn't something wrong since I thought the point of this PR was to make sure our exit codes were set properly?
There was a problem hiding this comment.
not ignoring this, just looking into it more to compare to derr.Errs...not very easy to test this package, actually.
There was a problem hiding this comment.
@duglin as I (now) understand it the pkg/integration is to set up/test the utilities used in integration-cli, so there's no need to add tests or change anything there. The new exit codes are tested in int-cli. Thnx for your patience :)
|
working on feedback now, I've fixed the 'image doesn't not exist' errors by adding a similar check in api/client/run.go for the already existing derr.ErrorCodeNoSuchImageHash and derr.ErrorCodeNoSuchImageTag errors that were created in daemon/create.go --should have seen this before :( working on other feedback now.... |
There was a problem hiding this comment.
I'm thinking the 2nd param should be false here.
$ docker run -ti ubuntu /tmp
exec: "/tmp": permission denied
docker: Error response from daemon: Contained command could not be invoked..
See '/root/docker/bundles/1.9.0-dev/binary/docker run --help'.
I think it makes sense to show the short help message in cases where we think there might be a docker specific error on the cmd line (like a bad option), but in cases where we've made it all the way to the container.start() we're probably into the user (or cmd) space and so I'm not sure showing the help message helps. Keeping it on the parsing error and create error, above, are ok though.
|
overall looks good - just a couple of minor comments |
|
@duglin, addressed your comments, thanks again. |
|
LGTM! |
|
ping @jfrazelle @tiborvass look good? |
|
LGTM. I'm rerunning the build before merging it to make sure this change doesn't break anything after so many changes. |
|
@calavera anything else? |
|
Hm, does this need documentation changes somewhere? I'm not sure actually if we mention exit-codes somewhere in the docs |
|
I counted two LGTM's, moving to docs review (but I'm not actually sure we have something around this, so suggestions are welcome) |
|
It's a good feature, I believe it should be documented indeed! |
|
@thaJeztah @icecrime in man/docker-run.1.md and docs/reference/run.md? I'm on it :) |
sgtm! I see you added to looks like this needs to be squashed and rebased as well (don't shoot the messenger! 😄) |
The purpose of this PR is for users to distinguish Docker errors from contained command errors. This PR modifies 'docker run' exit codes to follow the chroot standard for exit codes. Exit status: 125 if 'docker run' itself fails 126 if contained command cannot be invoked 127 if contained command cannot be found the exit status otherwise Signed-off-by: Sally O'Malley <somalley@redhat.com>
|
@thaJeztah done! |
|
thanks @sallyom. Man LGTM too. @thaJeztah can you check it out and merge if it looks good to you? |
|
Oh cool, it's updated, thanks!! LGTM \o/ |
|
woohoo! |
|
/me waits for the world to burn with this one :) This is a good change though, I like having distinct exit codes. |
|
@sallyom thanks! and great job! |
15 participants
close #6734
The purpose of this PR is for users to distinguish Docker errors from
contained command errors.
This PR modifies 'docker run' exit codes to follow the chroot standard
for exit codes.
Exit status:
125 if 'docker run' itself fails
126 if contained command cannot be invoked
127 if contained command cannot be found
the exit status otherwise
Signed-off-by: Sally O'Malley somalley@redhat.com