From 07ccc82af048494cb7d4c4b0cc191cc30544a270 Mon Sep 17 00:00:00 2001 From: Jay Pipes Date: Tue, 6 Nov 2018 10:59:40 -0500 Subject: [PATCH] prevent common kwargs from glance client failure When creating a snapshot of a server using the nova API, failure occurs if the image contains the metadata property "version". This was due to the way that the GlanceClientWrapper.call() function signature was structured. This patch forces all client positional args to be passed as a named "args" argument to the call() function and all client named args to be pass as a named "kwargs" argument to the call() function. This eliminates any argument name-shadowing that previously caused issues. Closes-bug: #1717547 Change-Id: I3ed3303309fe2a25c0043fd206f36bada4b3b8f9 (cherry picked from commit 5c21a00e89539bbb271ccfa05e4a2ba1cddae58e) --- nova/image/glance.py | 45 ++++++--- nova/tests/unit/image/test_glance.py | 141 ++++++++++++++++----------- 2 files changed, 116 insertions(+), 70 deletions(-) diff --git a/nova/image/glance.py b/nova/image/glance.py index 8a387947990f..facc71c10899 100644 --- a/nova/image/glance.py +++ b/nova/image/glance.py @@ -161,21 +161,35 @@ class GlanceClientWrapper(object): self.api_server = next(self.api_servers) return _glanceclient_from_endpoint(context, self.api_server, version) - def call(self, context, version, method, *args, **kwargs): + def call(self, context, version, method, controller=None, args=None, + kwargs=None): """Call a glance client method. If we get a connection error, retry the request according to CONF.glance.num_retries. + + :param context: RequestContext to use + :param version: Numeric version of the *Glance API* to use + :param method: string method name to execute on the glanceclient + :param controller: optional string name of the client controller to + use. Default (None) is to use the 'images' + controller + :param args: optional iterable of arguments to pass to the + glanceclient method, splatted as positional args + :param kwargs: optional dict of arguments to pass to the glanceclient, + splatted into named arguments """ + args = args or [] + kwargs = kwargs or {} retry_excs = (glanceclient.exc.ServiceUnavailable, glanceclient.exc.InvalidEndpoint, glanceclient.exc.CommunicationError) num_attempts = 1 + CONF.glance.num_retries + controller_name = controller or 'images' for attempt in range(1, num_attempts + 1): client = self.client or self._create_onetime_client(context, version) try: - controller = getattr(client, - kwargs.pop('controller', 'images')) + controller = getattr(client, controller_name) result = getattr(controller, method)(*args, **kwargs) if inspect.isgenerator(result): # Convert generator results to a list, so that we can @@ -239,7 +253,7 @@ class GlanceImageServiceV2(object): image is deleted. """ try: - image = self._client.call(context, 2, 'get', image_id) + image = self._client.call(context, 2, 'get', args=(image_id,)) except Exception: _reraise_translated_image_exception(image_id) @@ -274,7 +288,7 @@ class GlanceImageServiceV2(object): """Calls out to Glance for a list of detailed image information.""" params = _extract_query_params_v2(kwargs) try: - images = self._client.call(context, 2, 'list', **params) + images = self._client.call(context, 2, 'list', kwargs=params) except Exception: _reraise_translated_exception() @@ -320,7 +334,8 @@ class GlanceImageServiceV2(object): LOG.exception("Download image error") try: - image_chunks = self._client.call(context, 2, 'data', image_id) + image_chunks = self._client.call( + context, 2, 'data', args=(image_id,)) except Exception: _reraise_translated_image_exception(image_id) @@ -466,14 +481,14 @@ class GlanceImageServiceV2(object): def _add_location(self, context, image_id, location): # 'show_multiple_locations' must be enabled in glance api conf file. try: - return self._client.call(context, 2, 'add_location', image_id, - location, {}) + return self._client.call( + context, 2, 'add_location', args=(image_id, location)) except glanceclient.exc.HTTPBadRequest: _reraise_translated_exception() def _upload_data(self, context, image_id, data): - self._client.call(context, 2, 'upload', image_id, data) - return self._client.call(context, 2, 'get', image_id) + self._client.call(context, 2, 'upload', args=(image_id, data)) + return self._client.call(context, 2, 'get', args=(image_id,)) def _get_image_create_disk_format_default(self, context): """Gets an acceptable default image disk_format based on the schema. @@ -493,8 +508,8 @@ class GlanceImageServiceV2(object): # Get the image schema - note we don't cache this value since it could # change under us. This looks a bit funky, but what it's basically # doing is calling glanceclient.v2.Client.schemas.get('image'). - image_schema = self._client.call(context, 2, 'get', 'image', - controller='schemas') + image_schema = self._client.call( + context, 2, 'get', args=('image',), controller='schemas') # get the disk_format schema property from the raw schema disk_format_schema = ( image_schema.raw()['properties'].get('disk_format') if image_schema @@ -534,7 +549,7 @@ class GlanceImageServiceV2(object): location = sent_service_image_meta.pop('location', None) image = self._client.call( - context, 2, 'create', **sent_service_image_meta) + context, 2, 'create', kwargs=sent_service_image_meta) image_id = image['id'] # Sending image location in a separate request. @@ -578,7 +593,7 @@ class GlanceImageServiceV2(object): location = sent_service_image_meta.pop('location', None) image_id = sent_service_image_meta['image_id'] image = self._client.call( - context, 2, 'update', **sent_service_image_meta) + context, 2, 'update', kwargs=sent_service_image_meta) # Sending image location in a separate request. if location: @@ -601,7 +616,7 @@ class GlanceImageServiceV2(object): """ try: - self._client.call(context, 2, 'delete', image_id) + self._client.call(context, 2, 'delete', args=(image_id,)) except glanceclient.exc.NotFound: raise exception.ImageNotFound(image_id=image_id) except glanceclient.exc.HTTPForbidden: diff --git a/nova/tests/unit/image/test_glance.py b/nova/tests/unit/image/test_glance.py index 5e718874097e..e0b80b421c6b 100644 --- a/nova/tests/unit/image/test_glance.py +++ b/nova/tests/unit/image/test_glance.py @@ -409,13 +409,13 @@ class TestGlanceClientWrapperRetries(test.NoDBTestCase): self.flags(api_servers=api_servers, group='glance') def assert_retry_attempted(self, sleep_mock, client, expected_url): - client.call(self.ctx, 1, 'get', 'meow') + client.call(self.ctx, 1, 'get', args=('meow',)) sleep_mock.assert_called_once_with(1) self.assertEqual(str(client.api_server), expected_url) def assert_retry_not_attempted(self, sleep_mock, client): self.assertRaises(exception.GlanceConnectionFailed, - client.call, self.ctx, 1, 'get', 'meow') + client.call, self.ctx, 1, 'get', args=('meow',)) self.assertFalse(sleep_mock.called) @mock.patch('time.sleep') @@ -489,12 +489,12 @@ class TestGlanceClientWrapperRetries(test.NoDBTestCase): # sleep (which would be an indication of a retry) self.assertRaises(exception.GlanceConnectionFailed, - client.call, self.ctx, 1, 'get', 'meow') + client.call, self.ctx, 1, 'get', args=('meow',)) self.assertEqual(str(client.api_server), 'http://host1:9292') self.assertFalse(sleep_mock.called) self.assertRaises(exception.GlanceConnectionFailed, - client.call, self.ctx, 1, 'get', 'meow') + client.call, self.ctx, 1, 'get', args=('meow',)) self.assertEqual(str(client.api_server), 'https://host2:9293') self.assertFalse(sleep_mock.called) @@ -513,6 +513,33 @@ class TestGlanceClientWrapperRetries(test.NoDBTestCase): create_client_mock.return_value = client_mock +class TestCommonPropertyNameConflicts(test.NoDBTestCase): + + """Tests that images that have common property names like "version" don't + cause an exception to be raised from the wacky GlanceClientWrapper magic + call() method. + + :see https://bugs.launchpad.net/nova/+bug/1717547 + """ + + @mock.patch('nova.image.glance.GlanceClientWrapper._create_onetime_client') + def test_version_property_conflicts(self, mock_glance_client): + client = mock.MagicMock() + mock_glance_client.return_value = client + ctx = mock.sentinel.ctx + service = glance.GlanceImageServiceV2() + + # Simulate the process of snapshotting a server that was launched with + # an image with the properties collection containing a (very + # commonly-named) "version" property. + image_meta = { + 'id': 1, + 'version': 'blows up', + } + # This call would blow up before the fix for 1717547 + service.create(ctx, image_meta) + + class TestDownloadNoDirectUri(test.NoDBTestCase): """Tests the download method of the GlanceImageServiceV2 when the @@ -531,8 +558,8 @@ class TestDownloadNoDirectUri(test.NoDBTestCase): self.assertFalse(show_mock.called) self.assertFalse(open_mock.called) - client.call.assert_called_once_with(ctx, 2, 'data', - mock.sentinel.image_id) + client.call.assert_called_once_with( + ctx, 2, 'data', args=(mock.sentinel.image_id,)) self.assertEqual(mock.sentinel.image_chunks, res) @mock.patch.object(six.moves.builtins, 'open') @@ -547,8 +574,8 @@ class TestDownloadNoDirectUri(test.NoDBTestCase): self.assertFalse(show_mock.called) self.assertFalse(open_mock.called) - client.call.assert_called_once_with(ctx, 2, 'data', - mock.sentinel.image_id) + client.call.assert_called_once_with( + ctx, 2, 'data', args=(mock.sentinel.image_id,)) self.assertIsNone(res) data.write.assert_has_calls( [ @@ -574,8 +601,8 @@ class TestDownloadNoDirectUri(test.NoDBTestCase): dst_path=mock.sentinel.dst_path) self.assertFalse(show_mock.called) - client.call.assert_called_once_with(ctx, 2, 'data', - mock.sentinel.image_id) + client.call.assert_called_once_with( + ctx, 2, 'data', args=(mock.sentinel.image_id,)) open_mock.assert_called_once_with(mock.sentinel.dst_path, 'wb') fsync_mock.assert_called_once_with(writer) self.assertIsNone(res) @@ -605,8 +632,8 @@ class TestDownloadNoDirectUri(test.NoDBTestCase): self.assertFalse(show_mock.called) self.assertFalse(open_mock.called) - client.call.assert_called_once_with(ctx, 2, 'data', - mock.sentinel.image_id) + client.call.assert_called_once_with( + ctx, 2, 'data', args=(mock.sentinel.image_id,)) self.assertIsNone(res) data.write.assert_has_calls( [ @@ -720,8 +747,8 @@ class TestDownloadNoDirectUri(test.NoDBTestCase): tran_mod.download.assert_called_once_with(ctx, mock.ANY, mock.sentinel.dst_path, mock.sentinel.loc_meta) - client.call.assert_called_once_with(ctx, 2, 'data', - mock.sentinel.image_id) + client.call.assert_called_once_with( + ctx, 2, 'data', args=(mock.sentinel.image_id,)) fsync_mock.assert_called_once_with(writer) # NOTE(jaypipes): log messages call open() in part of the # download path, so here, we just check that the last open() @@ -770,8 +797,8 @@ class TestDownloadNoDirectUri(test.NoDBTestCase): mock.sentinel.image_id, include_locations=True) get_tran_mock.assert_called_once_with('file') - client.call.assert_called_once_with(ctx, 2, 'data', - mock.sentinel.image_id) + client.call.assert_called_once_with( + ctx, 2, 'data', args=(mock.sentinel.image_id,)) fsync_mock.assert_called_once_with(writer) # NOTE(jaypipes): log messages call open() in part of the # download path, so here, we just check that the last open() @@ -1228,8 +1255,8 @@ class TestShow(test.NoDBTestCase): service = glance.GlanceImageServiceV2(client) info = service.show(ctx, mock.sentinel.image_id) - client.call.assert_called_once_with(ctx, 2, 'get', - mock.sentinel.image_id) + client.call.assert_called_once_with( + ctx, 2, 'get', args=(mock.sentinel.image_id,)) is_avail_mock.assert_called_once_with(ctx, {}) trans_from_mock.assert_called_once_with({}, include_locations=False) self.assertIn('mock', info) @@ -1247,8 +1274,8 @@ class TestShow(test.NoDBTestCase): with testtools.ExpectedException(exception.ImageNotFound): service.show(ctx, mock.sentinel.image_id) - client.call.assert_called_once_with(ctx, 2, 'get', - mock.sentinel.image_id) + client.call.assert_called_once_with( + ctx, 2, 'get', args=(mock.sentinel.image_id,)) is_avail_mock.assert_called_once_with(ctx, mock.sentinel.images_0) self.assertFalse(trans_from_mock.called) @@ -1266,8 +1293,8 @@ class TestShow(test.NoDBTestCase): with testtools.ExpectedException(exception.ImageNotAuthorized): service.show(ctx, mock.sentinel.image_id) - client.call.assert_called_once_with(ctx, 2, 'get', - mock.sentinel.image_id) + client.call.assert_called_once_with( + ctx, 2, 'get', args=(mock.sentinel.image_id,)) self.assertFalse(is_avail_mock.called) self.assertFalse(trans_from_mock.called) reraise_mock.assert_called_once_with(mock.sentinel.image_id) @@ -1302,8 +1329,8 @@ class TestShow(test.NoDBTestCase): ctx = mock.sentinel.ctx service = glance.GlanceImageServiceV2(client) image_info = service.show(ctx, glance_image.id) - client.call.assert_called_once_with(ctx, 2, 'get', - glance_image.id) + client.call.assert_called_once_with( + ctx, 2, 'get', args=(glance_image.id,)) NOVA_IMAGE_ATTRIBUTES = set(['size', 'disk_format', 'owner', 'container_format', 'status', 'id', 'name', 'created_at', 'updated_at', @@ -1327,7 +1354,8 @@ class TestShow(test.NoDBTestCase): image_id = mock.sentinel.image_id info = service.show(ctx, image_id, include_locations=True) - client.call.assert_called_once_with(ctx, 2, 'get', image_id) + client.call.assert_called_once_with( + ctx, 2, 'get', args=(image_id,)) avail_mock.assert_called_once_with(ctx, mock.sentinel.image) trans_from_mock.assert_called_once_with(mock.sentinel.image, include_locations=True) @@ -1349,7 +1377,8 @@ class TestShow(test.NoDBTestCase): image_id = mock.sentinel.image_id info = service.show(ctx, image_id, include_locations=True) - client.call.assert_called_once_with(ctx, 2, 'get', image_id) + client.call.assert_called_once_with( + ctx, 2, 'get', args=(image_id,)) expected = locations expected.append({'url': mock.sentinel.duri, 'metadata': {}}) self.assertIn('locations', info) @@ -1373,8 +1402,8 @@ class TestShow(test.NoDBTestCase): with testtools.ExpectedException(exception.ImageNotFound): service.show(ctx, glance_image.id, show_deleted=False) - client.call.assert_called_once_with(ctx, 2, 'get', - glance_image.id) + client.call.assert_called_once_with( + ctx, 2, 'get', args=(glance_image.id,)) self.assertFalse(is_avail_mock.called) self.assertFalse(trans_from_mock.called) @@ -1398,7 +1427,7 @@ class TestDetail(test.NoDBTestCase): service = glance.GlanceImageServiceV2(client) images = service.detail(ctx, **params) - client.call.assert_called_once_with(ctx, 2, 'list') + client.call.assert_called_once_with(ctx, 2, 'list', kwargs={}) is_avail_mock.assert_called_once_with(ctx, mock.sentinel.images_0) trans_from_mock.assert_called_once_with(mock.sentinel.images_0) self.assertEqual([mock.sentinel.trans_from], images) @@ -1418,7 +1447,7 @@ class TestDetail(test.NoDBTestCase): service = glance.GlanceImageServiceV2(client) images = service.detail(ctx, **params) - client.call.assert_called_once_with(ctx, 2, 'list') + client.call.assert_called_once_with(ctx, 2, 'list', kwargs={}) is_avail_mock.assert_called_once_with(ctx, mock.sentinel.images_0) self.assertFalse(trans_from_mock.called) self.assertEqual([], images) @@ -1432,10 +1461,8 @@ class TestDetail(test.NoDBTestCase): service = glance.GlanceImageServiceV2(client) service.detail(ctx, page_size=5, limit=10) - client.call.assert_called_once_with(ctx, 2, 'list', - filters={}, - page_size=5, - limit=10) + client.call.assert_called_once_with( + ctx, 2, 'list', kwargs=dict(filters={}, page_size=5, limit=10)) @mock.patch('nova.image.glance._reraise_translated_exception') @mock.patch('nova.image.glance._extract_query_params_v2') @@ -1455,7 +1482,7 @@ class TestDetail(test.NoDBTestCase): with testtools.ExpectedException(exception.Forbidden): service.detail(ctx, **params) - client.call.assert_called_once_with(ctx, 2, 'list') + client.call.assert_called_once_with(ctx, 2, 'list', kwargs={}) self.assertFalse(is_avail_mock.called) self.assertFalse(trans_from_mock.called) reraise_mock.assert_called_once_with() @@ -1485,8 +1512,8 @@ class TestCreate(test.NoDBTestCase): # the call to glanceclient's update (since the image ID is # supplied as a positional arg), and that the # purge_props default is True. - client.call.assert_called_once_with(ctx, 2, 'create', - name=mock.sentinel.name) + client.call.assert_called_once_with( + ctx, 2, 'create', kwargs=dict(name=mock.sentinel.name)) trans_from_mock.assert_called_once_with({'id': '123'}) self.assertEqual(mock.sentinel.trans_from, image_meta) @@ -1522,7 +1549,7 @@ class TestCreate(test.NoDBTestCase): image_meta = service.create(ctx, image_mock) trans_to_mock.assert_called_once_with(image_mock) # Verify that the disk_format and container_format kwargs are passed. - create_call_kwargs = client.call.call_args_list[0][1] + create_call_kwargs = client.call.call_args_list[0][1]['kwargs'] self.assertEqual('vdi', create_call_kwargs['disk_format']) self.assertEqual('bare', create_call_kwargs['container_format']) trans_from_mock.assert_called_once_with({'id': '123'}) @@ -1579,7 +1606,7 @@ class TestCreate(test.NoDBTestCase): mock.sentinel.ctx) self.assertEqual(expected_disk_format, disk_format) mock_client.call.assert_called_once_with( - mock.sentinel.ctx, 2, 'get', 'image', controller='schemas') + mock.sentinel.ctx, 2, 'get', args=('image',), controller='schemas') def test_get_image_create_disk_format_default_no_schema(self): """Tests that if there is no disk_format schema we default to qcow2. @@ -1670,11 +1697,11 @@ class TestUpdate(test.NoDBTestCase): # the call to glanceclient's update (since the image ID is # supplied as a positional arg), and that the # purge_props default is True. - client.call.assert_called_once_with(ctx, 2, 'update', - image_id=mock.sentinel.image_id, - name=mock.sentinel.name, - prop_to_keep='4', - remove_props=['prop_to_remove']) + client.call.assert_called_once_with( + ctx, 2, 'update', kwargs=dict( + image_id=mock.sentinel.image_id, name=mock.sentinel.name, + prop_to_keep='4', remove_props=['prop_to_remove'], + )) trans_from_mock.assert_called_once_with(mock.sentinel.image_meta) self.assertEqual(mock.sentinel.trans_from, image_meta) @@ -1746,11 +1773,13 @@ class TestUpdate(test.NoDBTestCase): self.assertRaises(exception.ImageNotAuthorized, service.update, ctx, mock.sentinel.image_id, image) - client.call.assert_called_once_with(ctx, 2, 'update', - image_id=mock.sentinel.image_id, - name=mock.sentinel.name, - prop_to_keep='4', - remove_props=['prop_to_remove']) + client.call.assert_called_once_with( + ctx, 2, 'update', kwargs=dict( + image_id=mock.sentinel.image_id, + name=mock.sentinel.name, + prop_to_keep='4', + remove_props=['prop_to_remove'], + )) reraise_mock.assert_called_once_with(mock.sentinel.image_id) @@ -1764,8 +1793,8 @@ class TestDelete(test.NoDBTestCase): ctx = mock.sentinel.ctx service = glance.GlanceImageServiceV2(client) service.delete(ctx, mock.sentinel.image_id) - client.call.assert_called_once_with(ctx, 2, 'delete', - mock.sentinel.image_id) + client.call.assert_called_once_with( + ctx, 2, 'delete', args=(mock.sentinel.image_id,)) def test_delete_client_failure_v2(self): client = mock.MagicMock() @@ -1894,10 +1923,12 @@ class TestExtractQueryParams(test.NoDBTestCase): 'kernel-id': 'some-id', 'updated_at': 'gte:some-date'} - client.call.assert_called_once_with(ctx, 2, 'list', - filters=expected_filters_v1, - page_size=5, - limit=10) + client.call.assert_called_once_with( + ctx, 2, 'list', kwargs=dict( + filters=expected_filters_v1, + page_size=5, + limit=10, + )) class TestTranslateToGlance(test.NoDBTestCase):