Merge pull request #1960 from dnephin/fix_smart_recreate_when_service_is_removed

Fix config_hash context to properly represent the service

compose/cli/main.py

@@ -326,7 +326,7 @@ class TopLevelCommand(Command):
             log.warn(INSECURE_SSL_WARNING)
 
         if not options['--no-deps']:
-            deps = service.get_linked_names()
+            deps = service.get_linked_service_names()
 
             if len(deps) > 0:
                 project.up(

compose/config/config.py

@@ -22,9 +22,9 @@ from compose.cli.utils import find_candidates_in_parent_dirs
 DOCKER_CONFIG_KEYS = [
     'cap_add',
     'cap_drop',
+    'command',
     'cpu_shares',
     'cpuset',
-    'command',
     'detach',
     'devices',
     'dns',
@@ -38,12 +38,12 @@ DOCKER_CONFIG_KEYS = [
     'image',
     'labels',
     'links',
+    'log_driver',
+    'log_opt',
     'mac_address',
     'mem_limit',
     'memswap_limit',
     'net',
-    'log_driver',
-    'log_opt',
     'pid',
     'ports',
     'privileged',

compose/project.py

@@ -14,7 +14,10 @@ from .const import LABEL_PROJECT
 from .const import LABEL_SERVICE
 from .container import Container
 from .legacy import check_for_legacy_containers
+from .service import ContainerNet
+from .service import Net
 from .service import Service
+from .service import ServiceNet
 from .utils import parallel_execute
 
 
@@ -87,8 +90,14 @@ class Project(object):
             volumes_from = project.get_volumes_from(service_dict)
             net = project.get_net(service_dict)
 
-            project.services.append(Service(client=client, project=name, links=links, net=net,
-                                            volumes_from=volumes_from, **service_dict))
+            project.services.append(
+                Service(
+                    client=client,
+                    project=name,
+                    links=links,
+                    net=net,
+                    volumes_from=volumes_from,
+                    **service_dict))
         return project
 
     @property
@@ -184,30 +193,26 @@ class Project(object):
         return volumes_from
 
     def get_net(self, service_dict):
-        if 'net' in service_dict:
-            net_name = get_service_name_from_net(service_dict.get('net'))
-
-            if net_name:
-                try:
-                    net = self.get_service(net_name)
-                except NoSuchService:
-                    try:
-                        net = Container.from_id(self.client, net_name)
-                    except APIError:
-                        raise ConfigurationError(
-                            'Service "%s" is trying to use the network of "%s", '
-                            'which is not the name of a service or container.' % (
-                                service_dict['name'],
-                                net_name))
-            else:
-                net = service_dict['net']
-
-            del service_dict['net']
-
-        else:
-            net = None
-
-        return net
+        net = service_dict.pop('net', None)
+        if not net:
+            return Net(None)
+
+        net_name = get_service_name_from_net(net)
+        if not net_name:
+            return Net(net)
+
+        try:
+            return ServiceNet(self.get_service(net_name))
+        except NoSuchService:
+            pass
+        try:
+            return ContainerNet(Container.from_id(self.client, net_name))
+        except APIError:
+            raise ConfigurationError(
+                'Service "%s" is trying to use the network of "%s", '
+                'which is not the name of a service or container.' % (
+                    service_dict['name'],
+                    net_name))
 
     def start(self, service_names=None, **options):
         for service in self.get_services(service_names):

compose/service.py

@@ -87,7 +87,16 @@ ConvergencePlan = namedtuple('ConvergencePlan', 'action containers')
 
 
 class Service(object):
-    def __init__(self, name, client=None, project='default', links=None, external_links=None, volumes_from=None, net=None, **options):
+    def __init__(
+        self,
+        name,
+        client=None,
+        project='default',
+        links=None,
+        volumes_from=None,
+        net=None,
+        **options
+    ):
         if not re.match('^%s+$' % VALID_NAME_CHARS, project):
             raise ConfigError('Invalid project name "%s" - only %s are allowed' % (project, VALID_NAME_CHARS))
 
@@ -95,9 +104,8 @@ class Service(object):
         self.client = client
         self.project = project
         self.links = links or []
-        self.external_links = external_links or []
         self.volumes_from = volumes_from or []
-        self.net = net or None
+        self.net = net or Net(None)
         self.options = options
 
     def containers(self, stopped=False, one_off=False, filters={}):
@@ -480,26 +488,26 @@ class Service(object):
         return {
             'options': self.options,
             'image_id': self.image()['Id'],
+            'links': self.get_link_names(),
+            'net': self.net.id,
+            'volumes_from': self.get_volumes_from_names(),
         }
 
     def get_dependency_names(self):
-        net_name = self.get_net_name()
-        return (self.get_linked_names() +
+        net_name = self.net.service_name
+        return (self.get_linked_service_names() +
                 self.get_volumes_from_names() +
                 ([net_name] if net_name else []))
 
-    def get_linked_names(self):
-        return [s.name for (s, _) in self.links]
+    def get_linked_service_names(self):
+        return [service.name for (service, _) in self.links]
+
+    def get_link_names(self):
+        return [(service.name, alias) for service, alias in self.links]
 
     def get_volumes_from_names(self):
         return [s.name for s in self.volumes_from if isinstance(s, Service)]
 
-    def get_net_name(self):
-        if isinstance(self.net, Service):
-            return self.net.name
-        else:
-            return
-
     def get_container_name(self, number, one_off=False):
         # TODO: Implement issue #652 here
         return build_container_name(self.project, self.name, number, one_off)
@@ -528,7 +536,7 @@ class Service(object):
                 links.append((container.name, self.name))
                 links.append((container.name, container.name))
                 links.append((container.name, container.name_without_project))
-        for external_link in self.external_links:
+        for external_link in self.options.get('external_links') or []:
             if ':' not in external_link:
                 link_name = external_link
             else:
@@ -551,25 +559,6 @@ class Service(object):
 
         return volumes_from
 
-    def _get_net(self):
-        if not self.net:
-            return None
-
-        if isinstance(self.net, Service):
-            containers = self.net.containers()
-            if len(containers) > 0:
-                net = 'container:' + containers[0].id
-            else:
-                log.warning("Warning: Service %s is trying to use reuse the network stack "
-                            "of another service that is not running." % (self.net.name))
-                net = None
-        elif isinstance(self.net, Container):
-            net = 'container:' + self.net.id
-        else:
-            net = self.net
-
-        return net
-
     def _get_container_create_options(
             self,
             override_options,
@@ -683,7 +672,7 @@ class Service(object):
             binds=options.get('binds'),
             volumes_from=self._get_volumes_from(),
             privileged=privileged,
-            network_mode=self._get_net(),
+            network_mode=self.net.mode,
             devices=devices,
             dns=dns,
             dns_search=dns_search,
@@ -782,6 +771,61 @@ class Service(object):
         stream_output(output, sys.stdout)
 
 
+class Net(object):
+    """A `standard` network mode (ex: host, bridge)"""
+
+    service_name = None
+
+    def __init__(self, net):
+        self.net = net
+
+    @property
+    def id(self):
+        return self.net
+
+    mode = id
+
+
+class ContainerNet(object):
+    """A network mode that uses a container's network stack."""
+
+    service_name = None
+
+    def __init__(self, container):
+        self.container = container
+
+    @property
+    def id(self):
+        return self.container.id
+
+    @property
+    def mode(self):
+        return 'container:' + self.container.id
+
+
+class ServiceNet(object):
+    """A network mode that uses a service's network stack."""
+
+    def __init__(self, service):
+        self.service = service
+
+    @property
+    def id(self):
+        return self.service.name
+
+    service_name = id
+
+    @property
+    def mode(self):
+        containers = self.service.containers()
+        if containers:
+            return 'container:' + containers[0].id
+
+        log.warn("Warning: Service %s is trying to use reuse the network stack "
+                 "of another service that is not running." % (self.id))
+        return None
+
+
 # Names
 
 

tests/integration/project_test.py

@@ -112,7 +112,7 @@ class ProjectTest(DockerClientTestCase):
 
         web = project.get_service('web')
         net = project.get_service('net')
-        self.assertEqual(web._get_net(), 'container:' + net.containers()[0].id)
+        self.assertEqual(web.net.mode, 'container:' + net.containers()[0].id)
 
     def test_net_from_container(self):
         net_container = Container.create(
@@ -138,7 +138,7 @@ class ProjectTest(DockerClientTestCase):
         project.up()
 
         web = project.get_service('web')
-        self.assertEqual(web._get_net(), 'container:' + net_container.id)
+        self.assertEqual(web.net.mode, 'container:' + net_container.id)
 
     def test_start_pause_unpause_stop_kill_remove(self):
         web = self.create_service('web')

tests/integration/service_test.py

@@ -22,6 +22,7 @@ from compose.container import Container
 from compose.service import build_extra_hosts
 from compose.service import ConfigError
 from compose.service import ConvergencePlan
+from compose.service import Net
 from compose.service import Service
 
 
@@ -707,17 +708,17 @@ class ServiceTest(DockerClientTestCase):
             self.assertEqual(list(container.inspect()['HostConfig']['PortBindings'].keys()), ['8000/tcp'])
 
     def test_network_mode_none(self):
-        service = self.create_service('web', net='none')
+        service = self.create_service('web', net=Net('none'))
         container = create_and_start_container(service)
         self.assertEqual(container.get('HostConfig.NetworkMode'), 'none')
 
     def test_network_mode_bridged(self):
-        service = self.create_service('web', net='bridge')
+        service = self.create_service('web', net=Net('bridge'))
         container = create_and_start_container(service)
         self.assertEqual(container.get('HostConfig.NetworkMode'), 'bridge')
 
     def test_network_mode_host(self):
-        service = self.create_service('web', net='host')
+        service = self.create_service('web', net=Net('host'))
         container = create_and_start_container(service)
         self.assertEqual(container.get('HostConfig.NetworkMode'), 'host')
 

tests/integration/state_test.py

@@ -1,3 +1,7 @@
+"""
+Integration tests which cover state convergence (aka smart recreate) performed
+by `docker-compose up`.
+"""
 from __future__ import unicode_literals
 
 import os
@@ -151,6 +155,24 @@ class ProjectWithDependenciesTest(ProjectTestCase):
 
         self.assertEqual(new_containers - old_containers, set())
 
+    def test_service_removed_while_down(self):
+        next_cfg = {
+            'web': {
+                'image': 'busybox:latest',
+                'command': 'tail -f /dev/null',
+            },
+            'nginx': self.cfg['nginx'],
+        }
+
+        containers = self.run_up(self.cfg)
+        self.assertEqual(len(containers), 3)
+
+        project = self.make_project(self.cfg)
+        project.stop(timeout=1)
+
+        containers = self.run_up(next_cfg)
+        self.assertEqual(len(containers), 2)
+
 
 def converge(service,
              allow_recreate=True,

tests/unit/project_test.py

@@ -221,7 +221,7 @@ class ProjectTest(unittest.TestCase):
             }
         ], self.mock_client)
         service = project.get_service('test')
-        self.assertEqual(service._get_net(), None)
+        self.assertEqual(service.net.id, None)
         self.assertNotIn('NetworkMode', service._get_container_host_config({}))
 
     def test_use_net_from_container(self):
@@ -236,7 +236,7 @@ class ProjectTest(unittest.TestCase):
             }
         ], self.mock_client)
         service = project.get_service('test')
-        self.assertEqual(service._get_net(), 'container:' + container_id)
+        self.assertEqual(service.net.mode, 'container:' + container_id)
 
     def test_use_net_from_service(self):
         container_name = 'test_aaa_1'
@@ -261,7 +261,7 @@ class ProjectTest(unittest.TestCase):
         ], self.mock_client)
 
         service = project.get_service('test')
