From 7591639c728744ac836b5e9dfe62b0963a78341e Mon Sep 17 00:00:00 2001 From: Ian Glen Neal Date: Sun, 10 Dec 2017 23:50:24 +0000 Subject: [PATCH 1/4] Fix #5465 by catching a no-image exception in get_container_data_volumes. The ImageNotFound exception is now bubbled up to the client, which prompts the user on the desired course of action (rebuild or abort). Added a case to the unit test to check that an empty existing image doesn't result in an exception. Closed old pull request #5466 because I did a rebase and it showed over 300 commits to merge, which I thought was messy. Signed-off-by: Ian Glen Neal --- compose/cli/main.py | 58 +++++++++++++++++++++++++++----------- compose/parallel.py | 12 +++++--- compose/project.py | 2 ++ compose/service.py | 55 ++++++++++++++++++++++++++---------- tests/unit/service_test.py | 16 +++++++++-- 5 files changed, 106 insertions(+), 37 deletions(-) diff --git a/compose/cli/main.py b/compose/cli/main.py index 4de7c5caa..bc10c5c4f 100644 --- a/compose/cli/main.py +++ b/compose/cli/main.py @@ -972,24 +972,50 @@ class TopLevelCommand(object): raise UserError("COMPOSE_IGNORE_ORPHANS and --remove-orphans cannot be combined.") if no_start: - for excluded in ['-d', '--abort-on-container-exit', '--exit-code-from']: - if options.get(excluded): - raise UserError('--no-start and {} cannot be combined.'.format(excluded)) + opts = ['-d', '--abort-on-container-exit', '--exit-code-from'] + for excluded in [x for x in opts if options.get(x)]: + raise UserError('--no-start and {} cannot be combined.'.format(excluded)) with up_shutdown_context(self.project, service_names, timeout, detached): - to_attach = self.project.up( - service_names=service_names, - start_deps=start_deps, - strategy=convergence_strategy_from_opts(options), - do_build=build_action_from_opts(options), - timeout=timeout, - detached=detached, - remove_orphans=remove_orphans, - ignore_orphans=ignore_orphans, - scale_override=parse_scale_args(options['--scale']), - start=not no_start, - always_recreate_deps=always_recreate_deps - ) + try: + to_attach = self.project.up( + service_names=service_names, + start_deps=start_deps, + strategy=convergence_strategy_from_opts(options), + do_build=build_action_from_opts(options), + timeout=timeout, + detached=detached, + remove_orphans=remove_orphans, + ignore_orphans=ignore_orphans, + scale_override=parse_scale_args(options['--scale']), + start=not no_start, + always_recreate_deps=always_recreate_deps, + rebuild=False + ) + except docker.errors.ImageNotFound as e: + log.error(("Image not found. If you continue, there is a " + "risk of data loss. Consider backing up your data " + "before continuing.\n\n" + "Full error message: {}\n" + ).format(e.explanation)) + res = yesno("Continue by rebuilding the image(s)? [yN]", False) + if res is None or not res: + raise e + + to_attach = self.project.up( + service_names=service_names, + start_deps=start_deps, + strategy=convergence_strategy_from_opts(options), + do_build=build_action_from_opts(options), + timeout=timeout, + detached=detached, + remove_orphans=remove_orphans, + ignore_orphans=ignore_orphans, + scale_override=parse_scale_args(options['--scale']), + start=not no_start, + always_recreate_deps=always_recreate_deps, + rebuild=True + ) if detached or no_start: return diff --git a/compose/parallel.py b/compose/parallel.py index 3c0098c05..341ca2f5e 100644 --- a/compose/parallel.py +++ b/compose/parallel.py @@ -8,6 +8,7 @@ from threading import Semaphore from threading import Thread from docker.errors import APIError +from docker.errors import ImageNotFound from six.moves import _thread as thread from six.moves.queue import Empty from six.moves.queue import Queue @@ -53,10 +54,7 @@ def parallel_execute(objects, func, get_name, msg, get_deps=None, limit=None, pa writer = ParallelStreamWriter(stream, msg) - if parent_objects: - display_objects = list(parent_objects) - else: - display_objects = objects + display_objects = list(parent_objects) if parent_objects else objects for obj in display_objects: writer.add_object(get_name(obj)) @@ -76,6 +74,12 @@ def parallel_execute(objects, func, get_name, msg, get_deps=None, limit=None, pa if exception is None: writer.write(get_name(obj), 'done', green) results.append(result) + elif isinstance(exception, ImageNotFound): + # This is to bubble up ImageNotFound exceptions to the client so we + # can prompt the user if they want to rebuild. + errors[get_name(obj)] = exception.explanation + writer.write(get_name(obj), 'error', red) + error_to_reraise = exception elif isinstance(exception, APIError): errors[get_name(obj)] = exception.explanation writer.write(get_name(obj), 'error', red) diff --git a/compose/project.py b/compose/project.py index 8e1c6a14d..f69aaa7d7 100644 --- a/compose/project.py +++ b/compose/project.py @@ -442,6 +442,7 @@ class Project(object): remove_orphans=False, ignore_orphans=False, scale_override=None, + rebuild=False, rescale=True, start=True, always_recreate_deps=False): @@ -472,6 +473,7 @@ class Project(object): timeout=timeout, detached=detached, scale_override=scale_override.get(service.name), + rebuild=rebuild, rescale=rescale, start=start, project_services=scaled_services diff --git a/compose/service.py b/compose/service.py index 8a2faba95..37bd2ca2f 100644 --- a/compose/service.py +++ b/compose/service.py @@ -280,6 +280,7 @@ class Service(object): previous_container=None, number=None, quiet=False, + rebuild=False, **override_options): """ Create a container for this service. If the image doesn't exist, attempt to pull @@ -293,6 +294,7 @@ class Service(object): override_options, number or self._next_container_number(one_off=one_off), one_off=one_off, + rebuild=rebuild, previous_container=previous_container, ) @@ -409,7 +411,7 @@ class Service(object): return containers - def _execute_convergence_recreate(self, containers, scale, timeout, detached, start): + def _execute_convergence_recreate(self, containers, scale, timeout, detached, start, rebuild): if scale is not None and len(containers) > scale: self._downscale(containers[scale:], timeout) containers = containers[:scale] @@ -417,7 +419,7 @@ class Service(object): def recreate(container): return self.recreate_container( container, timeout=timeout, attach_logs=not detached, - start_new_container=start + start_new_container=start, rebuild=rebuild ) containers, errors = parallel_execute( containers, @@ -468,7 +470,8 @@ class Service(object): ) def execute_convergence_plan(self, plan, timeout=None, detached=False, - start=True, scale_override=None, rescale=True, project_services=None): + start=True, scale_override=None, rebuild=False, + rescale=True, project_services=None): (action, containers) = plan scale = scale_override if scale_override is not None else self.scale_num containers = sorted(containers, key=attrgetter('number')) @@ -487,7 +490,7 @@ class Service(object): if action == 'recreate': return self._execute_convergence_recreate( - containers, scale, timeout, detached, start + containers, scale, timeout, detached, start, rebuild ) if action == 'start': @@ -512,6 +515,7 @@ class Service(object): container, timeout=None, attach_logs=False, + rebuild=False, start_new_container=True): """Recreate a container. @@ -526,6 +530,7 @@ class Service(object): previous_container=container, number=container.labels.get(LABEL_CONTAINER_NUMBER), quiet=True, + rebuild=rebuild ) if attach_logs: new_container.attach_log_stream() @@ -746,6 +751,7 @@ class Service(object): override_options, number, one_off=False, + rebuild=False, previous_container=None): add_config_hash = (not one_off and not override_options) @@ -795,7 +801,7 @@ class Service(object): override_options.get('labels')) container_options, override_options = self._build_container_volume_options( - previous_container, container_options, override_options + previous_container, container_options, override_options, rebuild ) container_options['image'] = self.image_name @@ -822,7 +828,8 @@ class Service(object): container_options['environment']) return container_options - def _build_container_volume_options(self, previous_container, container_options, override_options): + def _build_container_volume_options(self, previous_container, container_options, + override_options, rebuild): container_volumes = [] container_mounts = [] if 'volumes' in container_options: @@ -833,7 +840,7 @@ class Service(object): binds, affinity = merge_volume_bindings( container_volumes, self.options.get('tmpfs') or [], previous_container, - container_mounts + container_mounts, rebuild ) override_options['binds'] = binds container_options['environment'].update(affinity) @@ -1281,7 +1288,7 @@ def parse_repository_tag(repo_path): # Volumes -def merge_volume_bindings(volumes, tmpfs, previous_container, mounts): +def merge_volume_bindings(volumes, tmpfs, previous_container, mounts, rebuild): """ Return a list of volume bindings for a container. Container data volumes are replaced by those from the previous container. @@ -1297,7 +1304,7 @@ def merge_volume_bindings(volumes, tmpfs, previous_container, mounts): if previous_container: old_volumes, old_mounts = get_container_data_volumes( - previous_container, volumes, tmpfs, mounts + previous_container, volumes, tmpfs, mounts, rebuild ) warn_on_masked_volume(volumes, old_volumes, previous_container.service) volume_bindings.update( @@ -1310,12 +1317,34 @@ def merge_volume_bindings(volumes, tmpfs, previous_container, mounts): return list(volume_bindings.values()), affinity -def get_container_data_volumes(container, volumes_option, tmpfs_option, mounts_option): +def try_get_image_volumes(container, rebuild): + """ + Try to get the volumes from the existing container. If the image does + not exist, prompt the user to either continue (rebuild the image from + scratch) or raise an exception. + """ + + try: + image_volumes = [ + VolumeSpec.parse(volume) + for volume in + container.image_config['ContainerConfig'].get('Volumes') or {} + ] + return image_volumes + except ImageNotFound: + if rebuild: + # This will force Compose to rebuild the images. + return [] + raise + + +def get_container_data_volumes(container, volumes_option, tmpfs_option, mounts_option, rebuild): """ Find the container data volumes that are in `volumes_option`, and return a mapping of volume bindings for those volumes. Anonymous volume mounts are updated in place instead. """ + volumes = [] volumes_option = volumes_option or [] @@ -1324,11 +1353,7 @@ def get_container_data_volumes(container, volumes_option, tmpfs_option, mounts_o for mount in container.get('Mounts') or {} ) - image_volumes = [ - VolumeSpec.parse(volume) - for volume in - container.image_config['ContainerConfig'].get('Volumes') or {} - ] + image_volumes = try_get_image_volumes(container, rebuild) for volume in set(volumes_option + image_volumes): # No need to preserve host volumes diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index 92d7f08d5..44f14e58b 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -923,7 +923,19 @@ class ServiceVolumesTest(unittest.TestCase): VolumeSpec.parse('imagedata:/mnt/image/data:rw'), ] - volumes, _ = get_container_data_volumes(container, options, ['/dev/tmpfs'], []) + volumes, _ = get_container_data_volumes(container, options, ['/dev/tmpfs'], [], False) + assert sorted(volumes) == sorted(expected) + + # Issue 5465, check for non-existant image. + + container = Container(self.mock_client, { + 'Image': None, + 'Mounts': [] + }, has_been_inspected=True) + + expected = [] + + volumes, _ = get_container_data_volumes(container, options, ['/dev/tmpfs'], [], False) assert sorted(volumes) == sorted(expected) def test_merge_volume_bindings(self): @@ -959,7 +971,7 @@ class ServiceVolumesTest(unittest.TestCase): 'existingvolume:/existing/volume:rw', ] - binds, affinity = merge_volume_bindings(options, ['/dev/tmpfs'], previous_container, []) + binds, affinity = merge_volume_bindings(options, ['/dev/tmpfs'], previous_container, [], False) assert sorted(binds) == sorted(expected) assert affinity == {'affinity:container': '=cdefab'} From 2d986dff79f6d8a63f66ad46e9c231e0d7910165 Mon Sep 17 00:00:00 2001 From: Joffrey F Date: Thu, 11 Jan 2018 16:37:09 -0800 Subject: [PATCH 2/4] Remove rebuild parameter ; set do_build instead Reduce repetition in main.py Signed-off-by: Joffrey F --- compose/cli/main.py | 25 ++++++---------------- compose/project.py | 2 -- compose/service.py | 44 +++++++++++++------------------------- tests/unit/service_test.py | 14 +++++++++--- 4 files changed, 33 insertions(+), 52 deletions(-) diff --git a/compose/cli/main.py b/compose/cli/main.py index bc10c5c4f..14f7a745c 100644 --- a/compose/cli/main.py +++ b/compose/cli/main.py @@ -977,12 +977,12 @@ class TopLevelCommand(object): raise UserError('--no-start and {} cannot be combined.'.format(excluded)) with up_shutdown_context(self.project, service_names, timeout, detached): - try: - to_attach = self.project.up( + def up(rebuild): + return self.project.up( service_names=service_names, start_deps=start_deps, strategy=convergence_strategy_from_opts(options), - do_build=build_action_from_opts(options), + do_build=build_action_from_opts(options) if not rebuild else BuildAction.force, timeout=timeout, detached=detached, remove_orphans=remove_orphans, @@ -990,8 +990,10 @@ class TopLevelCommand(object): scale_override=parse_scale_args(options['--scale']), start=not no_start, always_recreate_deps=always_recreate_deps, - rebuild=False ) + + try: + to_attach = up(False) except docker.errors.ImageNotFound as e: log.error(("Image not found. If you continue, there is a " "risk of data loss. Consider backing up your data " @@ -1002,20 +1004,7 @@ class TopLevelCommand(object): if res is None or not res: raise e - to_attach = self.project.up( - service_names=service_names, - start_deps=start_deps, - strategy=convergence_strategy_from_opts(options), - do_build=build_action_from_opts(options), - timeout=timeout, - detached=detached, - remove_orphans=remove_orphans, - ignore_orphans=ignore_orphans, - scale_override=parse_scale_args(options['--scale']), - start=not no_start, - always_recreate_deps=always_recreate_deps, - rebuild=True - ) + to_attach = up(True) if detached or no_start: return diff --git a/compose/project.py b/compose/project.py index f69aaa7d7..8e1c6a14d 100644 --- a/compose/project.py +++ b/compose/project.py @@ -442,7 +442,6 @@ class Project(object): remove_orphans=False, ignore_orphans=False, scale_override=None, - rebuild=False, rescale=True, start=True, always_recreate_deps=False): @@ -473,7 +472,6 @@ class Project(object): timeout=timeout, detached=detached, scale_override=scale_override.get(service.name), - rebuild=rebuild, rescale=rescale, start=start, project_services=scaled_services diff --git a/compose/service.py b/compose/service.py index 37bd2ca2f..195257c39 100644 --- a/compose/service.py +++ b/compose/service.py @@ -280,7 +280,6 @@ class Service(object): previous_container=None, number=None, quiet=False, - rebuild=False, **override_options): """ Create a container for this service. If the image doesn't exist, attempt to pull @@ -294,7 +293,6 @@ class Service(object): override_options, number or self._next_container_number(one_off=one_off), one_off=one_off, - rebuild=rebuild, previous_container=previous_container, ) @@ -411,7 +409,7 @@ class Service(object): return containers - def _execute_convergence_recreate(self, containers, scale, timeout, detached, start, rebuild): + def _execute_convergence_recreate(self, containers, scale, timeout, detached, start): if scale is not None and len(containers) > scale: self._downscale(containers[scale:], timeout) containers = containers[:scale] @@ -419,7 +417,7 @@ class Service(object): def recreate(container): return self.recreate_container( container, timeout=timeout, attach_logs=not detached, - start_new_container=start, rebuild=rebuild + start_new_container=start ) containers, errors = parallel_execute( containers, @@ -470,7 +468,7 @@ class Service(object): ) def execute_convergence_plan(self, plan, timeout=None, detached=False, - start=True, scale_override=None, rebuild=False, + start=True, scale_override=None, rescale=True, project_services=None): (action, containers) = plan scale = scale_override if scale_override is not None else self.scale_num @@ -490,7 +488,7 @@ class Service(object): if action == 'recreate': return self._execute_convergence_recreate( - containers, scale, timeout, detached, start, rebuild + containers, scale, timeout, detached, start ) if action == 'start': @@ -510,13 +508,7 @@ class Service(object): raise Exception("Invalid action: {}".format(action)) - def recreate_container( - self, - container, - timeout=None, - attach_logs=False, - rebuild=False, - start_new_container=True): + def recreate_container(self, container, timeout=None, attach_logs=False, start_new_container=True): """Recreate a container. The original container is renamed to a temporary name so that data @@ -530,7 +522,6 @@ class Service(object): previous_container=container, number=container.labels.get(LABEL_CONTAINER_NUMBER), quiet=True, - rebuild=rebuild ) if attach_logs: new_container.attach_log_stream() @@ -751,7 +742,6 @@ class Service(object): override_options, number, one_off=False, - rebuild=False, previous_container=None): add_config_hash = (not one_off and not override_options) @@ -801,7 +791,7 @@ class Service(object): override_options.get('labels')) container_options, override_options = self._build_container_volume_options( - previous_container, container_options, override_options, rebuild + previous_container, container_options, override_options ) container_options['image'] = self.image_name @@ -828,8 +818,7 @@ class Service(object): container_options['environment']) return container_options - def _build_container_volume_options(self, previous_container, container_options, - override_options, rebuild): + def _build_container_volume_options(self, previous_container, container_options, override_options): container_volumes = [] container_mounts = [] if 'volumes' in container_options: @@ -840,7 +829,7 @@ class Service(object): binds, affinity = merge_volume_bindings( container_volumes, self.options.get('tmpfs') or [], previous_container, - container_mounts, rebuild + container_mounts ) override_options['binds'] = binds container_options['environment'].update(affinity) @@ -1288,7 +1277,7 @@ def parse_repository_tag(repo_path): # Volumes -def merge_volume_bindings(volumes, tmpfs, previous_container, mounts, rebuild): +def merge_volume_bindings(volumes, tmpfs, previous_container, mounts): """ Return a list of volume bindings for a container. Container data volumes are replaced by those from the previous container. @@ -1304,7 +1293,7 @@ def merge_volume_bindings(volumes, tmpfs, previous_container, mounts, rebuild): if previous_container: old_volumes, old_mounts = get_container_data_volumes( - previous_container, volumes, tmpfs, mounts, rebuild + previous_container, volumes, tmpfs, mounts ) warn_on_masked_volume(volumes, old_volumes, previous_container.service) volume_bindings.update( @@ -1317,11 +1306,11 @@ def merge_volume_bindings(volumes, tmpfs, previous_container, mounts, rebuild): return list(volume_bindings.values()), affinity -def try_get_image_volumes(container, rebuild): +def try_get_image_volumes(container): """ Try to get the volumes from the existing container. If the image does - not exist, prompt the user to either continue (rebuild the image from - scratch) or raise an exception. + not exist, raise an exception that will be caught at the CLI level to + prompt user for a rebuild. """ try: @@ -1332,13 +1321,10 @@ def try_get_image_volumes(container, rebuild): ] return image_volumes except ImageNotFound: - if rebuild: - # This will force Compose to rebuild the images. - return [] raise -def get_container_data_volumes(container, volumes_option, tmpfs_option, mounts_option, rebuild): +def get_container_data_volumes(container, volumes_option, tmpfs_option, mounts_option): """ Find the container data volumes that are in `volumes_option`, and return a mapping of volume bindings for those volumes. @@ -1353,7 +1339,7 @@ def get_container_data_volumes(container, volumes_option, tmpfs_option, mounts_o for mount in container.get('Mounts') or {} ) - image_volumes = try_get_image_volumes(container, rebuild) + image_volumes = try_get_image_volumes(container) for volume in set(volumes_option + image_volumes): # No need to preserve host volumes diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index 44f14e58b..ef250a4d7 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -923,10 +923,18 @@ class ServiceVolumesTest(unittest.TestCase): VolumeSpec.parse('imagedata:/mnt/image/data:rw'), ] - volumes, _ = get_container_data_volumes(container, options, ['/dev/tmpfs'], [], False) + volumes, _ = get_container_data_volumes(container, options, ['/dev/tmpfs'], []) assert sorted(volumes) == sorted(expected) + def test_get_container_data_volumes_image_is_none(self): # Issue 5465, check for non-existant image. + options = [VolumeSpec.parse(v) for v in [ + '/host/volume:/host/volume:ro', + '/new/volume', + '/existing/volume', + 'named:/named/vol', + '/dev/tmpfs' + ]] container = Container(self.mock_client, { 'Image': None, @@ -935,7 +943,7 @@ class ServiceVolumesTest(unittest.TestCase): expected = [] - volumes, _ = get_container_data_volumes(container, options, ['/dev/tmpfs'], [], False) + volumes, _ = get_container_data_volumes(container, options, ['/dev/tmpfs'], []) assert sorted(volumes) == sorted(expected) def test_merge_volume_bindings(self): @@ -971,7 +979,7 @@ class ServiceVolumesTest(unittest.TestCase): 'existingvolume:/existing/volume:rw', ] - binds, affinity = merge_volume_bindings(options, ['/dev/tmpfs'], previous_container, [], False) + binds, affinity = merge_volume_bindings(options, ['/dev/tmpfs'], previous_container, []) assert sorted(binds) == sorted(expected) assert affinity == {'affinity:container': '=cdefab'} From 4042121f6e8e302206189bd110c46791ce72bc28 Mon Sep 17 00:00:00 2001 From: Ian Glen Neal Date: Fri, 12 Jan 2018 06:29:16 +0000 Subject: [PATCH 3/4] The updated unit test simulates the error in the bug. What's happening is that the container has an image sha (represented by 'shaDOES_NOT_EXIST') which causes an error when client.inspect_image is called on it, because the image has actually been removed. Signed-off-by: Ian Glen Neal --- compose/cli/main.py | 7 +++---- compose/service.py | 24 +++++------------------- tests/unit/service_test.py | 12 ++++++++++-- 3 files changed, 18 insertions(+), 25 deletions(-) diff --git a/compose/cli/main.py b/compose/cli/main.py index 14f7a745c..ef7ea0c9d 100644 --- a/compose/cli/main.py +++ b/compose/cli/main.py @@ -971,10 +971,9 @@ class TopLevelCommand(object): if ignore_orphans and remove_orphans: raise UserError("COMPOSE_IGNORE_ORPHANS and --remove-orphans cannot be combined.") - if no_start: - opts = ['-d', '--abort-on-container-exit', '--exit-code-from'] - for excluded in [x for x in opts if options.get(x)]: - raise UserError('--no-start and {} cannot be combined.'.format(excluded)) + opts = ['-d', '--abort-on-container-exit', '--exit-code-from'] + for excluded in [x for x in opts if options.get(x) and no_start]: + raise UserError('--no-start and {} cannot be combined.'.format(excluded)) with up_shutdown_context(self.project, service_names, timeout, detached): def up(rebuild): diff --git a/compose/service.py b/compose/service.py index 195257c39..97455f6c2 100644 --- a/compose/service.py +++ b/compose/service.py @@ -1306,24 +1306,6 @@ def merge_volume_bindings(volumes, tmpfs, previous_container, mounts): return list(volume_bindings.values()), affinity -def try_get_image_volumes(container): - """ - Try to get the volumes from the existing container. If the image does - not exist, raise an exception that will be caught at the CLI level to - prompt user for a rebuild. - """ - - try: - image_volumes = [ - VolumeSpec.parse(volume) - for volume in - container.image_config['ContainerConfig'].get('Volumes') or {} - ] - return image_volumes - except ImageNotFound: - raise - - def get_container_data_volumes(container, volumes_option, tmpfs_option, mounts_option): """ Find the container data volumes that are in `volumes_option`, and return @@ -1339,7 +1321,11 @@ def get_container_data_volumes(container, volumes_option, tmpfs_option, mounts_o for mount in container.get('Mounts') or {} ) - image_volumes = try_get_image_volumes(container) + image_volumes = [ + VolumeSpec.parse(volume) + for volume in + container.image_config['ContainerConfig'].get('Volumes') or {} + ] for volume in set(volumes_option + image_volumes): # No need to preserve host volumes diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index ef250a4d7..bdeaebdd7 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -5,6 +5,7 @@ import docker import pytest from docker.constants import DEFAULT_DOCKER_API_VERSION from docker.errors import APIError +from docker.errors import ImageNotFound from .. import mock from .. import unittest @@ -926,7 +927,7 @@ class ServiceVolumesTest(unittest.TestCase): volumes, _ = get_container_data_volumes(container, options, ['/dev/tmpfs'], []) assert sorted(volumes) == sorted(expected) - def test_get_container_data_volumes_image_is_none(self): + def test_get_container_data_volumes_image_does_not_exist(self): # Issue 5465, check for non-existant image. options = [VolumeSpec.parse(v) for v in [ '/host/volume:/host/volume:ro', @@ -936,8 +937,15 @@ class ServiceVolumesTest(unittest.TestCase): '/dev/tmpfs' ]] + def inspect_fn(image): + if image == 'shaDOES_NOT_EXIST': + raise ImageNotFound("inspect_fn: {}".format(image)) + return {'ContainerConfig': None} + + self.mock_client.inspect_image = inspect_fn + container = Container(self.mock_client, { - 'Image': None, + 'Image': 'shaDOES_NOT_EXIST', 'Mounts': [] }, has_been_inspected=True) From 9ba9016cbc66bf2473fa62b1e831ae2c70408509 Mon Sep 17 00:00:00 2001 From: Joffrey F Date: Wed, 17 Jan 2018 17:08:13 -0800 Subject: [PATCH 4/4] Improve ImageNotFound handling in convergence - Improved CLI prompt - Attempt volume preservation with new image config - Fix container.rename_to_tmp_name() - Integration test replicates behavior ; remove obsolete unit test Signed-off-by: Joffrey F --- compose/cli/main.py | 33 ++++++++++++++++++++++++------- compose/container.py | 32 +++++++++++++++++++++++------- compose/project.py | 24 ++++------------------ compose/service.py | 9 ++++++++- tests/integration/service_test.py | 30 ++++++++++++++++++++++++++++ tests/unit/cli/main_test.py | 10 ++++++++++ tests/unit/project_test.py | 8 -------- tests/unit/service_test.py | 28 -------------------------- 8 files changed, 103 insertions(+), 71 deletions(-) diff --git a/compose/cli/main.py b/compose/cli/main.py index ef7ea0c9d..de4fbd430 100644 --- a/compose/cli/main.py +++ b/compose/cli/main.py @@ -976,12 +976,14 @@ class TopLevelCommand(object): raise UserError('--no-start and {} cannot be combined.'.format(excluded)) with up_shutdown_context(self.project, service_names, timeout, detached): + warn_for_swarm_mode(self.project.client) + def up(rebuild): return self.project.up( service_names=service_names, start_deps=start_deps, strategy=convergence_strategy_from_opts(options), - do_build=build_action_from_opts(options) if not rebuild else BuildAction.force, + do_build=build_action_from_opts(options), timeout=timeout, detached=detached, remove_orphans=remove_orphans, @@ -989,17 +991,18 @@ class TopLevelCommand(object): scale_override=parse_scale_args(options['--scale']), start=not no_start, always_recreate_deps=always_recreate_deps, + reset_container_image=rebuild, ) try: to_attach = up(False) except docker.errors.ImageNotFound as e: - log.error(("Image not found. If you continue, there is a " - "risk of data loss. Consider backing up your data " - "before continuing.\n\n" - "Full error message: {}\n" - ).format(e.explanation)) - res = yesno("Continue by rebuilding the image(s)? [yN]", False) + log.error( + "The image for the service you're trying to recreate has been removed. " + "If you continue, volume data could be lost. Consider backing up your data " + "before continuing.\n".format(e.explanation) + ) + res = yesno("Continue with the new image? [yN]", False) if res is None or not res: raise e @@ -1426,3 +1429,19 @@ def build_filter(arg): key, val = arg.split('=', 1) filt[key] = val return filt + + +def warn_for_swarm_mode(client): + info = client.info() + if info.get('Swarm', {}).get('LocalNodeState') == 'active': + if info.get('ServerVersion', '').startswith('ucp'): + # UCP does multi-node scheduling with traditional Compose files. + return + + log.warn( + "The Docker Engine you're using is running in swarm mode.\n\n" + "Compose does not use swarm mode to deploy services to multiple nodes in a swarm. " + "All containers will be scheduled on the current node.\n\n" + "To deploy your application across the swarm, " + "use `docker stack deploy`.\n" + ) diff --git a/compose/container.py b/compose/container.py index 4bc7f54f9..4ab99ffa8 100644 --- a/compose/container.py +++ b/compose/container.py @@ -4,6 +4,7 @@ from __future__ import unicode_literals from functools import reduce import six +from docker.errors import ImageNotFound from .const import LABEL_CONTAINER_NUMBER from .const import LABEL_PROJECT @@ -66,15 +67,17 @@ class Container(object): def name(self): return self.dictionary['Name'][1:] + @property + def project(self): + return self.labels.get(LABEL_PROJECT) + @property def service(self): return self.labels.get(LABEL_SERVICE) @property def name_without_project(self): - project = self.labels.get(LABEL_PROJECT) - - if self.name.startswith('{0}_{1}'.format(project, self.service)): + if self.name.startswith('{0}_{1}'.format(self.project, self.service)): return '{0}_{1}'.format(self.service, self.number) else: return self.name @@ -230,10 +233,10 @@ class Container(object): """Rename the container to a hopefully unique temporary container name by prepending the short id. """ - self.client.rename( - self.id, - '%s_%s' % (self.short_id, self.name) - ) + if not self.name.startswith(self.short_id): + self.client.rename( + self.id, '{0}_{1}'.format(self.short_id, self.name) + ) def inspect_if_not_inspected(self): if not self.has_been_inspected: @@ -250,6 +253,21 @@ class Container(object): self.has_been_inspected = True return self.dictionary + def image_exists(self): + try: + self.client.inspect_image(self.image) + except ImageNotFound: + return False + + return True + + def reset_image(self, img_id): + """ If this container's image has been removed, temporarily replace the old image ID + with `img_id`. + """ + if not self.image_exists(): + self.dictionary['Image'] = img_id + def attach(self, *args, **kwargs): return self.client.attach(self.id, *args, **kwargs) diff --git a/compose/project.py b/compose/project.py index 8e1c6a14d..6af4cd94a 100644 --- a/compose/project.py +++ b/compose/project.py @@ -444,9 +444,8 @@ class Project(object): scale_override=None, rescale=True, start=True, - always_recreate_deps=False): - - warn_for_swarm_mode(self.client) + always_recreate_deps=False, + reset_container_image=False): self.initialize() if not ignore_orphans: @@ -474,7 +473,8 @@ class Project(object): scale_override=scale_override.get(service.name), rescale=rescale, start=start, - project_services=scaled_services + project_services=scaled_services, + reset_container_image=reset_container_image ) def get_deps(service): @@ -686,22 +686,6 @@ def get_secrets(service, service_secrets, secret_defs): return secrets -def warn_for_swarm_mode(client): - info = client.info() - if info.get('Swarm', {}).get('LocalNodeState') == 'active': - if info.get('ServerVersion', '').startswith('ucp'): - # UCP does multi-node scheduling with traditional Compose files. - return - - log.warn( - "The Docker Engine you're using is running in swarm mode.\n\n" - "Compose does not use swarm mode to deploy services to multiple nodes in a swarm. " - "All containers will be scheduled on the current node.\n\n" - "To deploy your application across the swarm, " - "use `docker stack deploy`.\n" - ) - - class NoSuchService(Exception): def __init__(self, name): if isinstance(name, six.binary_type): diff --git a/compose/service.py b/compose/service.py index 97455f6c2..4e2363511 100644 --- a/compose/service.py +++ b/compose/service.py @@ -469,7 +469,8 @@ class Service(object): def execute_convergence_plan(self, plan, timeout=None, detached=False, start=True, scale_override=None, - rescale=True, project_services=None): + rescale=True, project_services=None, + reset_container_image=False): (action, containers) = plan scale = scale_override if scale_override is not None else self.scale_num containers = sorted(containers, key=attrgetter('number')) @@ -487,6 +488,12 @@ class Service(object): scale = None if action == 'recreate': + if reset_container_image: + # Updating the image ID on the container object lets us recover old volumes if + # the new image uses them as well + img_id = self.image()['Id'] + for c in containers: + c.reset_image(img_id) return self._execute_convergence_recreate( containers, scale, timeout, detached, start ) diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index 01be48005..a6efc24a9 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -10,6 +10,7 @@ from os import path import pytest from docker.errors import APIError +from docker.errors import ImageNotFound from six import StringIO from six import text_type @@ -659,6 +660,35 @@ class ServiceTest(DockerClientTestCase): assert len(service_containers) == 1 assert not service_containers[0].is_running + def test_execute_convergence_plan_image_with_volume_is_removed(self): + service = self.create_service( + 'db', build={'context': 'tests/fixtures/dockerfile-with-volume'} + ) + + old_container = create_and_start_container(service) + assert ( + [mount['Destination'] for mount in old_container.get('Mounts')] == + ['/data'] + ) + volume_path = old_container.get_mount('/data')['Source'] + + old_container.stop() + self.client.remove_image(service.image(), force=True) + + service.ensure_image_exists() + with pytest.raises(ImageNotFound): + service.execute_convergence_plan( + ConvergencePlan('recreate', [old_container]) + ) + old_container.inspect() # retrieve new name from server + + new_container, = service.execute_convergence_plan( + ConvergencePlan('recreate', [old_container]), + reset_container_image=True + ) + assert [mount['Destination'] for mount in new_container.get('Mounts')] == ['/data'] + assert new_container.get_mount('/data')['Source'] == volume_path + 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/cli/main_test.py b/tests/unit/cli/main_test.py index dc5278800..b1546d6f3 100644 --- a/tests/unit/cli/main_test.py +++ b/tests/unit/cli/main_test.py @@ -3,6 +3,7 @@ from __future__ import unicode_literals import logging +import docker import pytest from compose import container @@ -11,6 +12,7 @@ from compose.cli.formatter import ConsoleWarningFormatter from compose.cli.main import convergence_strategy_from_opts from compose.cli.main import filter_containers_to_service_names from compose.cli.main import setup_console_handler +from compose.cli.main import warn_for_swarm_mode from compose.service import ConvergenceStrategy from tests import mock @@ -54,6 +56,14 @@ class TestCLIMainTestCase(object): actual = filter_containers_to_service_names(containers, service_names) assert actual == containers + def test_warning_in_swarm_mode(self): + mock_client = mock.create_autospec(docker.APIClient) + mock_client.info.return_value = {'Swarm': {'LocalNodeState': 'active'}} + + with mock.patch('compose.cli.main.log') as fake_log: + warn_for_swarm_mode(mock_client) + assert fake_log.warn.call_count == 1 + class TestSetupConsoleHandlerTestCase(object): diff --git a/tests/unit/project_test.py b/tests/unit/project_test.py index 397287ad9..a0291d9f9 100644 --- a/tests/unit/project_test.py +++ b/tests/unit/project_test.py @@ -533,14 +533,6 @@ class ProjectTest(unittest.TestCase): project.down(ImageType.all, True) self.mock_client.remove_image.assert_called_once_with("busybox:latest") - def test_warning_in_swarm_mode(self): - self.mock_client.info.return_value = {'Swarm': {'LocalNodeState': 'active'}} - project = Project('composetest', [], self.mock_client) - - with mock.patch('compose.project.log') as fake_log: - project.up() - assert fake_log.warn.call_count == 1 - def test_no_warning_on_stop(self): self.mock_client.info.return_value = {'Swarm': {'LocalNodeState': 'active'}} project = Project('composetest', [], self.mock_client) diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index bdeaebdd7..92d7f08d5 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -5,7 +5,6 @@ import docker import pytest from docker.constants import DEFAULT_DOCKER_API_VERSION from docker.errors import APIError -from docker.errors import ImageNotFound from .. import mock from .. import unittest @@ -927,33 +926,6 @@ class ServiceVolumesTest(unittest.TestCase): volumes, _ = get_container_data_volumes(container, options, ['/dev/tmpfs'], []) assert sorted(volumes) == sorted(expected) - def test_get_container_data_volumes_image_does_not_exist(self): - # Issue 5465, check for non-existant image. - options = [VolumeSpec.parse(v) for v in [ - '/host/volume:/host/volume:ro', - '/new/volume', - '/existing/volume', - 'named:/named/vol', - '/dev/tmpfs' - ]] - - def inspect_fn(image): - if image == 'shaDOES_NOT_EXIST': - raise ImageNotFound("inspect_fn: {}".format(image)) - return {'ContainerConfig': None} - - self.mock_client.inspect_image = inspect_fn - - container = Container(self.mock_client, { - 'Image': 'shaDOES_NOT_EXIST', - 'Mounts': [] - }, has_been_inspected=True) - - expected = [] - - volumes, _ = get_container_data_volumes(container, options, ['/dev/tmpfs'], []) - assert sorted(volumes) == sorted(expected) - def test_merge_volume_bindings(self): options = [ VolumeSpec.parse(v, True) for v in [