Add volume_type to volume object expected_attrs

We haven't had volume_type in expected_attrs for volume objects lists.
This resulted in situation in which although we were joining the
volume_type explicitely in DB API, the object just dropped the data.
Volume type was then lazy loaded when needed, so every "cinder list"
call was making additional DB queries per returned volume, causing
massive performance drop.

Actually there were two unnecessary DB calls per volume, because not
only volume_type was fetched, but also volume_type.extra_specs as a result
of passing 'extra_specs' in expected_attrs when calling
VolumeType._from_db_volume in Volume._from_db_volume (wow, that's
complicated…).

This commit sorts this out by adding volume_type to expected_attrs to
match what we join in the DB. Please note that I'm not adding
consistencygroup and volume_attachment on purpose - addition causes some
unit tests failure and that late in the release it seems risky to try
fixing that. The changes also required minor rework of expected_attrs
infrastructure in the o.vo layer to be able to pass different values
when we query for just a single volume and when we fetch whole list (as
we're doing different joins in the DB layer in both cases).

Change-Id: Iabf9c3fab56ffef50695ce45745f193273822b39
Closes-Bug: 1555153

diff --git a/cinder/objects/base.py b/cinder/objects/base.py
index 4534b703b70b73d12d4359c7932ef994024f6e36..b015c0215276e33115003c4f5aeaef9db15bfa6e 100644 (file)
--- a/cinder/objects/base.py
+++ b/cinder/objects/base.py
@@ -149,6 +149,10 @@ class CinderObject(base.VersionedObject):
         # Return modified dict
         return changes
 
+    @classmethod
+    def _get_expected_attrs(cls, context):
+        return None
+
     @base.remotable_classmethod
     def get_by_id(cls, context, id, *args, **kwargs):
         # To get by id we need to have a model and for the model to
@@ -160,9 +164,10 @@ class CinderObject(base.VersionedObject):
 
         model = db.get_model_for_versioned_object(cls)
         orm_obj = db.get_by_id(context, model, id, *args, **kwargs)
+        expected_attrs = cls._get_expected_attrs(context)
         kargs = {}
-        if hasattr(cls, 'DEFAULT_EXPECTED_ATTR'):
-            kargs = {'expected_attrs': getattr(cls, 'DEFAULT_EXPECTED_ATTR')}
+        if expected_attrs:
+            kargs = {'expected_attrs': expected_attrs}
         return cls._from_db_object(context, cls(context), orm_obj, **kargs)
 
     def conditional_update(self, values, expected_values=None, filters=(),

diff --git a/cinder/objects/snapshot.py b/cinder/objects/snapshot.py
index 1c7cc64affbf43f3a3c1f4e4aca5fce0305bb947..a601e74b8341fba2e42707a992be8a0b2d7ec3bf 100644 (file)
--- a/cinder/objects/snapshot.py
+++ b/cinder/objects/snapshot.py
@@ -37,8 +37,6 @@ class Snapshot(base.CinderPersistentObject, base.CinderObject,
     # are typically the relationship in the sqlalchemy object.
     OPTIONAL_FIELDS = ('volume', 'metadata', 'cgsnapshot')
 
-    DEFAULT_EXPECTED_ATTR = ('metadata',)
-
     fields = {
         'id': fields.UUIDField(),
 
@@ -66,6 +64,10 @@ class Snapshot(base.CinderPersistentObject, base.CinderObject,
         'cgsnapshot': fields.ObjectField('CGSnapshot', nullable=True),
     }
 
+    @classmethod
+    def _get_expected_attrs(cls, context):
+        return 'metadata',
+
     # NOTE(thangp): obj_extra_fields is used to hold properties that are not
     # usually part of the model
     obj_extra_fields = ['name', 'volume_name']
@@ -232,14 +234,16 @@ class SnapshotList(base.ObjectListBase, base.CinderObject):
                 sort_keys=None, sort_dirs=None, offset=None):
         snapshots = db.snapshot_get_all(context, search_opts, marker, limit,
                                         sort_keys, sort_dirs, offset)
-        return base.obj_make_list(context, cls(), objects.Snapshot,
-                                  snapshots, expected_attrs=['metadata'])
+        expected_attrs = Snapshot._get_expected_attrs(context)
+        return base.obj_make_list(context, cls(context), objects.Snapshot,
+                                  snapshots, expected_attrs=expected_attrs)
 
     @base.remotable_classmethod
     def get_by_host(cls, context, host, filters=None):
         snapshots = db.snapshot_get_by_host(context, host, filters)
+        expected_attrs = Snapshot._get_expected_attrs(context)
         return base.obj_make_list(context, cls(context), objects.Snapshot,
-                                  snapshots, expected_attrs=['metadata'])
+                                  snapshots, expected_attrs=expected_attrs)
 
     @base.remotable_classmethod
     def get_all_by_project(cls, context, project_id, search_opts, marker=None,
@@ -248,23 +252,27 @@ class SnapshotList(base.ObjectListBase, base.CinderObject):
         snapshots = db.snapshot_get_all_by_project(
             context, project_id, search_opts, marker, limit, sort_keys,
             sort_dirs, offset)
+        expected_attrs = Snapshot._get_expected_attrs(context)
         return base.obj_make_list(context, cls(context), objects.Snapshot,
-                                  snapshots, expected_attrs=['metadata'])
+                                  snapshots, expected_attrs=expected_attrs)
 
     @base.remotable_classmethod
     def get_all_for_volume(cls, context, volume_id):
         snapshots = db.snapshot_get_all_for_volume(context, volume_id)
+        expected_attrs = Snapshot._get_expected_attrs(context)
         return base.obj_make_list(context, cls(context), objects.Snapshot,
-                                  snapshots, expected_attrs=['metadata'])
+                                  snapshots, expected_attrs=expected_attrs)
 
     @base.remotable_classmethod
     def get_active_by_window(cls, context, begin, end):
         snapshots = db.snapshot_get_active_by_window(context, begin, end)
+        expected_attrs = Snapshot._get_expected_attrs(context)
         return base.obj_make_list(context, cls(context), objects.Snapshot,
-                                  snapshots, expected_attrs=['metadata'])
+                                  snapshots, expected_attrs=expected_attrs)
 
     @base.remotable_classmethod
     def get_all_for_cgsnapshot(cls, context, cgsnapshot_id):
         snapshots = db.snapshot_get_all_for_cgsnapshot(context, cgsnapshot_id)
