Fixed depends_on recreation behaviour for issue #6589

Previously any containers which did *not* have any links were always recreated.
In order to fix depends_on and preserve expected links recreation behaviour, we now only use the ConvergenceStrategy.always recreation strategy for a service if any of the the following conditions are true:
* --always-recreate-deps flag provided
* service container is stopped
* service defines links but the container does not have any
* container has links but the service definition does not

Signed-off-by: joeweoj <joewardell@gmail.com>

2 files changed, 143 insertions, 2 deletions

diff --git a/compose/project.py b/compose/project.py
index a7403372..e3b14d62 100644
--- a/compose/project.py
+++ b/compose/project.py
@@ -586,8 +586,10 @@ class Project(object):
                           ", ".join(updated_dependencies))
                 containers_stopped = any(
                     service.containers(stopped=True, filters={'status': ['created', 'exited']}))
-                has_links = any(c.get('HostConfig.Links') for c in service.containers())
-                if always_recreate_deps or containers_stopped or not has_links:
+                service_has_links = any(service.get_link_names())
+                container_has_links = any(c.get('HostConfig.Links') for c in service.containers())
+                should_recreate_for_links = service_has_links ^ container_has_links
+                if always_recreate_deps or containers_stopped or should_recreate_for_links:
                     plan = service.convergence_plan(ConvergenceStrategy.always)
                 else:
                     plan = service.convergence_plan(strategy)
diff --git a/tests/integration/state_test.py b/tests/integration/state_test.py
index b7d38a4b..0d69217c 100644
--- a/tests/integration/state_test.py
+++ b/tests/integration/state_test.py
@@ -5,6 +5,8 @@ by `docker-compose up`.
 from __future__ import absolute_import
 from __future__ import unicode_literals
 
+import copy
+
 import py
 from docker.errors import ImageNotFound
 
@@ -209,6 +211,143 @@ class ProjectWithDependenciesTest(ProjectTestCase):
         }
 
 
+class ProjectWithDependsOnDependenciesTest(ProjectTestCase):
+    def setUp(self):
+        super(ProjectWithDependsOnDependenciesTest, self).setUp()
+
+        self.cfg = {
+            'version': '2',
+            'services': {
+                'db': {
+                    'image': 'busybox:latest',
+                    'command': 'tail -f /dev/null',
+                },
+                'web': {
+                    'image': 'busybox:latest',
+                    'command': 'tail -f /dev/null',
+                    'depends_on': ['db'],
+                },
+                'nginx': {
+                    'image': 'busybox:latest',
+                    'command': 'tail -f /dev/null',
+                    'depends_on': ['web'],
+                },
+            }
+        }
+
+    def test_up(self):
+        local_cfg = copy.deepcopy(self.cfg)
+        containers = self.run_up(local_cfg)
+        assert set(c.service for c in containers) == set(['db', 'web', 'nginx'])
+
+    def test_change_leaf(self):
+        local_cfg = copy.deepcopy(self.cfg)
+        old_containers = self.run_up(local_cfg)
+
+        local_cfg['services']['nginx']['environment'] = {'NEW_VAR': '1'}
+        new_containers = self.run_up(local_cfg)
+
+        assert set(c.service for c in new_containers - old_containers) == set(['nginx'])
+
+    def test_change_middle(self):
+        local_cfg = copy.deepcopy(self.cfg)
+        old_containers = self.run_up(local_cfg)
+
+        local_cfg['services']['web']['environment'] = {'NEW_VAR': '1'}
+        new_containers = self.run_up(local_cfg)
+
+        assert set(c.service for c in new_containers - old_containers) == set(['web'])
+
+    def test_change_middle_always_recreate_deps(self):
+        local_cfg = copy.deepcopy(self.cfg)
+        old_containers = self.run_up(local_cfg, always_recreate_deps=True)
+
+        local_cfg['services']['web']['environment'] = {'NEW_VAR': '1'}
+        new_containers = self.run_up(local_cfg, always_recreate_deps=True)
+
+        assert set(c.service for c in new_containers - old_containers) == set(['web', 'nginx'])
+
+    def test_change_root(self):
+        local_cfg = copy.deepcopy(self.cfg)
+        old_containers = self.run_up(local_cfg)
+
+        local_cfg['services']['db']['environment'] = {'NEW_VAR': '1'}
+        new_containers = self.run_up(local_cfg)
+
+        assert set(c.service for c in new_containers - old_containers) == set(['db'])
+
+    def test_change_root_always_recreate_deps(self):
+        local_cfg = copy.deepcopy(self.cfg)
+        old_containers = self.run_up(local_cfg, always_recreate_deps=True)
+
+        local_cfg['services']['db']['environment'] = {'NEW_VAR': '1'}
+        new_containers = self.run_up(local_cfg, always_recreate_deps=True)
+
+        assert set(c.service for c in new_containers - old_containers) == set(['db', 'web', 'nginx'])
+
+    def test_change_root_no_recreate(self):
+        local_cfg = copy.deepcopy(self.cfg)
+        old_containers = self.run_up(local_cfg)
+
+        local_cfg['services']['db']['environment'] = {'NEW_VAR': '1'}
+        new_containers = self.run_up(
+            local_cfg,
+            strategy=ConvergenceStrategy.never)
+
+        assert new_containers - old_containers == set()
+
+    def test_service_removed_while_down(self):
+        local_cfg = copy.deepcopy(self.cfg)
+        next_cfg = copy.deepcopy(self.cfg)
+        del next_cfg['services']['db']
+        del next_cfg['services']['web']['depends_on']
+
+        containers = self.run_up(local_cfg)
+        assert set(c.service for c in containers) == set(['db', 'web', 'nginx'])
+
+        project = self.make_project(local_cfg)
+        project.stop(timeout=1)
+
+        next_containers = self.run_up(next_cfg)
+        assert set(c.service for c in next_containers) == set(['web', 'nginx'])
+
+    def test_service_removed_while_up(self):
+        local_cfg = copy.deepcopy(self.cfg)
+        containers = self.run_up(local_cfg)
+        assert set(c.service for c in containers) == set(['db', 'web', 'nginx'])
+
+        del local_cfg['services']['db']
+        del local_cfg['services']['web']['depends_on']
+
+        containers = self.run_up(local_cfg)
+        assert set(c.service for c in containers) == set(['web', 'nginx'])
+
+    def test_dependency_removed(self):
+        local_cfg = copy.deepcopy(self.cfg)
+        next_cfg = copy.deepcopy(self.cfg)
+        del next_cfg['services']['nginx']['depends_on']
+
+        containers = self.run_up(local_cfg, service_names=['nginx'])
+        assert set(c.service for c in containers) == set(['db', 'web', 'nginx'])
+
+        project = self.make_project(local_cfg)
+        project.stop(timeout=1)
+
+        next_containers = self.run_up(next_cfg, service_names=['nginx'])
+        assert set(c.service for c in next_containers if c.is_running) == set(['nginx'])
+
+    def test_dependency_added(self):
+        local_cfg = copy.deepcopy(self.cfg)
+
+        del local_cfg['services']['nginx']['depends_on']
+        containers = self.run_up(local_cfg, service_names=['nginx'])
+        assert set(c.service for c in containers) == set(['nginx'])
+
+        local_cfg['services']['nginx']['depends_on'] = ['db']
+        containers = self.run_up(local_cfg, service_names=['nginx'])
+        assert set(c.service for c in containers) == set(['nginx', 'db'])
+
+
 class ServiceStateTest(DockerClientTestCase):
     """Test cases for Service.convergence_plan."""