openstack/glance
Code Issues Proposed changes

Make web-download revert all stores on fail

Browse Source 
If web-download fails to stage the image, no subsequent import to
store tasks could have run. Thus, we should remove all of them from
"importing" and add them to "failed" so that it is obvious from the
outside that we failed.

This is another good reason for having the $image/tasks API, but we
also should not continue advertising "yes we're importing to $store"
when we are not.

Change-Id: Iebbb2dcb767ecf3c965f34f1ca04af20a2039be1
Closes-Bug: #1914826
This commit is contained in:
Dan Smith
2021-02-05 14:35:51 -08:00
parent 68646db35d
commit cbb2af6e32
2 changed files with 48 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
glance/async_/flows/_internal_plugins/web_download.py
View File

@@ -35,12 +35,13 @@ class _WebDownload(task.Task):
default_provides = 'file_uri'
def __init__(self, task_id, task_type, uri, action_wrapper):
def __init__(self, task_id, task_type, uri, action_wrapper, stores):
self.task_id = task_id
self.task_type = task_type
self.image_id = action_wrapper.image_id
self.uri = uri
self.action_wrapper = action_wrapper
self.stores = stores
self._path = None
super(_WebDownload, self).__init__(
name='%s-WebDownload-%s' % (task_type, task_id))
@@ -140,8 +141,13 @@ class _WebDownload(task.Task):
'image_id': self.image_id})
# NOTE(abhishekk): Revert image state back to 'queued' as
# something went wrong.
# NOTE(danms): If we failed to stage the image, then none
# of the _ImportToStore() tasks could have run, so we need
# to move all stores out of "importing" and into "failed".
with self.action_wrapper as action:
action.set_image_status('queued')
action.remove_importing_stores(self.stores)
action.add_failed_stores(self.stores)
# NOTE(abhishekk): Deleting partial image data from staging area
if self._path is not None:
@@ -171,7 +177,8 @@ def get_flow(**kwargs):
task_type = kwargs.get('task_type')
uri = kwargs.get('import_req')['method'].get('uri')
action_wrapper = kwargs.get('action_wrapper')
stores = kwargs.get('backend', [None])
return lf.Flow(task_type).add(
_WebDownload(task_id, task_type, uri, action_wrapper),
_WebDownload(task_id, task_type, uri, action_wrapper, stores),
)

52
glance/tests/unit/async_/flows/test_web_download.py
View File

@@ -69,7 +69,8 @@ class TestWebDownloadTask(test_utils.BaseTestCase):
@mock.patch.object(filesystem.Store, 'add')
def test_web_download(self, mock_add):
web_download_task = web_download._WebDownload(
self.task.task_id, self.task_type, self.uri, self.action_wrapper)
self.task.task_id, self.task_type, self.uri, self.action_wrapper,
['foo'])
with mock.patch.object(script_utils,
'get_image_data_iter') as mock_iter:
mock_add.return_value = ["path", 4]
@@ -81,7 +82,8 @@ class TestWebDownloadTask(test_utils.BaseTestCase):
@mock.patch.object(filesystem.Store, 'add')
def test_web_download_with_content_length(self, mock_add):
web_download_task = web_download._WebDownload(
self.task.task_id, self.task_type, self.uri, self.action_wrapper)
self.task.task_id, self.task_type, self.uri, self.action_wrapper,
['foo'])
with mock.patch.object(script_utils,
'get_image_data_iter') as mock_iter:
mock_iter.return_value.headers = {'content-length': '4'}
@@ -93,7 +95,8 @@ class TestWebDownloadTask(test_utils.BaseTestCase):
@mock.patch.object(filesystem.Store, 'add')
def test_web_download_with_invalid_content_length(self, mock_add):
web_download_task = web_download._WebDownload(
self.task.task_id, self.task_type, self.uri, self.action_wrapper)
self.task.task_id, self.task_type, self.uri, self.action_wrapper,
['foo'])
with mock.patch.object(script_utils,
'get_image_data_iter') as mock_iter:
mock_iter.return_value.headers = {'content-length': "not_valid"}
@@ -105,7 +108,8 @@ class TestWebDownloadTask(test_utils.BaseTestCase):
@mock.patch.object(filesystem.Store, 'add')
def test_web_download_fails_when_data_size_different(self, mock_add):
web_download_task = web_download._WebDownload(
self.task.task_id, self.task_type, self.uri, self.action_wrapper)
self.task.task_id, self.task_type, self.uri, self.action_wrapper,
['foo'])
with mock.patch.object(script_utils,
'get_image_data_iter') as mock_iter:
mock_iter.return_value.headers = {'content-length': '4'}
@@ -118,7 +122,8 @@ class TestWebDownloadTask(test_utils.BaseTestCase):
self.config(node_staging_uri=None)
self.assertRaises(glance.common.exception.BadTaskConfiguration,
web_download._WebDownload, self.task.task_id,
self.task_type, self.uri, self.action_wrapper)
self.task_type, self.uri, self.action_wrapper,
['foo'])
@mock.patch.object(cfg.ConfigOpts, "set_override")
def test_web_download_node_store_initialization_failed(self,
@@ -127,12 +132,14 @@ class TestWebDownloadTask(test_utils.BaseTestCase):
mock_load_store.return_value = None
self.assertRaises(glance.common.exception.BadTaskConfiguration,
web_download._WebDownload, self.task.task_id,
self.task_type, self.uri, self.action_wrapper)
self.task_type, self.uri, self.action_wrapper,
['foo'])
mock_override.assert_called()
def test_web_download_failed(self):
web_download_task = web_download._WebDownload(
self.task.task_id, self.task_type, self.uri, self.action_wrapper)
self.task.task_id, self.task_type, self.uri, self.action_wrapper,
['foo'])
with mock.patch.object(script_utils,
"get_image_data_iter") as mock_iter:
mock_iter.side_effect = glance.common.exception.NotFound
@@ -180,8 +187,12 @@ class TestWebDownloadTask(test_utils.BaseTestCase):
@mock.patch("glance.async_.flows._internal_plugins.web_download.store_api")
def test_web_download_revert_with_failure(self, mock_store_api,
mock_add):
image = self.image_repo.get.return_value
image.extra_properties['os_glance_importing_to_stores'] = 'foo'
image.extra_properties['os_glance_failed_import'] = ''
web_download_task = web_download._WebDownload(
self.task.task_id, self.task_type, self.uri, self.action_wrapper)
self.task.task_id, self.task_type, self.uri, self.action_wrapper,
['foo'])
with mock.patch.object(script_utils,
'get_image_data_iter') as mock_iter:
mock_iter.return_value.headers = {'content-length': '4'}
@@ -196,6 +207,10 @@ class TestWebDownloadTask(test_utils.BaseTestCase):
# NOTE(danms): Since we told revert that we were not at fault,
# we should not have updated the image.
self.image_repo.save.assert_not_called()
self.assertEqual(
'foo', image.extra_properties['os_glance_importing_to_stores'])
self.assertEqual(
'', image.extra_properties['os_glance_failed_import'])
@mock.patch("glance.async_.flows._internal_plugins.web_download.store_api")
def test_web_download_revert_without_failure_multi_store(self,
@@ -206,7 +221,8 @@ class TestWebDownloadTask(test_utils.BaseTestCase):
}
self.config(enabled_backends=enabled_backends)
web_download_task = web_download._WebDownload(
self.task.task_id, self.task_type, self.uri, self.action_wrapper)
self.task.task_id, self.task_type, self.uri, self.action_wrapper,
['foo'])
web_download_task._path = "/path/to_downloaded_data"
web_download_task.revert("/path/to_downloaded_data")
mock_store_api.delete.assert_called_once_with(
@@ -217,24 +233,33 @@ class TestWebDownloadTask(test_utils.BaseTestCase):
mock_store_api):
image = self.image_repo.get.return_value
image.status = 'importing'
image.extra_properties['os_glance_importing_to_stores'] = 'foo'
image.extra_properties['os_glance_failed_import'] = ''
result = failure.Failure.from_exception(
glance.common.exception.ImportTaskError())
web_download_task = web_download._WebDownload(
self.task.task_id, self.task_type, self.uri, self.action_wrapper)
self.task.task_id, self.task_type, self.uri, self.action_wrapper,
['foo'])
web_download_task.revert(result)
mock_store_api.delete_from_backend.assert_not_called()
# NOTE(danms): Since we told revert that we were the problem,
# we should have updated the image status
# we should have updated the image status and moved the stores
# to the failed list.
self.image_repo.save.assert_called_once_with(image, 'importing')
self.assertEqual('queued', image.status)
self.assertEqual(
'', image.extra_properties['os_glance_importing_to_stores'])
self.assertEqual(
'foo', image.extra_properties['os_glance_failed_import'])
@mock.patch("glance.async_.flows._internal_plugins.web_download.store_api")
def test_web_download_revert_with_failure_with_path(self, mock_store_api):
result = failure.Failure.from_exception(
glance.common.exception.ImportTaskError())
web_download_task = web_download._WebDownload(
self.task.task_id, self.task_type, self.uri, self.action_wrapper)
self.task.task_id, self.task_type, self.uri, self.action_wrapper,
['foo'])
web_download_task._path = "/path/to_downloaded_data"
web_download_task.revert(result)
mock_store_api.delete_from_backend.assert_called_once_with(
@@ -246,7 +271,8 @@ class TestWebDownloadTask(test_utils.BaseTestCase):
glance.common.exception.ImportTaskError())
mock_store_api.delete_from_backend.side_effect = Exception
web_download_task = web_download._WebDownload(
self.task.task_id, self.task_type, self.uri, self.action_wrapper)
self.task.task_id, self.task_type, self.uri, self.action_wrapper,
['foo'])
web_download_task._path = "/path/to_downloaded_data"
# this will verify that revert does not break because of failure
# while deleting data in staging area