return codes of docker commands not verified

[jcrobak]

I noticed that the LWRP's are not verifying return codes except in one place:

$ git grep -n 'error!'
providers/container.rb:224:  dr.error!

Is this on purpose? It seems like it'd be more appropriate to use shell_out! (which verifies exit codes) everywhere instead of shell_out (which doesn't). Specifically, it'd be nice if docker_image with action :pull failed fast so that docker_container doesn't try to start a container that failed to pull.

[bflad]

I agree this could be improved. I thought error! Fails with any non-zero exit status. Is this what makes sense in all cases? Should we have retries, etc for exit codes in any places and how would the implementation look? Thanks for the consideration

[errm]

It seems till the next release of docker this won't be much use anyway as there a lot of fixes for things exiting with the wrong exit code mainly, 0 with an error. See a
moby/moby#3767 and moby/moby#3775

[jcrobak]

I agree this could be improved. I thought error! Fails with any non-zero exit status. Is this what makes sense in all cases? Should we have retries, etc for exit codes in any places and how would the implementation look? Thanks for the consideration

It probably makes sense to have both a docker_cmd! that checks error codes and a docker_cmd that doesn't. I've started working on a local changeset, and it seems to have already uncovered some issues in the current cookbook. I've been trying to work through some of those before sharing. I'm not sure on the retries -- that is typically used for network commands, so it might make sense for e.g. a docker pull. Although it seems like that'd be better place in docker itself.

It seems till the next release of docker this won't be much use anyway as there a lot of fixes for things exiting with the wrong exit code mainly, 0 with an error. See a
moby/moby#3767 and moby/moby#3775

AFAICT, the use of return codes has gotten better and better with each release. docker build used to always return 0, but it doesn't in the latest release. We could look at output of commands that don't return proper codes if it's important before there's a new docker release...

[jhulten]

@jcrobak Are your changes in a public repo somewhere? I was also thinking about refactoring the docker_cmd work into the helper library.

[jcrobak]

@jhulten I don't have anything up yet, but I'll try to push up what I have after some cleanups.

[jcrobak]

Here's most of what I have so far: https://github.com/jcrobak/chef-docker/tree/return-code-validation

I haven't been able to get the tests to pass on debian 7 (in particular, the test that service('testcontainerd').must_be_running -- it seems that chef is falling back to ps to check that the service is running, which obviously won't work. That doesn't seem related to my changes, though...

[bflad]

@jcrobak nice work so far. I'd be happy to merge that into next release. With respect to Debian 7 support, I still have it listed as "Debian 7 (experimental)" in the README, so I wouldn't let that stop you for now given your changes would likely not change that behavior.

[jcrobak]

@bflad Sounds good. I was able to verify that my full changeset (there are a couple more commits atop of what's pushed publicly) passes kitchen tests on all platforms other than Debian 7. Would you prefer if I break the branch into a couple PRs or are you ok with a PR with a few semi-related commits?

[bflad]

Ah, sorry I didn't respond to this yesterday. If you already have it in one branch, please feel free to submit as one PR, although may need to rebase. I also have some more minor functionality going into the image provider tonight to match (ha!) the functionality #57 just added to the container provider. If I have time I may look into #59 as well, but that shouldn't affect your work.

[bflad]

Fixed in 0.30.0 which will be released tonight.