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 5c21a00e89)
(cherry picked from commit 07ccc82af0)
This commit is contained in:
Jay Pipes 2018-11-06 10:59:40 -05:00 committed by Matt Riedemann
parent 1b6f48354b
commit f1554d1b52
2 changed files with 116 additions and 70 deletions

View File

@ -160,21 +160,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
@ -238,7 +252,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)
@ -273,7 +287,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()
@ -318,7 +332,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)
@ -430,14 +445,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.
@ -457,8 +472,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
@ -498,7 +513,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.
@ -542,7 +557,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:
@ -565,7 +580,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:

View File

@ -408,13 +408,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')
@ -488,12 +488,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)
@ -512,6 +512,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
@ -530,8 +557,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')
@ -546,8 +573,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(
[
@ -573,8 +600,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)
@ -604,8 +631,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(
[
@ -718,8 +745,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()
@ -767,8 +794,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()
@ -1044,8 +1071,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)
@ -1063,8 +1090,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)
@ -1082,8 +1109,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)
@ -1118,8 +1145,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',
@ -1143,7 +1170,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)
@ -1165,7 +1193,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)
@ -1189,8 +1218,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)
@ -1214,7 +1243,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)
@ -1234,7 +1263,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)
@ -1248,10 +1277,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')
@ -1271,7 +1298,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()
@ -1301,8 +1328,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)
@ -1338,7 +1365,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'})
@ -1395,7 +1422,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.
@ -1486,11 +1513,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)
@ -1562,11 +1589,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)
@ -1580,8 +1609,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()
@ -1710,10 +1739,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):