Merge pull request #863 from dnephin/revert_volume_recreate_changes

Revert #711 from dnephin/fix_volumes_on_recreate

Author: aanand

fig/project.py

@@ -176,14 +176,18 @@ def up(self,
            do_build=True):
         running_containers = []
         for service in self.get_services(service_names, include_links=start_links):
-            create_func = (service.recreate_containers if recreate
-                           else service.start_or_create_containers)
-
-            for container in create_func(
-                    insecure_registry=insecure_registry,
-                    detach=detach,
-                    do_build=do_build):
-                running_containers.append(container)
+            if recreate:
+                for (_, container) in service.recreate_containers(
+                        insecure_registry=insecure_registry,
+                        detach=detach,
+                        do_build=do_build):
+                    running_containers.append(container)
+            else:
+                for container in service.start_or_create_containers(
+                        insecure_registry=insecure_registry,
+                        detach=detach,
+                        do_build=do_build):
+                    running_containers.append(container)
 
         return running_containers
 

fig/service.py

@@ -242,9 +242,8 @@ def create_container(self,
 
     def recreate_containers(self, insecure_registry=False, do_build=True, **override_options):
         """
-        If a container for this service doesn't exist, create and start one. If
-        there are any, stop them, create+start new ones, and remove the old
-        containers.
+        If a container for this service doesn't exist, create and start one. If there are
+        any, stop them, create+start new ones, and remove the old containers.
         """
         containers = self.containers(stopped=True)
         if not containers:
@@ -254,22 +253,21 @@ def recreate_containers(self, insecure_registry=False, do_build=True, **override
                 do_build=do_build,
                 **override_options)
             self.start_container(container)
-            return [container]
+            return [(None, container)]
         else:
-            return [
-                self.recreate_container(
-                    container,
-                    insecure_registry=insecure_registry,
-                    **override_options)
-                for container in containers
-            ]
+            tuples = []
+
+            for c in containers:
+                log.info("Recreating %s..." % c.name)
+                tuples.append(self.recreate_container(c, insecure_registry=insecure_registry, **override_options))
+
+            return tuples
 
     def recreate_container(self, container, **override_options):
         """Recreate a container. An intermediate container is created so that
         the new container has the same name, while still supporting
         `volumes-from` the original container.
         """
-        log.info("Recreating %s..." % container.name)
         try:
             container.stop()
         except APIError as e:
@@ -280,30 +278,24 @@ def recreate_container(self, container, **override_options):
             else:
                 raise
 
-        intermediate_options = dict(self.options, **override_options)
         intermediate_container = Container.create(
             self.client,
             image=container.image,
             entrypoint=['/bin/echo'],
             command=[],
             detach=True,
         )
-        intermediate_container.start(
-            binds=get_container_data_volumes(
-                container, intermediate_options.get('volumes')))
+        intermediate_container.start(volumes_from=container.id)
         intermediate_container.wait()
         container.remove()
 
-        # TODO: volumes are being passed to both start and create, this is
-        # probably unnecessary
         options = dict(override_options)
         new_container = self.create_container(do_build=False, **options)
-        self.start_container(
-            new_container,
-            intermediate_container=intermediate_container)
+        self.start_container(new_container, intermediate_container=intermediate_container)
 
         intermediate_container.remove()
-        return new_container
+
+        return (intermediate_container, new_container)
 
     def start_container_if_stopped(self, container, **options):
         if container.is_running:
@@ -315,6 +307,12 @@ def start_container_if_stopped(self, container, **options):
     def start_container(self, container, intermediate_container=None, **override_options):
         options = dict(self.options, **override_options)
         port_bindings = build_port_bindings(options.get('ports') or [])
+
+        volume_bindings = dict(
+            build_volume_binding(parse_volume_spec(volume))
+            for volume in options.get('volumes') or []
+            if ':' in volume)
+
         privileged = options.get('privileged', False)
         net = options.get('net', 'bridge')
         dns = options.get('dns', None)
@@ -323,14 +321,12 @@ def start_container(self, container, intermediate_container=None, **override_opt
         cap_drop = options.get('cap_drop', None)
 
         restart = parse_restart_spec(options.get('restart', None))
-        binds = get_volume_bindings(
-            options.get('volumes'), intermediate_container)
 
         container.start(
             links=self._get_links(link_to_self=options.get('one_off', False)),
             port_bindings=port_bindings,
-            binds=binds,
-            volumes_from=self._get_volumes_from(),
+            binds=volume_bindings,
+            volumes_from=self._get_volumes_from(intermediate_container),
             privileged=privileged,
             network_mode=net,
             dns=dns,
@@ -392,7 +388,7 @@ def _get_links(self, link_to_self):
             links.append((external_link, link_name))
         return links
 
-    def _get_volumes_from(self):
+    def _get_volumes_from(self, intermediate_container=None):
         volumes_from = []
         for volume_source in self.volumes_from:
             if isinstance(volume_source, Service):
@@ -406,6 +402,9 @@ def _get_volumes_from(self):
             elif isinstance(volume_source, Container):
                 volumes_from.append(volume_source.id)
 
+        if intermediate_container:
+            volumes_from.append(intermediate_container.id)
+
         return volumes_from
 
     def _get_container_create_options(self, override_options, one_off=False):
@@ -520,45 +519,6 @@ def pull(self, insecure_registry=False):
             )
 
 
-def get_container_data_volumes(container, volumes_option):
-    """Find the container data volumes that are in `volumes_option`, and return
-    a mapping of volume bindings for those volumes.
-    """
-    volumes = []
-    for volume in volumes_option or []:
-        volume = parse_volume_spec(volume)
-        # No need to preserve host volumes
-        if volume.external:
-            continue
-
-        volume_path = (container.get('Volumes') or {}).get(volume.internal)
-        # New volume, doesn't exist in the old container
-        if not volume_path:
-            continue
-
-        # Copy existing volume from old container
-        volume = volume._replace(external=volume_path)
-        volumes.append(build_volume_binding(volume))
-
-    return dict(volumes)
-
-
-def get_volume_bindings(volumes_option, intermediate_container):
-    """Return a list of volume bindings for a container. Container data volume
-    bindings are replaced by those in the intermediate container.
-    """
-    volume_bindings = dict(
-        build_volume_binding(parse_volume_spec(volume))
-        for volume in volumes_option or []
-        if ':' in volume)
-
-    if intermediate_container:
-        volume_bindings.update(
-            get_container_data_volumes(intermediate_container, volumes_option))
-
-    return volume_bindings
-
-
 NAME_RE = re.compile(r'^([^_]+)_([^_]+)_(run_)?(\d+)$')
 
 

tests/integration/service_test.py

@@ -98,7 +98,7 @@ def test_create_container_with_unspecified_volume(self):
         service = self.create_service('db', volumes=['/var/db'])
         container = service.create_container()
         service.start_container(container)
-        self.assertIn('/var/db', container.get('Volumes'))
+        self.assertIn('/var/db', container.inspect()['Volumes'])
 
     def test_create_container_with_cpu_shares(self):
         service = self.create_service('db', cpu_shares=73)
@@ -148,23 +148,30 @@ def test_recreate_containers(self):
         self.assertIn('FOO=1', old_container.dictionary['Config']['Env'])
         self.assertEqual(old_container.name, 'figtest_db_1')
         service.start_container(old_container)
-        volume_path = old_container.get('Volumes')['/etc']
+        volume_path = old_container.inspect()['Volumes']['/etc']
 
         num_containers_before = len(self.client.containers(all=True))
 
         service.options['environment']['FOO'] = '2'
-        containers = service.recreate_containers()
-        self.assertEqual(len(containers), 1)
+        tuples = service.recreate_containers()
+        self.assertEqual(len(tuples), 1)
 
-        new_container = containers[0]
-        self.assertEqual(new_container.get('Config.Entrypoint'), ['sleep'])
-        self.assertEqual(new_container.get('Config.Cmd'), ['300'])
-        self.assertIn('FOO=2', new_container.get('Config.Env'))
+        intermediate_container = tuples[0][0]
+        new_container = tuples[0][1]
+        self.assertEqual(intermediate_container.dictionary['Config']['Entrypoint'], ['/bin/echo'])
+
+        self.assertEqual(new_container.dictionary['Config']['Entrypoint'], ['sleep'])
+        self.assertEqual(new_container.dictionary['Config']['Cmd'], ['300'])
+        self.assertIn('FOO=2', new_container.dictionary['Config']['Env'])
         self.assertEqual(new_container.name, 'figtest_db_1')
-        self.assertEqual(new_container.get('Volumes')['/etc'], volume_path)
+        self.assertEqual(new_container.inspect()['Volumes']['/etc'], volume_path)
+        self.assertIn(intermediate_container.id, new_container.dictionary['HostConfig']['VolumesFrom'])
 
         self.assertEqual(len(self.client.containers(all=True)), num_containers_before)
         self.assertNotEqual(old_container.id, new_container.id)
+        self.assertRaises(APIError,
+                          self.client.inspect_container,
+                          intermediate_container.id)
 
     def test_recreate_containers_when_containers_are_stopped(self):
         service = self.create_service(
@@ -179,16 +186,6 @@ def test_recreate_containers_when_containers_are_stopped(self):
         service.recreate_containers()
         self.assertEqual(len(service.containers(stopped=True)), 1)
 
-    def test_recreate_containers_with_volume_changes(self):
-        service = self.create_service('withvolumes', volumes=['/etc'])
-        old_container = create_and_start_container(service)
-        self.assertEqual(old_container.get('Volumes').keys(), ['/etc'])
-
-        service = self.create_service('withvolumes')
-        container, = service.recreate_containers()
-        service.start_container(container)
-        self.assertEqual(container.get('Volumes'), {})
-
     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

@@ -11,15 +11,13 @@
 from fig import Service
 from fig.container import Container
 from fig.service import (
-    APIError,
     ConfigError,
+    split_port,
     build_port_bindings,
+    parse_volume_spec,
     build_volume_binding,
-    get_container_data_volumes,
-    get_volume_bindings,
+    APIError,
     parse_repository_tag,
-    parse_volume_spec,
-    split_port,
 )
 
 
@@ -59,6 +57,13 @@ def test_get_volumes_from_container(self):
 
         self.assertEqual(service._get_volumes_from(), [container_id])
 
+    def test_get_volumes_from_intermediate_container(self):
+        container_id = 'aabbccddee'
+        service = Service('test')
+        container = mock.Mock(id=container_id, spec=Container)
+
+        self.assertEqual(service._get_volumes_from(container), [container_id])
+
     def test_get_volumes_from_service_container_exists(self):
         container_ids = ['aabbccddee', '12345']
         from_service = mock.create_autospec(Service)
@@ -283,50 +288,6 @@ def test_building_volume_binding_with_home(self):
             binding,
             ('/home/user', dict(bind='/home/user', ro=False)))
 
-    def test_get_container_data_volumes(self):
-        options = [
-            '/host/volume:/host/volume:ro',
-            '/new/volume',
-            '/existing/volume',
-        ]
-
-        container = Container(None, {
-            'Volumes': {
-                '/host/volume':     '/host/volume',
-                '/existing/volume': '/var/lib/docker/aaaaaaaa',
-                '/removed/volume':  '/var/lib/docker/bbbbbbbb',
-            },
-        }, has_been_inspected=True)
-
-        expected = {
-            '/var/lib/docker/aaaaaaaa': {'bind': '/existing/volume', 'ro': False},
-        }
-
-        binds = get_container_data_volumes(container, options)
-        self.assertEqual(binds, expected)
-
-    def test_get_volume_bindings(self):
-        options = [
-            '/host/volume:/host/volume:ro',
-            '/host/rw/volume:/host/rw/volume',
-            '/new/volume',
-            '/existing/volume',
-        ]
-
-        intermediate_container = Container(None, {
-            'Volumes': {'/existing/volume': '/var/lib/docker/aaaaaaaa'},
-            }, has_been_inspected=True)
-
-        expected = {
-            '/host/volume': {'bind': '/host/volume', 'ro': True},
-            '/host/rw/volume': {'bind': '/host/rw/volume', 'ro': False},
-            '/var/lib/docker/aaaaaaaa': {'bind': '/existing/volume', 'ro': False},
-        }
-
-        binds = get_volume_bindings(options, intermediate_container)
-        self.assertEqual(binds, expected)
-
-
 class ServiceEnvironmentTest(unittest.TestCase):
 
     def setUp(self):