From ef56523883a8c2d5bd2d48a556ece6a3b8b130f5 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 1 Sep 2015 18:56:40 -0400 Subject: [PATCH 1/6] Make external_links a regular service.option so that it's part of the config hash Signed-off-by: Daniel Nephin --- compose/service.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/compose/service.py b/compose/service.py index b48f2e14..5942fca5 100644 --- a/compose/service.py +++ b/compose/service.py @@ -87,7 +87,16 @@ ConvergencePlan = namedtuple('ConvergencePlan', 'action containers') class Service(object): - def __init__(self, name, client=None, project='default', links=None, external_links=None, volumes_from=None, net=None, **options): + def __init__( + self, + name, + client=None, + project='default', + links=None, + volumes_from=None, + net=None, + **options + ): if not re.match('^%s+$' % VALID_NAME_CHARS, project): raise ConfigError('Invalid project name "%s" - only %s are allowed' % (project, VALID_NAME_CHARS)) @@ -95,7 +104,6 @@ class Service(object): self.client = client self.project = project self.links = links or [] - self.external_links = external_links or [] self.volumes_from = volumes_from or [] self.net = net or None self.options = options @@ -528,7 +536,7 @@ class Service(object): links.append((container.name, self.name)) links.append((container.name, container.name)) links.append((container.name, container.name_without_project)) - for external_link in self.external_links: + for external_link in self.options.get('external_links') or []: if ':' not in external_link: link_name = external_link else: From e801981fed4049d0f7eab94ed969ec20f3ba8a76 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 1 Sep 2015 19:21:22 -0400 Subject: [PATCH 2/6] Sort config keys Signed-off-by: Daniel Nephin --- compose/config/config.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index 8df45b8a..d9b06f3e 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -22,9 +22,9 @@ from compose.cli.utils import find_candidates_in_parent_dirs DOCKER_CONFIG_KEYS = [ 'cap_add', 'cap_drop', + 'command', 'cpu_shares', 'cpuset', - 'command', 'detach', 'devices', 'dns', @@ -38,12 +38,12 @@ DOCKER_CONFIG_KEYS = [ 'image', 'labels', 'links', + 'log_driver', + 'log_opt', 'mac_address', 'mem_limit', 'memswap_limit', 'net', - 'log_driver', - 'log_opt', 'pid', 'ports', 'privileged', From 08ba857807753f43a2b64844fe53ca70756bfa14 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 1 Sep 2015 19:54:49 -0400 Subject: [PATCH 3/6] Cleanup some project logic. Signed-off-by: Daniel Nephin --- compose/project.py | 50 ++++++++++++++++++++++++---------------------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/compose/project.py b/compose/project.py index 4e8696ba..54d6c443 100644 --- a/compose/project.py +++ b/compose/project.py @@ -87,8 +87,14 @@ class Project(object): volumes_from = project.get_volumes_from(service_dict) net = project.get_net(service_dict) - project.services.append(Service(client=client, project=name, links=links, net=net, - volumes_from=volumes_from, **service_dict)) + project.services.append( + Service( + client=client, + project=name, + links=links, + net=net, + volumes_from=volumes_from, + **service_dict)) return project @property @@ -184,30 +190,26 @@ class Project(object): return volumes_from def get_net(self, service_dict): - if 'net' in service_dict: - net_name = get_service_name_from_net(service_dict.get('net')) + net = service_dict.pop('net', None) + if not net: + return - if net_name: - try: - net = self.get_service(net_name) - except NoSuchService: - try: - net = Container.from_id(self.client, net_name) - except APIError: - raise ConfigurationError( - 'Service "%s" is trying to use the network of "%s", ' - 'which is not the name of a service or container.' % ( - service_dict['name'], - net_name)) - else: - net = service_dict['net'] + net_name = get_service_name_from_net(net) + if not net_name: + return net - del service_dict['net'] - - else: - net = None - - return net + try: + return self.get_service(net_name) + except NoSuchService: + pass + try: + return Container.from_id(self.client, net_name) + except APIError: + raise ConfigurationError( + 'Service "%s" is trying to use the network of "%s", ' + 'which is not the name of a service or container.' % ( + service_dict['name'], + net_name)) def start(self, service_names=None, **options): for service in self.get_services(service_names): From c183e52502da8efd3e60f104b4d25f0577f55c04 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 1 Sep 2015 19:40:15 -0400 Subject: [PATCH 4/6] Fixes #1757 - include all service properties in the config_dict() Signed-off-by: Daniel Nephin --- compose/service.py | 3 +++ tests/integration/state_test.py | 22 +++++++++++++++++ tests/unit/service_test.py | 43 ++++++++++++++++++++++++++++++++- 3 files changed, 67 insertions(+), 1 deletion(-) diff --git a/compose/service.py b/compose/service.py index 5942fca5..f60d57bf 100644 --- a/compose/service.py +++ b/compose/service.py @@ -488,6 +488,9 @@ class Service(object): return { 'options': self.options, 'image_id': self.image()['Id'], + 'links': [(service.name, alias) for service, alias in self.links], + 'net': self.get_net_name() or getattr(self.net, 'id', self.net), + 'volumes_from': self.get_volumes_from_names(), } def get_dependency_names(self): diff --git a/tests/integration/state_test.py b/tests/integration/state_test.py index b3dd42d9..d077f094 100644 --- a/tests/integration/state_test.py +++ b/tests/integration/state_test.py @@ -1,3 +1,7 @@ +""" +Integration tests which cover state convergence (aka smart recreate) performed +by `docker-compose up`. +""" from __future__ import unicode_literals import os @@ -151,6 +155,24 @@ class ProjectWithDependenciesTest(ProjectTestCase): self.assertEqual(new_containers - old_containers, set()) + def test_service_removed_while_down(self): + next_cfg = { + 'web': { + 'image': 'busybox:latest', + 'command': 'tail -f /dev/null', + }, + 'nginx': self.cfg['nginx'], + } + + containers = self.run_up(self.cfg) + self.assertEqual(len(containers), 3) + + project = self.make_project(self.cfg) + project.stop(timeout=1) + + containers = self.run_up(next_cfg) + self.assertEqual(len(containers), 2) + def converge(service, allow_recreate=True, diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index aa6d4d74..3981cad2 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -189,7 +189,7 @@ class ServiceTest(unittest.TestCase): self.assertEqual( opts['labels'][LABEL_CONFIG_HASH], - 'b30306d0a73b67f67a45b99b88d36c359e470e6fa0c04dda1cf62d2087205b81') + '3c85881a8903b9d73a06c41860c8be08acce1494ab4cf8408375966dccd714de') self.assertEqual( opts['environment'], { @@ -331,6 +331,47 @@ class ServiceTest(unittest.TestCase): self.assertEqual(self.mock_client.build.call_count, 1) self.assertFalse(self.mock_client.build.call_args[1]['pull']) + def test_config_dict(self): + self.mock_client.inspect_image.return_value = {'Id': 'abcd'} + service = Service( + 'foo', + image='example.com/foo', + client=self.mock_client, + net=Service('other'), + links=[(Service('one'), 'one')], + volumes_from=[Service('two')]) + + config_dict = service.config_dict() + expected = { + 'image_id': 'abcd', + 'options': {'image': 'example.com/foo'}, + 'links': [('one', 'one')], + 'net': 'other', + 'volumes_from': ['two'], + } + self.assertEqual(config_dict, expected) + + def test_config_dict_with_net_from_container(self): + self.mock_client.inspect_image.return_value = {'Id': 'abcd'} + container = Container( + self.mock_client, + {'Id': 'aaabbb', 'Name': '/foo_1'}) + service = Service( + 'foo', + image='example.com/foo', + client=self.mock_client, + net=container) + + config_dict = service.config_dict() + expected = { + 'image_id': 'abcd', + 'options': {'image': 'example.com/foo'}, + 'links': [], + 'net': 'aaabbb', + 'volumes_from': [], + } + self.assertEqual(config_dict, expected) + def mock_get_image(images): if images: From 187ad4ce26401aaa10984c3c9a9782d6b2efdb87 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 3 Sep 2015 13:02:46 -0400 Subject: [PATCH 5/6] Refactor network_mode logic out of Service. Signed-off-by: Daniel Nephin --- compose/project.py | 11 +++-- compose/service.py | 88 +++++++++++++++++++++++++------------- tests/unit/project_test.py | 6 +-- tests/unit/service_test.py | 48 ++++++++++++++++++++- 4 files changed, 116 insertions(+), 37 deletions(-) diff --git a/compose/project.py b/compose/project.py index 54d6c443..8db20e76 100644 --- a/compose/project.py +++ b/compose/project.py @@ -14,7 +14,10 @@ from .const import LABEL_PROJECT from .const import LABEL_SERVICE from .container import Container from .legacy import check_for_legacy_containers +from .service import ContainerNet +from .service import Net from .service import Service +from .service import ServiceNet from .utils import parallel_execute @@ -192,18 +195,18 @@ class Project(object): def get_net(self, service_dict): net = service_dict.pop('net', None) if not net: - return + return Net(None) net_name = get_service_name_from_net(net) if not net_name: - return net + return Net(net) try: - return self.get_service(net_name) + return ServiceNet(self.get_service(net_name)) except NoSuchService: pass try: - return Container.from_id(self.client, net_name) + return ContainerNet(Container.from_id(self.client, net_name)) except APIError: raise ConfigurationError( 'Service "%s" is trying to use the network of "%s", ' diff --git a/compose/service.py b/compose/service.py index f60d57bf..bfc6f904 100644 --- a/compose/service.py +++ b/compose/service.py @@ -105,7 +105,7 @@ class Service(object): self.project = project self.links = links or [] self.volumes_from = volumes_from or [] - self.net = net or None + self.net = net or Net(None) self.options = options def containers(self, stopped=False, one_off=False, filters={}): @@ -489,12 +489,12 @@ class Service(object): 'options': self.options, 'image_id': self.image()['Id'], 'links': [(service.name, alias) for service, alias in self.links], - 'net': self.get_net_name() or getattr(self.net, 'id', self.net), + 'net': self.net.id, 'volumes_from': self.get_volumes_from_names(), } def get_dependency_names(self): - net_name = self.get_net_name() + net_name = self.net.service_name return (self.get_linked_names() + self.get_volumes_from_names() + ([net_name] if net_name else [])) @@ -505,12 +505,6 @@ class Service(object): def get_volumes_from_names(self): return [s.name for s in self.volumes_from if isinstance(s, Service)] - def get_net_name(self): - if isinstance(self.net, Service): - return self.net.name - else: - return - def get_container_name(self, number, one_off=False): # TODO: Implement issue #652 here return build_container_name(self.project, self.name, number, one_off) @@ -562,25 +556,6 @@ class Service(object): return volumes_from - def _get_net(self): - if not self.net: - return None - - if isinstance(self.net, Service): - containers = self.net.containers() - if len(containers) > 0: - net = 'container:' + containers[0].id - else: - log.warning("Warning: Service %s is trying to use reuse the network stack " - "of another service that is not running." % (self.net.name)) - net = None - elif isinstance(self.net, Container): - net = 'container:' + self.net.id - else: - net = self.net - - return net - def _get_container_create_options( self, override_options, @@ -694,7 +669,7 @@ class Service(object): binds=options.get('binds'), volumes_from=self._get_volumes_from(), privileged=privileged, - network_mode=self._get_net(), + network_mode=self.net.mode, devices=devices, dns=dns, dns_search=dns_search, @@ -793,6 +768,61 @@ class Service(object): stream_output(output, sys.stdout) +class Net(object): + """A `standard` network mode (ex: host, bridge)""" + + service_name = None + + def __init__(self, net): + self.net = net + + @property + def id(self): + return self.net + + mode = id + + +class ContainerNet(object): + """A network mode that uses a containers network stack.""" + + service_name = None + + def __init__(self, container): + self.container = container + + @property + def id(self): + return self.container.id + + @property + def mode(self): + return 'container:' + self.container.id + + +class ServiceNet(object): + """A network mode that uses a service's network stack.""" + + def __init__(self, service): + self.service = service + + @property + def id(self): + return self.service.name + + service_name = id + + @property + def mode(self): + containers = self.service.containers() + if containers: + return 'container:' + containers[0].id + + log.warn("Warning: Service %s is trying to use reuse the network stack " + "of another service that is not running." % (self.id)) + return None + + # Names diff --git a/tests/unit/project_test.py b/tests/unit/project_test.py index 37ebe514..ce74eb30 100644 --- a/tests/unit/project_test.py +++ b/tests/unit/project_test.py @@ -221,7 +221,7 @@ class ProjectTest(unittest.TestCase): } ], self.mock_client) service = project.get_service('test') - self.assertEqual(service._get_net(), None) + self.assertEqual(service.net.id, None) self.assertNotIn('NetworkMode', service._get_container_host_config({})) def test_use_net_from_container(self): @@ -236,7 +236,7 @@ class ProjectTest(unittest.TestCase): } ], self.mock_client) service = project.get_service('test') - self.assertEqual(service._get_net(), 'container:' + container_id) + self.assertEqual(service.net.mode, 'container:' + container_id) def test_use_net_from_service(self): container_name = 'test_aaa_1' @@ -261,7 +261,7 @@ class ProjectTest(unittest.TestCase): ], self.mock_client) service = project.get_service('test') - self.assertEqual(service._get_net(), 'container:' + container_name) + self.assertEqual(service.net.mode, 'container:' + container_name) def test_container_without_name(self): self.mock_client.containers.return_value = [ diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index 3981cad2..de973339 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -13,13 +13,16 @@ from compose.const import LABEL_SERVICE from compose.container import Container from compose.service import build_volume_binding from compose.service import ConfigError +from compose.service import ContainerNet from compose.service import get_container_data_volumes from compose.service import merge_volume_bindings from compose.service import NeedsBuildError +from compose.service import Net from compose.service import NoSuchImageError from compose.service import parse_repository_tag from compose.service import parse_volume_spec from compose.service import Service +from compose.service import ServiceNet class ServiceTest(unittest.TestCase): @@ -337,7 +340,7 @@ class ServiceTest(unittest.TestCase): 'foo', image='example.com/foo', client=self.mock_client, - net=Service('other'), + net=ServiceNet(Service('other')), links=[(Service('one'), 'one')], volumes_from=[Service('two')]) @@ -373,6 +376,49 @@ class ServiceTest(unittest.TestCase): self.assertEqual(config_dict, expected) +class NetTestCase(unittest.TestCase): + + def test_net(self): + net = Net('host') + self.assertEqual(net.id, 'host') + self.assertEqual(net.mode, 'host') + self.assertEqual(net.service_name, None) + + def test_net_container(self): + container_id = 'abcd' + net = ContainerNet(Container(None, {'Id': container_id})) + self.assertEqual(net.id, container_id) + self.assertEqual(net.mode, 'container:' + container_id) + self.assertEqual(net.service_name, None) + + def test_net_service(self): + container_id = 'bbbb' + service_name = 'web' + mock_client = mock.create_autospec(docker.Client) + mock_client.containers.return_value = [ + {'Id': container_id, 'Name': container_id, 'Image': 'abcd'}, + ] + + service = Service(name=service_name, client=mock_client) + net = ServiceNet(service) + + self.assertEqual(net.id, service_name) + self.assertEqual(net.mode, 'container:' + container_id) + self.assertEqual(net.service_name, service_name) + + def test_net_service_no_containers(self): + service_name = 'web' + mock_client = mock.create_autospec(docker.Client) + mock_client.containers.return_value = [] + + service = Service(name=service_name, client=mock_client) + net = ServiceNet(service) + + self.assertEqual(net.id, service_name) + self.assertEqual(net.mode, None) + self.assertEqual(net.service_name, service_name) + + def mock_get_image(images): if images: return images[0] From db9f577ad6cdadfb8eaa33b492fd513821ed57b6 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 3 Sep 2015 13:13:22 -0400 Subject: [PATCH 6/6] Extract link names into a function. Signed-off-by: Daniel Nephin --- compose/cli/main.py | 2 +- compose/service.py | 13 ++++++++----- tests/integration/project_test.py | 4 ++-- tests/integration/service_test.py | 7 ++++--- 4 files changed, 15 insertions(+), 11 deletions(-) diff --git a/compose/cli/main.py b/compose/cli/main.py index 2d72646d..11d2d104 100644 --- a/compose/cli/main.py +++ b/compose/cli/main.py @@ -326,7 +326,7 @@ class TopLevelCommand(Command): log.warn(INSECURE_SSL_WARNING) if not options['--no-deps']: - deps = service.get_linked_names() + deps = service.get_linked_service_names() if len(deps) > 0: project.up( diff --git a/compose/service.py b/compose/service.py index bfc6f904..8dc1efa1 100644 --- a/compose/service.py +++ b/compose/service.py @@ -488,19 +488,22 @@ class Service(object): return { 'options': self.options, 'image_id': self.image()['Id'], - 'links': [(service.name, alias) for service, alias in self.links], + 'links': self.get_link_names(), 'net': self.net.id, 'volumes_from': self.get_volumes_from_names(), } def get_dependency_names(self): net_name = self.net.service_name - return (self.get_linked_names() + + return (self.get_linked_service_names() + self.get_volumes_from_names() + ([net_name] if net_name else [])) - def get_linked_names(self): - return [s.name for (s, _) in self.links] + def get_linked_service_names(self): + return [service.name for (service, _) in self.links] + + def get_link_names(self): + return [(service.name, alias) for service, alias in self.links] def get_volumes_from_names(self): return [s.name for s in self.volumes_from if isinstance(s, Service)] @@ -784,7 +787,7 @@ class Net(object): class ContainerNet(object): - """A network mode that uses a containers network stack.""" + """A network mode that uses a container's network stack.""" service_name = None diff --git a/tests/integration/project_test.py b/tests/integration/project_test.py index 51619cb5..fe63838f 100644 --- a/tests/integration/project_test.py +++ b/tests/integration/project_test.py @@ -112,7 +112,7 @@ class ProjectTest(DockerClientTestCase): web = project.get_service('web') net = project.get_service('net') - self.assertEqual(web._get_net(), 'container:' + net.containers()[0].id) + self.assertEqual(web.net.mode, 'container:' + net.containers()[0].id) def test_net_from_container(self): net_container = Container.create( @@ -138,7 +138,7 @@ class ProjectTest(DockerClientTestCase): project.up() web = project.get_service('web') - self.assertEqual(web._get_net(), 'container:' + net_container.id) + self.assertEqual(web.net.mode, 'container:' + net_container.id) def test_start_pause_unpause_stop_kill_remove(self): web = self.create_service('web') diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index 177471ff..b6257821 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -22,6 +22,7 @@ from compose.container import Container from compose.service import build_extra_hosts from compose.service import ConfigError from compose.service import ConvergencePlan +from compose.service import Net from compose.service import Service @@ -707,17 +708,17 @@ class ServiceTest(DockerClientTestCase): self.assertEqual(list(container.inspect()['HostConfig']['PortBindings'].keys()), ['8000/tcp']) def test_network_mode_none(self): - service = self.create_service('web', net='none') + service = self.create_service('web', net=Net('none')) container = create_and_start_container(service) self.assertEqual(container.get('HostConfig.NetworkMode'), 'none') def test_network_mode_bridged(self): - service = self.create_service('web', net='bridge') + service = self.create_service('web', net=Net('bridge')) container = create_and_start_container(service) self.assertEqual(container.get('HostConfig.NetworkMode'), 'bridge') def test_network_mode_host(self): - service = self.create_service('web', net='host') + service = self.create_service('web', net=Net('host')) container = create_and_start_container(service) self.assertEqual(container.get('HostConfig.NetworkMode'), 'host')