From ed50a0a3a02821f5a6e50fc46db864e386258921 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Sun, 26 Apr 2015 17:09:20 -0400 Subject: [PATCH] Resolves #1066, use labels to identify containers Signed-off-by: Daniel Nephin --- compose/__init__.py | 1 - compose/const.py | 6 ++ compose/container.py | 10 ++-- compose/project.py | 13 +++- compose/service.py | 98 +++++++++++++++++-------------- tests/integration/service_test.py | 33 +++++++---- tests/integration/testcases.py | 1 + tests/unit/container_test.py | 29 ++++++--- tests/unit/service_test.py | 49 +++++++--------- 9 files changed, 138 insertions(+), 102 deletions(-) create mode 100644 compose/const.py diff --git a/compose/__init__.py b/compose/__init__.py index 2de2a7f8..045e7914 100644 --- a/compose/__init__.py +++ b/compose/__init__.py @@ -1,4 +1,3 @@ from __future__ import unicode_literals -from .service import Service # noqa:flake8 __version__ = '1.3.0dev' diff --git a/compose/const.py b/compose/const.py new file mode 100644 index 00000000..0714a6db --- /dev/null +++ b/compose/const.py @@ -0,0 +1,6 @@ + +LABEL_CONTAINER_NUMBER = 'com.docker.compose.container-number' +LABEL_ONE_OFF = 'com.docker.compose.oneoff' +LABEL_PROJECT = 'com.docker.compose.project' +LABEL_SERVICE = 'com.docker.compose.service' +LABEL_VERSION = 'com.docker.compose.version' diff --git a/compose/container.py b/compose/container.py index 6388ca80..183d5fde 100644 --- a/compose/container.py +++ b/compose/container.py @@ -4,6 +4,8 @@ from __future__ import absolute_import import six from functools import reduce +from .const import LABEL_CONTAINER_NUMBER, LABEL_SERVICE + class Container(object): """ @@ -58,14 +60,11 @@ class Container(object): @property def name_without_project(self): - return '_'.join(self.dictionary['Name'].split('_')[1:]) + return '{0}_{1}'.format(self.labels.get(LABEL_SERVICE), self.number) @property def number(self): - try: - return int(self.name.split('_')[-1]) - except ValueError: - return None + return int(self.labels.get(LABEL_CONTAINER_NUMBER) or 0) @property def ports(self): @@ -159,6 +158,7 @@ class Container(object): self.has_been_inspected = True return self.dictionary + # TODO: only used by tests, move to test module def links(self): links = [] for container in self.client.containers(): diff --git a/compose/project.py b/compose/project.py index 2f3675ff..d22bdf4d 100644 --- a/compose/project.py +++ b/compose/project.py @@ -4,6 +4,7 @@ import logging from functools import reduce from .config import get_service_name_from_net, ConfigurationError +from .const import LABEL_PROJECT, LABEL_ONE_OFF from .service import Service from .container import Container from docker.errors import APIError @@ -60,6 +61,12 @@ class Project(object): self.services = services self.client = client + def labels(self, one_off=False): + return [ + '{0}={1}'.format(LABEL_PROJECT, self.name), + '{0}={1}'.format(LABEL_ONE_OFF, "True" if one_off else "False"), + ] + @classmethod def from_dicts(cls, name, service_dicts, client): """ @@ -224,9 +231,9 @@ class Project(object): def containers(self, service_names=None, stopped=False, one_off=False): return [Container.from_ps(self.client, container) - for container in self.client.containers(all=stopped) - for service in self.get_services(service_names) - if service.has_container(container, one_off=one_off)] + for container in self.client.containers( + all=stopped, + filters={'label': self.labels(one_off=one_off)})] def _inject_deps(self, acc, service): net_name = service.get_net_name() diff --git a/compose/service.py b/compose/service.py index 08d20327..3c62dbeb 100644 --- a/compose/service.py +++ b/compose/service.py @@ -10,8 +10,16 @@ import six from docker.errors import APIError from docker.utils import create_host_config, LogConfig +from . import __version__ from .config import DOCKER_CONFIG_KEYS, merge_environment -from .container import Container, get_container_name +from .const import ( + LABEL_CONTAINER_NUMBER, + LABEL_ONE_OFF, + LABEL_PROJECT, + LABEL_SERVICE, + LABEL_VERSION, +) +from .container import Container from .progress_stream import stream_output, StreamOutputError log = logging.getLogger(__name__) @@ -79,27 +87,17 @@ class Service(object): def containers(self, stopped=False, one_off=False): return [Container.from_ps(self.client, container) - for container in self.client.containers(all=stopped) - if self.has_container(container, one_off=one_off)] - - def has_container(self, container, one_off=False): - """Return True if `container` was created to fulfill this service.""" - name = get_container_name(container) - if not name or not is_valid_name(name, one_off): - return False - project, name, _number = parse_name(name) - return project == self.project and name == self.name + for container in self.client.containers( + all=stopped, + filters={'label': self.labels(one_off=one_off)})] def get_container(self, number=1): """Return a :class:`compose.container.Container` for this service. The container must be active, and match `number`. """ - for container in self.client.containers(): - if not self.has_container(container): - continue - _, _, container_number = parse_name(get_container_name(container)) - if container_number == number: - return Container.from_ps(self.client, container) + labels = self.labels() + ['{0}={1}'.format(LABEL_CONTAINER_NUMBER, number)] + for container in self.client.containers(filters={'label': labels}): + return Container.from_ps(self.client, container) raise ValueError("No container found for %s_%s" % (self.name, number)) @@ -138,7 +136,6 @@ class Service(object): # Create enough containers containers = self.containers(stopped=True) while len(containers) < desired_num: - log.info("Creating %s..." % self._next_container_name(containers)) containers.append(self.create_container(detach=True)) running_containers = [] @@ -178,6 +175,7 @@ class Service(object): insecure_registry=False, do_build=True, previous_container=None, + number=None, **override_options): """ Create a container for this service. If the image doesn't exist, attempt to pull @@ -185,6 +183,7 @@ class Service(object): """ container_options = self._get_container_create_options( override_options, + number or self._next_container_number(one_off=one_off), one_off=one_off, previous_container=previous_container, ) @@ -209,7 +208,6 @@ class Service(object): """ containers = self.containers(stopped=True) if not containers: - log.info("Creating %s..." % self._next_container_name(containers)) container = self.create_container( insecure_registry=insecure_registry, do_build=do_build, @@ -256,6 +254,7 @@ class Service(object): new_container = self.create_container( do_build=False, previous_container=container, + number=container.labels.get(LABEL_CONTAINER_NUMBER), **override_options) self.start_container(new_container) container.remove() @@ -280,7 +279,6 @@ class Service(object): containers = self.containers(stopped=True) if not containers: - log.info("Creating %s..." % self._next_container_name(containers)) new_container = self.create_container( insecure_registry=insecure_registry, detach=detach, @@ -302,14 +300,19 @@ class Service(object): else: return - def _next_container_name(self, all_containers, one_off=False): - bits = [self.project, self.name] - if one_off: - bits.append('run') - return '_'.join(bits + [str(self._next_container_number(all_containers))]) + 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) - def _next_container_number(self, all_containers): - numbers = [parse_name(c.name).number for c in all_containers] + # TODO: this would benefit from github.com/docker/docker/pull/11943 + # to remove the need to inspect every container + def _next_container_number(self, one_off=False): + numbers = [ + Container.from_ps(self.client, container).number + for container in self.client.containers( + all=True, + filters={'label': self.labels(one_off=one_off)}) + ] return 1 if not numbers else max(numbers) + 1 def _get_links(self, link_to_self): @@ -369,6 +372,7 @@ class Service(object): def _get_container_create_options( self, override_options, + number, one_off=False, previous_container=None): container_options = dict( @@ -376,9 +380,7 @@ class Service(object): for k in DOCKER_CONFIG_KEYS if k in self.options) container_options.update(override_options) - container_options['name'] = self._next_container_name( - self.containers(stopped=True, one_off=one_off), - one_off) + container_options['name'] = self.get_container_name(number, one_off) # If a qualified hostname was given, split it into an # unqualified hostname and a domainname unless domainname @@ -419,6 +421,11 @@ class Service(object): if self.can_be_built(): container_options['image'] = self.full_name + container_options['labels'] = build_container_labels( + container_options.get('labels', {}), + self.labels(one_off=one_off), + number) + # Delete options which are only used when starting for key in DOCKER_START_KEYS: container_options.pop(key, None) @@ -520,6 +527,13 @@ class Service(object): """ return '%s_%s' % (self.project, self.name) + def labels(self, one_off=False): + return [ + '{0}={1}'.format(LABEL_PROJECT, self.project), + '{0}={1}'.format(LABEL_SERVICE, self.name), + '{0}={1}'.format(LABEL_ONE_OFF, "True" if one_off else "False") + ] + def can_be_scaled(self): for port in self.options.get('ports', []): if ':' in str(port): @@ -585,23 +599,19 @@ def merge_volume_bindings(volumes_option, previous_container): return volume_bindings -NAME_RE = re.compile(r'^([^_]+)_([^_]+)_(run_)?(\d+)$') - - -def is_valid_name(name, one_off=False): - match = NAME_RE.match(name) - if match is None: - return False +def build_container_name(project, service, number, one_off=False): + bits = [project, service] if one_off: - return match.group(3) == 'run_' - else: - return match.group(3) is None + bits.append('run') + return '_'.join(bits + [str(number)]) -def parse_name(name): - match = NAME_RE.match(name) - (project, service_name, _, suffix) = match.groups() - return ServiceName(project, service_name, int(suffix)) +def build_container_labels(label_options, service_labels, number, one_off=False): + labels = label_options or {} + labels.update(label.split('=', 1) for label in service_labels) + labels[LABEL_CONTAINER_NUMBER] = str(number) + labels[LABEL_VERSION] = __version__ + return labels def parse_restart_spec(restart_config): diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index bc21ab01..47c826ec 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -8,11 +8,19 @@ import tempfile import shutil import six -from compose import Service +from compose import __version__ +from compose.const import ( + LABEL_CONTAINER_NUMBER, + LABEL_ONE_OFF, + LABEL_PROJECT, + LABEL_SERVICE, + LABEL_VERSION, +) from compose.service import ( CannotBeScaledError, - build_extra_hosts, ConfigError, + Service, + build_extra_hosts, ) from compose.container import Container from docker.errors import APIError @@ -633,17 +641,18 @@ class ServiceTest(DockerClientTestCase): 'com.example.label-with-empty-value': "", } + compose_labels = { + LABEL_CONTAINER_NUMBER: '1', + LABEL_ONE_OFF: 'False', + LABEL_PROJECT: 'composetest', + LABEL_SERVICE: 'web', + LABEL_VERSION: __version__, + } + expected = dict(labels_dict, **compose_labels) + service = self.create_service('web', labels=labels_dict) - labels = create_and_start_container(service).labels.items() - for pair in labels_dict.items(): - self.assertIn(pair, labels) - - labels_list = ["%s=%s" % pair for pair in labels_dict.items()] - - service = self.create_service('web', labels=labels_list) - labels = create_and_start_container(service).labels.items() - for pair in labels_dict.items(): - self.assertIn(pair, labels) + labels = create_and_start_container(service).labels + self.assertEqual(labels, expected) def test_empty_labels(self): labels_list = ['foo', 'bar'] diff --git a/tests/integration/testcases.py b/tests/integration/testcases.py index 31281a1d..4a0f7248 100644 --- a/tests/integration/testcases.py +++ b/tests/integration/testcases.py @@ -12,6 +12,7 @@ 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]: diff --git a/tests/unit/container_test.py b/tests/unit/container_test.py index 7637adf5..b04df659 100644 --- a/tests/unit/container_test.py +++ b/tests/unit/container_test.py @@ -5,6 +5,7 @@ import mock import docker from compose.container import Container +from compose.container import get_container_name class ContainerTest(unittest.TestCase): @@ -23,6 +24,13 @@ class ContainerTest(unittest.TestCase): "NetworkSettings": { "Ports": {}, }, + "Config": { + "Labels": { + "com.docker.compose.project": "composetest", + "com.docker.compose.service": "web", + "com.docker.compose.container_number": 7, + }, + } } def test_from_ps(self): @@ -65,10 +73,8 @@ class ContainerTest(unittest.TestCase): }) def test_number(self): - container = Container.from_ps(None, - self.container_dict, - has_been_inspected=True) - self.assertEqual(container.number, 1) + container = Container(None, self.container_dict, has_been_inspected=True) + self.assertEqual(container.number, 7) def test_name(self): container = Container.from_ps(None, @@ -77,10 +83,8 @@ class ContainerTest(unittest.TestCase): self.assertEqual(container.name, "composetest_db_1") def test_name_without_project(self): - container = Container.from_ps(None, - self.container_dict, - has_been_inspected=True) - self.assertEqual(container.name_without_project, "db_1") + container = Container(None, self.container_dict, has_been_inspected=True) + self.assertEqual(container.name_without_project, "web_7") def test_inspect_if_not_inspected(self): mock_client = mock.create_autospec(docker.Client) @@ -130,3 +134,12 @@ class ContainerTest(unittest.TestCase): self.assertEqual(container.get('Status'), "Up 8 seconds") self.assertEqual(container.get('HostConfig.VolumesFrom'), ["volume_id"]) self.assertEqual(container.get('Foo.Bar.DoesNotExist'), None) + + +class GetContainerNameTestCase(unittest.TestCase): + + def test_get_container_name(self): + self.assertIsNone(get_container_name({})) + self.assertEqual(get_container_name({'Name': 'myproject_db_1'}), 'myproject_db_1') + self.assertEqual(get_container_name({'Names': ['/myproject_db_1', '/myproject_web_1/db']}), 'myproject_db_1') + self.assertEqual(get_container_name({'Names': ['/swarm-host-1/myproject_db_1', '/swarm-host-1/myproject_web_1/db']}), 'myproject_db_1') diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index 2ea94edb..fa252062 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -7,15 +7,15 @@ import mock import docker from requests import Response -from compose import Service +from compose.service import Service from compose.container import Container +from compose.const import LABEL_SERVICE, LABEL_PROJECT, LABEL_ONE_OFF from compose.service import ( APIError, ConfigError, build_port_bindings, build_volume_binding, get_container_data_volumes, - get_container_name, merge_volume_bindings, parse_repository_tag, parse_volume_spec, @@ -48,36 +48,27 @@ class ServiceTest(unittest.TestCase): self.assertRaises(ConfigError, lambda: Service(name='foo', project='_', image='foo')) Service(name='foo', project='bar', image='foo') - def test_get_container_name(self): - self.assertIsNone(get_container_name({})) - self.assertEqual(get_container_name({'Name': 'myproject_db_1'}), 'myproject_db_1') - self.assertEqual(get_container_name({'Names': ['/myproject_db_1', '/myproject_web_1/db']}), 'myproject_db_1') - self.assertEqual(get_container_name({'Names': ['/swarm-host-1/myproject_db_1', '/swarm-host-1/myproject_web_1/db']}), 'myproject_db_1') - def test_containers(self): - service = Service('db', client=self.mock_client, image='foo', project='myproject') - + service = Service('db', self.mock_client, 'myproject', image='foo') self.mock_client.containers.return_value = [] self.assertEqual(service.containers(), []) + def test_containers_with_containers(self): self.mock_client.containers.return_value = [ - {'Image': 'busybox', 'Id': 'OUT_1', 'Names': ['/myproject', '/foo/bar']}, - {'Image': 'busybox', 'Id': 'OUT_2', 'Names': ['/myproject_db']}, - {'Image': 'busybox', 'Id': 'OUT_3', 'Names': ['/db_1']}, - {'Image': 'busybox', 'Id': 'IN_1', 'Names': ['/myproject_db_1', '/myproject_web_1/db']}, + dict(Name=str(i), Image='foo', Id=i) for i in range(3) ] - self.assertEqual([c.id for c in service.containers()], ['IN_1']) + service = Service('db', self.mock_client, 'myproject', image='foo') + self.assertEqual([c.id for c in service.containers()], range(3)) - def test_containers_prefixed(self): - service = Service('db', client=self.mock_client, image='foo', project='myproject') - - self.mock_client.containers.return_value = [ - {'Image': 'busybox', 'Id': 'OUT_1', 'Names': ['/swarm-host-1/myproject', '/swarm-host-1/foo/bar']}, - {'Image': 'busybox', 'Id': 'OUT_2', 'Names': ['/swarm-host-1/myproject_db']}, - {'Image': 'busybox', 'Id': 'OUT_3', 'Names': ['/swarm-host-1/db_1']}, - {'Image': 'busybox', 'Id': 'IN_1', 'Names': ['/swarm-host-1/myproject_db_1', '/swarm-host-1/myproject_web_1/db']}, + expected_labels = [ + '{0}=myproject'.format(LABEL_PROJECT), + '{0}=db'.format(LABEL_SERVICE), + '{0}=False'.format(LABEL_ONE_OFF), ] - self.assertEqual([c.id for c in service.containers()], ['IN_1']) + + self.mock_client.containers.assert_called_once_with( + all=False, + filters={'label': expected_labels}) def test_get_volumes_from_container(self): container_id = 'aabbccddee' @@ -156,7 +147,7 @@ class ServiceTest(unittest.TestCase): def test_split_domainname_none(self): service = Service('foo', image='foo', hostname='name', client=self.mock_client) self.mock_client.containers.return_value = [] - opts = service._get_container_create_options({'image': 'foo'}) + opts = service._get_container_create_options({'image': 'foo'}, 1) self.assertEqual(opts['hostname'], 'name', 'hostname') self.assertFalse('domainname' in opts, 'domainname') @@ -167,7 +158,7 @@ class ServiceTest(unittest.TestCase): image='foo', client=self.mock_client) self.mock_client.containers.return_value = [] - opts = service._get_container_create_options({'image': 'foo'}) + opts = service._get_container_create_options({'image': 'foo'}, 1) self.assertEqual(opts['hostname'], 'name', 'hostname') self.assertEqual(opts['domainname'], 'domain.tld', 'domainname') @@ -179,7 +170,7 @@ class ServiceTest(unittest.TestCase): domainname='domain.tld', client=self.mock_client) self.mock_client.containers.return_value = [] - opts = service._get_container_create_options({'image': 'foo'}) + opts = service._get_container_create_options({'image': 'foo'}, 1) self.assertEqual(opts['hostname'], 'name', 'hostname') self.assertEqual(opts['domainname'], 'domain.tld', 'domainname') @@ -191,7 +182,7 @@ class ServiceTest(unittest.TestCase): image='foo', client=self.mock_client) self.mock_client.containers.return_value = [] - opts = service._get_container_create_options({'image': 'foo'}) + opts = service._get_container_create_options({'image': 'foo'}, 1) self.assertEqual(opts['hostname'], 'name.sub', 'hostname') self.assertEqual(opts['domainname'], 'domain.tld', 'domainname') @@ -255,7 +246,7 @@ class ServiceTest(unittest.TestCase): tag='sometag', insecure_registry=True, stream=True) - mock_log.info.assert_called_once_with( + mock_log.info.assert_called_with( 'Pulling foo (someimage:sometag)...') @mock.patch('compose.service.Container', autospec=True)