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

diff --git a/cinder/api/contrib/volume_image_metadata.py b/cinder/api/contrib/volume_image_metadata.py
index 80c19472f11ed7f22fd4bdb12dc240b85d796a09..a5f0468cd5bf70fc9de99775c9cf9878c8fbec29 100644 (file)
--- 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 a783b28a8fc56303fb94dc2b986bbeadde36d85f..258323a457d24ea3a426d747bd707b0c5d7c4ed3 100644 (file)
--- 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 9e540ea14fd2d27027316864a62ee96e2e7ad6d9..38aa4e5ed5af207ebc112119904a48178a132c38 100644 (file)
--- 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 2b91ed5f453978920bb8bddf2f8b85e28ffca028..937d4ca2eee9939998ffd813bfeadb13b0a61385 100644 (file)
--- 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 7e66f733ab0a56dd270fd112b7830cda0f67fb76..f091533b4bef85fc396f548793baadf8a86e89ed 100644 (file)
--- 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 fa10f492814c915ea04bd9d709d96cb1238672f6..46d3e8477524f41c030cc007edd1bcd5df618147 100644 (file)
--- 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 f3d89c6e903e41ed9e8bdd21c2bb8145ee1153f5..aa71bb4832e5afa67b71453c92275789a7ebc4a7 100644 (file)
--- 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']: