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

Commit merges config from image, not container. #4885

Closed
wants to merge 1 commit into from
Closed

Commit merges config from image, not container. #4885

wants to merge 1 commit into from

Conversation

pda
Copy link
Contributor

@pda pda commented Mar 27, 2014

Background

#1141 reported that docker commit drops the entire config in the resulting image. Specifically, the wording was “the configuration the image had is not kept.”
#4000 landed in v0.9.1, which uses the container config as the base config for docker commit, and merges in any additional config passed by docker commit --run=….

Problem

Any command specified on the docker run command line will clobber the default command configured in the original image (config.Cmd). For example, given $image built with CMD /bin/true, running docker run $image touch /hello and then docker commit $container new_image, the resulting image will have touch /hello instead of /bin/true.

Proposal

The image created by docker commit $container $image should have its config based on $container's image, not on $container's own config.

This is more in line with the original request, to keep the config that “the image had”. It also makes it more useful for preserving Cmd, which is a prime use-case for the config-merge.

If a user wants config changes introduced by the docker run command to land in the committed image, they can pass them to docker commit --run=…. It's preferable to have to do this, compared to Dockerfile-declared configuration being clobbered.

Example

#!/bin/bash -e

docker --version

echo "Building 'clock' image with CMD /bin/date"
cat >Dockerfile <<END
FROM busybox:latest
CMD /bin/date
END
docker build --tag=clock . &>/dev/null

echo "Committing 'clock_committed' as result of 'docker run clock touch /hello'"
if [ -e cid ]; then rm cid; fi
docker run --cidfile=cid clock touch /hello
docker commit $(cat cid) clock_committed >/dev/null

echo -n "Cmd in 'clock build':  "
docker inspect --format='{{json .config.Cmd}}' clock

echo -n "Cmd in 'clock_committed': "
docker inspect --format='{{json .config.Cmd}}' clock_committed

Output (before this PR):

Docker version 0.9.1, build 3600720
Building 'clock' image with CMD /bin/date
Committing 'clock_committed' as result of 'docker run clock touch /hello'
Cmd in 'clock build':  ["/bin/sh","-c","/bin/date"]
Cmd in 'clock_committed': ["touch","/hello"]

Output (after this PR):

Docker version 0.9.1-dev, build 7767e78
Building 'clock' image with CMD /bin/date
Committing 'clock_committed' as result of 'docker run clock touch /hello'
Cmd in 'clock build':  ["/bin/sh","-c","/bin/date"]
Cmd in 'clock_committed': ["/bin/sh","-c","/bin/date"]

TODO

  • Ensure unit tests pass.
  • Ensure TestMergeConfigOnCommit in integration/server_test.go reflects this change.
  • Ensure integration tests pass.

This means, for example, the command specified on the `docker run`
command line will not clobber that of the image which the container was
run from.

Docker-DCO-1.1-Signed-off-by: Paul Annesley <paul@annesley.cc> (github: pda)
@crosbymichael
Copy link
Contributor

@pda In your Problem I think that is totally valid. If you want to override the command on your run you can. This is how CMD has worked forever.

@tianon
Copy link
Member

tianon commented Mar 27, 2014

As a side note, that's how RUN works - RUN doesn't overwrite CMD.

@crosbymichael
Copy link
Contributor

@pda why are you passing a CMD on docker run when you don't want it?

@pda
Copy link
Contributor Author

pda commented Mar 27, 2014

Argh, I accidentally deleted my comment. You probably got it via email. Feel free to paste it as a quote, I've lost it completely.

I was going to add:

This is how CMD has worked forever.

Worth noting this change would only effect a behavior that is brand new in Docker 0.9.1.

@pda
Copy link
Contributor Author

pda commented Mar 27, 2014

Here's an example of a docker run that specified a one-off command which I don't want to land in a subsequent image. It's part of a build pipeline that injects the current codebase and runs necessary build tasks.

# Copy and build codebase.
docker run \
  --cidfile=$CID_FILE \
  --volume $(pwd):/codebase-volume \
  --env MAKE_PROFILE=1 \
  $BASE_IMAGE \
  bash -c "mkdir -p /codebase && cd /codebase && \
    git clone /codebase-volume/.git . && \
    rm -r .git && \
    make setup-toolchain build"

docker commit $(cat $CID_FILE) $BUILD_IMAGE

@crosbymichael
Copy link
Contributor

@pda why would you commit that if you don't want the results? Just trying to understand how you are using it.

@pda
Copy link
Contributor Author

pda commented Mar 27, 2014

We absolutely want the resulting container/image filesystem that came from that docker run. It's introduced the built codebase into the base image which didn't have it before.

What we don't want is for that one-off command to become part of the image metadata / config.