-        self.assertEqual(service._get_net(), 'container:' + container_name)
+        self.assertEqual(service.net.mode, 'container:' + container_name)
 
     def test_container_without_name(self):
         self.mock_client.containers.return_value = [

tests/unit/service_test.py

@@ -13,13 +13,16 @@ from compose.const import LABEL_SERVICE
 from compose.container import Container
 from compose.service import build_volume_binding
 from compose.service import ConfigError
+from compose.service import ContainerNet
 from compose.service import get_container_data_volumes
 from compose.service import merge_volume_bindings
 from compose.service import NeedsBuildError
+from compose.service import Net
 from compose.service import NoSuchImageError
 from compose.service import parse_repository_tag
 from compose.service import parse_volume_spec
 from compose.service import Service
+from compose.service import ServiceNet
 
 
 class ServiceTest(unittest.TestCase):
@@ -189,7 +192,7 @@ class ServiceTest(unittest.TestCase):
 
         self.assertEqual(
             opts['labels'][LABEL_CONFIG_HASH],
-            'b30306d0a73b67f67a45b99b88d36c359e470e6fa0c04dda1cf62d2087205b81')
+            '3c85881a8903b9d73a06c41860c8be08acce1494ab4cf8408375966dccd714de')
         self.assertEqual(
             opts['environment'],
             {
@@ -331,6 +334,90 @@ class ServiceTest(unittest.TestCase):
         self.assertEqual(self.mock_client.build.call_count, 1)
         self.assertFalse(self.mock_client.build.call_args[1]['pull'])
 
+    def test_config_dict(self):
+        self.mock_client.inspect_image.return_value = {'Id': 'abcd'}
+        service = Service(
+            'foo',
+            image='example.com/foo',
+            client=self.mock_client,
+            net=ServiceNet(Service('other')),
+            links=[(Service('one'), 'one')],
+            volumes_from=[Service('two')])
+
+        config_dict = service.config_dict()
+        expected = {
+            'image_id': 'abcd',
+            'options': {'image': 'example.com/foo'},
+            'links': [('one', 'one')],
+            'net': 'other',
+            'volumes_from': ['two'],
+        }
+        self.assertEqual(config_dict, expected)
+
+    def test_config_dict_with_net_from_container(self):
+        self.mock_client.inspect_image.return_value = {'Id': 'abcd'}
+        container = Container(
+            self.mock_client,
+            {'Id': 'aaabbb', 'Name': '/foo_1'})
+        service = Service(
+            'foo',
+            image='example.com/foo',
+            client=self.mock_client,
+            net=container)
+
+        config_dict = service.config_dict()
+        expected = {
+            'image_id': 'abcd',
+            'options': {'image': 'example.com/foo'},
+            'links': [],
+            'net': 'aaabbb',
+            'volumes_from': [],
+        }
+        self.assertEqual(config_dict, expected)
+
+
+class NetTestCase(unittest.TestCase):
+
+    def test_net(self):
+        net = Net('host')
+        self.assertEqual(net.id, 'host')
+        self.assertEqual(net.mode, 'host')
+        self.assertEqual(net.service_name, None)
+
+    def test_net_container(self):
+        container_id = 'abcd'
+        net = ContainerNet(Container(None, {'Id': container_id}))
+        self.assertEqual(net.id, container_id)
+        self.assertEqual(net.mode, 'container:' + container_id)
+        self.assertEqual(net.service_name, None)
+
+    def test_net_service(self):
+        container_id = 'bbbb'
+        service_name = 'web'
+        mock_client = mock.create_autospec(docker.Client)
+        mock_client.containers.return_value = [
+            {'Id': container_id, 'Name': container_id, 'Image': 'abcd'},
+        ]
+
+        service = Service(name=service_name, client=mock_client)
+        net = ServiceNet(service)
+
+        self.assertEqual(net.id, service_name)
+        self.assertEqual(net.mode, 'container:' + container_id)
+        self.assertEqual(net.service_name, service_name)
+
+    def test_net_service_no_containers(self):
+        service_name = 'web'
+        mock_client = mock.create_autospec(docker.Client)
+        mock_client.containers.return_value = []
+
+        service = Service(name=service_name, client=mock_client)
+        net = ServiceNet(service)
+
+        self.assertEqual(net.id, service_name)
+        self.assertEqual(net.mode, None)
+        self.assertEqual(net.service_name, service_name)
+
 
 def mock_get_image(images):
     if images: