Preserve individual volumes on recreate #858

dnephin · 2015-01-20T05:31:34Z

Resolves: #836

When a container is being recreated (during docker-compose up) we use volumes-from to copy every volume over to the new container. This has lead to a lot of confusion (there are numerous github issues, and stackoverflow questions about it).

Instead of blanket copying every volume with volumes-from this PR changes the behaviour to only copy over container data volumes that are still present in the configuration. Host volumes, and volumes that were removed from the configuration will no longer be copied over (although host volumes are still applied, they just aren't copied) .
#711 was the first attempt, but wasn't compatible with docker 1.4.1+

Related Issue #447

aanand · 2015-01-20T13:45:31Z

Looks like tests are failing on 1.2.0 - this line fixed it:

diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py
index 5e00ec1..6df2301 100644
--- a/tests/integration/service_test.py
+++ b/tests/integration/service_test.py
@@ -148,6 +148,7 @@ class ServiceTest(DockerClientTestCase):
         self.assertIn('FOO=1', old_container.get('Config.Env'))
         self.assertEqual(old_container.name, 'figtest_db_1')
         service.start_container(old_container)
+        old_container.inspect() # reload volumes
         volume_path = old_container.get('Volumes')['/etc']

         num_containers_before = len(self.client.containers(all=True))

However, there's a more serious regression - see #859. The new logic introduced in #711 doesn't preserve volumes that are declared in the image.

What if the volume copying logic were:

copy all volume binds from an existing container
add/overwrite all volumes with explicit host paths from fig.yml
add any missing volumes without explicit host paths from fig.yml

This wouldn't handle the case where a volume is deleted from fig.yml, but at least it'd handle the case where a volume is remapped to a different host path.

dnephin · 2015-01-21T03:11:41Z

compose/service.py

+    container_volumes = container.get('Volumes') or {}
+    original_volumes = set(volumes_option + container_volumes.keys())
+
+    for volume in original_volumes:


This works in docker 1.2 to fix the test added in #859

Using docker 1.4.1 it seems like docker is refusing to override the volume with a bind. This function is returning the correct value, but when container.start(binds=...) is called, the old volume remains.

Maybe volumes_from has to be used to override the volume? Maybe this is a bug in docker? I'm not really clear on what the expected behaviour is.

Maybe volumes-upper-guru @cpuguy83 has some time to have a look? Think you want this solved in the final release?

Have you tried against Docker master?
You should indeed be able to override an existing volume, and volumeFrom have priority over all.

@cpuguy83 Per @dnephin request adding info from my case here: https://gist.github.com/andreyst/e213c18ef32dcb28dc5e

This works if I try to mount something else like /tmp. If I ln -s the dir I need in /tmp and try to pass /tmp/dirname as a volume, it still doesn't work. If then I pass /tmp as a volume, it does work, but if then I try to cd to it from a container, it gives me "No such file or directory".

I tried to create a minimal reproduction of this failure, but when I tried with the docker cli I was not able to reproduce the failure.

https://gist.github.com/dnephin/e584a7741a514d631632

From the docker inspect they look almost identical, except you can see in the fig example the volume is not set to the path specified by binds.

I tried to use mitmproxy to inspect the differences between the http requests being used, but it seems like the docker cli ignores HTTP_PROXY (I was able to get the requests made by fig at least).

I'm struggling to find the difference between this example and the test case. Does anyone know of a way to make the docker cli use a proxy?

@dnephin Support for proxies was added in Docker 1.5 via moby/moby#9951 (docs changes for it are in moby/moby#10192) not sure if there's another way to specify this. Given that it's just a one line change, you could try to build a 1.4.1 version with just that one change added?

(Sorry if it's a lame answer, but perhaps the links are useful :))

dnephin · 2015-01-26T20:49:24Z

Ok, This is working for 1.4.1 now, but requires api version 1.5 , which I think is 1.4 ?

So blocking on #886 and makes compose incompatible with earlier versions.

To support older versions we'd have to detect the server version and pass the binds to start instead of create. Not sure if it's worth it.

thaJeztah · 2015-01-26T21:10:20Z

If the docs are correct, API v1.15 corresponds with docker 1.3 (https://docs.docker.com/v1.3/reference/api/docker_remote_api/). In my opinion, that's reasonable. Several CVEs were addressed in docker 1.3.x, so people should be encouraged to run that version at minimum.

Otherwise, people can always run Fig 1.0.1 (that won't dissapear) 😄

cpuguy83 · 2015-01-26T21:20:19Z

But, keep in mind, there should be no issue with doing this on start (at least from the Docker side)!

dnephin · 2015-01-26T21:26:57Z

@cpuguy83 from https://gist.github.com/dnephin/e584a7741a514d631632 it looks like (at least in 1.4.1) the binds in start are not overriding the volumes from the Dockerfile. Maybe that's fixed in 1.5, I haven't had a chance to try it yet.

cpuguy83 · 2015-01-26T21:42:18Z

@dnephin I think I've found the problem in Docker.
It's this check here: https://github.com/docker/docker/blob/v1.4.1/daemon/volumes.go#L83

For volumes that are created by docker, IsBindMount would always be false.
This may not work on master, let me do some tests and see if I can get a fix in for 1.5.

cpuguy83 · 2015-01-27T01:31:33Z

And that one may be different actually.
Testing fig against master seems to be OK where as it has issues with 1.4.1

# fig.yml
web:
  build: .
  volumes:
    - "/foo"
    - "/bar"

# Dockerfile
FROM busybox
ADD . /foo
VOLUME /foo
VOLUME /bar
CMD exec top

Does this look right?

dnephin · 2015-01-27T03:54:59Z

I don't think you'll be able to reproduce it with master, since it's still using volumes_from, and it only manifests when you recreate a container.

This gist does reproduce it: https://gist.github.com/dnephin/083a1052b8293a5b46a4

It prints the volumes from both containers. They should be the same, but the second container has a different volume for /data.

cpuguy83 · 2015-01-27T16:02:10Z

Yep, this will be fixed by moby/moby#10365

dnephin · 2015-03-05T16:27:27Z

I've updated the title and description of this issue to better reflect the changes.

I'd love to chat about getting this into 1.2. I think this is still the most common issue people have with docker-compose (see the long list of related tickets above, there are also multiple stack-overflow questions about it).

This PR will need to be updated once we remove the intermediary container (in #874), but there is nothing in docker 1.5 (or coming in docker 1.6) that actually fixes the issue addressed by this PR.

How do you feel about putting this into the 1.2 milestone?

thaJeztah · 2015-03-05T18:16:04Z

volumes that were removed from the configuration will no longer be copied over

This does have the nasty side-effect that those volumes will be left behind (orphaned), which makes it very difficult to access them or clean then up until docker implements volume management

dnephin · 2015-03-05T19:12:37Z

That is true, but this is basically the case with any docker run -v command that is run, right?

thaJeztah · 2015-03-05T19:23:09Z

Yes, but with those I have some control, ie docker rm -v <container>.

Given how many questions I've seen from people unaware of this ("docker leaks containers", "docker eats my disk space"), it should be documented properly.

Lol, maybe compose should create a dummy container and attach all old volumes to that :)

In general, Docker really needs better volume management. There are various proposals, so I hope it someday arrives natively.

Apart from those concerns, I agree with this PR

dnephin · 2015-05-10T01:48:27Z

Ok, this is rebased with master and tests are passing.

aanand · 2015-05-11T10:43:11Z

Nice.

One more thing: Swarm uses volumes_from to co-schedule containers. It won't have that to go on any more, so we'll need to explicitly co-schedule with an environment variable:

affinity:container==previous_container_id

dnephin · 2015-05-11T16:01:48Z

Cool, is swarm still using environment variables for this? The README suggests it's using labels now as well. Maybe both are supported? I wonder if it makes sense to just use labels?

Edit: I guess I misread, it seems like label affinity is a different thing. I'll use environment variables

dnephin · 2015-05-11T16:38:47Z

Added the affinity. I was also able to cleanup some of the merge_environment() logic for run that was in cli.main.

aanand · 2015-05-11T16:47:09Z

compose/service.py

+            override_options,
+            environment=merge_environment(
+                override_options.get('environment'),
+                {'affinity:container': container.id}))


Hmm, it needs to come out as affinity:container==<id>, i.e. a double-equals. Maybe we should add a test for it?

(I'm going to investigate running Compose's test suite against Swarm, btw)

Oh, I did add a test for it, but I didn't realize it required the ==.

If I set the value to =<id> that should work, right ?

Signed-off-by: Daniel Nephin <dnephin@gmail.com>

dnephin · 2015-05-11T17:02:13Z

Using == now

aanand · 2015-05-14T15:12:06Z

LGTM

Preserve individual volumes on recreate

dnephin force-pushed the fix_volumes_recreate_on_1.4.1 branch 2 times, most recently from 5baba4f to f7355be Compare January 21, 2015 03:07

dnephin reviewed Jan 21, 2015
View reviewed changes

dnephin mentioned this pull request Jan 26, 2015

Revert #711 from dnephin/fix_volumes_on_recreate #863

Merged

dnephin force-pushed the fix_volumes_recreate_on_1.4.1 branch from f7355be to ab4893c Compare January 26, 2015 20:47

dnephin force-pushed the fix_volumes_recreate_on_1.4.1 branch 2 times, most recently from 0ded765 to b6294b1 Compare January 27, 2015 06:20

This was referenced Feb 9, 2015

mount folder with fig vs with docker #938

Closed

unexpected behavior with volumes #936

Closed

Since docker 1.3.0 / fig 1.0.0 volumes are not mounted when fig up'ing existing containers #622

Closed

Run CI tests on multiple Docker versions #886

Closed

This was referenced Feb 16, 2015

Make Swarm schedule containers on the same node when they are linked together #972

Merged

Proposal: Option to disable volumes_from when using fig up #984

Closed

aanand mentioned this pull request Feb 27, 2015

Data Only Containers/No Run Containers #942

Closed

dnephin changed the title ~~Handle recreate volumes properly for docker 1.4.1~~ Preserve individual volumes on recreate Mar 5, 2015

dnephin mentioned this pull request Mar 11, 2015

Run + data only container + volumes_from + volumes won't mount to host #1091

Closed

dnephin mentioned this pull request Apr 4, 2015

Getting /bin/bash: /usr/src/app/run.sh: No such file or directory #1254

Closed

dnephin mentioned this pull request Apr 15, 2015

Fig caching mounted volumes #836

Closed

aanand mentioned this pull request May 8, 2015

Only recreate what's changed #1399

Merged

3 tasks

dnephin force-pushed the fix_volumes_recreate_on_1.4.1 branch 2 times, most recently from 490e97a to b8cec3f Compare May 10, 2015 01:22

dnephin force-pushed the fix_volumes_recreate_on_1.4.1 branch from b8cec3f to 9bddd6b Compare May 11, 2015 16:26

aanand reviewed May 11, 2015
View reviewed changes

Use individual volumes for recreate instead of volumes_from

417d9c2

Signed-off-by: Daniel Nephin <dnephin@gmail.com>

dnephin force-pushed the fix_volumes_recreate_on_1.4.1 branch from 9bddd6b to 417d9c2 Compare May 11, 2015 17:01

dnephin added this to the 1.3.0 milestone May 14, 2015

aanand added a commit that referenced this pull request May 14, 2015

Merge pull request #858 from dnephin/fix_volumes_recreate_on_1.4.1

0ac8c3c

Preserve individual volumes on recreate

aanand merged commit 0ac8c3c into docker:master May 14, 2015

aanand mentioned this pull request Jun 12, 2015

Fix volume binds de-duplication #1544

Merged

dnephin deleted the fix_volumes_recreate_on_1.4.1 branch July 30, 2015 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve individual volumes on recreate #858

Preserve individual volumes on recreate #858

dnephin commented Jan 20, 2015

aanand commented Jan 20, 2015

dnephin Jan 21, 2015

thaJeztah Jan 21, 2015

cpuguy83 Jan 21, 2015

andreyst Jan 22, 2015

dnephin Jan 25, 2015

thaJeztah Jan 25, 2015

dnephin commented Jan 26, 2015

thaJeztah commented Jan 26, 2015

cpuguy83 commented Jan 26, 2015

dnephin commented Jan 26, 2015

cpuguy83 commented Jan 26, 2015

cpuguy83 commented Jan 27, 2015

dnephin commented Jan 27, 2015

cpuguy83 commented Jan 27, 2015

dnephin commented Mar 5, 2015

thaJeztah commented Mar 5, 2015

dnephin commented Mar 5, 2015

thaJeztah commented Mar 5, 2015

dnephin commented May 10, 2015

aanand commented May 11, 2015

dnephin commented May 11, 2015

dnephin commented May 11, 2015

aanand May 11, 2015

dnephin May 11, 2015

dnephin commented May 11, 2015

aanand commented May 14, 2015

Preserve individual volumes on recreate #858

Preserve individual volumes on recreate #858

Conversation

dnephin commented Jan 20, 2015

aanand commented Jan 20, 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

dnephin commented Jan 26, 2015

thaJeztah commented Jan 26, 2015

cpuguy83 commented Jan 26, 2015

dnephin commented Jan 26, 2015

cpuguy83 commented Jan 26, 2015

cpuguy83 commented Jan 27, 2015

dnephin commented Jan 27, 2015

cpuguy83 commented Jan 27, 2015

dnephin commented Mar 5, 2015

thaJeztah commented Mar 5, 2015

dnephin commented Mar 5, 2015

thaJeztah commented Mar 5, 2015

dnephin commented May 10, 2015

aanand commented May 11, 2015

dnephin commented May 11, 2015

dnephin commented May 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin commented May 11, 2015

aanand commented May 14, 2015