Move converge() to a test module, and use a short timeout for tests.

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

compose/cli/main.py

@@ -10,7 +10,9 @@ import sys
 from docker.errors import APIError
 import dockerpty
 
-from .. import __version__, legacy
+from .. import __version__
+from .. import legacy
+from ..const import DEFAULT_TIMEOUT
 from ..project import NoSuchService, ConfigurationError
 from ..service import BuildError, NeedsBuildError
 from ..config import parse_environment
@@ -394,9 +396,8 @@ class TopLevelCommand(Command):
           -t, --timeout TIMEOUT      Specify a shutdown timeout in seconds.
                                      (default: 10)
         """
-        timeout = options.get('--timeout')
+        timeout = float(options.get('--timeout') or DEFAULT_TIMEOUT)
-        params = {} if timeout is None else {'timeout': int(timeout)}
+        project.stop(service_names=options['SERVICE'], timeout=timeout)
-        project.stop(service_names=options['SERVICE'], **params)
 
     def restart(self, project, options):
         """
@@ -408,9 +409,8 @@ class TopLevelCommand(Command):
           -t, --timeout TIMEOUT      Specify a shutdown timeout in seconds.
                                      (default: 10)
         """
-        timeout = options.get('--timeout')
+        timeout = float(options.get('--timeout') or DEFAULT_TIMEOUT)
-        params = {} if timeout is None else {'timeout': int(timeout)}
+        project.restart(service_names=options['SERVICE'], timeout=timeout)
-        project.restart(service_names=options['SERVICE'], **params)
 
     def up(self, project, options):
         """
@@ -452,7 +452,7 @@ class TopLevelCommand(Command):
         allow_recreate = not options['--no-recreate']
         smart_recreate = options['--x-smart-recreate']
         service_names = options['SERVICE']
