openstack
/
cinder
Code Issues Proposed changes

Remove second get call to list/show volumes 

Recently it was observed that with large numbers of volumes
that things like "cinder list" could take extremely long to
return.

For example, running cinder list on a system with 1000 volumes took
greater than 30 seconds to return.  It turns out that the cause of
this is the addition of visible admin_metadata.

There's two problems with this:
1. The original patch probably shouldn't have gone in
   data is either admin data or it's not, selectively picking
   pieces of admin data out to provide to the user just creates
   complications and introduces confusion.
2. The REAL issue here is that since the standar gets are made
   with the standard user context, the add_visible_admin_metadata
   would go through and do an elevated context get on every single
   volume individually.  This is what caused the horrible performance
   issue on cinder list with large numbers of volumes.

Running as admin, or removing the second call drops this down to about 3
seconds for the same 1000 volume list.

This patch removes the secondary admin context get_call.  Instead where
we expect to do display the visible admin_meta, we pass in a flag
requesting that the volume object we're getting has the appropriate
metadata.  This way we can elevate the context if needed and avoid
iterating through the gets again.

This patch also cleans up the get_visible_admin_meta methods, and
consolidates both V1 and V2 to use the utils method.

Change-Id: I3fb7aefb7d8a5664b0a3fb3958f509b5cd621320
Closes-Bug: 1317606
(cherry picked from commit 0505bb2689)
This commit is contained in:
John Griffith 2014-05-29 18:18:12 +00:00 committed by John Griffith
parent 1c494a15ac
commit fe37a6ee1d
8 changed files with 80 additions and 132 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
cinder/api/contrib/volume_manage.py
View File

@ -144,7 +144,7 @@ class VolumeManageController(wsgi.Controller):
raise exc.HTTPNotFound(explanation=msg)
new_volume = dict(new_volume.iteritems())
utils.add_visible_admin_metadata(context, new_volume, self.volume_api)
utils.add_visible_admin_metadata(new_volume)
return self._view_builder.detail(req, new_volume)

63
cinder/api/v1/volumes.py
View File

@ -218,71 +218,23 @@ class CreateDeserializer(CommonDeserializer):
class VolumeController(wsgi.Controller):
"""The Volumes API controller for the OpenStack API."""
_visible_admin_metadata_keys = ['readonly', 'attached_mode']
def __init__(self, ext_mgr):
self.volume_api = cinder_volume.API()
self.ext_mgr = ext_mgr
super(VolumeController, self).__init__()
def _add_visible_admin_metadata(self, context, volume):
if context is None:
return
visible_admin_meta = {}
if context.is_admin:
volume_tmp = volume
else:
try:
volume_tmp = self.volume_api.get(context.elevated(),
volume['id'])
except Exception:
return
if volume_tmp.get('volume_admin_metadata'):
for item in volume_tmp['volume_admin_metadata']:
if item['key'] in self._visible_admin_metadata_keys:
visible_admin_meta[item['key']] = item['value']
# avoid circular ref when volume is a Volume instance
elif (volume_tmp.get('admin_metadata') and
isinstance(volume_tmp.get('admin_metadata'), dict)):
for key in self._visible_admin_metadata_keys:
if key in volume_tmp['admin_metadata'].keys():
visible_admin_meta[key] = volume_tmp['admin_metadata'][key]
if not visible_admin_meta:
return
# NOTE(zhiyan): update visible administration metadata to
# volume metadata, administration metadata will rewrite existing key.
if volume.get('volume_metadata'):
orig_meta = list(volume.get('volume_metadata'))
for item in orig_meta:
if item['key'] in visible_admin_meta.keys():
item['value'] = visible_admin_meta.pop(item['key'])
for key, value in visible_admin_meta.iteritems():
orig_meta.append({'key': key, 'value': value})
volume['volume_metadata'] = orig_meta
# avoid circular ref when vol is a Volume instance
elif (volume.get('metadata') and
isinstance(volume.get('metadata'), dict)):
volume['metadata'].update(visible_admin_meta)
else:
volume['metadata'] = visible_admin_meta
@wsgi.serializers(xml=VolumeTemplate)
def show(self, req, id):
"""Return data about the given volume."""
context = req.environ['cinder.context']
try:
vol = self.volume_api.get(context, id)
vol = self.volume_api.get(context, id, viewable_admin_meta=True)
req.cache_resource(vol)
except exception.NotFound:
raise exc.HTTPNotFound()
self._add_visible_admin_metadata(context, vol)
utils.add_visible_admin_metadata(vol)
return {'volume': _translate_volume_detail_view(context, vol)}
@ -326,12 +278,13 @@ class VolumeController(wsgi.Controller):
volumes = self.volume_api.get_all(context, marker=None, limit=None,
sort_key='created_at',
sort_dir='desc', filters=search_opts)
sort_dir='desc', filters=search_opts,
viewable_admin_meta=True)
volumes = [dict(vol.iteritems()) for vol in volumes]
for volume in volumes:
self._add_visible_admin_metadata(context, volume)
utils.add_visible_admin_metadata(volume)
limited_list = common.limited(volumes, req)
req.cache_resource(limited_list)
@ -436,8 +389,6 @@ class VolumeController(wsgi.Controller):
# a dict to avoid an error.
new_volume = dict(new_volume.iteritems())
self._add_visible_admin_metadata(context, new_volume)
retval = _translate_volume_detail_view(context, new_volume, image_uuid)
return {'volume': retval}
@ -471,7 +422,7 @@ class VolumeController(wsgi.Controller):
update_dict[key] = volume[key]
try:
volume = self.volume_api.get(context, id)
volume = self.volume_api.get(context, id, viewable_admin_meta=True)
volume_utils.notify_about_volume_usage(context, volume,
'update.start')
self.volume_api.update(context, volume, update_dict)
@ -480,7 +431,7 @@ class VolumeController(wsgi.Controller):
volume.update(update_dict)
self._add_visible_admin_metadata(context, volume)
utils.add_visible_admin_metadata(volume)
volume_utils.notify_about_volume_usage(context, volume,
'update.end')

