From 5b55a08846088d6cdbc4aca08d3143f5c9c3d3b7 Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Sat, 18 Jul 2015 11:38:46 +0200 Subject: [PATCH 1/5] Add support for ro option in volumes_from Fixes #1188 Signed-off-by: Vincent Demeester --- compose/project.py | 24 +++++++++----- compose/service.py | 52 ++++++++++++++++++++++++------- docs/yml.md | 4 ++- tests/integration/project_test.py | 5 +-- tests/integration/service_test.py | 7 +++-- tests/unit/project_test.py | 6 ++-- tests/unit/service_test.py | 34 ++++++++++++++++---- 7 files changed, 97 insertions(+), 35 deletions(-) diff --git a/compose/project.py b/compose/project.py index 4750a7a9a..919a201f1 100644 --- a/compose/project.py +++ b/compose/project.py @@ -17,8 +17,10 @@ from .legacy import check_for_legacy_containers from .service import ContainerNet from .service import ConvergenceStrategy from .service import Net +from .service import parse_volume_from_spec from .service import Service from .service import ServiceNet +from .service import VolumeFromSpec from .utils import parallel_execute @@ -34,12 +36,15 @@ def sort_service_dicts(services): def get_service_names(links): return [link.split(':')[0] for link in links] + def get_service_names_from_volumes_from(volumes_from): + return [volume_from.split(':')[0] for volume_from in volumes_from] + def get_service_dependents(service_dict, services): name = service_dict['name'] return [ service for service in services if (name in get_service_names(service.get('links', [])) or - name in service.get('volumes_from', []) or + name in get_service_names_from_volumes_from(service.get('volumes_from', [])) or name == get_service_name_from_net(service.get('net'))) ] @@ -176,20 +181,23 @@ class Project(object): def get_volumes_from(self, service_dict): volumes_from = [] if 'volumes_from' in service_dict: - for volume_name in service_dict.get('volumes_from', []): + for volume_from_config in service_dict.get('volumes_from', []): + volume_from_spec = parse_volume_from_spec(volume_from_config) + # Get service try: - service = self.get_service(volume_name) - volumes_from.append(service) + service_name = self.get_service(volume_from_spec.source) + volume_from_spec = VolumeFromSpec(service_name, volume_from_spec.mode) except NoSuchService: try: - container = Container.from_id(self.client, volume_name) - volumes_from.append(container) + container_name = Container.from_id(self.client, volume_from_spec.source) + volume_from_spec = VolumeFromSpec(container_name, volume_from_spec.mode) except APIError: raise ConfigurationError( 'Service "%s" mounts volumes from "%s", which is ' 'not the name of a service or container.' % ( - service_dict['name'], - volume_name)) + volume_from_config, + volume_from_spec.source)) + volumes_from.append(volume_from_spec) del service_dict['volumes_from'] return volumes_from diff --git a/compose/service.py b/compose/service.py index c9ca00ae4..79a138aac 100644 --- a/compose/service.py +++ b/compose/service.py @@ -6,7 +6,6 @@ import os import re import sys from collections import namedtuple -from operator import attrgetter import enum import six @@ -82,6 +81,9 @@ class NoSuchImageError(Exception): VolumeSpec = namedtuple('VolumeSpec', 'external internal mode') +VolumeFromSpec = namedtuple('VolumeFromSpec', 'source mode') + + ServiceName = namedtuple('ServiceName', 'project service number') @@ -519,7 +521,7 @@ class Service(object): 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)] + return [s.source.name for s in self.volumes_from if isinstance(s.source, Service)] def get_container_name(self, number, one_off=False): # TODO: Implement issue #652 here @@ -559,16 +561,9 @@ class Service(object): def _get_volumes_from(self): volumes_from = [] - for volume_source in self.volumes_from: - if isinstance(volume_source, Service): - containers = volume_source.containers(stopped=True) - if not containers: - volumes_from.append(volume_source.create_container().id) - else: - volumes_from.extend(map(attrgetter('id'), containers)) - - elif isinstance(volume_source, Container): - volumes_from.append(volume_source.id) + for volume_from_spec in self.volumes_from: + volumes = build_volume_from(volume_from_spec) + volumes_from.extend(volumes) return volumes_from @@ -988,6 +983,39 @@ def parse_volume_spec(volume_config): return VolumeSpec(external, internal, mode) + +def build_volume_from(volume_from_spec): + volumes_from = [] + if isinstance(volume_from_spec.source, Service): + containers = volume_from_spec.source.containers(stopped=True) + if not containers: + volumes_from = ["{}:{}".format(volume_from_spec.source.create_container().id, volume_from_spec.mode)] + else: + volumes_from = ["{}:{}".format(container.id, volume_from_spec.mode) for container in containers] + elif isinstance(volume_from_spec.source, Container): + volumes_from = ["{}:{}".format(volume_from_spec.source.id, volume_from_spec.mode)] + return volumes_from + + +def parse_volume_from_spec(volume_from_config): + parts = volume_from_config.split(':') + if len(parts) > 2: + raise ConfigError("Volume %s has incorrect format, should be " + "external:internal[:mode]" % volume_from_config) + + if len(parts) == 1: + source = parts[0] + mode = 'rw' + else: + source, mode = parts + + if mode not in ('rw', 'ro'): + raise ConfigError("VolumeFrom %s has invalid mode (%s), should be " + "one of: rw, ro." % (volume_from_config, mode)) + + return VolumeFromSpec(source, mode) + + # Labels diff --git a/docs/yml.md b/docs/yml.md index 81357df3d..12c9b554a 100644 --- a/docs/yml.md +++ b/docs/yml.md @@ -346,11 +346,13 @@ should always begin with `.` or `..`. ### volumes_from -Mount all of the volumes from another service or container. +Mount all of the volumes from another service or container, with the +supported flags by docker : ``ro``, ``rw``. volumes_from: - service_name - container_name + - service_name:rw ### cpu\_shares, cpuset, domainname, entrypoint, hostname, ipc, mac\_address, mem\_limit, memswap\_limit, privileged, read\_only, restart, stdin\_open, tty, user, volume\_driver, working\_dir diff --git a/tests/integration/project_test.py b/tests/integration/project_test.py index bd7ecccbe..ff50c80b2 100644 --- a/tests/integration/project_test.py +++ b/tests/integration/project_test.py @@ -6,6 +6,7 @@ from compose.const import LABEL_PROJECT from compose.container import Container from compose.project import Project from compose.service import ConvergenceStrategy +from compose.service import VolumeFromSpec def build_service_dicts(service_config): @@ -72,7 +73,7 @@ class ProjectTest(DockerClientTestCase): ) db = project.get_service('db') data = project.get_service('data') - self.assertEqual(db.volumes_from, [data]) + self.assertEqual(db.volumes_from, [VolumeFromSpec(data, 'rw')]) def test_volumes_from_container(self): data_container = Container.create( @@ -93,7 +94,7 @@ class ProjectTest(DockerClientTestCase): client=self.client, ) db = project.get_service('db') - self.assertEqual(db.volumes_from, [data_container]) + self.assertEqual(db._get_volumes_from(), [data_container.id + ':rw']) def test_net_from_service(self): project = Project.from_dicts( diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index 2c9c6fc20..306060960 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -25,6 +25,7 @@ from compose.service import ConfigError from compose.service import ConvergencePlan from compose.service import Net from compose.service import Service +from compose.service import VolumeFromSpec def create_and_start_container(service, **override_options): @@ -272,12 +273,12 @@ class ServiceTest(DockerClientTestCase): command=["top"], labels={LABEL_PROJECT: 'composetest'}, ) - host_service = self.create_service('host', volumes_from=[volume_service, volume_container_2]) + host_service = self.create_service('host', volumes_from=[VolumeFromSpec(volume_service, 'rw'), VolumeFromSpec(volume_container_2, 'rw')]) host_container = host_service.create_container() host_service.start_container(host_container) - self.assertIn(volume_container_1.id, + self.assertIn(volume_container_1.id + ':rw', host_container.get('HostConfig.VolumesFrom')) - self.assertIn(volume_container_2.id, + self.assertIn(volume_container_2.id + ':rw', host_container.get('HostConfig.VolumesFrom')) def test_execute_convergence_plan_recreate(self): diff --git a/tests/unit/project_test.py b/tests/unit/project_test.py index ce74eb30b..f3cf9e294 100644 --- a/tests/unit/project_test.py +++ b/tests/unit/project_test.py @@ -168,7 +168,7 @@ class ProjectTest(unittest.TestCase): 'volumes_from': ['aaa'] } ], self.mock_client) - self.assertEqual(project.get_service('test')._get_volumes_from(), [container_id]) + self.assertEqual(project.get_service('test')._get_volumes_from(), [container_id + ":rw"]) def test_use_volumes_from_service_no_container(self): container_name = 'test_vol_1' @@ -191,7 +191,7 @@ class ProjectTest(unittest.TestCase): 'volumes_from': ['vol'] } ], self.mock_client) - self.assertEqual(project.get_service('test')._get_volumes_from(), [container_name]) + self.assertEqual(project.get_service('test')._get_volumes_from(), [container_name + ":rw"]) @mock.patch.object(Service, 'containers') def test_use_volumes_from_service_container(self, mock_return): @@ -211,7 +211,7 @@ class ProjectTest(unittest.TestCase): 'volumes_from': ['vol'] } ], None) - self.assertEqual(project.get_service('test')._get_volumes_from(), container_ids) + self.assertEqual(project.get_service('test')._get_volumes_from(), [cid + ':rw' for cid in container_ids]) def test_net_unset(self): project = Project.from_dicts('test', [ diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index c682b8237..f85d34d2a 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -24,6 +24,7 @@ from compose.service import parse_repository_tag from compose.service import parse_volume_spec from compose.service import Service from compose.service import ServiceNet +from compose.service import VolumeFromSpec class ServiceTest(unittest.TestCase): @@ -75,9 +76,18 @@ class ServiceTest(unittest.TestCase): service = Service( 'test', image='foo', - volumes_from=[mock.Mock(id=container_id, spec=Container)]) + volumes_from=[VolumeFromSpec(mock.Mock(id=container_id, spec=Container), 'rw')]) - self.assertEqual(service._get_volumes_from(), [container_id]) + self.assertEqual(service._get_volumes_from(), [container_id + ':rw']) + + def test_get_volumes_from_container_read_only(self): + container_id = 'aabbccddee' + service = Service( + 'test', + image='foo', + volumes_from=[VolumeFromSpec(mock.Mock(id=container_id, spec=Container), 'ro')]) + + self.assertEqual(service._get_volumes_from(), [container_id + ':ro']) def test_get_volumes_from_service_container_exists(self): container_ids = ['aabbccddee', '12345'] @@ -86,9 +96,21 @@ class ServiceTest(unittest.TestCase): mock.Mock(id=container_id, spec=Container) for container_id in container_ids ] - service = Service('test', volumes_from=[from_service], image='foo') + service = Service('test', volumes_from=[VolumeFromSpec(from_service, 'rw')], image='foo') - self.assertEqual(service._get_volumes_from(), container_ids) + self.assertEqual(service._get_volumes_from(), [cid + ":rw" for cid in container_ids]) + + def test_get_volumes_from_service_container_exists_with_flags(self): + for mode in ['ro', 'rw', 'z', 'rw,z', 'z,rw']: + container_ids = ['aabbccddee:' + mode, '12345:' + mode] + from_service = mock.create_autospec(Service) + from_service.containers.return_value = [ + mock.Mock(id=container_id.split(':')[0], spec=Container) + for container_id in container_ids + ] + service = Service('test', volumes_from=[VolumeFromSpec(from_service, mode)], image='foo') + + self.assertEqual(service._get_volumes_from(), container_ids) def test_get_volumes_from_service_no_container(self): container_id = 'abababab' @@ -97,9 +119,9 @@ class ServiceTest(unittest.TestCase): from_service.create_container.return_value = mock.Mock( id=container_id, spec=Container) - service = Service('test', image='foo', volumes_from=[from_service]) + service = Service('test', image='foo', volumes_from=[VolumeFromSpec(from_service, 'rw')]) - self.assertEqual(service._get_volumes_from(), [container_id]) + self.assertEqual(service._get_volumes_from(), [container_id + ':rw']) from_service.create_container.assert_called_once_with() def test_split_domainname_none(self): From 467c73186996465a7bb1e5873ab829d2d1c90f42 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Fri, 25 Sep 2015 17:18:46 +0100 Subject: [PATCH 2/5] address PR feedback Signed-off-by: Mazz Mosley --- compose/project.py | 7 +++++-- compose/service.py | 5 +---- tests/integration/service_test.py | 8 +++++++- tests/unit/service_test.py | 2 +- 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/compose/project.py b/compose/project.py index 919a201f1..999c28904 100644 --- a/compose/project.py +++ b/compose/project.py @@ -37,7 +37,10 @@ def sort_service_dicts(services): return [link.split(':')[0] for link in links] def get_service_names_from_volumes_from(volumes_from): - return [volume_from.split(':')[0] for volume_from in volumes_from] + return [ + parse_volume_from_spec(volume_from).source + for volume_from in volumes_from + ] def get_service_dependents(service_dict, services): name = service_dict['name'] @@ -195,7 +198,7 @@ class Project(object): raise ConfigurationError( 'Service "%s" mounts volumes from "%s", which is ' 'not the name of a service or container.' % ( - volume_from_config, + service_dict['name'], volume_from_spec.source)) volumes_from.append(volume_from_spec) del service_dict['volumes_from'] diff --git a/compose/service.py b/compose/service.py index 79a138aac..f2f82f6ce 100644 --- a/compose/service.py +++ b/compose/service.py @@ -6,6 +6,7 @@ import os import re import sys from collections import namedtuple +from operator import attrgetter import enum import six @@ -1009,10 +1010,6 @@ def parse_volume_from_spec(volume_from_config): else: source, mode = parts - if mode not in ('rw', 'ro'): - raise ConfigError("VolumeFrom %s has invalid mode (%s), should be " - "one of: rw, ro." % (volume_from_config, mode)) - return VolumeFromSpec(source, mode) diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index 306060960..64ce2c658 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -273,7 +273,13 @@ class ServiceTest(DockerClientTestCase): command=["top"], labels={LABEL_PROJECT: 'composetest'}, ) - host_service = self.create_service('host', volumes_from=[VolumeFromSpec(volume_service, 'rw'), VolumeFromSpec(volume_container_2, 'rw')]) + host_service = self.create_service( + 'host', + volumes_from=[ + VolumeFromSpec(volume_service, 'rw'), + VolumeFromSpec(volume_container_2, 'rw') + ] + ) host_container = host_service.create_container() host_service.start_container(host_container) self.assertIn(volume_container_1.id + ':rw', diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index f85d34d2a..48e31b11b 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -379,7 +379,7 @@ class ServiceTest(unittest.TestCase): client=self.mock_client, net=ServiceNet(Service('other')), links=[(Service('one'), 'one')], - volumes_from=[Service('two')]) + volumes_from=[VolumeFromSpec(Service('two'), 'rw')]) config_dict = service.config_dict() expected = { From 0ff84a78c64b241ffc9cb037db0b17044bb9941d Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Tue, 6 Oct 2015 13:10:38 +0100 Subject: [PATCH 3/5] Use multiple returns rather than overriding. Also added a doc string for clarity. Signed-off-by: Mazz Mosley --- compose/service.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/compose/service.py b/compose/service.py index f2f82f6ce..a24138540 100644 --- a/compose/service.py +++ b/compose/service.py @@ -986,16 +986,18 @@ def parse_volume_spec(volume_config): def build_volume_from(volume_from_spec): - volumes_from = [] + """ + volume_from can be either a service or a container. We want to return the + container.id and format it into a string complete with the mode. + """ if isinstance(volume_from_spec.source, Service): containers = volume_from_spec.source.containers(stopped=True) if not containers: - volumes_from = ["{}:{}".format(volume_from_spec.source.create_container().id, volume_from_spec.mode)] + return ["{}:{}".format(volume_from_spec.source.create_container().id, volume_from_spec.mode)] else: - volumes_from = ["{}:{}".format(container.id, volume_from_spec.mode) for container in containers] + return ["{}:{}".format(container.id, volume_from_spec.mode) for container in containers] elif isinstance(volume_from_spec.source, Container): - volumes_from = ["{}:{}".format(volume_from_spec.source.id, volume_from_spec.mode)] - return volumes_from + return ["{}:{}".format(volume_from_spec.source.id, volume_from_spec.mode)] def parse_volume_from_spec(volume_from_config): From f9028703f4a527dc05302999f8d21c18f84b7055 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Tue, 6 Oct 2015 13:11:49 +0100 Subject: [PATCH 4/5] Pick the first container Rather than inefficiently looping through all the containers that a service has and overriding each volumes_from value, pick the first one and return that. Signed-off-by: Mazz Mosley --- compose/service.py | 5 +++-- tests/unit/project_test.py | 2 +- tests/unit/service_test.py | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/compose/service.py b/compose/service.py index a24138540..0dbd7f8d1 100644 --- a/compose/service.py +++ b/compose/service.py @@ -994,8 +994,9 @@ def build_volume_from(volume_from_spec): containers = volume_from_spec.source.containers(stopped=True) if not containers: return ["{}:{}".format(volume_from_spec.source.create_container().id, volume_from_spec.mode)] - else: - return ["{}:{}".format(container.id, volume_from_spec.mode) for container in containers] + + container = containers[0] + return ["{}:{}".format(container.id, volume_from_spec.mode)] elif isinstance(volume_from_spec.source, Container): return ["{}:{}".format(volume_from_spec.source.id, volume_from_spec.mode)] diff --git a/tests/unit/project_test.py b/tests/unit/project_test.py index f3cf9e294..fc189fbb1 100644 --- a/tests/unit/project_test.py +++ b/tests/unit/project_test.py @@ -211,7 +211,7 @@ class ProjectTest(unittest.TestCase): 'volumes_from': ['vol'] } ], None) - self.assertEqual(project.get_service('test')._get_volumes_from(), [cid + ':rw' for cid in container_ids]) + self.assertEqual(project.get_service('test')._get_volumes_from(), [container_ids[0] + ':rw']) def test_net_unset(self): project = Project.from_dicts('test', [ diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index 48e31b11b..19d25e2ed 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -98,7 +98,7 @@ class ServiceTest(unittest.TestCase): ] service = Service('test', volumes_from=[VolumeFromSpec(from_service, 'rw')], image='foo') - self.assertEqual(service._get_volumes_from(), [cid + ":rw" for cid in container_ids]) + self.assertEqual(service._get_volumes_from(), [container_ids[0] + ":rw"]) def test_get_volumes_from_service_container_exists_with_flags(self): for mode in ['ro', 'rw', 'z', 'rw,z', 'z,rw']: @@ -110,7 +110,7 @@ class ServiceTest(unittest.TestCase): ] service = Service('test', volumes_from=[VolumeFromSpec(from_service, mode)], image='foo') - self.assertEqual(service._get_volumes_from(), container_ids) + self.assertEqual(service._get_volumes_from(), [container_ids[0]]) def test_get_volumes_from_service_no_container(self): container_id = 'abababab' From 21a1affc6395a524273d5788cac5e0b7c92a50ce Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Tue, 6 Oct 2015 15:43:26 +0100 Subject: [PATCH 5/5] Re-word docs. Signed-off-by: Mazz Mosley --- docs/yml.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/yml.md b/docs/yml.md index 12c9b554a..a476fd33f 100644 --- a/docs/yml.md +++ b/docs/yml.md @@ -346,8 +346,8 @@ should always begin with `.` or `..`. ### volumes_from -Mount all of the volumes from another service or container, with the -supported flags by docker : ``ro``, ``rw``. +Mount all of the volumes from another service or container, optionally +specifying read-only access(``ro``) or read-write(``rw``). volumes_from: - service_name