+        expected_attrs = Snapshot._get_expected_attrs(context)
         return base.obj_make_list(context, cls(context), objects.Snapshot,
-                                  snapshots, expected_attrs=['metadata'])
+                                  snapshots, expected_attrs=expected_attrs)

diff --git a/cinder/objects/volume.py b/cinder/objects/volume.py
index 3ce5b7d567673a48581867e56daa3391a3e2121c..ccabee9d327a375a28fa93a743e5151b8a3ce20d 100644 (file)
--- a/cinder/objects/volume.py
+++ b/cinder/objects/volume.py
@@ -62,8 +62,6 @@ class Volume(base.CinderPersistentObject, base.CinderObject,
                        'volume_type', 'volume_attachment', 'consistencygroup',
                        'snapshots')
 
-    DEFAULT_EXPECTED_ATTR = ('admin_metadata', 'metadata')
-
     fields = {
         'id': fields.UUIDField(),
         '_name_id': fields.UUIDField(nullable=True),
@@ -124,6 +122,14 @@ class Volume(base.CinderPersistentObject, base.CinderObject,
     obj_extra_fields = ['name', 'name_id', 'volume_metadata',
                         'volume_admin_metadata', 'volume_glance_metadata']
 
+    @classmethod
+    def _get_expected_attrs(cls, context):
+        expected_attrs = ['metadata', 'volume_type', 'volume_type.extra_specs']
+        if context.is_admin:
+            expected_attrs.append('admin_metadata')
+
+        return expected_attrs
+
     @property
     def name_id(self):
         return self.id if not self._name_id else self._name_id
@@ -248,9 +254,12 @@ class Volume(base.CinderPersistentObject, base.CinderObject,
         if 'volume_type' in expected_attrs:
             db_volume_type = db_volume.get('volume_type')
             if db_volume_type:
+                vt_expected_attrs = []
+                if 'volume_type.extra_specs' in expected_attrs:
+                    vt_expected_attrs.append('extra_specs')
                 volume.volume_type = objects.VolumeType._from_db_object(
                     context, objects.VolumeType(), db_volume_type,
-                    expected_attrs='extra_specs')
+                    expected_attrs=vt_expected_attrs)
         if 'volume_attachment' in expected_attrs:
             attachments = base.obj_make_list(
                 context, objects.VolumeAttachmentList(context),
@@ -430,27 +439,35 @@ class VolumeList(base.ObjectListBase, base.CinderObject):
         '1.1': '1.1',
     }
 
+    @classmethod
+    def _get_expected_attrs(cls, context):
+        expected_attrs = ['metadata', 'volume_type']
+        if context.is_admin:
+            expected_attrs.append('admin_metadata')
+
+        return expected_attrs
+
     @base.remotable_classmethod
     def get_all(cls, context, marker, limit, sort_keys=None, sort_dirs=None,
                 filters=None, offset=None):
         volumes = db.volume_get_all(context, marker, limit,
                                     sort_keys=sort_keys, sort_dirs=sort_dirs,
                                     filters=filters, offset=offset)
-        expected_attrs = ['admin_metadata', 'metadata']
+        expected_attrs = cls._get_expected_attrs(context)
         return base.obj_make_list(context, cls(context), objects.Volume,
                                   volumes, expected_attrs=expected_attrs)
 
     @base.remotable_classmethod
     def get_all_by_host(cls, context, host, filters=None):
         volumes = db.volume_get_all_by_host(context, host, filters)
-        expected_attrs = ['admin_metadata', 'metadata']
+        expected_attrs = cls._get_expected_attrs(context)
         return base.obj_make_list(context, cls(context), objects.Volume,
                                   volumes, expected_attrs=expected_attrs)
 
     @base.remotable_classmethod
     def get_all_by_group(cls, context, group_id, filters=None):
         volumes = db.volume_get_all_by_group(context, group_id, filters)
-        expected_attrs = ['admin_metadata', 'metadata']
+        expected_attrs = cls._get_expected_attrs(context)
         return base.obj_make_list(context, cls(context), objects.Volume,
                                   volumes, expected_attrs=expected_attrs)
 
@@ -462,6 +479,6 @@ class VolumeList(base.ObjectListBase, base.CinderObject):
                                                limit, sort_keys=sort_keys,
                                                sort_dirs=sort_dirs,
                                                filters=filters, offset=offset)
-        expected_attrs = ['admin_metadata', 'metadata']
+        expected_attrs = cls._get_expected_attrs(context)
         return base.obj_make_list(context, cls(context), objects.Volume,
                                   volumes, expected_attrs=expected_attrs)

diff --git a/cinder/objects/volume_type.py b/cinder/objects/volume_type.py
index b6471fd775a669873034fee4f88b35f916d7be33..de964d8631cf5c1fc917d3fbccf7ad2b25aba4da 100644 (file)
--- a/cinder/objects/volume_type.py
+++ b/cinder/objects/volume_type.py
@@ -31,8 +31,6 @@ class VolumeType(base.CinderPersistentObject, base.CinderObject,
     # Version 1.0: Initial version
     VERSION = '1.0'
 
-    DEFAULT_EXPECTED_ATTR = ('extra_specs', 'projects')
-
     fields = {
         'id': fields.UUIDField(),
         'name': fields.StringField(nullable=True),
@@ -42,6 +40,10 @@ class VolumeType(base.CinderPersistentObject, base.CinderObject,
         'extra_specs': fields.DictOfStringsField(nullable=True),
     }
 
+    @classmethod
+    def _get_expected_attrs(cls, context):
+        return 'extra_specs', 'projects'
+
     @staticmethod
     def _from_db_object(context, type, db_type, expected_attrs=None):
         if expected_attrs is None:
@@ -118,7 +120,7 @@ class VolumeTypeList(base.ObjectListBase, base.CinderObject):
                                            marker=marker, limit=limit,
                                            sort_keys=sort_keys,
                                            sort_dirs=sort_dirs, offset=offset)
-        expected_attrs = ['extra_specs', 'projects']
+        expected_attrs = VolumeType._get_expected_attrs(context)
         return base.obj_make_list(context, cls(context),
                                   objects.VolumeType, types.values(),
                                   expected_attrs=expected_attrs)

diff --git a/cinder/tests/unit/api/contrib/test_volume_actions.py b/cinder/tests/unit/api/contrib/test_volume_actions.py
index 21d1a138eff630e19fed394fef6a49e786588bbc..cc0596ec920312f9a3bfe88a7641079bd84cef39 100644 (file)
--- a/cinder/tests/unit/api/contrib/test_volume_actions.py
+++ b/cinder/tests/unit/api/contrib/test_volume_actions.py
@@ -619,7 +619,8 @@ class VolumeImageActionsTest(test.TestCase):
                      'status': 'uploading',
                      'display_description': 'displaydesc',
                      'size': 1,
-                     'volume_type': {'name': 'vol_type_name'},
+                     'volume_type': fake_volume.fake_db_volume_type(
+                         name='vol_type_name'),
                      'image_id': 1,
                      'container_format': 'bare',
                      'disk_format': 'raw',
@@ -803,7 +804,8 @@ class VolumeImageActionsTest(test.TestCase):
                                 'status': 'uploading',
                                 'display_description': 'displaydesc',
                                 'size': 1,
-                                'volume_type': {'name': 'vol_type_name'},
+                                'volume_type': fake_volume.fake_db_volume_type(
+                                    name='vol_type_name'),
                                 'image_id': 1,
                                 'container_format': 'bare',
                                 'disk_format': 'raw',
@@ -860,7 +862,8 @@ class VolumeImageActionsTest(test.TestCase):
                                 'status': 'uploading',
                                 'display_description': 'displaydesc',
                                 'size': 1,
-                                'volume_type': {'name': 'vol_type_name'},
+                                'volume_type': fake_volume.fake_db_volume_type(
+                                    name='vol_type_name'),
                                 'image_id': 1,
                                 'container_format': 'bare',
                                 'disk_format': 'raw',
@@ -914,7 +917,8 @@ class VolumeImageActionsTest(test.TestCase):
                                 'status': 'uploading',
                                 'display_description': 'displaydesc',
                                 'size': 1,
-                                'volume_type': {'name': 'vol_type_name'},
+                                'volume_type': fake_volume.fake_db_volume_type(
+                                    name='vol_type_name'),
                                 'image_id': 1,
                                 'container_format': 'bare',
                                 'disk_format': 'raw',
@@ -961,7 +965,8 @@ class VolumeImageActionsTest(test.TestCase):
                             'status': 'uploading',
                             'display_description': 'displaydesc',
                             'size': 1,
-                            'volume_type': {'name': 'vol_type_name'},
+                            'volume_type': fake_volume.fake_db_volume_type(
+                                name='vol_type_name'),
                             'image_id': 1,
                             'container_format': 'bare',
                             'disk_format': 'raw',

diff --git a/cinder/tests/unit/api/v2/stubs.py b/cinder/tests/unit/api/v2/stubs.py
index e478658c0caf5795f89392b3ed7d408c32668947..42b89e2be1789cf32b697f6ecbbbd018ba504582 100644 (file)
--- a/cinder/tests/unit/api/v2/stubs.py
+++ b/cinder/tests/unit/api/v2/stubs.py
@@ -65,7 +65,7 @@ def stub_volume(id, **kwargs):
         'bootable': False,
         'launched_at': datetime.datetime(1900, 1, 1, 1, 1, 1,
                                          tzinfo=iso8601.iso8601.Utc()),
-        'volume_type': {'name': DEFAULT_VOL_TYPE},
+        'volume_type': fake_volume.fake_db_volume_type(name=DEFAULT_VOL_TYPE),
         'replication_status': 'disabled',
         'replication_extended_status': None,
         'replication_driver_data': None,