16
cinder/api/v2/volumes.py
View File

@ -164,13 +164,13 @@ class VolumeController(wsgi.Controller):
context = req.environ['cinder.context']
try:
vol = self.volume_api.get(context, id)
vol = self.volume_api.get(context, id, viewable_admin_meta=True)
req.cache_resource(vol)
except exception.NotFound:
msg = _("Volume could not be found")
raise exc.HTTPNotFound(explanation=msg)
utils.add_visible_admin_metadata(context, vol, self.volume_api)
utils.add_visible_admin_metadata(vol)
return self._view_builder.detail(req, vol)
@ -226,12 +226,13 @@ class VolumeController(wsgi.Controller):
filters['metadata'] = ast.literal_eval(filters['metadata'])
volumes = self.volume_api.get_all(context, marker, limit, sort_key,
sort_dir, filters)
sort_dir, filters,
viewable_admin_meta=True)
volumes = [dict(vol.iteritems()) for vol in volumes]
for volume in volumes:
utils.add_visible_admin_metadata(context, volume, self.volume_api)
utils.add_visible_admin_metadata(volume)
limited_list = common.limited(volumes, req)
@ -349,9 +350,6 @@ class VolumeController(wsgi.Controller):
# trying to lazy load, but for now we turn it into
# a dict to avoid an error.
new_volume = dict(new_volume.iteritems())
utils.add_visible_admin_metadata(context, new_volume, self.volume_api)
retval = self._view_builder.detail(req, new_volume)
return retval
@ -399,7 +397,7 @@ class VolumeController(wsgi.Controller):
del update_dict['description']
try:
volume = self.volume_api.get(context, id)
volume = self.volume_api.get(context, id, viewable_admin_meta=True)
volume_utils.notify_about_volume_usage(context, volume,
'update.start')
self.volume_api.update(context, volume, update_dict)
@ -409,7 +407,7 @@ class VolumeController(wsgi.Controller):
volume.update(update_dict)
utils.add_visible_admin_metadata(context, volume, self.volume_api)
utils.add_visible_admin_metadata(volume)
volume_utils.notify_about_volume_usage(context, volume,
'update.end')

26
cinder/tests/api/v1/test_volumes.py
View File

