Merge pull request #1690 from aanand/bump-1.3.2

Bump 1.3.2

CHANGES.md

@@ -1,6 +1,21 @@
 Change log
 ==========
 
+1.3.2 (2015-07-14)
+------------------
+
+The following bugs have been fixed:
+
+- When there were one-off containers created by running `docker-compose run` on an older version of Compose, `docker-compose run` would fail with a name collision. Compose now shows an error if you have leftover containers of this type lying around, and tells you how to remove them.
+- Compose was not reading Docker authentication config files created in the new location, `~/docker/config.json`, and authentication against private registries would therefore fail.
+- When a container had a pseudo-TTY attached, its output in `docker-compose up` would be truncated.
+- `docker-compose up --x-smart-recreate` would sometimes fail when an image tag was updated.
+- `docker-compose up` would sometimes create two containers with the same numeric suffix.
+- `docker-compose rm` and `docker-compose ps` would sometimes list services that aren't part of the current project (though no containers were erroneously removed).
+- Some `docker-compose` commands would not show an error if invalid service names were passed in.
+
+Thanks @dano, @josephpage, @kevinsimper, @lieryan, @phemmer, @soulrebel and @sschepens!
+
 1.3.1 (2015-06-21)
 ------------------
 

compose/__init__.py

@@ -1,3 +1,3 @@
 from __future__ import unicode_literals
 
-__version__ = '1.3.1'
+__version__ = '1.3.2'

compose/cli/main.py

@@ -11,6 +11,7 @@ from docker.errors import APIError
 import dockerpty
 
 from .. import legacy
+from ..const import DEFAULT_TIMEOUT
 from ..project import NoSuchService, ConfigurationError
 from ..service import BuildError, NeedsBuildError
 from ..config import parse_environment
@@ -32,7 +33,7 @@ def main():
     except KeyboardInterrupt:
         log.error("\nAborting.")
         sys.exit(1)
-    except (UserError, NoSuchService, ConfigurationError, legacy.LegacyContainersError) as e:
+    except (UserError, NoSuchService, ConfigurationError, legacy.LegacyError) as e:
         log.error(e.msg)
         sys.exit(1)
     except NoSuchCommand as e:
@@ -333,12 +334,22 @@ class TopLevelCommand(Command):
         if not options['--service-ports']:
             container_options['ports'] = []
 
-        container = service.create_container(
-            quiet=True,
-            one_off=True,
-            insecure_registry=insecure_registry,
-            **container_options
-        )
+        try:
+            container = service.create_container(
+                quiet=True,
+                one_off=True,
+                insecure_registry=insecure_registry,
+                **container_options
+            )
+        except APIError as e:
+            legacy.check_for_legacy_containers(
+                project.client,
+                project.name,
+                [service.name],
+                allow_one_off=False,
+            )
+
+            raise e
 
         if options['-d']:
             service.start_container(container)
@@ -392,9 +403,8 @@ class TopLevelCommand(Command):
           -t, --timeout TIMEOUT      Specify a shutdown timeout in seconds.
                                      (default: 10)
         """
-        timeout = options.get('--timeout')
-        params = {} if timeout is None else {'timeout': int(timeout)}
-        project.stop(service_names=options['SERVICE'], **params)
+        timeout = float(options.get('--timeout') or DEFAULT_TIMEOUT)
+        project.stop(service_names=options['SERVICE'], timeout=timeout)
 
     def restart(self, project, options):
         """
@@ -406,9 +416,8 @@ class TopLevelCommand(Command):
           -t, --timeout TIMEOUT      Specify a shutdown timeout in seconds.
                                      (default: 10)
         """
-        timeout = options.get('--timeout')
-        params = {} if timeout is None else {'timeout': int(timeout)}
-        project.restart(service_names=options['SERVICE'], **params)
+        timeout = float(options.get('--timeout') or DEFAULT_TIMEOUT)
+        project.restart(service_names=options['SERVICE'], timeout=timeout)
 
     def up(self, project, options):
         """
@@ -437,9 +446,9 @@ class TopLevelCommand(Command):
                                    image needs to be updated. (EXPERIMENTAL)
             --no-recreate          If containers already exist, don't recreate them.
             --no-build             Don't build an image, even if it's missing
-            -t, --timeout TIMEOUT  When attached, use this timeout in seconds
-                                   for the shutdown. (default: 10)
-
+            -t, --timeout TIMEOUT  Use this timeout in seconds for container shutdown
+                                   when attached or when containers are already
+                                   running. (default: 10)
         """
         insecure_registry = options['--allow-insecure-ssl']
         detached = options['-d']
@@ -450,6 +459,7 @@ class TopLevelCommand(Command):
         allow_recreate = not options['--no-recreate']
         smart_recreate = options['--x-smart-recreate']
         service_names = options['SERVICE']
+        timeout = float(options.get('--timeout') or DEFAULT_TIMEOUT)
 
         project.up(
             service_names=service_names,
@@ -458,6 +468,7 @@ class TopLevelCommand(Command):
             smart_recreate=smart_recreate,
             insecure_registry=insecure_registry,
             do_build=not options['--no-build'],
+            timeout=timeout
         )
 
         to_attach = [c for s in project.get_services(service_names) for c in s.containers()]
@@ -475,9 +486,7 @@ class TopLevelCommand(Command):
                 signal.signal(signal.SIGINT, handler)
 
                 print("Gracefully stopping... (press Ctrl+C again to force)")
-                timeout = options.get('--timeout')
-                params = {} if timeout is None else {'timeout': int(timeout)}
-                project.stop(service_names=service_names, **params)
+                project.stop(service_names=service_names, timeout=timeout)
 
     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/legacy.py

@@ -1,6 +1,7 @@
 import logging
 import re
 
+from .const import LABEL_VERSION
 from .container import get_container_name, Container
 
 
@@ -24,41 +25,82 @@ Alternatively, remove them:
     $ docker rm -f {rm_args}
 """
 
+ONE_OFF_ADDENDUM_FORMAT = """
+You should also remove your one-off containers:
+
+    $ docker rm -f {rm_args}
+"""
+
+ONE_OFF_ERROR_MESSAGE_FORMAT = """
+Compose found the following containers without labels:
+
+{names_list}
+
+As of Compose 1.3.0, containers are identified with labels instead of naming convention.
+
+Remove them before continuing:
+
+    $ docker rm -f {rm_args}
+"""
+
 
 def check_for_legacy_containers(
         client,
         project,
         services,
-        stopped=False,
-        one_off=False):
+        allow_one_off=True):
     """Check if there are containers named using the old naming convention
     and warn the user that those containers may need to be migrated to
     using labels, so that compose can find them.
     """
-    containers = list(get_legacy_containers(
-        client,
-        project,
-        services,
-        stopped=stopped,
-        one_off=one_off))
+    containers = get_legacy_containers(client, project, services, one_off=False)
 
     if containers:
-        raise LegacyContainersError([c.name for c in containers])
+        one_off_containers = get_legacy_containers(client, project, services, one_off=True)
+
+        raise LegacyContainersError(
+            [c.name for c in containers],
+            [c.name for c in one_off_containers],
+        )
+
+    if not allow_one_off:
+        one_off_containers = get_legacy_containers(client, project, services, one_off=True)
+
+        if one_off_containers:
+            raise LegacyOneOffContainersError(
+                [c.name for c in one_off_containers],
+            )
+
+
+class LegacyError(Exception):
+    def __unicode__(self):
+        return self.msg
+
+    __str__ = __unicode__
 
 
-class LegacyContainersError(Exception):
-    def __init__(self, names):
+class LegacyContainersError(LegacyError):
+    def __init__(self, names, one_off_names):
         self.names = names
+        self.one_off_names = one_off_names
 
         self.msg = ERROR_MESSAGE_FORMAT.format(
             names_list="\n".join("    {}".format(name) for name in names),
             rm_args=" ".join(names),
         )
 
-    def __unicode__(self):
-        return self.msg
+        if one_off_names:
+            self.msg += ONE_OFF_ADDENDUM_FORMAT.format(rm_args=" ".join(one_off_names))
 
-    __str__ = __unicode__
+
+class LegacyOneOffContainersError(LegacyError):
+    def __init__(self, one_off_names):
+        self.one_off_names = one_off_names
+
+        self.msg = ONE_OFF_ERROR_MESSAGE_FORMAT.format(
+            names_list="\n".join("    {}".format(name) for name in one_off_names),
+            rm_args=" ".join(one_off_names),
+        )
 
 
 def add_labels(project, container):
@@ -76,8 +118,8 @@ def migrate_project_to_labels(project):
         project.client,
         project.name,
         project.service_names,
-        stopped=True,
-        one_off=False)
+        one_off=False,
+    )
 
     for container in containers:
         add_labels(project, container)
@@ -87,13 +129,29 @@ def get_legacy_containers(
         client,
         project,
         services,
-        stopped=False,
         one_off=False):
 
-    containers = client.containers(all=stopped)
+    return list(_get_legacy_containers_iter(
+        client,
+        project,
+        services,
+        one_off=one_off,
+    ))
+
+
+def _get_legacy_containers_iter(
+        client,
+        project,
+        services,
+        one_off=False):
+
+    containers = client.containers(all=True)
 
     for service in services:
         for container in containers:
+            if LABEL_VERSION in container['Labels']:
+                continue
+
             name = get_container_name(container)
             if has_container(project, service, name, one_off=one_off):
                 yield Container.from_ps(client, container)

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
@@ -99,6 +99,16 @@ class Project(object):
 
         raise NoSuchService(name)
 
+    def validate_service_names(self, service_names):
+        """
+        Validate that the given list of service names only contains valid
+        services. Raises NoSuchService if one of the names is invalid.
+        """
+        valid_names = self.service_names
+        for name in service_names:
+            if name not in valid_names:
+                raise NoSuchService(name)
+
     def get_services(self, service_names=None, include_deps=False):
         """
         Returns a list of this project's services filtered
@@ -211,10 +221,14 @@ class Project(object):
            allow_recreate=True,
            smart_recreate=False,
            insecure_registry=False,
-           do_build=True):
+           do_build=True,
+           timeout=DEFAULT_TIMEOUT):
 
         services = self.get_services(service_names, include_deps=start_deps)
 
+        for service in services:
+            service.remove_duplicate_containers()
+
         plans = self._get_convergence_plans(
             services,
             allow_recreate=allow_recreate,
@@ -228,6 +242,7 @@ class Project(object):
                 plans[service.name],
                 insecure_registry=insecure_registry,
                 do_build=do_build,
+                timeout=timeout
             )
         ]
 
@@ -274,6 +289,11 @@ class Project(object):
             service.remove_stopped(**options)
 
     def containers(self, service_names=None, stopped=False, one_off=False):
+        if service_names:
+            self.validate_service_names(service_names)
+        else:
+            service_names = self.service_names
+
         containers = [
             Container.from_ps(self.client, container)
             for container in self.client.containers(
@@ -281,8 +301,6 @@ class Project(object):
                 filters={'label': self.labels(one_off=one_off)})]
 
         def matches_service_names(container):
-            if not service_names:
-                return True
             return container.labels.get(LABEL_SERVICE) in service_names
 
         if not containers:
@@ -290,8 +308,7 @@ class Project(object):
                 self.client,
                 self.name,
                 self.service_names,
-                stopped=stopped,
-                one_off=one_off)
+            )
 
         return filter(matches_service_names, containers)
 

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,
@@ -64,6 +65,10 @@ class NeedsBuildError(Exception):
         self.service = service
 
 
+class NoSuchImageError(Exception):
+    pass
+
+
 VolumeSpec = namedtuple('VolumeSpec', 'external internal mode')
 
 
@@ -105,8 +110,7 @@ class Service(object):
                 self.client,
                 self.project,
                 [self.name],
-                stopped=stopped,
-                one_off=one_off)
+            )
 
         return containers
 
@@ -224,8 +228,11 @@ class Service(object):
                             do_build=True,
                             insecure_registry=False):
 
-        if self.image():
+        try:
+            self.image()
             return
+        except NoSuchImageError:
+            pass
 
         if self.can_be_built():
             if do_build:
@@ -240,7 +247,7 @@ class Service(object):
             return self.client.inspect_image(self.image_name)
         except APIError as e:
             if e.response.status_code == 404 and e.explanation and 'No such image' in str(e.explanation):
-                return None
+                raise NoSuchImageError("Image '{}' not found".format(self.image_name))
             else:
                 raise
 
@@ -251,26 +258,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):
@@ -294,7 +281,17 @@ class Service(object):
         return ConvergencePlan('recreate', containers)
 
     def _containers_have_diverged(self, containers):
-        config_hash = self.config_hash()
+        config_hash = None
+
+        try:
+            config_hash = self.config_hash()
+        except NoSuchImageError as e:
+            log.debug(
+                'Service %s has diverged: %s',
+                self.name, six.text_type(e),
+            )
+            return True
+
         has_diverged = False
 
         for c in containers:
@@ -311,7 +308,8 @@ class Service(object):
     def execute_convergence_plan(self,
                                  plan,
                                  insecure_registry=False,
-                                 do_build=True):
+                                 do_build=True,
+                                 timeout=DEFAULT_TIMEOUT):
         (action, containers) = plan
 
         if action == 'create':
@@ -328,6 +326,7 @@ class Service(object):
                 self.recreate_container(
                     c,
                     insecure_registry=insecure_registry,
+                    timeout=timeout
                 )
                 for c in containers
             ]
@@ -349,7 +348,8 @@ class Service(object):
 
     def recreate_container(self,
                            container,
-                           insecure_registry=False):
+                           insecure_registry=False,
+                           timeout=DEFAULT_TIMEOUT):
         """Recreate a container.
 
         The original container is renamed to a temporary name so that data
@@ -358,7 +358,7 @@ class Service(object):
         """
         log.info("Recreating %s..." % container.name)
         try:
-            container.stop()
+            container.stop(timeout=timeout)
         except APIError as e:
             if (e.response.status_code == 500
                     and e.explanation
@@ -394,6 +394,26 @@ class Service(object):
         container.start()
         return container
 
+    def remove_duplicate_containers(self, timeout=DEFAULT_TIMEOUT):
+        for c in self.duplicate_containers():
+            log.info('Removing %s...' % c.name)
+            c.stop(timeout=timeout)
+            c.remove()
+
+    def duplicate_containers(self):
+        containers = sorted(
+            self.containers(stopped=True),
+            key=lambda c: c.get('Created'),
+        )
+
+        numbers = set()
+
+        for c in containers:
+            if c.number in numbers:
+                yield c
+            else:
+                numbers.add(c.number)
+
     def config_hash(self):
         return json_hash(self.config_dict())
 

docs/install.md

@@ -27,7 +27,7 @@ First, install Docker version 1.6 or greater:
 
 To install Compose, run the following commands:
 
-    curl -L https://github.com/docker/compose/releases/download/1.3.1/docker-compose-`uname -s`-`uname -m` > /usr/local/bin/docker-compose
+    curl -L https://github.com/docker/compose/releases/download/1.3.2/docker-compose-`uname -s`-`uname -m` > /usr/local/bin/docker-compose
     chmod +x /usr/local/bin/docker-compose
 
 > Note: If you get a "Permission denied" error, your `/usr/local/bin` directory probably isn't writable and you'll need to install Compose as the superuser. Run `sudo -i`, then the two commands above, then `exit`.

requirements.txt

@@ -1,8 +1,8 @@
 PyYAML==3.10
-docker-py==1.2.3
+docker-py==1.3.0
 dockerpty==0.3.4
 docopt==0.6.1
 requests==2.6.1
 six==1.7.3
 texttable==0.8.2
-websocket-client==0.11.0
+websocket-client==0.32.0

setup.py

@@ -30,7 +30,7 @@ install_requires = [
     'requests >= 2.6.1, < 2.7',
     'texttable >= 0.8.1, < 0.9',
     'websocket-client >= 0.11.0, < 1.0',
-    'docker-py >= 1.2.3, < 1.3',
+    'docker-py >= 1.3.0, < 1.4',
     'dockerpty >= 0.3.4, < 0.4',
     'six >= 1.3.0, < 2',
 ]

tests/fixtures/build-ctx/Dockerfile

@@ -1,2 +1,3 @@
 FROM busybox:latest
+LABEL com.docker.compose.test_image=true
 CMD echo "success"

tests/fixtures/dockerfile-with-volume/Dockerfile

@@ -1,3 +1,4 @@
-FROM busybox
+FROM busybox:latest
+LABEL com.docker.compose.test_image=true
 VOLUME /data
 CMD top

tests/fixtures/dockerfile_with_entrypoint/Dockerfile

@@ -1,2 +1,3 @@
 FROM busybox:latest
+LABEL com.docker.compose.test_image=true
 ENTRYPOINT echo "From prebuilt entrypoint"

tests/fixtures/simple-dockerfile/Dockerfile

@@ -1,2 +1,3 @@
 FROM busybox:latest
+LABEL com.docker.compose.test_image=true
 CMD echo "success"

tests/integration/cli_test.py

@@ -9,6 +9,7 @@ from mock import patch
 
 from .testcases import DockerClientTestCase
 from compose.cli.main import TopLevelCommand
+from compose.project import NoSuchService
 
 
 class CLITestCase(DockerClientTestCase):
@@ -25,6 +26,7 @@ class CLITestCase(DockerClientTestCase):
         self.project.remove_stopped()
         for container in self.project.containers(stopped=True, one_off=True):
             container.remove(force=True)
+        super(CLITestCase, self).tearDown()
 
     @property
     def project(self):
@@ -162,6 +164,19 @@ class CLITestCase(DockerClientTestCase):
 
         self.assertEqual(old_ids, new_ids)
 
+    def test_up_with_timeout(self):
+        self.command.dispatch(['up', '-d', '-t', '1'], None)
+        service = self.project.get_service('simple')
+        another = self.project.get_service('another')
+        self.assertEqual(len(service.containers()), 1)
+        self.assertEqual(len(another.containers()), 1)
+
+        # Ensure containers don't have stdin and stdout connected in -d mode
+        config = service.containers()[0].inspect()['Config']
+        self.assertFalse(config['AttachStderr'])
+        self.assertFalse(config['AttachStdout'])
+        self.assertFalse(config['AttachStdin'])
+
     @patch('dockerpty.start')
     def test_run_service_without_links(self, mock_stdout):
         self.command.base_dir = 'tests/fixtures/links-composefile'
@@ -208,13 +223,10 @@ class CLITestCase(DockerClientTestCase):
         self.assertEqual(old_ids, new_ids)
 
     @patch('dockerpty.start')
-    def test_run_without_command(self, __):
+    def test_run_without_command(self, _):
         self.command.base_dir = 'tests/fixtures/commands-composefile'
         self.check_build('tests/fixtures/simple-dockerfile', tag='composetest_test')
 
-        for c in self.project.containers(stopped=True, one_off=True):
-            c.remove()
-
         self.command.dispatch(['run', 'implicit'], None)
         service = self.project.get_service('implicit')
         containers = service.containers(stopped=True, one_off=True)
@@ -351,6 +363,10 @@ class CLITestCase(DockerClientTestCase):
         self.assertEqual(len(service.containers(stopped=True)), 1)
         self.assertFalse(service.containers(stopped=True)[0].is_running)
 
+    def test_logs_invalid_service_name(self):
+        with self.assertRaises(NoSuchService):
+            self.command.dispatch(['logs', 'madeupname'], None)
+
     def test_kill(self):
         self.command.dispatch(['up', '-d'], None)
         service = self.project.get_service('simple')

tests/integration/legacy_test.py

@@ -1,12 +1,75 @@
+import unittest
+
+from docker.errors import APIError
+
 from compose import legacy
 from compose.project import Project
 from .testcases import DockerClientTestCase
 
 
-class ProjectTest(DockerClientTestCase):
+class UtilitiesTestCase(unittest.TestCase):
+    def test_has_container(self):
+        self.assertTrue(
+            legacy.has_container("composetest", "web", "composetest_web_1", one_off=False),
+        )
+        self.assertFalse(
+            legacy.has_container("composetest", "web", "composetest_web_run_1", one_off=False),
+        )
+
+    def test_has_container_one_off(self):
+        self.assertFalse(
+            legacy.has_container("composetest", "web", "composetest_web_1", one_off=True),
+        )
+        self.assertTrue(
+            legacy.has_container("composetest", "web", "composetest_web_run_1", one_off=True),
+        )
+
+    def test_has_container_different_project(self):
+        self.assertFalse(
+            legacy.has_container("composetest", "web", "otherapp_web_1", one_off=False),
+        )
+        self.assertFalse(
+            legacy.has_container("composetest", "web", "otherapp_web_run_1", one_off=True),
+        )
+
+    def test_has_container_different_service(self):
+        self.assertFalse(
+            legacy.has_container("composetest", "web", "composetest_db_1", one_off=False),
+        )
+        self.assertFalse(
+            legacy.has_container("composetest", "web", "composetest_db_run_1", one_off=True),
+        )
+
+    def test_is_valid_name(self):
+        self.assertTrue(
+            legacy.is_valid_name("composetest_web_1", one_off=False),
+        )
+        self.assertFalse(
+            legacy.is_valid_name("composetest_web_run_1", one_off=False),
+        )
+
+    def test_is_valid_name_one_off(self):
+        self.assertFalse(
+            legacy.is_valid_name("composetest_web_1", one_off=True),
+        )
+        self.assertTrue(
+            legacy.is_valid_name("composetest_web_run_1", one_off=True),
+        )
+
+    def test_is_valid_name_invalid(self):
+        self.assertFalse(
+            legacy.is_valid_name("foo"),
+        )
+        self.assertFalse(
+            legacy.is_valid_name("composetest_web_lol_1", one_off=True),
+        )
+
+
+class LegacyTestCase(DockerClientTestCase):
 
     def setUp(self):
-        super(ProjectTest, self).setUp()
+        super(LegacyTestCase, self).setUp()
+        self.containers = []
 
         db = self.create_service('db')
         web = self.create_service('web', links=[(db, 'db')])
@@ -23,35 +86,105 @@ class ProjectTest(DockerClientTestCase):
                 **service.options
             )
             self.client.start(container)
+            self.containers.append(container)
 
         # Create a single one-off legacy container
-        self.client.create_container(
-            name='{}_{}_run_1'.format(self.project.name, self.services[0].name),
+        self.containers.append(self.client.create_container(
+            name='{}_{}_run_1'.format(self.project.name, db.name),
             **self.services[0].options
-        )
+        ))
+
+    def tearDown(self):
+        super(LegacyTestCase, self).tearDown()
+        for container in self.containers:
+            try:
+                self.client.kill(container)
+            except APIError:
+                pass
+            try:
+                self.client.remove_container(container)
+            except APIError:
+                pass
 
     def get_legacy_containers(self, **kwargs):
-        return list(legacy.get_legacy_containers(
+        return legacy.get_legacy_containers(
             self.client,
             self.project.name,
             [s.name for s in self.services],
             **kwargs
-        ))
+        )
 
     def test_get_legacy_container_names(self):
         self.assertEqual(len(self.get_legacy_containers()), len(self.services))
 
     def test_get_legacy_container_names_one_off(self):
-        self.assertEqual(len(self.get_legacy_containers(stopped=True, one_off=True)), 1)
+        self.assertEqual(len(self.get_legacy_containers(one_off=True)), 1)
 
     def test_migration_to_labels(self):
+        # Trying to get the container list raises an exception
+
         with self.assertRaises(legacy.LegacyContainersError) as cm:
-            self.assertEqual(self.project.containers(stopped=True), [])
+            self.project.containers(stopped=True)
 
         self.assertEqual(
             set(cm.exception.names),
             set(['composetest_db_1', 'composetest_web_1', 'composetest_nginx_1']),
         )
 
+        self.assertEqual(
+            set(cm.exception.one_off_names),
+            set(['composetest_db_run_1']),
+        )
+
+        # Migrate the containers
+
+        legacy.migrate_project_to_labels(self.project)
+
+        # Getting the list no longer raises an exception
+
+        containers = self.project.containers(stopped=True)
+        self.assertEqual(len(containers), len(self.services))
+
+    def test_migration_one_off(self):
+        # We've already migrated
+
         legacy.migrate_project_to_labels(self.project)
-        self.assertEqual(len(self.project.containers(stopped=True)), len(self.services))
+
+        # Trying to create a one-off container results in a Docker API error
+
+        with self.assertRaises(APIError) as cm:
+            self.project.get_service('db').create_container(one_off=True)
+
+        # Checking for legacy one-off containers raises an exception
+
+        with self.assertRaises(legacy.LegacyOneOffContainersError) as cm:
+            legacy.check_for_legacy_containers(
+                self.client,
+                self.project.name,
+                ['db'],
+                allow_one_off=False,
+            )
+
+        self.assertEqual(
+            set(cm.exception.one_off_names),
+            set(['composetest_db_run_1']),
+        )
+
+        # Remove the old one-off container
+
+        c = self.client.inspect_container('composetest_db_run_1')
+        self.client.remove_container(c)
+
+        # Checking no longer raises an exception
+
+        legacy.check_for_legacy_containers(
+            self.client,
+            self.project.name,
+            ['db'],
+            allow_one_off=False,
+        )
+
+        # Creating a one-off container no longer results in an API error
+
+        self.project.get_service('db').create_container(one_off=True)
+        self.assertIsInstance(self.client.inspect_container('composetest_db_run_1'), dict)

tests/integration/project_test.py

@@ -1,5 +1,7 @@
 from __future__ import unicode_literals
+
 from compose import config
+from compose.const import LABEL_PROJECT
 from compose.project import Project
 from compose.container import Container
 from .testcases import DockerClientTestCase
@@ -29,6 +31,21 @@ class ProjectTest(DockerClientTestCase):
             [c.name for c in containers],
             ['composetest_web_1'])
 
+    def test_containers_with_extra_service(self):
+        web = self.create_service('web')
+        web_1 = web.create_container()
+
+        db = self.create_service('db')
+        db_1 = db.create_container()
+
+        self.create_service('extra').create_container()
+
+        project = Project('composetest', [web, db], self.client)
+        self.assertEqual(
+            set(project.containers(stopped=True)),
+            set([web_1, db_1]),
+        )
+
     def test_volumes_from_service(self):
         service_dicts = config.from_dictionary({
             'data': {
@@ -55,6 +72,7 @@ class ProjectTest(DockerClientTestCase):
             image='busybox:latest',
             volumes=['/var/data'],
             name='composetest_data_container',
+            labels={LABEL_PROJECT: 'composetest'},
         )
         project = Project.from_dicts(
             name='composetest',
@@ -69,9 +87,6 @@ class ProjectTest(DockerClientTestCase):
         db = project.get_service('db')
         self.assertEqual(db.volumes_from, [data_container])
 
-        project.kill()
-        project.remove_stopped()
-
     def test_net_from_service(self):
         project = Project.from_dicts(
             name='composetest',
@@ -95,15 +110,13 @@ class ProjectTest(DockerClientTestCase):
         net = project.get_service('net')
         self.assertEqual(web._get_net(), 'container:' + net.containers()[0].id)
 
-        project.kill()
-        project.remove_stopped()
-
     def test_net_from_container(self):
         net_container = Container.create(
             self.client,
             image='busybox:latest',
             name='composetest_net_container',
-            command='top'
+            command='top',
+            labels={LABEL_PROJECT: 'composetest'},
         )
         net_container.start()
 
@@ -123,9 +136,6 @@ class ProjectTest(DockerClientTestCase):
         web = project.get_service('web')
         self.assertEqual(web._get_net(), 'container:' + net_container.id)
 
-        project.kill()
-        project.remove_stopped()
-
     def test_start_stop_kill_remove(self):
         web = self.create_service('web')
         db = self.create_service('db')
@@ -171,9 +181,6 @@ class ProjectTest(DockerClientTestCase):
         self.assertEqual(len(db.containers()), 1)
         self.assertEqual(len(web.containers()), 0)
 
-        project.kill()
-        project.remove_stopped()
-
     def test_project_up_starts_uncreated_services(self):
         db = self.create_service('db')
         web = self.create_service('web', links=[(db, 'db')])
@@ -205,9 +212,6 @@ class ProjectTest(DockerClientTestCase):
         self.assertNotEqual(db_container.id, old_db_id)
         self.assertEqual(db_container.get('Volumes./etc'), db_volume_path)
 
-        project.kill()
-        project.remove_stopped()
-
     def test_project_up_with_no_recreate_running(self):
         web = self.create_service('web')
         db = self.create_service('db', volumes=['/var/db'])
@@ -228,9 +232,6 @@ class ProjectTest(DockerClientTestCase):
         self.assertEqual(db_container.inspect()['Volumes']['/var/db'],
                          db_volume_path)
 
-        project.kill()
-        project.remove_stopped()
-
     def test_project_up_with_no_recreate_stopped(self):
         web = self.create_service('web')
         db = self.create_service('db', volumes=['/var/db'])
@@ -258,9 +259,6 @@ class ProjectTest(DockerClientTestCase):
         self.assertEqual(db_container.inspect()['Volumes']['/var/db'],
                          db_volume_path)
 
-        project.kill()
-        project.remove_stopped()
-
     def test_project_up_without_all_services(self):
         console = self.create_service('console')
         db = self.create_service('db')
@@ -273,9 +271,6 @@ class ProjectTest(DockerClientTestCase):
         self.assertEqual(len(db.containers()), 1)
         self.assertEqual(len(console.containers()), 1)
 
-        project.kill()
-        project.remove_stopped()
-
     def test_project_up_starts_links(self):
         console = self.create_service('console')
         db = self.create_service('db', volumes=['/var/db'])
@@ -291,9 +286,6 @@ class ProjectTest(DockerClientTestCase):
         self.assertEqual(len(db.containers()), 1)
         self.assertEqual(len(console.containers()), 0)
 
-        project.kill()
-        project.remove_stopped()
-
     def test_project_up_starts_depends(self):
         project = Project.from_dicts(
             name='composetest',
@@ -329,9 +321,6 @@ class ProjectTest(DockerClientTestCase):
         self.assertEqual(len(project.get_service('data').containers()), 1)
         self.assertEqual(len(project.get_service('console').containers()), 0)
 
-        project.kill()
-        project.remove_stopped()
-
     def test_project_up_with_no_deps(self):
         project = Project.from_dicts(
             name='composetest',
@@ -368,9 +357,6 @@ class ProjectTest(DockerClientTestCase):
         self.assertEqual(len(project.get_service('data').containers(stopped=True)), 1)
         self.assertEqual(len(project.get_service('console').containers()), 0)
 
-        project.kill()
-        project.remove_stopped()
-
     def test_unscale_after_restart(self):
         web = self.create_service('web')
         project = Project('composetest', [web], self.client)
@@ -395,5 +381,3 @@ class ProjectTest(DockerClientTestCase):
         project.up()
         service = project.get_service('web')
         self.assertEqual(len(service.containers()), 1)
-        project.kill()
-        project.remove_stopped()

tests/integration/resilience_test.py

@@ -8,25 +8,36 @@ from .testcases import DockerClientTestCase
 
 
 class ResilienceTest(DockerClientTestCase):
-    def test_recreate_fails(self):
-        db = self.create_service('db', volumes=['/var/db'], command='top')
-        project = Project('composetest', [db], self.client)
+    def setUp(self):
+        self.db = self.create_service('db', volumes=['/var/db'], command='top')
+        self.project = Project('composetest', [self.db], self.client)
 
-        container = db.create_container()
-        db.start_container(container)
-        host_path = container.get('Volumes')['/var/db']
+        container = self.db.create_container()
+        self.db.start_container(container)
+        self.host_path = container.get('Volumes')['/var/db']
 
-        project.up()
-        container = db.containers()[0]
-        self.assertEqual(container.get('Volumes')['/var/db'], host_path)
+    def test_successful_recreate(self):
+        self.project.up()
+        container = self.db.containers()[0]
+        self.assertEqual(container.get('Volumes')['/var/db'], self.host_path)
 
+    def test_create_failure(self):
         with mock.patch('compose.service.Service.create_container', crash):
             with self.assertRaises(Crash):
-                project.up()
+                self.project.up()
 
-        project.up()
-        container = db.containers()[0]
-        self.assertEqual(container.get('Volumes')['/var/db'], host_path)
+        self.project.up()
+        container = self.db.containers()[0]
+        self.assertEqual(container.get('Volumes')['/var/db'], self.host_path)
+
+    def test_start_failure(self):
+        with mock.patch('compose.service.Service.start_container', crash):
+            with self.assertRaises(Crash):
+                self.project.up()
+
+        self.project.up()
+        container = self.db.containers()[0]
+        self.assertEqual(container.get('Volumes')['/var/db'], self.host_path)
 
 
 class Crash(Exception):

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
 
 
@@ -235,7 +236,12 @@ class ServiceTest(DockerClientTestCase):
     def test_create_container_with_volumes_from(self):
         volume_service = self.create_service('data')
         volume_container_1 = volume_service.create_container()
-        volume_container_2 = Container.create(self.client, image='busybox:latest', command=["top"])
+        volume_container_2 = Container.create(
+            self.client,
+            image='busybox:latest',
+            command=["top"],
+            labels={LABEL_PROJECT: 'composetest'},
+        )
         host_service = self.create_service('host', volumes_from=[volume_service, volume_container_2])
         host_container = host_service.create_container()
         host_service.start_container(host_container)
@@ -244,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'},
@@ -264,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'])
@@ -281,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'},
@@ -290,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',
@@ -306,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)
 
@@ -408,7 +426,7 @@ class ServiceTest(DockerClientTestCase):
         self.assertEqual(len(self.client.images(name='composetest_test')), 1)
 
     def test_start_container_uses_tagged_image_if_it_exists(self):
-        self.client.build('tests/fixtures/simple-dockerfile', tag='composetest_test')
+        self.check_build('tests/fixtures/simple-dockerfile', tag='composetest_test')
         service = Service(
             name='test',
             client=self.client,
@@ -705,3 +723,18 @@ class ServiceTest(DockerClientTestCase):
 
         self.assertEqual(1, len(device_config))
         self.assertDictEqual(device_dict, device_config[0])
+
+    def test_duplicate_containers(self):
+        service = self.create_service('web')
+
+        options = service._get_container_create_options({}, 1)
+        original = Container.create(service.client, **options)
+
+        self.assertEqual(set(service.containers(stopped=True)), set([original]))
+        self.assertEqual(set(service.duplicate_containers()), set())
+
+        options['name'] = 'temporary_container_name'
+        duplicate = Container.create(service.client, **options)
+
+        self.assertEqual(set(service.containers(stopped=True)), set([original, duplicate]))
+        self.assertEqual(set(service.duplicate_containers()), set([duplicate]))

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['smart_recreate'] = True
+        kwargs.setdefault('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))
@@ -191,6 +215,13 @@ class ServiceStateTest(DockerClientTestCase):
         web = self.create_service('web', command=["top", "-d", "1"])
         self.assertEqual(('recreate', [container]), web.convergence_plan(smart_recreate=True))
 
+    def test_trigger_recreate_with_nonexistent_image_tag(self):
+        web = self.create_service('web', image="busybox:latest")
+        container = web.create_container()
+
+        web = self.create_service('web', image="nonexistent-image")
+        self.assertEqual(('recreate', [container]), web.convergence_plan(smart_recreate=True))
+
     def test_trigger_recreate_with_image_change(self):
         repo = 'composetest_myimage'
         tag = 'latest'
@@ -216,18 +247,19 @@ class ServiceStateTest(DockerClientTestCase):
 
     def test_trigger_recreate_with_build(self):
         context = tempfile.mkdtemp()
+        base_image = "FROM busybox\nLABEL com.docker.compose.test_image=true\n"
 
         try:
             dockerfile = os.path.join(context, 'Dockerfile')
 
             with open(dockerfile, 'w') as f:
-                f.write('FROM busybox\n')
+                f.write(base_image)
 
             web = self.create_service('web', build=context)
             container = web.create_container()
 
             with open(dockerfile, 'w') as f:
-                f.write('FROM busybox\nCMD echo hello world\n')
+                f.write(base_image + 'CMD echo hello world\n')
             web.build()
 
             web = self.create_service('web', build=context)
@@ -249,15 +281,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/integration/testcases.py

@@ -2,6 +2,7 @@ from __future__ import unicode_literals
 from __future__ import absolute_import
 from compose.service import Service
 from compose.config import make_service_dict
+from compose.const import LABEL_PROJECT
 from compose.cli.docker_client import docker_client
 from compose.progress_stream import stream_output
 from .. import unittest
@@ -12,15 +13,15 @@ class DockerClientTestCase(unittest.TestCase):
     def setUpClass(cls):
         cls.client = docker_client()
 
-    # TODO: update to use labels in #652
-    def setUp(self):
-        for c in self.client.containers(all=True):
-            if c['Names'] and 'composetest' in c['Names'][0]:
-                self.client.kill(c['Id'])
-                self.client.remove_container(c['Id'])
-        for i in self.client.images():
-            if isinstance(i.get('Tag'), basestring) and 'composetest' in i['Tag']:
-                self.client.remove_image(i)
+    def tearDown(self):
+        for c in self.client.containers(
+                all=True,
+                filters={'label': '%s=composetest' % LABEL_PROJECT}):
+            self.client.kill(c['Id'])
+            self.client.remove_container(c['Id'])
+        for i in self.client.images(
+                filters={'label': 'com.docker.compose.test_image'}):
+            self.client.remove_image(i)
 
     def create_service(self, name, **kwargs):
         if 'image' not in kwargs and 'build' not in kwargs:
@@ -36,5 +37,6 @@ class DockerClientTestCase(unittest.TestCase):
         )
 
     def check_build(self, *args, **kwargs):
+        kwargs.setdefault('rm', True)
         build_output = self.client.build(*args, **kwargs)
         stream_output(build_output, open('/dev/null', 'w'))

tests/unit/cli_test.py

@@ -127,7 +127,7 @@ class CLITestCase(unittest.TestCase):
     def test_run_with_environment_merged_with_options_list(self, mock_dockerpty):
         command = TopLevelCommand()
         mock_client = mock.create_autospec(docker.Client)
-        mock_project = mock.Mock()
+        mock_project = mock.Mock(client=mock_client)
         mock_project.get_service.return_value = Service(
             'service',
             client=mock_client,
@@ -156,7 +156,7 @@ class CLITestCase(unittest.TestCase):
     def test_run_service_with_restart_always(self):
         command = TopLevelCommand()
         mock_client = mock.create_autospec(docker.Client)
-        mock_project = mock.Mock()
+        mock_project = mock.Mock(client=mock_client)
         mock_project.get_service.return_value = Service(
             'service',
             client=mock_client,
@@ -180,7 +180,7 @@ class CLITestCase(unittest.TestCase):
 
         command = TopLevelCommand()
         mock_client = mock.create_autospec(docker.Client)
-        mock_project = mock.Mock()
+        mock_project = mock.Mock(client=mock_client)
         mock_project.get_service.return_value = Service(
             'service',
             client=mock_client,

tests/unit/service_test.py

@@ -12,6 +12,7 @@ from compose.const import LABEL_SERVICE, LABEL_PROJECT, LABEL_ONE_OFF
 from compose.service import (
     ConfigError,
     NeedsBuildError,
+    NoSuchImageError,
     build_port_bindings,
     build_volume_binding,
     get_container_data_volumes,
@@ -233,7 +234,7 @@ class ServiceTest(unittest.TestCase):
             images.append({'Id': 'abc123'})
             return []
 
-        service.image = lambda: images[0] if images else None
+        service.image = lambda *args, **kwargs: mock_get_image(images)
         self.mock_client.pull = pull
 
         service.create_container(insecure_registry=True)
@@ -246,7 +247,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))
@@ -254,6 +255,15 @@ class ServiceTest(unittest.TestCase):
         new_container.start.assert_called_once_with()
         mock_container.remove.assert_called_once_with()
 
+    @mock.patch('compose.service.Container', autospec=True)
+    def test_recreate_container_with_timeout(self, _):
+        mock_container = mock.create_autospec(Container)
+        self.mock_client.inspect_image.return_value = {'Id': 'abc123'}
+        service = Service('foo', client=self.mock_client, image='someimage')
+        service.recreate_container(mock_container, timeout=1)
+
+        mock_container.stop.assert_called_once_with(timeout=1)
+
     def test_parse_repository_tag(self):
         self.assertEqual(parse_repository_tag("root"), ("root", ""))
         self.assertEqual(parse_repository_tag("root:tag"), ("root", "tag"))
@@ -273,7 +283,7 @@ class ServiceTest(unittest.TestCase):
             images.append({'Id': 'abc123'})
             return []
 
-        service.image = lambda: images[0] if images else None
+        service.image = lambda *args, **kwargs: mock_get_image(images)
         self.mock_client.pull = pull
 
         service.create_container()
@@ -283,7 +293,7 @@ class ServiceTest(unittest.TestCase):
         service = Service('foo', client=self.mock_client, build='.')
 
         images = []
-        service.image = lambda *args, **kwargs: images[0] if images else None
+        service.image = lambda *args, **kwargs: mock_get_image(images)
         service.build = lambda: images.append({'Id': 'abc123'})
 
         service.create_container(do_build=True)
@@ -298,7 +308,7 @@ class ServiceTest(unittest.TestCase):
 
     def test_create_container_no_build_but_needs_build(self):
         service = Service('foo', client=self.mock_client, build='.')
-        service.image = lambda: None
+        service.image = lambda *args, **kwargs: mock_get_image([])
 
         with self.assertRaises(NeedsBuildError):
             service.create_container(do_build=False)
@@ -315,6 +325,13 @@ class ServiceTest(unittest.TestCase):
         self.assertFalse(self.mock_client.build.call_args[1]['pull'])
 
 
+def mock_get_image(images):
+    if images:
+        return images[0]
+    else:
+        raise NoSuchImageError()
+
+
 class ServiceVolumesTest(unittest.TestCase):
 
     def setUp(self):