openstack
/
glance
Code Issues Proposed changes

Make web-download revert all stores on fail 

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.

NOTE(lyarwood): This includes a partial squashed revert of the
.zuul.yaml changes in Ifd55c44fef4e2187052d77084dc9c0fa9c9a0d16 that
removed nova-ceph-multistore. This is reintroduced here to ensure this
change resolves the stable/victoria specific bug #1919993 and the
original bug #1914826.

Change-Id: Iebbb2dcb767ecf3c965f34f1ca04af20a2039be1
Closes-Bug: #1914826
Closes-Bug: #1919993
(cherry picked from commit cbb2af6e32)
This commit is contained in:
Dan Smith 2021-02-05 14:35:51 -08:00 committed by Lee Yarwood
parent 43d4115978
commit 9cb3240b94
3 changed files with 50 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
.zuul.yaml
View File

@ -312,6 +312,7 @@
irrelevant-files: *tempest-irrelevant-files irrelevant-files: *tempest-irrelevant-files
- tempest-ipv6-only: - tempest-ipv6-only:
irrelevant-files: *tempest-irrelevant-files irrelevant-files: *tempest-irrelevant-files
- nova-ceph-multistore
gate: gate:
jobs: jobs:
@ -325,6 +326,7 @@
irrelevant-files: *tempest-irrelevant-files irrelevant-files: *tempest-irrelevant-files
- tempest-ipv6-only: - tempest-ipv6-only:
irrelevant-files: *tempest-irrelevant-files irrelevant-files: *tempest-irrelevant-files
- nova-ceph-multistore
experimental: experimental:
jobs: jobs:
- glance-tox-py38-glance_store-tips - glance-tox-py38-glance_store-tips

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

