From d31fe66983ca06b1f15a7e41d124c5018bdf44a7 Mon Sep 17 00:00:00 2001 From: wanghao Date: Mon, 20 Jul 2015 17:09:19 +0800 Subject: [PATCH] Improve performance listing detail for volumes When large numbers of volumes are created from images in cloud, and user uses pagination querying, there is no need to query all volumes' volume_glance_metadata, just get those volumes which are in response list. Implement a new method "volume_api.get_list_volumes_image_metadata" to query image metadata in list of volumes by using one sql querying. Change-Id: I42307d51133bab58a166a781fccf797f0608843e Closes-Bug: #1476150 --- cinder/api/contrib/volume_image_metadata.py | 41 +++++++++++-------- cinder/db/api.py | 5 +++ cinder/db/sqlalchemy/api.py | 11 +++++ .../api/contrib/test_volume_image_metadata.py | 27 ++++++++++++ cinder/tests/unit/test_db_api.py | 31 ++++++++++++++ cinder/tests/unit/test_volume.py | 20 +++++++++ cinder/volume/api.py | 9 ++++ 7 files changed, 126 insertions(+), 18 deletions(-) diff --git a/cinder/api/contrib/volume_image_metadata.py b/cinder/api/contrib/volume_image_metadata.py index 80c19472f..a5f0468cd 100644 --- a/cinder/api/contrib/volume_image_metadata.py +++ b/cinder/api/contrib/volume_image_metadata.py @@ -57,41 +57,46 @@ class VolumeImageMetadataController(wsgi.Controller): all_metadata = {} return all_metadata - def _add_image_metadata(self, context, resp_volume, image_meta=None): - """Appends the image metadata to the given volume. + def _add_image_metadata(self, context, resp_volume_list, image_metas=None): + """Appends the image metadata to each of the given volume. :param context: the request context - :param resp_volume: the response volume - :param image_meta: The image metadata to append, if None is provided it - will be retrieved from the database. An empty dict - means there is no metadata and it should not be - retrieved from the db. + :param resp_volume_list: the response volume list + :param image_metas: The image metadata to append, if None is provided + it will be retrieved from the database. An empty + dict means there is no metadata and it should not + be retrieved from the db. """ - if image_meta is None: + vol_id_list = [] + for vol in resp_volume_list: + vol_id_list.append(vol['id']) + if image_metas is None: try: - image_meta = self.volume_api.get_volume_image_metadata( - context, resp_volume) - except Exception: + image_metas = self.volume_api.get_list_volumes_image_metadata( + context, vol_id_list) + except Exception as e: + LOG.debug('Get image metadata error: %s', e) return - if image_meta: - resp_volume['volume_image_metadata'] = dict(image_meta) + if image_metas: + for vol in resp_volume_list: + image_meta = image_metas.get(vol['id'], {}) + vol['volume_image_metadata'] = dict(image_meta) @wsgi.extends def show(self, req, resp_obj, id): context = req.environ['cinder.context'] if authorize(context): resp_obj.attach(xml=VolumeImageMetadataTemplate()) - self._add_image_metadata(context, resp_obj.obj['volume']) + self._add_image_metadata(context, [resp_obj.obj['volume']]) @wsgi.extends def detail(self, req, resp_obj): context = req.environ['cinder.context'] if authorize(context): resp_obj.attach(xml=VolumesImageMetadataTemplate()) - all_meta = self._get_all_images_metadata(context) - for vol in list(resp_obj.obj.get('volumes', [])): - image_meta = all_meta.get(vol['id'], {}) - self._add_image_metadata(context, vol, image_meta) + # Just get the image metadata of those volumes in response. + self._add_image_metadata(context, + list(resp_obj.obj.get('volumes', []))) @wsgi.action("os-set_image_metadata") @wsgi.serializers(xml=common.MetadataTemplate) diff --git a/cinder/db/api.py b/cinder/db/api.py index a783b28a8..258323a45 100644 --- a/cinder/db/api.py +++ b/cinder/db/api.py @@ -666,6 +666,11 @@ def volume_glance_metadata_get(context, volume_id): return IMPL.volume_glance_metadata_get(context, volume_id) +def volume_glance_metadata_list_get(context, volume_id_list): + """Return the glance metadata for a volume list.""" + return IMPL.volume_glance_metadata_list_get(context, volume_id_list) + + def volume_snapshot_glance_metadata_get(context, snapshot_id): """Return the Glance metadata for the specified snapshot.""" return IMPL.volume_snapshot_glance_metadata_get(context, snapshot_id) diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index 9e540ea14..38aa4e5ed 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -3256,6 +3256,17 @@ def volume_glance_metadata_get_all(context): return _volume_glance_metadata_get_all(context) +@require_context +def volume_glance_metadata_list_get(context, volume_id_list): + """Return the glance metadata for a volume list.""" + query = model_query(context, + models.VolumeGlanceMetadata, + session=None) + query = query.filter( + models.VolumeGlanceMetadata.volume_id.in_(volume_id_list)) + return query.all() + + @require_context @require_volume_exists def _volume_glance_metadata_get(context, volume_id, session=None): diff --git a/cinder/tests/unit/api/contrib/test_volume_image_metadata.py b/cinder/tests/unit/api/contrib/test_volume_image_metadata.py index 2b91ed5f4..937d4ca2e 100644 --- a/cinder/tests/unit/api/contrib/test_volume_image_metadata.py +++ b/cinder/tests/unit/api/contrib/test_volume_image_metadata.py @@ -23,6 +23,7 @@ import webob from cinder.api import common from cinder.api.contrib import volume_image_metadata from cinder.api.openstack import wsgi +from cinder import context from cinder import db from cinder import exception from cinder import test @@ -115,18 +116,44 @@ class VolumeImageMetadataTest(test.TestCase): for volume in json.loads(body)['volumes'] ] + def _create_volume_and_glance_metadata(self): + ctxt = context.get_admin_context() + db.volume_create(ctxt, {'id': 'fake', 'status': 'available', + 'host': 'test', 'provider_location': '', + 'size': 1}) + db.volume_glance_metadata_create(ctxt, 'fake', 'image_id', 'someid') + db.volume_glance_metadata_create(ctxt, 'fake', 'image_name', 'fake') + db.volume_glance_metadata_create(ctxt, 'fake', 'kernel_id', + 'somekernel') + db.volume_glance_metadata_create(ctxt, 'fake', 'ramdisk_id', + 'someramdisk') + def test_get_volume(self): + self._create_volume_and_glance_metadata() res = self._make_request('/v2/fake/volumes/%s' % self.UUID) self.assertEqual(200, res.status_int) self.assertEqual(fake_image_metadata, self._get_image_metadata(res.body)) def test_list_detail_volumes(self): + self._create_volume_and_glance_metadata() res = self._make_request('/v2/fake/volumes/detail') self.assertEqual(200, res.status_int) self.assertEqual(fake_image_metadata, self._get_image_metadata_list(res.body)[0]) + def test_list_detail_volumes_with_limit(self): + ctxt = context.get_admin_context() + db.volume_create(ctxt, {'id': 'fake', 'status': 'available', + 'host': 'test', 'provider_location': '', + 'size': 1}) + db.volume_glance_metadata_create(ctxt, 'fake', 'key1', 'value1') + db.volume_glance_metadata_create(ctxt, 'fake', 'key2', 'value2') + res = self._make_request('/v2/fake/volumes/detail?limit=1') + self.assertEqual(200, res.status_int) + self.assertEqual({'key1': 'value1', 'key2': 'value2'}, + self._get_image_metadata_list(res.body)[0]) + def test_create_image_metadata(self): self.stubs.Set(volume.API, 'get_volume_image_metadata', return_empty_image_metadata) diff --git a/cinder/tests/unit/test_db_api.py b/cinder/tests/unit/test_db_api.py index 7e66f733a..f091533b4 100644 --- a/cinder/tests/unit/test_db_api.py +++ b/cinder/tests/unit/test_db_api.py @@ -1031,6 +1031,37 @@ class DBAPIVolumeTestCase(BaseTest): image_name = meta_entry.value self.assertEqual(u'\xe4\xbd\xa0\xe5\xa5\xbd', image_name) + def test_volume_glance_metadata_list_get(self): + """Test volume_glance_metadata_list_get in DB API.""" + db.volume_create(self.ctxt, {'id': 'fake1', 'status': 'available', + 'host': 'test', 'provider_location': '', + 'size': 1}) + db.volume_glance_metadata_create(self.ctxt, 'fake1', 'key1', 'value1') + db.volume_glance_metadata_create(self.ctxt, 'fake1', 'key2', 'value2') + + db.volume_create(self.ctxt, {'id': 'fake2', 'status': 'available', + 'host': 'test', 'provider_location': '', + 'size': 1}) + db.volume_glance_metadata_create(self.ctxt, 'fake2', 'key3', 'value3') + db.volume_glance_metadata_create(self.ctxt, 'fake2', 'key4', 'value4') + + expect_result = [{'volume_id': 'fake1', 'key': 'key1', + 'value': 'value1'}, + {'volume_id': 'fake1', 'key': 'key2', + 'value': 'value2'}, + {'volume_id': 'fake2', 'key': 'key3', + 'value': 'value3'}, + {'volume_id': 'fake2', 'key': 'key4', + 'value': 'value4'}] + self._assertEqualListsOfObjects(expect_result, + db.volume_glance_metadata_list_get( + self.ctxt, ['fake1', 'fake2']), + ignored_keys=['id', + 'snapshot_id', + 'created_at', + 'deleted', 'deleted_at', + 'updated_at']) + class DBAPISnapshotTestCase(BaseTest): diff --git a/cinder/tests/unit/test_volume.py b/cinder/tests/unit/test_volume.py index fa10f4928..46d3e8477 100644 --- a/cinder/tests/unit/test_volume.py +++ b/cinder/tests/unit/test_volume.py @@ -3886,6 +3886,26 @@ class VolumeTestCase(BaseVolumeTestCase): snapshot_id) self.assertEqual('test update name', snap.display_name) + def test_volume_api_get_list_volumes_image_metadata(self): + """Test get_list_volumes_image_metadata in volume API.""" + ctxt = context.get_admin_context() + db.volume_create(ctxt, {'id': 'fake1', 'status': 'available', + 'host': 'test', 'provider_location': '', + 'size': 1}) + db.volume_glance_metadata_create(ctxt, 'fake1', 'key1', 'value1') + db.volume_glance_metadata_create(ctxt, 'fake1', 'key2', 'value2') + db.volume_create(ctxt, {'id': 'fake2', 'status': 'available', + 'host': 'test', 'provider_location': '', + 'size': 1}) + db.volume_glance_metadata_create(ctxt, 'fake2', 'key3', 'value3') + db.volume_glance_metadata_create(ctxt, 'fake2', 'key4', 'value4') + volume_api = cinder.volume.api.API() + results = volume_api.get_list_volumes_image_metadata(ctxt, ['fake1', + 'fake2']) + expect_results = {'fake1': {'key1': 'value1', 'key2': 'value2'}, + 'fake2': {'key3': 'value3', 'key4': 'value4'}} + self.assertEqual(expect_results, results) + @mock.patch.object(QUOTAS, 'reserve') def test_extend_volume(self, reserve): """Test volume can be extended at API level.""" diff --git a/cinder/volume/api.py b/cinder/volume/api.py index f3d89c6e9..aa71bb483 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -1182,6 +1182,15 @@ class API(base.Base): resource=volume) return {meta_entry.key: meta_entry.value for meta_entry in db_data} + def get_list_volumes_image_metadata(self, context, volume_id_list): + db_data = self.db.volume_glance_metadata_list_get(context, + volume_id_list) + results = collections.defaultdict(dict) + for meta_entry in db_data: + results[meta_entry['volume_id']].update({meta_entry['key']: + meta_entry['value']}) + return results + def _check_volume_availability(self, volume, force): """Check if the volume can be used.""" if volume['status'] not in ['available', 'in-use']: