From 7eb476e61de7abf5a70ba8e824c7e2e765e2ac09 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Sat, 6 Dec 2014 17:51:49 -0500 Subject: [PATCH] Resolves #447, fix volume logic for recreate container Signed-off-by: Daniel Nephin --- fig/project.py | 4 +- fig/service.py | 68 ++++++++++++++++++++++++------- tests/integration/service_test.py | 12 +++++- tests/unit/service_test.py | 63 ++++++++++++++++++++++------ 4 files changed, 117 insertions(+), 30 deletions(-) diff --git a/fig/project.py b/fig/project.py index cb6404a7..f5ac88e4 100644 --- a/fig/project.py +++ b/fig/project.py @@ -181,8 +181,8 @@ class Project(object): for container in create_func( insecure_registry=insecure_registry, - detach=detach, - do_build=do_build): + detach=detach, + do_build=do_build): running_containers.append(container) return running_containers diff --git a/fig/service.py b/fig/service.py index 64735f4d..3b1273d1 100644 --- a/fig/service.py +++ b/fig/service.py @@ -280,6 +280,7 @@ class Service(object): else: raise + intermediate_options = dict(self.options, **override_options) intermediate_container = Container.create( self.client, image=container.image, @@ -287,16 +288,21 @@ class Service(object): command=[], detach=True, ) - intermediate_container.start(volumes_from=container.id) + intermediate_container.start( + binds=get_container_data_volumes( + container, intermediate_options.get('volumes'))) intermediate_container.wait() container.remove() + # TODO: volumes are being passed to both start and create, this is + # probably unnecessary options = dict(override_options) new_container = self.create_container(do_build=False, **options) - self.start_container(new_container, intermediate_container=intermediate_container) + self.start_container( + new_container, + intermediate_container=intermediate_container) intermediate_container.remove() - return new_container def start_container_if_stopped(self, container, **options): @@ -309,12 +315,6 @@ class Service(object): def start_container(self, container, intermediate_container=None, **override_options): options = dict(self.options, **override_options) port_bindings = build_port_bindings(options.get('ports') or []) - - volume_bindings = dict( - build_volume_binding(parse_volume_spec(volume)) - for volume in options.get('volumes') or [] - if ':' in volume) - privileged = options.get('privileged', False) net = options.get('net', 'bridge') dns = options.get('dns', None) @@ -323,12 +323,14 @@ class Service(object): cap_drop = options.get('cap_drop', None) restart = parse_restart_spec(options.get('restart', None)) + binds = get_volume_bindings( + options.get('volumes'), intermediate_container) container.start( links=self._get_links(link_to_self=options.get('one_off', False)), port_bindings=port_bindings, - binds=volume_bindings, - volumes_from=self._get_volumes_from(intermediate_container), + binds=binds, + volumes_from=self._get_volumes_from(), privileged=privileged, network_mode=net, dns=dns, @@ -390,7 +392,7 @@ class Service(object): links.append((external_link, link_name)) return links - def _get_volumes_from(self, intermediate_container=None): + def _get_volumes_from(self): volumes_from = [] for volume_source in self.volumes_from: if isinstance(volume_source, Service): @@ -404,9 +406,6 @@ class Service(object): elif isinstance(volume_source, Container): volumes_from.append(volume_source.id) - if intermediate_container: - volumes_from.append(intermediate_container.id) - return volumes_from def _get_container_create_options(self, override_options, one_off=False): @@ -521,6 +520,45 @@ class Service(object): ) +def get_container_data_volumes(container, volumes_option): + """Find the container data volumes that are in `volumes_option`, and return + a mapping of volume bindings for those volumes. + """ + volumes = [] + for volume in volumes_option or []: + volume = parse_volume_spec(volume) + # No need to preserve host volumes + if volume.external: + continue + + volume_path = (container.get('Volumes') or {}).get(volume.internal) + # New volume, doesn't exist in the old container + if not volume_path: + continue + + # Copy existing volume from old container + volume = volume._replace(external=volume_path) + volumes.append(build_volume_binding(volume)) + + return dict(volumes) + + +def get_volume_bindings(volumes_option, intermediate_container): + """Return a list of volume bindings for a container. Container data volume + bindings are replaced by those in the intermediate container. + """ + volume_bindings = dict( + build_volume_binding(parse_volume_spec(volume)) + for volume in volumes_option or [] + if ':' in volume) + + if intermediate_container: + volume_bindings.update( + get_container_data_volumes(intermediate_container, volumes_option)) + + return volume_bindings + + NAME_RE = re.compile(r'^([^_]+)_([^_]+)_(run_)?(\d+)$') diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index fcf46038..dadd8d4a 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -98,7 +98,7 @@ class ServiceTest(DockerClientTestCase): service = self.create_service('db', volumes=['/var/db']) container = service.create_container() service.start_container(container) - self.assertIn('/var/db', container.inspect()['Volumes']) + self.assertIn('/var/db', container.get('Volumes')) def test_create_container_with_cpu_shares(self): service = self.create_service('db', cpu_shares=73) @@ -179,6 +179,16 @@ class ServiceTest(DockerClientTestCase): service.recreate_containers() self.assertEqual(len(service.containers(stopped=True)), 1) + def test_recreate_containers_with_volume_changes(self): + service = self.create_service('withvolumes', volumes=['/etc']) + old_container = create_and_start_container(service) + self.assertEqual(old_container.get('Volumes').keys(), ['/etc']) + + service = self.create_service('withvolumes') + container, = service.recreate_containers() + service.start_container(container) + self.assertEqual(container.get('Volumes'), {}) + def test_start_container_passes_through_options(self): db = self.create_service('db') create_and_start_container(db, environment={'FOO': 'BAR'}) diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index 68dcf06a..3e6b7c4e 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -11,13 +11,15 @@ from requests import Response from fig import Service from fig.container import Container from fig.service import ( - ConfigError, - split_port, - build_port_bindings, - parse_volume_spec, - build_volume_binding, APIError, + ConfigError, + build_port_bindings, + build_volume_binding, + get_container_data_volumes, + get_volume_bindings, parse_repository_tag, + parse_volume_spec, + split_port, ) @@ -57,13 +59,6 @@ class ServiceTest(unittest.TestCase): self.assertEqual(service._get_volumes_from(), [container_id]) - def test_get_volumes_from_intermediate_container(self): - container_id = 'aabbccddee' - service = Service('test') - container = mock.Mock(id=container_id, spec=Container) - - self.assertEqual(service._get_volumes_from(container), [container_id]) - def test_get_volumes_from_service_container_exists(self): container_ids = ['aabbccddee', '12345'] from_service = mock.create_autospec(Service) @@ -288,6 +283,50 @@ class ServiceVolumesTest(unittest.TestCase): binding, ('/home/user', dict(bind='/home/user', ro=False))) + def test_get_container_data_volumes(self): + options = [ + '/host/volume:/host/volume:ro', + '/new/volume', + '/existing/volume', + ] + + container = Container(None, { + 'Volumes': { + '/host/volume': '/host/volume', + '/existing/volume': '/var/lib/docker/aaaaaaaa', + '/removed/volume': '/var/lib/docker/bbbbbbbb', + }, + }, has_been_inspected=True) + + expected = { + '/var/lib/docker/aaaaaaaa': {'bind': '/existing/volume', 'ro': False}, + } + + binds = get_container_data_volumes(container, options) + self.assertEqual(binds, expected) + + def test_get_volume_bindings(self): + options = [ + '/host/volume:/host/volume:ro', + '/host/rw/volume:/host/rw/volume', + '/new/volume', + '/existing/volume', + ] + + intermediate_container = Container(None, { + 'Volumes': {'/existing/volume': '/var/lib/docker/aaaaaaaa'}, + }, has_been_inspected=True) + + expected = { + '/host/volume': {'bind': '/host/volume', 'ro': True}, + '/host/rw/volume': {'bind': '/host/rw/volume', 'ro': False}, + '/var/lib/docker/aaaaaaaa': {'bind': '/existing/volume', 'ro': False}, + } + + binds = get_volume_bindings(options, intermediate_container) + self.assertEqual(binds, expected) + + class ServiceEnvironmentTest(unittest.TestCase): def setUp(self):