@ -35,12 +35,13 @@ class _WebDownload(task.Task):
default_provides = 'file_uri' 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_id = task_id
self.task_type = task_type self.task_type = task_type
self.image_id = action_wrapper.image_id self.image_id = action_wrapper.image_id
self.uri = uri self.uri = uri
self.action_wrapper = action_wrapper self.action_wrapper = action_wrapper
self.stores = stores
self._path = None self._path = None
super(_WebDownload, self).__init__( super(_WebDownload, self).__init__(
name='%s-WebDownload-%s' % (task_type, task_id)) name='%s-WebDownload-%s' % (task_type, task_id))
@ -140,8 +141,13 @@ class _WebDownload(task.Task):
'image_id': self.image_id}) 'image_id': self.image_id})
# NOTE(abhishekk): Revert image state back to 'queued' as # NOTE(abhishekk): Revert image state back to 'queued' as
# something went wrong. # 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: with self.action_wrapper as action:
action.set_image_status('queued') 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 # NOTE(abhishekk): Deleting partial image data from staging area
if self._path is not None: if self._path is not None:
@ -171,7 +177,8 @@ def get_flow(**kwargs):
task_type = kwargs.get('task_type') task_type = kwargs.get('task_type')
uri = kwargs.get('import_req')['method'].get('uri') uri = kwargs.get('import_req')['method'].get('uri')
action_wrapper = kwargs.get('action_wrapper') action_wrapper = kwargs.get('action_wrapper')
stores = kwargs.get('backend', [None])
return lf.Flow(task_type).add( 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') @mock.patch.object(filesystem.Store, 'add')
def test_web_download(self, mock_add): def test_web_download(self, mock_add):
web_download_task = web_download._WebDownload( 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, with mock.patch.object(script_utils,
'get_image_data_iter') as mock_iter: 'get_image_data_iter') as mock_iter:
mock_add.return_value = ["path", 4] mock_add.return_value = ["path", 4]
@ -81,7 +82,8 @@ class TestWebDownloadTask(test_utils.BaseTestCase):
@mock.patch.object(filesystem.Store, 'add') @mock.patch.object(filesystem.Store, 'add')
def test_web_download_with_content_length(self, mock_add): def test_web_download_with_content_length(self, mock_add):
web_download_task = web_download._WebDownload( 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, with mock.patch.object(script_utils,
'get_image_data_iter') as mock_iter: 'get_image_data_iter') as mock_iter:
mock_iter.return_value.headers = {'content-length': '4'} mock_iter.return_value.headers = {'content-length': '4'}
@ -93,7 +95,8 @@ class TestWebDownloadTask(test_utils.BaseTestCase):
@mock.patch.object(filesystem.Store, 'add') @mock.patch.object(filesystem.Store, 'add')
def test_web_download_with_invalid_content_length(self, mock_add): def test_web_download_with_invalid_content_length(self, mock_add):
web_download_task = web_download._WebDownload( 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, with mock.patch.object(script_utils,
'get_image_data_iter') as mock_iter: 'get_image_data_iter') as mock_iter:
mock_iter.return_value.headers = {'content-length': "not_valid"} mock_iter.return_value.headers = {'content-length': "not_valid"}
@ -105,7 +108,8 @@ class TestWebDownloadTask(test_utils.BaseTestCase):
@mock.patch.object(filesystem.Store, 'add') @mock.patch.object(filesystem.Store, 'add')
def test_web_download_fails_when_data_size_different(self, mock_add): def test_web_download_fails_when_data_size_different(self, mock_add):
web_download_task = web_download._WebDownload( 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, with mock.patch.object(script_utils,
'get_image_data_iter') as mock_iter: 'get_image_data_iter') as mock_iter:
mock_iter.return_value.headers = {'content-length': '4'} mock_iter.return_value.headers = {'content-length': '4'}
@ -118,7 +122,8 @@ class TestWebDownloadTask(test_utils.BaseTestCase):
self.config(node_staging_uri=None) self.config(node_staging_uri=None)
self.assertRaises(glance.common.exception.BadTaskConfiguration, self.assertRaises(glance.common.exception.BadTaskConfiguration,
web_download._WebDownload, self.task.task_id, 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") @mock.patch.object(cfg.ConfigOpts, "set_override")
def test_web_download_node_store_initialization_failed(self, def test_web_download_node_store_initialization_failed(self,
@ -127,12 +132,14 @@ class TestWebDownloadTask(test_utils.BaseTestCase):
mock_load_store.return_value = None mock_load_store.return_value = None
self.assertRaises(glance.common.exception.BadTaskConfiguration, self.assertRaises(glance.common.exception.BadTaskConfiguration,
web_download._WebDownload, self.task.task_id, 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() mock_override.assert_called()
def test_web_download_failed(self): def test_web_download_failed(self):
web_download_task = web_download._WebDownload( 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, with mock.patch.object(script_utils,
"get_image_data_iter") as mock_iter: "get_image_data_iter") as mock_iter:
mock_iter.side_effect = glance.common.exception.NotFound 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") @mock.patch("glance.async_.flows._internal_plugins.web_download.store_api")
def test_web_download_revert_with_failure(self, mock_store_api, def test_web_download_revert_with_failure(self, mock_store_api,
mock_add): 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( 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, with mock.patch.object(script_utils,
'get_image_data_iter') as mock_iter: 'get_image_data_iter') as mock_iter:
mock_iter.return_value.headers = {'content-length': '4'} 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, # NOTE(danms): Since we told revert that we were not at fault,
# we should not have updated the image. # we should not have updated the image.
self.image_repo.save.assert_not_called() 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") @mock.patch("glance.async_.flows._internal_plugins.web_download.store_api")
def test_web_download_revert_without_failure_multi_store(self, 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) self.config(enabled_backends=enabled_backends)
web_download_task = web_download._WebDownload( 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._path = "/path/to_downloaded_data"
web_download_task.revert("/path/to_downloaded_data") web_download_task.revert("/path/to_downloaded_data")
mock_store_api.delete.assert_called_once_with( mock_store_api.delete.assert_called_once_with(
@ -217,24 +233,33 @@ class TestWebDownloadTask(test_utils.BaseTestCase):
mock_store_api): mock_store_api):
image = self.image_repo.get.return_value image = self.image_repo.get.return_value
image.status = 'importing' image.status = 'importing'
image.extra_properties['os_glance_importing_to_stores'] = 'foo'
image.extra_properties['os_glance_failed_import'] = ''
result = failure.Failure.from_exception( result = failure.Failure.from_exception(
glance.common.exception.ImportTaskError()) glance.common.exception.ImportTaskError())
web_download_task = web_download._WebDownload( 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) web_download_task.revert(result)
mock_store_api.delete_from_backend.assert_not_called() mock_store_api.delete_from_backend.assert_not_called()
# NOTE(danms): Since we told revert that we were the problem, # 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.image_repo.save.assert_called_once_with(image, 'importing')
self.assertEqual('queued', image.status) 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") @mock.patch("glance.async_.flows._internal_plugins.web_download.store_api")
def test_web_download_revert_with_failure_with_path(self, mock_store_api): def test_web_download_revert_with_failure_with_path(self, mock_store_api):
result = failure.Failure.from_exception( result = failure.Failure.from_exception(
glance.common.exception.ImportTaskError()) glance.common.exception.ImportTaskError())
web_download_task = web_download._WebDownload( 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._path = "/path/to_downloaded_data"
web_download_task.revert(result) web_download_task.revert(result)
mock_store_api.delete_from_backend.assert_called_once_with( mock_store_api.delete_from_backend.assert_called_once_with(
@ -246,7 +271,8 @@ class TestWebDownloadTask(test_utils.BaseTestCase):
glance.common.exception.ImportTaskError()) glance.common.exception.ImportTaskError())
mock_store_api.delete_from_backend.side_effect = Exception mock_store_api.delete_from_backend.side_effect = Exception
web_download_task = web_download._WebDownload( 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._path = "/path/to_downloaded_data"
# this will verify that revert does not break because of failure # this will verify that revert does not break because of failure
# while deleting data in staging area # while deleting data in staging area