-        timeout = int(options['--timeout']) if options['--timeout'] is not None else None
+        timeout = float(options.get('--timeout') or DEFAULT_TIMEOUT)
 
         project.up(
             service_names=service_names,
@@ -479,8 +479,7 @@ class TopLevelCommand(Command):
                 signal.signal(signal.SIGINT, handler)
 
                 print("Gracefully stopping... (press Ctrl+C again to force)")
-                params = {} if timeout is None else {'timeout': timeout}
+                project.stop(service_names=service_names, timeout=timeout)
-                project.stop(service_names=service_names, **params)
 
     def migrate_to_labels(self, project, _options):
         """

compose/const.py

@@ -1,4 +1,5 @@
 
+DEFAULT_TIMEOUT = 10
 LABEL_CONTAINER_NUMBER = 'com.docker.compose.container-number'
 LABEL_ONE_OFF = 'com.docker.compose.oneoff'
 LABEL_PROJECT = 'com.docker.compose.project'

compose/project.py

@@ -6,7 +6,7 @@ from functools import reduce
 from docker.errors import APIError
 
 from .config import get_service_name_from_net, ConfigurationError
-from .const import LABEL_PROJECT, LABEL_SERVICE, LABEL_ONE_OFF
+from .const import LABEL_PROJECT, LABEL_SERVICE, LABEL_ONE_OFF, DEFAULT_TIMEOUT
 from .service import Service
 from .container import Container
 from .legacy import check_for_legacy_containers
@@ -212,7 +212,7 @@ class Project(object):
            smart_recreate=False,
            insecure_registry=False,
            do_build=True,
-           timeout=None):
+           timeout=DEFAULT_TIMEOUT):
 
         services = self.get_services(service_names, include_deps=start_deps)
 

compose/service.py

@@ -13,6 +13,7 @@ from docker.utils import create_host_config, LogConfig
 from . import __version__
 from .config import DOCKER_CONFIG_KEYS, merge_environment
 from .const import (
+    DEFAULT_TIMEOUT,
     LABEL_CONTAINER_NUMBER,
     LABEL_ONE_OFF,
     LABEL_PROJECT,
@@ -251,26 +252,6 @@ class Service(object):
         else:
             return self.options['image']
 
-    def converge(self,
-                 allow_recreate=True,
-                 smart_recreate=False,
-                 insecure_registry=False,
-                 do_build=True):
-        """
-        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.
-        """
-        plan = self.convergence_plan(
-            allow_recreate=allow_recreate,
-            smart_recreate=smart_recreate,
-        )
-
-        return self.execute_convergence_plan(
-            plan,
-            insecure_registry=insecure_registry,
-            do_build=do_build,
-        )
-
     def convergence_plan(self,
                          allow_recreate=True,
                          smart_recreate=False):
@@ -312,7 +293,7 @@ class Service(object):
                                  plan,
                                  insecure_registry=False,
                                  do_build=True,
-                                 timeout=None):
+                                 timeout=DEFAULT_TIMEOUT):
         (action, containers) = plan
 
         if action == 'create':
@@ -352,7 +333,7 @@ class Service(object):
     def recreate_container(self,
                            container,
                            insecure_registry=False,
-                           timeout=None):
+                           timeout=DEFAULT_TIMEOUT):
         """Recreate a container.
 
         The original container is renamed to a temporary name so that data
@@ -361,8 +342,7 @@ class Service(object):
         """
         log.info("Recreating %s..." % container.name)
         try:
-            stop_params = {} if timeout is None else {'timeout': timeout}
+            container.stop(timeout=timeout)
-            container.stop(**stop_params)
         except APIError as e:
             if (e.response.status_code == 500
                     and e.explanation

tests/integration/service_test.py

@@ -2,8 +2,9 @@ from __future__ import unicode_literals
 from __future__ import absolute_import
 import os
 from os import path
-import mock
 
+from docker.errors import APIError
+import mock
 import tempfile
 import shutil
 import six
@@ -18,11 +19,11 @@ from compose.const import (
 )
 from compose.service import (
     ConfigError,
+    ConvergencePlan,
     Service,
     build_extra_hosts,
 )
 from compose.container import Container
-from docker.errors import APIError
 from .testcases import DockerClientTestCase
 
 
@@ -249,7 +250,7 @@ class ServiceTest(DockerClientTestCase):
         self.assertIn(volume_container_2.id,
                       host_container.get('HostConfig.VolumesFrom'))
 
-    def test_converge(self):
+    def test_execute_convergence_plan_recreate(self):
         service = self.create_service(
             'db',
             environment={'FOO': '1'},
@@ -269,7 +270,8 @@ class ServiceTest(DockerClientTestCase):
         num_containers_before = len(self.client.containers(all=True))
 
         service.options['environment']['FOO'] = '2'
-        new_container = service.converge()[0]
+        new_container, = service.execute_convergence_plan(
+            ConvergencePlan('recreate', [old_container]))
 
         self.assertEqual(new_container.get('Config.Entrypoint'), ['top'])
         self.assertEqual(new_container.get('Config.Cmd'), ['-d', '1'])
@@ -286,7 +288,7 @@ class ServiceTest(DockerClientTestCase):
                           self.client.inspect_container,
                           old_container.id)
 
-    def test_converge_when_containers_are_stopped(self):
+    def test_execute_convergence_plan_when_containers_are_stopped(self):
         service = self.create_service(
             'db',
             environment={'FOO': '1'},
@@ -295,11 +297,21 @@ class ServiceTest(DockerClientTestCase):
             command=['-d', '1']
         )
         service.create_container()
-        self.assertEqual(len(service.containers(stopped=True)), 1)
-        service.converge()
-        self.assertEqual(len(service.containers(stopped=True)), 1)
 
-    def test_converge_with_image_declared_volume(self):
+        containers = service.containers(stopped=True)
+        self.assertEqual(len(containers), 1)
+        container, = containers
+        self.assertFalse(container.is_running)
+
+        service.execute_convergence_plan(ConvergencePlan('start', [container]))
+
+        containers = service.containers()
+        self.assertEqual(len(containers), 1)
+        container.inspect()
+        self.assertEqual(container, containers[0])
+        self.assertTrue(container.is_running)
+
+    def test_execute_convergence_plan_with_image_declared_volume(self):
         service = Service(
             project='composetest',
             name='db',
@@ -311,7 +323,8 @@ class ServiceTest(DockerClientTestCase):
         self.assertEqual(old_container.get('Volumes').keys(), ['/data'])
         volume_path = old_container.get('Volumes')['/data']
 
-        new_container = service.converge()[0]
+        new_container, = service.execute_convergence_plan(
+            ConvergencePlan('recreate', [old_container]))
         self.assertEqual(new_container.get('Volumes').keys(), ['/data'])
         self.assertEqual(new_container.get('Volumes')['/data'], volume_path)
 

tests/integration/state_test.py

@@ -12,8 +12,8 @@ from .testcases import DockerClientTestCase
 
 class ProjectTestCase(DockerClientTestCase):
     def run_up(self, cfg, **kwargs):
-        if 'smart_recreate' not in kwargs:
+        kwargs.setdefault('smart_recreate', True)
-            kwargs['smart_recreate'] = True
+        kwargs.setdefault('timeout', 0.1)
 
         project = self.make_project(cfg)
         project.up(**kwargs)
@@ -153,7 +153,31 @@ class ProjectWithDependenciesTest(ProjectTestCase):
         self.assertEqual(new_containers - old_containers, set())
 
 
+def converge(service,
+             allow_recreate=True,
+             smart_recreate=False,
+             insecure_registry=False,
+             do_build=True):
+    """
+    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.
+    """
+    plan = service.convergence_plan(
+        allow_recreate=allow_recreate,
+        smart_recreate=smart_recreate,
+    )
+
+    return service.execute_convergence_plan(
+        plan,
+        insecure_registry=insecure_registry,
+        do_build=do_build,
+        timeout=0.1,
+    )
+
+
 class ServiceStateTest(DockerClientTestCase):
+    """Test cases for Service.convergence_plan."""
+
     def test_trigger_create(self):
         web = self.create_service('web')
         self.assertEqual(('create', []), web.convergence_plan(smart_recreate=True))
@@ -250,15 +274,15 @@ class ConfigHashTest(DockerClientTestCase):
 
     def test_config_hash_with_custom_labels(self):
         web = self.create_service('web', labels={'foo': '1'})
-        container = web.converge()[0]
+        container = converge(web)[0]
         self.assertIn(LABEL_CONFIG_HASH, container.labels)
         self.assertIn('foo', container.labels)
 
     def test_config_hash_sticks_around(self):
         web = self.create_service('web', command=["top"])
-        container = web.converge()[0]
+        container = converge(web)[0]
         self.assertIn(LABEL_CONFIG_HASH, container.labels)
 
         web = self.create_service('web', command=["top", "-d", "1"])
-        container = web.converge()[0]
+        container = converge(web)[0]
         self.assertIn(LABEL_CONFIG_HASH, container.labels)

tests/unit/service_test.py

@@ -246,7 +246,7 @@ class ServiceTest(unittest.TestCase):
         service.image = lambda: {'Id': 'abc123'}
         new_container = service.recreate_container(mock_container)
 
-        mock_container.stop.assert_called_once_with()
+        mock_container.stop.assert_called_once_with(timeout=10)
         self.mock_client.rename.assert_called_once_with(
             mock_container.id,
             '%s_%s' % (mock_container.short_id, mock_container.name))