diff --git a/fig/project.py b/fig/project.py index f5ac88e4ee..e013da4e98 100644 --- a/fig/project.py +++ b/fig/project.py @@ -176,14 +176,18 @@ def up(self, do_build=True): running_containers = [] for service in self.get_services(service_names, include_links=start_links): - create_func = (service.recreate_containers if recreate - else service.start_or_create_containers) - - for container in create_func( - insecure_registry=insecure_registry, - detach=detach, - do_build=do_build): - running_containers.append(container) + if recreate: + for (_, container) in service.recreate_containers( + insecure_registry=insecure_registry, + detach=detach, + do_build=do_build): + running_containers.append(container) + else: + for container in service.start_or_create_containers( + insecure_registry=insecure_registry, + detach=detach, + do_build=do_build): + running_containers.append(container) return running_containers diff --git a/fig/service.py b/fig/service.py index 3b1273d101..18ba3f618f 100644 --- a/fig/service.py +++ b/fig/service.py @@ -242,9 +242,8 @@ def create_container(self, def recreate_containers(self, insecure_registry=False, do_build=True, **override_options): """ - 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. + 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. """ containers = self.containers(stopped=True) if not containers: @@ -254,22 +253,21 @@ def recreate_containers(self, insecure_registry=False, do_build=True, **override do_build=do_build, **override_options) self.start_container(container) - return [container] + return [(None, container)] else: - return [ - self.recreate_container( - container, - insecure_registry=insecure_registry, - **override_options) - for container in containers - ] + tuples = [] + + for c in containers: + log.info("Recreating %s..." % c.name) + tuples.append(self.recreate_container(c, insecure_registry=insecure_registry, **override_options)) + + return tuples def recreate_container(self, container, **override_options): """Recreate a container. An intermediate container is created so that the new container has the same name, while still supporting `volumes-from` the original container. """ - log.info("Recreating %s..." % container.name) try: container.stop() except APIError as e: @@ -280,7 +278,6 @@ def recreate_container(self, container, **override_options): else: raise - intermediate_options = dict(self.options, **override_options) intermediate_container = Container.create( self.client, image=container.image, @@ -288,22 +285,17 @@ def recreate_container(self, container, **override_options): command=[], detach=True, ) - intermediate_container.start( - binds=get_container_data_volumes( - container, intermediate_options.get('volumes'))) + intermediate_container.start(volumes_from=container.id) 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 + + return (intermediate_container, new_container) def start_container_if_stopped(self, container, **options): if container.is_running: @@ -315,6 +307,12 @@ def start_container_if_stopped(self, container, **options): 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,14 +321,12 @@ def start_container(self, container, intermediate_container=None, **override_opt 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=binds, - volumes_from=self._get_volumes_from(), + binds=volume_bindings, + volumes_from=self._get_volumes_from(intermediate_container), privileged=privileged, network_mode=net, dns=dns, @@ -392,7 +388,7 @@ def _get_links(self, link_to_self): links.append((external_link, link_name)) return links - def _get_volumes_from(self): + def _get_volumes_from(self, intermediate_container=None): volumes_from = [] for volume_source in self.volumes_from: if isinstance(volume_source, Service): @@ -406,6 +402,9 @@ def _get_volumes_from(self): 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): @@ -520,45 +519,6 @@ def pull(self, insecure_registry=False): ) -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 dadd8d4a91..d01d118ff2 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -98,7 +98,7 @@ def test_create_container_with_unspecified_volume(self): service = self.create_service('db', volumes=['/var/db']) container = service.create_container() service.start_container(container) - self.assertIn('/var/db', container.get('Volumes')) + self.assertIn('/var/db', container.inspect()['Volumes']) def test_create_container_with_cpu_shares(self): service = self.create_service('db', cpu_shares=73) @@ -148,23 +148,30 @@ def test_recreate_containers(self): self.assertIn('FOO=1', old_container.dictionary['Config']['Env']) self.assertEqual(old_container.name, 'figtest_db_1') service.start_container(old_container) - volume_path = old_container.get('Volumes')['/etc'] + volume_path = old_container.inspect()['Volumes']['/etc'] num_containers_before = len(self.client.containers(all=True)) service.options['environment']['FOO'] = '2' - containers = service.recreate_containers() - self.assertEqual(len(containers), 1) + tuples = service.recreate_containers() + self.assertEqual(len(tuples), 1) - new_container = containers[0] - self.assertEqual(new_container.get('Config.Entrypoint'), ['sleep']) - self.assertEqual(new_container.get('Config.Cmd'), ['300']) - self.assertIn('FOO=2', new_container.get('Config.Env')) + intermediate_container = tuples[0][0] + new_container = tuples[0][1] + self.assertEqual(intermediate_container.dictionary['Config']['Entrypoint'], ['/bin/echo']) + + self.assertEqual(new_container.dictionary['Config']['Entrypoint'], ['sleep']) + self.assertEqual(new_container.dictionary['Config']['Cmd'], ['300']) + self.assertIn('FOO=2', new_container.dictionary['Config']['Env']) self.assertEqual(new_container.name, 'figtest_db_1') - self.assertEqual(new_container.get('Volumes')['/etc'], volume_path) + self.assertEqual(new_container.inspect()['Volumes']['/etc'], volume_path) + self.assertIn(intermediate_container.id, new_container.dictionary['HostConfig']['VolumesFrom']) self.assertEqual(len(self.client.containers(all=True)), num_containers_before) self.assertNotEqual(old_container.id, new_container.id) + self.assertRaises(APIError, + self.client.inspect_container, + intermediate_container.id) def test_recreate_containers_when_containers_are_stopped(self): service = self.create_service( @@ -179,16 +186,6 @@ def test_recreate_containers_when_containers_are_stopped(self): 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 3e6b7c4ed1..68dcf06ab4 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -11,15 +11,13 @@ from fig import Service from fig.container import Container from fig.service import ( - APIError, ConfigError, + split_port, build_port_bindings, + parse_volume_spec, build_volume_binding, - get_container_data_volumes, - get_volume_bindings, + APIError, parse_repository_tag, - parse_volume_spec, - split_port, ) @@ -59,6 +57,13 @@ def test_get_volumes_from_container(self): 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) @@ -283,50 +288,6 @@ def test_building_volume_binding_with_home(self): 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):