From e31e31689073323ceba2bb4c993af73df8274121 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Micha=C5=82=20Dulko?= Date: Thu, 19 Nov 2015 19:36:27 +0100 Subject: [PATCH] Add metadata aliases to Volume object When writing Volume object we've renamed some fields from SQLAlchemy model. This included volume_metadata (renamed to metadata) and volume_admin_metadata (renamed to admin_metadata) and volume_glance_metadata (recently added as glance_metadata). Some code were relying on old names. As right now we're in the transitional phase it's hard to tell if driver method will get versioned object or old SQLAlchemy object. To mitigate that and mimic old SQLAlchemy object behavior on versioned object we should add properties to serve as aliases for older names. This commit also fixes the tests for EMC VNX that were blocking the patch. Tests were setting volume_metadata property on volume object, which actually had no effect without this patch. Another thing done is moving overriding __contains__ from CinderObjectDictCompat to CinderObject class to solve MRO issue because both of these classes were defining __contains__ method. Change-Id: I79e24c5ad20f17bb6b21b2d47f955afde47d9794 Closes-Bug: 1529877 Related-Bug: 1516903 --- cinder/objects/base.py | 14 +-- cinder/objects/volume.py | 90 +++++++++++++++---- .../unit/api/contrib/test_volume_actions.py | 4 + cinder/tests/unit/objects/test_volume.py | 37 +++++++- cinder/tests/unit/test_emc_vnx.py | 4 +- cinder/tests/unit/test_volume.py | 2 +- 6 files changed, 119 insertions(+), 32 deletions(-) diff --git a/cinder/objects/base.py b/cinder/objects/base.py index a673c4ff0..ef8c1150a 100644 --- a/cinder/objects/base.py +++ b/cinder/objects/base.py @@ -223,6 +223,13 @@ class CinderObject(base.VersionedObject): self[field] = current[field] self.obj_reset_changes() + def __contains__(self, name): + # We're using obj_extra_fields to provide aliases for some fields while + # in transition period. This override is to make these aliases pass + # "'foo' in obj" tests. + return name in self.obj_extra_fields or super(CinderObject, + self).__contains__(name) + class CinderObjectDictCompat(base.VersionedObjectDictCompat): """Mix-in to provide dictionary key access compat. @@ -264,13 +271,6 @@ class CinderObjectDictCompat(base.VersionedObjectDictCompat): # behavior we should still return None return None - def __contains__(self, name): - try: - # Overriding this to make extra fields pass "'foo' in obj" tests - return name in self.obj_extra_fields or self.obj_attr_is_set(name) - except AttributeError: - return False - class CinderPersistentObject(object): """Mixin class for Persistent objects. diff --git a/cinder/objects/volume.py b/cinder/objects/volume.py index 70533c54d..4e5202ea7 100644 --- a/cinder/objects/volume.py +++ b/cinder/objects/volume.py @@ -27,6 +27,27 @@ CONF = cfg.CONF LOG = logging.getLogger(__name__) +class MetadataObject(dict): + # This is a wrapper class that simulates SQLAlchemy (.*)Metadata objects to + # maintain compatibility with older representations of Volume that some + # drivers rely on. This is helpful in transition period while some driver + # methods are invoked with volume versioned object and some SQLAlchemy + # object or dict. + def __init__(self, key=None, value=None): + super(MetadataObject, self).__init__() + self.key = key + self.value = value + + def __getattr__(self, name): + if name in self: + return self[name] + else: + raise AttributeError("No such attribute: " + name) + + def __setattr__(self, name, value): + self[name] = value + + @base.CinderObjectRegistry.register class Volume(base.CinderPersistentObject, base.CinderObject, base.CinderObjectDictCompat, base.CinderComparableObject): @@ -99,7 +120,8 @@ class Volume(base.CinderPersistentObject, base.CinderObject, # NOTE(thangp): obj_extra_fields is used to hold properties that are not # usually part of the model - obj_extra_fields = ['name', 'name_id'] + obj_extra_fields = ['name', 'name_id', 'volume_metadata', + 'volume_admin_metadata', 'volume_glance_metadata'] @property def name_id(self): @@ -113,6 +135,40 @@ class Volume(base.CinderPersistentObject, base.CinderObject, def name(self): return CONF.volume_name_template % self.name_id + # TODO(dulek): Three properties below are for compatibility with dict + # representation of volume. The format there is different (list of + # SQLAlchemy models) so we need a conversion. Anyway - these should be + # removed when we stop this class from deriving from DictObjectCompat. + @property + def volume_metadata(self): + md = [MetadataObject(k, v) for k, v in self.metadata.items()] + return md + + @volume_metadata.setter + def volume_metadata(self, value): + md = {d['key']: d['value'] for d in value} + self.metadata = md + + @property + def volume_admin_metadata(self): + md = [MetadataObject(k, v) for k, v in self.admin_metadata.items()] + return md + + @volume_admin_metadata.setter + def volume_admin_metadata(self, value): + md = {d['key']: d['value'] for d in value} + self.admin_metadata = md + + @property + def volume_glance_metadata(self): + md = [MetadataObject(k, v) for k, v in self.glance_metadata.items()] + return md + + @volume_glance_metadata.setter + def volume_glance_metadata(self, value): + md = {d['key']: d['value'] for d in value} + self.glance_metadata = md + def __init__(self, *args, **kwargs): super(Volume, self).__init__(*args, **kwargs) self._orig_metadata = {} @@ -171,23 +227,16 @@ class Volume(base.CinderPersistentObject, base.CinderObject, # Get data from db_volume object that was queried by joined query # from DB if 'metadata' in expected_attrs: - volume.metadata = {} metadata = db_volume.get('volume_metadata', []) - if metadata: - volume.metadata = {item['key']: item['value'] - for item in metadata} + volume.metadata = {item['key']: item['value'] for item in metadata} if 'admin_metadata' in expected_attrs: - volume.admin_metadata = {} metadata = db_volume.get('volume_admin_metadata', []) - if metadata: - volume.admin_metadata = {item['key']: item['value'] - for item in metadata} + volume.admin_metadata = {item['key']: item['value'] + for item in metadata} if 'glance_metadata' in expected_attrs: - volume.glance_metadata = {} metadata = db_volume.get('volume_glance_metadata', []) - if metadata: - volume.glance_metadata = {item['key']: item['value'] - for item in metadata} + volume.glance_metadata = {item['key']: item['value'] + for item in metadata} if 'volume_type' in expected_attrs: db_volume_type = db_volume.get('volume_type') if db_volume_type: @@ -227,9 +276,6 @@ class Volume(base.CinderPersistentObject, base.CinderObject, if 'consistencygroup' in updates: raise exception.ObjectActionError( action='create', reason=_('consistencygroup assigned')) - if 'glance_metadata' in updates: - raise exception.ObjectActionError( - action='create', reason=_('glance_metadata assigned')) if 'snapshots' in updates: raise exception.ObjectActionError( action='create', reason=_('snapshots assigned')) @@ -287,8 +333,16 @@ class Volume(base.CinderPersistentObject, base.CinderObject, self.admin_metadata = db.volume_admin_metadata_get( self._context, self.id) elif attrname == 'glance_metadata': - self.glance_metadata = db.volume_glance_metadata_get( - self._context, self.id) + try: + # NOTE(dulek): We're using alias here to have conversion from + # list to dict done there. + self.volume_glance_metadata = db.volume_glance_metadata_get( + self._context, self.id) + except exception.GlanceMetadataNotFound: + # NOTE(dulek): DB API raises when volume has no + # glance_metadata. Silencing this because at this level no + # metadata is a completely valid result. + self.glance_metadata = {} elif attrname == 'volume_type': # If the volume doesn't have volume_type, VolumeType.get_by_id # would trigger a db call which raise VolumeTypeNotFound exception. diff --git a/cinder/tests/unit/api/contrib/test_volume_actions.py b/cinder/tests/unit/api/contrib/test_volume_actions.py index c34da3b9b..5c25c94fc 100644 --- a/cinder/tests/unit/api/contrib/test_volume_actions.py +++ b/cinder/tests/unit/api/contrib/test_volume_actions.py @@ -69,6 +69,10 @@ class VolumeActionsTest(test.TestCase): self.mock_volume_update = self.update_patcher.start() self.addCleanup(self.update_patcher.stop) self.mock_volume_update.return_value = vol + self.db_get_patcher = mock.patch('cinder.db.sqlalchemy.api.volume_get') + self.mock_volume_db_get = self.db_get_patcher.start() + self.addCleanup(self.db_get_patcher.stop) + self.mock_volume_db_get.return_value = vol self.flags(rpc_backend='cinder.openstack.common.rpc.impl_fake') diff --git a/cinder/tests/unit/objects/test_volume.py b/cinder/tests/unit/objects/test_volume.py index 2beea8639..5a463add4 100644 --- a/cinder/tests/unit/objects/test_volume.py +++ b/cinder/tests/unit/objects/test_volume.py @@ -35,8 +35,8 @@ class TestVolume(test_objects.BaseObjectsTestCase): db_volume = fake_volume.fake_db_volume() volume_get.return_value = db_volume volume = objects.Volume.get_by_id(self.context, 1) - self._compare(self, db_volume, volume) volume_get.assert_called_once_with(self.context, 1) + self._compare(self, db_volume, volume) @mock.patch('cinder.db.sqlalchemy.api.model_query') def test_get_by_id_no_existing_id(self, model_query): @@ -140,7 +140,9 @@ class TestVolume(test_objects.BaseObjectsTestCase): def test_obj_fields(self): volume = objects.Volume(context=self.context, id=2, _name_id=2) - self.assertEqual(['name', 'name_id'], volume.obj_extra_fields) + self.assertEqual(['name', 'name_id', 'volume_metadata', + 'volume_admin_metadata', 'volume_glance_metadata'], + volume.obj_extra_fields) self.assertEqual('volume-2', volume.name) self.assertEqual('2', volume.name_id) @@ -180,9 +182,9 @@ class TestVolume(test_objects.BaseObjectsTestCase): mock_metadata_get.assert_called_once_with(self.context, volume.id) # Test glance_metadata lazy-loaded field - glance_metadata = {'foo': 'bar'} + glance_metadata = [{'key': 'foo', 'value': 'bar'}] mock_glance_metadata_get.return_value = glance_metadata - self.assertEqual(glance_metadata, volume.glance_metadata) + self.assertEqual({'foo': 'bar'}, volume.glance_metadata) mock_glance_metadata_get.assert_called_once_with( self.context, volume.id) @@ -293,6 +295,33 @@ class TestVolume(test_objects.BaseObjectsTestCase): mock.call.__nonzero__(), mock.call(self.context, '1')]) + def test_metadata_aliases(self): + volume = objects.Volume(context=self.context) + # metadata<->volume_metadata + volume.metadata = {'abc': 'def'} + self.assertEqual([{'key': 'abc', 'value': 'def'}], + volume.volume_metadata) + + md = [{'key': 'def', 'value': 'abc'}] + volume.volume_metadata = md + self.assertEqual({'def': 'abc'}, volume.metadata) + + # admin_metadata<->volume_admin_metadata + volume.admin_metadata = {'foo': 'bar'} + self.assertEqual([{'key': 'foo', 'value': 'bar'}], + volume.volume_admin_metadata) + + volume.volume_admin_metadata = [{'key': 'xyz', 'value': '42'}] + self.assertEqual({'xyz': '42'}, volume.admin_metadata) + + # glance_metadata<->volume_glance_metadata + volume.glance_metadata = {'jkl': 'mno'} + self.assertEqual([{'key': 'jkl', 'value': 'mno'}], + volume.volume_glance_metadata) + + volume.volume_glance_metadata = [{'key': 'prs', 'value': 'tuw'}] + self.assertEqual({'prs': 'tuw'}, volume.glance_metadata) + class TestVolumeList(test_objects.BaseObjectsTestCase): @mock.patch('cinder.db.volume_get_all') diff --git a/cinder/tests/unit/test_emc_vnx.py b/cinder/tests/unit/test_emc_vnx.py index 8a1b80b42..d4f3f2acd 100644 --- a/cinder/tests/unit/test_emc_vnx.py +++ b/cinder/tests/unit/test_emc_vnx.py @@ -2073,7 +2073,7 @@ Time Remaining: 0 second(s) self.testData.test_volume) vol['provider_location'] = 'system^FNM11111|type^smp|id^1' vol['volume_metadata'] = [{'key': 'snapcopy', 'value': 'True'}] - tmp_snap = "tmp-snap-%s" % vol['id'] + tmp_snap = "snap-as-vol-%s" % vol['id'] ret = self.driver.migrate_volume(None, vol, fake_host) @@ -3839,7 +3839,7 @@ Time Remaining: 0 second(s) self.testData.test_volume3) vol['provider_location'] = 'system^FNM11111|type^smp|id^1' vol['volume_metadata'] = [{'key': 'snapcopy', 'value': 'True'}] - tmp_snap = 'tmp-snap-%s' % vol['id'] + tmp_snap = 'snap-as-vol-%s' % vol['id'] ret = self.driver.retype(None, vol, new_type_data, diff_data, diff --git a/cinder/tests/unit/test_volume.py b/cinder/tests/unit/test_volume.py index adb9c7004..db996d820 100644 --- a/cinder/tests/unit/test_volume.py +++ b/cinder/tests/unit/test_volume.py @@ -566,7 +566,7 @@ class VolumeTestCase(BaseVolumeTestCase): 'replication_status': 'disabled', 'replication_extended_status': None, 'replication_driver_data': None, - 'metadata': None, + 'metadata': [], 'volume_attachment': [], } self.assertDictMatch(expected, msg['payload']) -- 2.45.2