@ -29,6 +29,7 @@ from cinder.tests.api import fakes
from cinder.tests.api.v2 import stubs
from cinder.tests import fake_notifier
from cinder.tests.image import fake as fake_image
from cinder import utils
from cinder.volume import api as volume_api
@ -97,8 +98,7 @@ class VolumeApiTest(test.TestCase):
'volume_type': 'vol_type_name',
'snapshot_id': None,
'source_volid': None,
'metadata': {'attached_mode': 'rw',
'readonly': 'False'},
'metadata': {},
'id': '1',
'created_at': datetime.datetime(1, 1, 1,
1, 1, 1),
@ -190,8 +190,7 @@ class VolumeApiTest(test.TestCase):
'image_id': test_id,
'snapshot_id': None,
'source_volid': None,
'metadata': {'attached_mode': 'rw',
'readonly': 'False'},
'metadata': {},
'id': '1',
'created_at': datetime.datetime(1, 1, 1,
1, 1, 1),
@ -546,7 +545,7 @@ class VolumeApiTest(test.TestCase):
self.assertIsNotNone(req.cached_resource_by_id('1'))
def test_volume_show_no_attachments(self):
def stub_volume_get(self, context, volume_id):
def stub_volume_get(self, context, volume_id, **kwargs):
return stubs.stub_volume(volume_id, attach_status='detached')
self.stubs.Set(volume_api.API, 'get', stub_volume_get)
@ -571,7 +570,7 @@ class VolumeApiTest(test.TestCase):
self.assertEqual(res_dict, expected)
def test_volume_show_bootable(self):
def stub_volume_get(self, context, volume_id):
def stub_volume_get(self, context, volume_id, **kwargs):
return (stubs.stub_volume(volume_id,
volume_glance_metadata=dict(foo='bar')))
@ -616,7 +615,8 @@ class VolumeApiTest(test.TestCase):
def volume_detail_limit_offset(is_admin):
def stub_volume_get_all_by_project(context, project_id, marker,
limit, sort_key, sort_dir,
filters=None):
filters=None,
viewable_admin_meta=False):
return [
stubs.stub_volume(1, display_name='vol1'),
stubs.stub_volume(2, display_name='vol2'),
@ -678,7 +678,7 @@ class VolumeApiTest(test.TestCase):
self.assertEqual(res_dict, expected)
def test_volume_show_with_encrypted_volume(self):
def stub_volume_get(self, context, volume_id):
def stub_volume_get(self, context, volume_id, **kwargs):
return stubs.stub_volume(volume_id, encryption_key_id='fake_id')
self.stubs.Set(volume_api.API, 'get', stub_volume_get)
@ -688,7 +688,7 @@ class VolumeApiTest(test.TestCase):
self.assertEqual(res_dict['volume']['encrypted'], True)
def test_volume_show_with_unencrypted_volume(self):
def stub_volume_get(self, context, volume_id):
def stub_volume_get(self, context, volume_id, **kwargs):
return stubs.stub_volume(volume_id, encryption_key_id=None)
self.stubs.Set(volume_api.API, 'get', stub_volume_get)
@ -758,9 +758,7 @@ class VolumeApiTest(test.TestCase):
metadata = [{"key": "key", "value": "value"}]
volume = dict(volume_admin_metadata=admin_metadata,
volume_metadata=metadata)
admin_ctx = context.get_admin_context()
self.controller._add_visible_admin_metadata(admin_ctx,
volume)
utils.add_visible_admin_metadata(volume)
self.assertEqual(volume['volume_metadata'],
[{"key": "key", "value": "value"},
{"key": "readonly", "value": "visible"},
@ -772,9 +770,7 @@ class VolumeApiTest(test.TestCase):
metadata = {"key": "value"}
volume = dict(admin_metadata=admin_metadata,
metadata=metadata)
admin_ctx = context.get_admin_context()
self.controller._add_visible_admin_metadata(admin_ctx,
volume)
utils.add_visible_admin_metadata(volume)
self.assertEqual(volume['metadata'],
{'key': 'value',
'attached_mode': 'visible',

11
cinder/tests/api/v2/stubs.py
View File

@ -96,11 +96,12 @@ def stub_volume_delete(self, context, *args, **param):
pass
def stub_volume_get(self, context, volume_id):
def stub_volume_get(self, context, volume_id, viewable_admin_meta=False):
return stub_volume(volume_id)
def stub_volume_get_notfound(self, context, volume_id):
def stub_volume_get_notfound(self, context,
volume_id, viewable_admin_meta=False):
raise exc.NotFound
@ -109,14 +110,16 @@ def stub_volume_get_db(context, volume_id):
def stub_volume_get_all(context, search_opts=None, marker=None, limit=None,
sort_key='created_at', sort_dir='desc', filters=None):
sort_key='created_at', sort_dir='desc', filters=None,
viewable_admin_meta=False):
return [stub_volume(100, project_id='fake'),
stub_volume(101, project_id='superfake'),
stub_volume(102, project_id='superduperfake')]
def stub_volume_get_all_by_project(self, context, marker, limit, sort_key,
sort_dir, filters={}):
sort_dir, filters={},
viewable_admin_meta=False):
return [stub_volume_get(self, context, '1')]

61
cinder/tests/api/v2/test_volumes.py
View File

@ -105,8 +105,7 @@ class VolumeApiTest(test.TestCase):
'rel': 'self'},
{'href': 'http://localhost/fake/volumes/1',
'rel': 'bookmark'}],
'metadata': {'attached_mode': 'rw',
'readonly': 'False'},
'metadata': {},
'name': 'Volume Test Name',
'size': 100,
'snapshot_id': None,
@ -209,8 +208,7 @@ class VolumeApiTest(test.TestCase):
'rel': 'self'},
{'href': 'http://localhost/fake/volumes/1',
'rel': 'bookmark'}],
'metadata': {'attached_mode': 'rw',
'readonly': 'False'},
'metadata': {},
'name': 'Volume Test Name',
'size': '1',
'snapshot_id': None,
@ -690,7 +688,8 @@ class VolumeApiTest(test.TestCase):
def test_volume_index_with_marker(self):
def stub_volume_get_all_by_project(context, project_id, marker, limit,
sort_key, sort_dir, filters=None):
sort_key, sort_dir, filters=None,
viewable_admin_meta=False):
return [
stubs.stub_volume(1, display_name='vol1'),
stubs.stub_volume(2, display_name='vol2'),
@ -741,7 +740,8 @@ class VolumeApiTest(test.TestCase):
def test_volume_index_limit_offset(self):
def stub_volume_get_all_by_project(context, project_id, marker, limit,
sort_key, sort_dir, filters=None):
sort_key, sort_dir, filters=None,
viewable_admin_meta=False):
return [
stubs.stub_volume(1, display_name='vol1'),
stubs.stub_volume(2, display_name='vol2'),
@ -768,7 +768,8 @@ class VolumeApiTest(test.TestCase):
def test_volume_detail_with_marker(self):
def stub_volume_get_all_by_project(context, project_id, marker, limit,
sort_key, sort_dir, filters=None):
sort_key, sort_dir, filters=None,
viewable_admin_meta=False):
return [
stubs.stub_volume(1, display_name='vol1'),
stubs.stub_volume(2, display_name='vol2'),
@ -819,7 +820,8 @@ class VolumeApiTest(test.TestCase):
def test_volume_detail_limit_offset(self):
def stub_volume_get_all_by_project(context, project_id, marker, limit,
sort_key, sort_dir, filters=None):
sort_key, sort_dir, filters=None,
viewable_admin_meta=False):
return [
stubs.stub_volume(1, display_name='vol1'),
stubs.stub_volume(2, display_name='vol2'),
@ -853,7 +855,7 @@ class VolumeApiTest(test.TestCase):
def test_volume_with_limit_zero(self):
def stub_volume_get_all(context, marker, limit,
sort_key, sort_dir):
sort_key, sort_dir, **kwargs):
return []
self.stubs.Set(db, 'volume_get_all', stub_volume_get_all)
req = fakes.HTTPRequest.blank('/v2/volumes?limit=0')
@ -867,7 +869,8 @@ class VolumeApiTest(test.TestCase):
# Number of volumes equals the max, include next link
def stub_volume_get_all(context, marker, limit,
sort_key, sort_dir,
filters=None):
filters=None,
viewable_admin_meta=False):
vols = [stubs.stub_volume(i)
for i in xrange(CONF.osapi_max_limit)]
if limit == None or limit >= len(vols):
@ -885,7 +888,8 @@ class VolumeApiTest(test.TestCase):
# Number of volumes less then max, do not include
def stub_volume_get_all2(context, marker, limit,
sort_key, sort_dir,
filters=None):
filters=None,
viewable_admin_meta=False):
vols = [stubs.stub_volume(i)
for i in xrange(100)]
if limit == None or limit >= len(vols):
@ -902,7 +906,8 @@ class VolumeApiTest(test.TestCase):
# Number of volumes more then the max, include next link
def stub_volume_get_all3(context, marker, limit,
sort_key, sort_dir,
filters=None):
filters=None,
viewable_admin_meta=False):
vols = [stubs.stub_volume(i)
for i in xrange(CONF.osapi_max_limit + 100)]
if limit == None or limit >= len(vols):
@ -941,13 +946,15 @@ class VolumeApiTest(test.TestCase):
"""
# Non-admin, project function should be called with no_migration_status
def stub_volume_get_all_by_project(context, project_id, marker, limit,
sort_key, sort_dir, filters=None):
sort_key, sort_dir, filters=None,
viewable_admin_meta=False):
self.assertEqual(filters['no_migration_targets'], True)
self.assertFalse('all_tenants' in filters)
return [stubs.stub_volume(1, display_name='vol1')]
def stub_volume_get_all(context, marker, limit,
sort_key, sort_dir, filters=None):
sort_key, sort_dir, filters=None,
viewable_admin_meta=False):
return []
self.stubs.Set(db, 'volume_get_all_by_project',
stub_volume_get_all_by_project)
@ -963,12 +970,14 @@ class VolumeApiTest(test.TestCase):
# Admin, all_tenants is not set, project function should be called
# without no_migration_status
def stub_volume_get_all_by_project2(context, project_id, marker, limit,
sort_key, sort_dir, filters=None):
sort_key, sort_dir, filters=None,
viewable_admin_meta=False):
self.assertFalse('no_migration_targets' in filters)
return [stubs.stub_volume(1, display_name='vol2')]
def stub_volume_get_all2(context, marker, limit,
sort_key, sort_dir, filters=None):
sort_key, sort_dir, filters=None,
viewable_admin_meta=False):
return []
self.stubs.Set(db, 'volume_get_all_by_project',
stub_volume_get_all_by_project2)
@ -982,11 +991,13 @@ class VolumeApiTest(test.TestCase):
# Admin, all_tenants is set, get_all function should be called
# without no_migration_status
def stub_volume_get_all_by_project3(context, project_id, marker, limit,
sort_key, sort_dir, filters=None):
sort_key, sort_dir, filters=None,
viewable_admin_meta=False):
return []
def stub_volume_get_all3(context, marker, limit,
sort_key, sort_dir, filters=None):
sort_key, sort_dir, filters=None,
viewable_admin_meta=False):
self.assertFalse('no_migration_targets' in filters)
self.assertFalse('all_tenants' in filters)
return [stubs.stub_volume(1, display_name='vol3')]
@ -1047,7 +1058,7 @@ class VolumeApiTest(test.TestCase):
self.assertIsNotNone(req.cached_resource_by_id('1'))
def test_volume_show_no_attachments(self):
def stub_volume_get(self, context, volume_id):
def stub_volume_get(self, context, volume_id, **kwargs):
return stubs.stub_volume(volume_id, attach_status='detached')
self.stubs.Set(volume_api.API, 'get', stub_volume_get)
@ -1152,7 +1163,7 @@ class VolumeApiTest(test.TestCase):
self.assertEqual(res_dict, expected)
def test_volume_show_with_encrypted_volume(self):
def stub_volume_get(self, context, volume_id):
def stub_volume_get(self, context, volume_id, **kwargs):
return stubs.stub_volume(volume_id, encryption_key_id='fake_id')
self.stubs.Set(volume_api.API, 'get', stub_volume_get)
@ -1162,7 +1173,7 @@ class VolumeApiTest(test.TestCase):
self.assertEqual(res_dict['volume']['encrypted'], True)
def test_volume_show_with_unencrypted_volume(self):
def stub_volume_get(self, context, volume_id):
def stub_volume_get(self, context, volume_id, **kwargs):
return stubs.stub_volume(volume_id, encryption_key_id=None)
self.stubs.Set(volume_api.API, 'get', stub_volume_get)
@ -1262,9 +1273,7 @@ class VolumeApiTest(test.TestCase):
metadata = [{"key": "key", "value": "value"}]
volume = dict(volume_admin_metadata=admin_metadata,
volume_metadata=metadata)
admin_ctx = context.get_admin_context()
utils.add_visible_admin_metadata(admin_ctx, volume,
self.controller.volume_api)
utils.add_visible_admin_metadata(volume)
self.assertEqual(volume['volume_metadata'],
[{"key": "key", "value": "value"},
@ -1277,9 +1286,7 @@ class VolumeApiTest(test.TestCase):
metadata = {"key": "value"}
volume = dict(admin_metadata=admin_metadata,
metadata=metadata)
admin_ctx = context.get_admin_context()
utils.add_visible_admin_metadata(admin_ctx, volume,
self.controller.volume_api)
utils.add_visible_admin_metadata(volume)
self.assertEqual(volume['metadata'],
{'key': 'value',
'attached_mode': 'visible',

25
cinder/utils.py
View File

@ -795,36 +795,25 @@ def check_string_length(value, name, min_length=0, max_length=None):
_visible_admin_metadata_keys = ['readonly', 'attached_mode']
def add_visible_admin_metadata(context, volume, volume_api):
def add_visible_admin_metadata(volume):
"""Add user-visible admin metadata to regular metadata.
Extracts the admin metadata keys that are to be made visible to
non-administrators, and adds them to the regular metadata structure for the
passed-in volume.
"""
if context is None:
return
visible_admin_meta = {}
if context.is_admin:
volume_tmp = volume
else:
try:
volume_tmp = volume_api.get(context.elevated(), volume['id'])
except Exception:
return
if volume_tmp.get('volume_admin_metadata'):
for item in volume_tmp['volume_admin_metadata']:
if volume.get('volume_admin_metadata'):
for item in volume['volume_admin_metadata']:
if item['key'] in _visible_admin_metadata_keys:
visible_admin_meta[item['key']] = item['value']
# avoid circular ref when volume is a Volume instance
elif (volume_tmp.get('admin_metadata') and
isinstance(volume_tmp.get('admin_metadata'), dict)):
elif (volume.get('admin_metadata') and
isinstance(volume.get('admin_metadata'), dict)):
for key in _visible_admin_metadata_keys:
if key in volume_tmp['admin_metadata'].keys():
visible_admin_meta[key] = volume_tmp['admin_metadata'][key]
if key in volume['admin_metadata'].keys():
visible_admin_meta[key] = volume['admin_metadata'][key]
if not visible_admin_meta:
return

8
cinder/volume/api.py
View File

@ -260,14 +260,16 @@ class API(base.Base):
def update(self, context, volume, fields):
self.db.volume_update(context, volume['id'], fields)
def get(self, context, volume_id):
def get(self, context, volume_id, viewable_admin_meta=False):
if viewable_admin_meta:
context = context.elevated()
rv = self.db.volume_get(context, volume_id)
volume = dict(rv.iteritems())
check_policy(context, 'get', volume)
return volume
def get_all(self, context, marker=None, limit=None, sort_key='created_at',
sort_dir='desc', filters=None):
sort_dir='desc', filters=None, viewable_admin_meta=False):
check_policy(context, 'get_all')
if filters == None:
filters = {}
@ -298,6 +300,8 @@ class API(base.Base):
volumes = self.db.volume_get_all(context, marker, limit, sort_key,
sort_dir, filters=filters)
else:
if viewable_admin_meta:
context = context.elevated()
volumes = self.db.volume_get_all_by_project(context,
context.project_id,
marker, limit,