To outline the build pipeline:

  • Dockerfile builds a developer-friendly image, no codebase, ready for it to be volume-mounted in.
  • Build/release pipeline takes that image, adds the codebase, runs make, commits the result as a "build image".
  • That "build image" should retain the CMD from the original Dockerfile. But it needed that one-off command run against it.

@pda
Copy link
Contributor Author

pda commented Mar 27, 2014

As a side note, that's how RUN works - RUN doesn't overwrite CMD. — @tianon

Interesting… so Dockerfile RUN <one-off command> does not overwrite config.Cmd but docker run $image <one-off command> does. It'd be nice if they matched, and I think the former is correct.

So this could be addressed as per this PR, or by changing how docker run works. The latter sounds nice, but would probably be a breaking change? The former (this PR) only changes behavior introduced in the current Docker 0.9.1.

@kajmagnus
Copy link

+1, I thought the image's CMD would be remembered after docker run and docker commit and was surprised that it wasn't and had to google for a while to understand what was happening.

@crosbymichael Re why would I commit when I don't want the results: I'm new to Docker, but thus far I've sometimes run /bin/bash and modified my dev container, e.g. logged into PostgreSQL and configured it, and since I want to keep the changes, I commit. But I don't want to change the CMD command, only keep the changes I made in the container.

@crosbymichael
Copy link
Contributor

ping @vieux I believe this as been discussed in another issue. Should we close?

@vieux
Copy link
Contributor

vieux commented Sep 29, 2014

I'm pretty sure it's not relevent anymore. We changed the way we merged the config.

@vieux vieux closed this Sep 29, 2014
@vieux
Copy link
Contributor

vieux commented Sep 29, 2014

Please say something if I'm wrong

@vektah
Copy link

vektah commented Jan 7, 2015

@vieux It looks like this issue is still present in 1.4.1. Here is an example of CMD getting clobbered:

With this base Dockerfile

From busybox

RUN echo "foo" > /foo

CMD cat /foo

I do:

$ docker build .
Sending build context to Docker daemon 2.048 kB
Sending build context to Docker daemon 
Step 0 : FROM busybox
 ---> 4986bf8c1536
Step 1 : RUN echo "foo" > /foo
 ---> Running in 7b566904f416
 ---> 76d9809edb1c
Removing intermediate container 7b566904f416
Step 2 : CMD cat /foo
 ---> Running in b61d93f8359a
 ---> 492f5228fd2e
Removing intermediate container b61d93f8359a
Successfully built 492f5228fd2e

$ docker run 492f5228fd2e
foo

Now I want to modify the file system by running some commands (real example could be updating source, building assets, changing config)

$ docker run 492f5228fd2e sh -c 'echo "bar" > /foo'
$ docker ps -a | head
CONTAINER ID        IMAGE                                       COMMAND                CREATED              STATUS                      PORTS                      NAMES
01591b73f3c5        492f5228fd2e                                "\"sh -c 'echo \"bar   17 seconds ago       Exited (0) 16 seconds ago                              elated_galileo             
05d5273d2bc2        492f5228fd2e                                "/bin/sh -c 'cat /fo   About a minute ago   Exited (0) 59 seconds ago                              goofy_pasteur4             

$ docker commit 01591b73f3c5 
c266702eca21aadec2b2a58677be416cb4bb766eaae53515ab6f643b157a9f28

$ docker run c266702eca21aadec2b2a58677be416cb4bb766eaae53515ab6f643b157a9f28

# WHAAAT??

$ docker inspect --format '{{js .Config.Cmd}}' c266702eca21aadec2b2a58677be416cb4bb766eaae53515ab6f643b157a9f28
[sh -c echo \"bar\" \x3E /foo]

I would have expected the output of that last run to be bar, but as you can see the CMD of the new container is echo.

@msumme
Copy link

msumme commented Jan 23, 2015

Yes, this makes docker somewhat useless for a fairly simple build process.

It seemed easy enough: Build a base image. Run a command in a container to update the sources. Commit that container to a deployable image, and push it to your repository as the latest version of the production container.

Only - when you commit, you get the command that was passed in as part of your build process, not the command in the base image that was missing sources.

Example:

# Dockerfile
FROM nginx

ADD ./nginx/nginx.conf /etc/nginx/
ADD ./nginx/sites-enabled /etc/nginx/sites-enabled/     

#build-script
... setting variables... 
container_name="build-$RANDOM-"`date +%s`
docker run --name $container_name -v $built_sources_dir:/mnt/app $base_runtime_image mkdir -p /app && cp -R /mnt/app/* /app

docker commit $container_name $built_image_name

docker rm -f $container_name

When I run the image with $built_image_name, it runs "mkdir -p /app" and exits.

It doesn't really make sense to create a new Dockerfile with an ADD statement here, since I want to be able to build images from multiple base images (i.e. staging/server, stable/server, etc) and have the rest of the process be the same - so we can test out upgrades before pushing to our production images.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants