From 98d17a6c332ca58079b833c7cd1de2cc907a23b5 Mon Sep 17 00:00:00 2001 From: NiallBunting Date: Thu, 20 Aug 2015 13:05:45 +0000 Subject: [PATCH] Changes behaviour when an image fails uploading Currently when an image fails being uploaded the task is left in the failed state (with the internal sever error message) and the image is left in the saving state. This patch calls for the image to be deleted as the image is never seen by the user, also returns a more specific task error message. APIImpact Change-Id: I15fbe9f0041407892144c0d26eadc4ed939e5abf Closes-Bug: 1487026 --- glance/async/flows/base_import.py | 28 ++++++++++++------- glance/async/taskflow_executor.py | 3 ++ .../unit/async/test_taskflow_executor.py | 13 ++++++++- 3 files changed, 33 insertions(+), 11 deletions(-) diff --git a/glance/async/flows/base_import.py b/glance/async/flows/base_import.py index 3d8aea8eca..e70ba3cc25 100644 --- a/glance/async/flows/base_import.py +++ b/glance/async/flows/base_import.py @@ -70,13 +70,16 @@ class _CreateImage(task.Task): return image.image_id def revert(self, *args, **kwargs): - # TODO(flaper87): Define the revert rules for images on failures. - # Deleting the image may not be what we want since users could upload - # the image data in a separate step. However, it really depends on - # when the failure happened. I guess we should check if data has been - # written, although at that point failures are (should be) unexpected, - # at least image-workflow wise. - pass + # TODO(NiallBunting): Deleting the image like this could be considered + # a brute force way of reverting images. It may be worth checking if + # data has been written. + result = kwargs.get('result', None) + if result is not None: + if kwargs.get('flow_failures', None) is not None: + image = self.image_repo.get(result) + LOG.debug("Deleting image whilst reverting.") + image.delete() + self.image_repo.remove(image) class _ImportToFS(task.Task): @@ -305,9 +308,14 @@ class _ImportToStore(task.Task): # os.rename(file_path, image_path) # # image_import.set_image_data(image, image_path, None) - - image_import.set_image_data(image, file_path or self.uri, self.task_id) - + try: + image_import.set_image_data(image, + file_path or self.uri, self.task_id) + except IOError as e: + msg = (_('Uploading the image failed due to: %(exc)s') % + {'exc': encodeutils.exception_to_unicode(e)}) + LOG.error(msg) + raise exception.UploadException(message=msg) # NOTE(flaper87): We need to save the image again after the locations # have been set in the image. self.image_repo.save(image) diff --git a/glance/async/taskflow_executor.py b/glance/async/taskflow_executor.py index a7d540f739..b89448ff40 100644 --- a/glance/async/taskflow_executor.py +++ b/glance/async/taskflow_executor.py @@ -134,6 +134,9 @@ class TaskExecutor(glance.async.TaskExecutor): max_workers=CONF.taskflow_executor.max_workers) with llistener.DynamicLoggingListener(engine, log=LOG): engine.run() + except exception.UploadException as exc: + task.fail(encodeutils.exception_to_unicode(exc)) + self.task_repo.save(task) except Exception as exc: with excutils.save_and_reraise_exception(): LOG.error(_LE('Failed to execute task %(task_id)s: %(exc)s') % diff --git a/glance/tests/unit/async/test_taskflow_executor.py b/glance/tests/unit/async/test_taskflow_executor.py index 6b722714ec..c1a4ef6760 100644 --- a/glance/tests/unit/async/test_taskflow_executor.py +++ b/glance/tests/unit/async/test_taskflow_executor.py @@ -20,10 +20,10 @@ from oslo_config import cfg from taskflow import engines from glance.async import taskflow_executor +from glance.common.scripts.image_import import main as image_import from glance import domain import glance.tests.utils as test_utils - CONF = cfg.CONF TENANT1 = '6838eb7b-6ded-434a-882c-b344c77fe8df' @@ -89,3 +89,14 @@ class TestTaskExecutor(test_utils.BaseTestCase): self.task.task_id) self.assertEqual('failure', self.task.status) self.task_repo.save.assert_called_with(self.task) + + def test_task_fail_upload(self): + with mock.patch.object(image_import, 'set_image_data') as import_mock: + import_mock.side_effect = IOError + + self.task_repo.get.return_value = self.task + self.executor.begin_processing(self.task.task_id) + + self.assertEqual('failure', self.task.status) + self.task_repo.save.assert_called_with(self.task) + self.assertEqual(1, import_mock.call_count)