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

diff --git a/cinder/api/contrib/volume_manage.py b/cinder/api/contrib/volume_manage.py
index 4fcbe12c4898706b2ed06bb274f6a8eb17a35c7a..d51ee8e9d999dcc6247fc5d6b28b2dc8d08913bf 100755 (executable)
--- a/cinder/api/contrib/volume_manage.py
+++ b/cinder/api/contrib/volume_manage.py
@@ -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)
 

diff --git a/cinder/api/v1/volumes.py b/cinder/api/v1/volumes.py
index 56458ddaef6c27c6f5ceb97bf22a70b98ba2075b..829237046f060e6f36a9cb57c96135608d265b3f 100644 (file)
--- a/cinder/api/v1/volumes.py
+++ b/cinder/api/v1/volumes.py
@@ -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')

diff --git a/cinder/api/v2/volumes.py b/cinder/api/v2/volumes.py
index a0910ba0e22e8e76d7a568a859428d4b62a3cf05..3434a9bb5ff9323b4b55c6f08ac88d25220401c5 100644 (file)
--- a/cinder/api/v2/volumes.py
+++ b/cinder/api/v2/volumes.py
@@ -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')

diff --git a/cinder/tests/api/v1/test_volumes.py b/cinder/tests/api/v1/test_volumes.py
index 41046be4592b7ab37d18ec3c12489286669e9ed5..f597202f3c23eb5bcb13db605e4aff58a4d1127a 100644 (file)
--- a/cinder/tests/api/v1/test_volumes.py
+++ b/cinder/tests/api/v1/test_volumes.py
@@ -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
 
 
@@ -93,8 +94,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),
@@ -186,8 +186,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),
@@ -557,7 +556,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)
@@ -582,7 +581,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')))
 
@@ -627,7 +626,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'),
@@ -689,7 +689,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)
@@ -699,7 +699,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)
@@ -769,9 +769,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"},
@@ -783,9 +781,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',

diff --git a/cinder/tests/api/v2/stubs.py b/cinder/tests/api/v2/stubs.py
index dc269a67d99e7995f8be713cbfb19c49b5fde8dd..d63f52c47abb6e20b2fb2b30b923a5c0c6666f94 100644 (file)
--- a/cinder/tests/api/v2/stubs.py
+++ b/cinder/tests/api/v2/stubs.py
@@ -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')]
 
 

diff --git a/cinder/tests/api/v2/test_volumes.py b/cinder/tests/api/v2/test_volumes.py
index f44eb7b329c0b062e7a17013611c57de50874328..7263eedcc2d758e5e8d0db62b75df79c4e84fc84 100644 (file)
--- a/cinder/tests/api/v2/test_volumes.py
+++ b/cinder/tests/api/v2/test_volumes.py
@@ -106,8 +106,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,
@@ -210,8 +209,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,
@@ -706,7 +704,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'),
@@ -766,7 +765,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'),
@@ -793,7 +793,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'),
@@ -853,7 +854,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'),
@@ -887,7 +889,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')
@@ -911,7 +913,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):
@@ -929,7 +932,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):
@@ -946,7 +950,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):
@@ -985,13 +990,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)
@@ -1007,12 +1014,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)
@@ -1026,11 +1035,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')]
@@ -1091,7 +1102,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)
@@ -1196,7 +1207,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)
@@ -1206,7 +1217,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)
@@ -1306,9 +1317,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"},
@@ -1321,9 +1330,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',

diff --git a/cinder/utils.py b/cinder/utils.py
index 3736163306e6ef1e9818a39e365b82a6c5eafcc1..6bc03835e1a2f9c5866d377797aa2df628d10158 100644 (file)
--- a/cinder/utils.py
+++ b/cinder/utils.py
@@ -807,36 +807,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

diff --git a/cinder/volume/api.py b/cinder/volume/api.py
index 1ce4ecbb0f05c6fdf727417d42fe0faf7a1cd6d8..075e29cd98ce0f44733f06263e81dd275550227e 100644 (file)
--- a/cinder/volume/api.py
+++ b/cinder/volume/api.py
@@ -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,