From e0aff5b117fb94b9c9c715f49cf0dc109cce78c4 Mon Sep 17 00:00:00 2001 From: Patrick East Date: Tue, 17 Jan 2017 13:43:42 -0800 Subject: [PATCH] Allow snapshots and volumes to have Null group values We currently blow up if we look at something like snapshot['group_snapshot'] if there is no group snapshot. Ideally this just returns None since there isn't a one. Change-Id: Icf48be99fb3890758070e0d724679e38a6103c8c Closes-Bug: #1657286 --- cinder/objects/snapshot.py | 21 +++++++++++++-------- cinder/objects/volume.py | 22 ++++++++++++++-------- cinder/tests/unit/objects/test_snapshot.py | 18 ++++++++++++++++++ cinder/tests/unit/objects/test_volume.py | 22 +++++++++++++++++++++- 4 files changed, 66 insertions(+), 17 deletions(-) diff --git a/cinder/objects/snapshot.py b/cinder/objects/snapshot.py index 374677407b4..a8b71f93dfa 100644 --- a/cinder/objects/snapshot.py +++ b/cinder/objects/snapshot.py @@ -138,12 +138,12 @@ class Snapshot(cleanable.CinderCleanableObject, base.CinderObject, volume = objects.Volume(context) volume._from_db_object(context, volume, db_snapshot['volume']) snapshot.volume = volume - if 'cgsnapshot' in expected_attrs: + if snapshot.cgsnapshot_id and 'cgsnapshot' in expected_attrs: cgsnapshot = objects.CGSnapshot(context) cgsnapshot._from_db_object(context, cgsnapshot, db_snapshot['cgsnapshot']) snapshot.cgsnapshot = cgsnapshot - if 'group_snapshot' in expected_attrs: + if snapshot.group_snapshot_id and 'group_snapshot' in expected_attrs: group_snapshot = objects.GroupSnapshot(context) group_snapshot._from_db_object(context, group_snapshot, db_snapshot['group_snapshot']) @@ -230,13 +230,18 @@ class Snapshot(cleanable.CinderCleanableObject, base.CinderObject, self.volume_id) if attrname == 'cgsnapshot': - self.cgsnapshot = objects.CGSnapshot.get_by_id(self._context, - self.cgsnapshot_id) - + if self.cgsnapshot_id is None: + self.cgsnapshot = None + else: + self.cgsnapshot = objects.CGSnapshot.get_by_id( + self._context, self.cgsnapshot_id) if attrname == 'group_snapshot': - self.group_snapshot = objects.GroupSnapshot.get_by_id( - self._context, - self.group_snapshot_id) + if self.group_snapshot_id is None: + self.group_snapshot = None + else: + self.group_snapshot = objects.GroupSnapshot.get_by_id( + self._context, + self.group_snapshot_id) self.obj_reset_changes(fields=[attrname]) diff --git a/cinder/objects/volume.py b/cinder/objects/volume.py index 836610c581e..1525a9e12be 100644 --- a/cinder/objects/volume.py +++ b/cinder/objects/volume.py @@ -281,7 +281,7 @@ class Volume(cleanable.CinderCleanableObject, base.CinderObject, objects.VolumeAttachment, db_volume.get('volume_attachment')) volume.volume_attachment = attachments - if 'consistencygroup' in expected_attrs: + if volume.consistencygroup_id and 'consistencygroup' in expected_attrs: consistencygroup = objects.ConsistencyGroup(context) consistencygroup._from_db_object(context, consistencygroup, @@ -303,7 +303,7 @@ class Volume(cleanable.CinderCleanableObject, base.CinderObject, db_cluster) else: volume.cluster = None - if 'group' in expected_attrs: + if volume.group_id and 'group' in expected_attrs: group = objects.Group(context) group._from_db_object(context, group, @@ -423,9 +423,12 @@ class Volume(cleanable.CinderCleanableObject, base.CinderObject, self._context, self.id) self.volume_attachment = attachments elif attrname == 'consistencygroup': - consistencygroup = objects.ConsistencyGroup.get_by_id( - self._context, self.consistencygroup_id) - self.consistencygroup = consistencygroup + if self.consistencygroup_id is None: + self.consistencygroup = None + else: + consistencygroup = objects.ConsistencyGroup.get_by_id( + self._context, self.consistencygroup_id) + self.consistencygroup = consistencygroup elif attrname == 'snapshots': self.snapshots = objects.SnapshotList.get_all_for_volume( self._context, self.id) @@ -438,9 +441,12 @@ class Volume(cleanable.CinderCleanableObject, base.CinderObject, else: self.cluster = None elif attrname == 'group': - group = objects.Group.get_by_id( - self._context, self.group_id) - self.group = group + if self.group_id is None: + self.group = None + else: + group = objects.Group.get_by_id( + self._context, self.group_id) + self.group = group self.obj_reset_changes(fields=[attrname]) diff --git a/cinder/tests/unit/objects/test_snapshot.py b/cinder/tests/unit/objects/test_snapshot.py index 5fc33ca61ba..5df9bccaf69 100644 --- a/cinder/tests/unit/objects/test_snapshot.py +++ b/cinder/tests/unit/objects/test_snapshot.py @@ -173,6 +173,24 @@ class TestSnapshot(test_objects.BaseObjectsTestCase): cgsnapshot_get_by_id.assert_called_once_with(self.context, snapshot.cgsnapshot_id) + @mock.patch('cinder.objects.cgsnapshot.CGSnapshot.get_by_id') + def test_obj_load_attr_cgroup_not_exist(self, cgsnapshot_get_by_id): + fake_non_cg_db_snapshot = fake_snapshot.fake_db_snapshot( + cgsnapshot_id=None) + snapshot = objects.Snapshot._from_db_object( + self.context, objects.Snapshot(), fake_non_cg_db_snapshot) + self.assertIsNone(snapshot.cgsnapshot) + cgsnapshot_get_by_id.assert_not_called() + + @mock.patch('cinder.objects.group_snapshot.GroupSnapshot.get_by_id') + def test_obj_load_attr_group_not_exist(self, group_snapshot_get_by_id): + fake_non_cg_db_snapshot = fake_snapshot.fake_db_snapshot( + group_snapshot_id=None) + snapshot = objects.Snapshot._from_db_object( + self.context, objects.Snapshot(), fake_non_cg_db_snapshot) + self.assertIsNone(snapshot.group_snapshot) + group_snapshot_get_by_id.assert_not_called() + @mock.patch('cinder.db.snapshot_data_get_for_project') def test_snapshot_data_get_for_project(self, snapshot_data_get): snapshot = objects.Snapshot._from_db_object( diff --git a/cinder/tests/unit/objects/test_volume.py b/cinder/tests/unit/objects/test_volume.py index 1190a3a4d3b..c229da32b73 100644 --- a/cinder/tests/unit/objects/test_volume.py +++ b/cinder/tests/unit/objects/test_volume.py @@ -192,8 +192,10 @@ class TestVolume(test_objects.BaseObjectsTestCase): mock_va_get_all_by_vol, mock_vt_get_by_id, mock_admin_metadata_get, mock_glance_metadata_get, mock_metadata_get): + fake_db_volume = fake_volume.fake_db_volume( + consistencygroup_id=fake.CONSISTENCY_GROUP_ID) volume = objects.Volume._from_db_object( - self.context, objects.Volume(), fake_volume.fake_db_volume()) + self.context, objects.Volume(), fake_db_volume) # Test metadata lazy-loaded field metadata = {'foo': 'bar'} @@ -264,6 +266,24 @@ class TestVolume(test_objects.BaseObjectsTestCase): self.assertEqual(adm_metadata, volume.admin_metadata) mock_admin_metadata_get.assert_called_once_with(adm_context, volume.id) + @mock.patch('cinder.objects.consistencygroup.ConsistencyGroup.get_by_id') + def test_obj_load_attr_cgroup_not_exist(self, mock_cg_get_by_id): + fake_db_volume = fake_volume.fake_db_volume(consistencygroup_id=None) + volume = objects.Volume._from_db_object( + self.context, objects.Volume(), fake_db_volume) + + self.assertIsNone(volume.consistencygroup) + mock_cg_get_by_id.assert_not_called() + + @mock.patch('cinder.objects.group.Group.get_by_id') + def test_obj_load_attr_group_not_exist(self, mock_group_get_by_id): + fake_db_volume = fake_volume.fake_db_volume(group_id=None) + volume = objects.Volume._from_db_object( + self.context, objects.Volume(), fake_db_volume) + + self.assertIsNone(volume.group) + mock_group_get_by_id.assert_not_called() + def test_from_db_object_with_all_expected_attributes(self): expected_attrs = ['metadata', 'admin_metadata', 'glance_metadata', 'volume_type', 'volume_attachment',