Merge pull request #2336 from dnephin/warn_on_volume_masking

Add a warning when the host volume in the compose file is masked by a data volume

compose/service.py

@@ -920,8 +920,10 @@ def merge_volume_bindings(volumes_option, previous_container):
         if volume.external)
 
     if previous_container:
+        data_volumes = get_container_data_volumes(previous_container, volumes)
+        warn_on_masked_volume(volumes, data_volumes, previous_container.service)
         volume_bindings.update(
-            get_container_data_volumes(previous_container, volumes))
+            build_volume_binding(volume) for volume in data_volumes)
 
     return list(volume_bindings.values())
 
@@ -931,7 +933,6 @@ def get_container_data_volumes(container, volumes_option):
     a mapping of volume bindings for those volumes.
     """
     volumes = []
-
     container_volumes = container.get('Volumes') or {}
     image_volumes = [
         parse_volume_spec(volume)
@@ -951,9 +952,27 @@ def get_container_data_volumes(container, volumes_option):
 
         # Copy existing volume from old container
         volume = volume._replace(external=volume_path)
-        volumes.append(build_volume_binding(volume))
+        volumes.append(volume)
 
-    return dict(volumes)
+    return volumes
+
+
+def warn_on_masked_volume(volumes_option, container_volumes, service):
+    container_volumes = dict(
+        (volume.internal, volume.external)
+        for volume in container_volumes)
+
+    for volume in volumes_option:
+        if container_volumes.get(volume.internal) != volume.external:
+            log.warn((
+                "Service \"{service}\" is using volume \"{volume}\" from the "
+                "previous container. Host mapping \"{host_path}\" has no effect. "
+                "Remove the existing containers (with `docker-compose rm {service}`) "
+                "to use the host volume mapping."
+            ).format(
+                service=service,
+                volume=volume.internal,
+                host_path=volume.external))
 
 
 def build_volume_binding(volume_spec):

tests/integration/service_test.py

@@ -369,6 +369,33 @@ class ServiceTest(DockerClientTestCase):
         self.assertEqual(list(new_container.get('Volumes')), ['/data'])
         self.assertEqual(new_container.get('Volumes')['/data'], volume_path)
 
+    def test_execute_convergence_plan_when_image_volume_masks_config(self):
+        service = Service(
+            project='composetest',
+            name='db',
+            client=self.client,
+            build='tests/fixtures/dockerfile-with-volume',
+        )
+
+        old_container = create_and_start_container(service)
+        self.assertEqual(list(old_container.get('Volumes').keys()), ['/data'])
+        volume_path = old_container.get('Volumes')['/data']
+
+        service.options['volumes'] = ['/tmp:/data']
+
+        with mock.patch('compose.service.log') as mock_log:
+            new_container, = service.execute_convergence_plan(
+                ConvergencePlan('recreate', [old_container]))
+
+        mock_log.warn.assert_called_once_with(mock.ANY)
+        _, args, kwargs = mock_log.warn.mock_calls[0]
+        self.assertIn(
+            "Service \"db\" is using volume \"/data\" from the previous container",
+            args[0])
+
+        self.assertEqual(list(new_container.get('Volumes')), ['/data'])
+        self.assertEqual(new_container.get('Volumes')['/data'], volume_path)
+
     def test_start_container_passes_through_options(self):
         db = self.create_service('db')
         create_and_start_container(db, environment={'FOO': 'BAR'})

tests/unit/service_test.py

@@ -649,13 +649,13 @@ class ServiceVolumesTest(unittest.TestCase):
             },
         }, has_been_inspected=True)
 
-        expected = {
-            '/existing/volume': '/var/lib/docker/aaaaaaaa:/existing/volume:rw',
-            '/mnt/image/data': '/var/lib/docker/cccccccc:/mnt/image/data:rw',
-        }
+        expected = [
+            parse_volume_spec('/var/lib/docker/aaaaaaaa:/existing/volume:rw'),
+            parse_volume_spec('/var/lib/docker/cccccccc:/mnt/image/data:rw'),
+        ]
 
-        binds = get_container_data_volumes(container, options)
-        self.assertEqual(binds, expected)
+        volumes = get_container_data_volumes(container, options)
+        self.assertEqual(sorted(volumes), sorted(expected))
 
     def test_merge_volume_bindings(self):
         options = [