From bcea359d54fcaa5a013204f7981f952e4329cf93 Mon Sep 17 00:00:00 2001 From: Sofia Enriquez Date: Thu, 20 Feb 2020 16:25:50 +0000 Subject: [PATCH] Cinder backup export broken Incremental patch I516b7c82b05b26e81195f7f106d43a9e0804082d introduced a regression, breaking the volume backup export functionality. Apparently, the newly introduced "parent" db field isn't serialized properly. This is the traceback from the launchpad bug: oslo_versionedobjects.exception.ObjectActionError: Object action obj_load_attr failed because: attribute parent not lazy-loadable Looking at the traceback, and the Backup OVO code it seems like we have 2 problems: - We don't support lazy loading the parent - We try to serialize the parent Co-Authored-By: Gorka Eguileor Change-Id: I017602353e96cf9f0922074f94276002b17d1359 Closes-Bug: #1862635 (cherry picked from commit a98969380ac2172766995cf02b889e3c863511ba) modified: cinder/tests/unit/objects/test_backup.py - pyflake issue #202 (cherry picked from commit 6dfbec7f7403bbbe498c5eaba16b5b68bd7d698c) (cherry picked from commit 196b6786db82de5afcaed2790395d869a7c2c183) --- cinder/objects/backup.py | 9 +++++-- cinder/tests/unit/objects/test_backup.py | 34 +++++++++++++++++++++++- 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/cinder/objects/backup.py b/cinder/objects/backup.py index 514a25315e2..f2cadfc860d 100644 --- a/cinder/objects/backup.py +++ b/cinder/objects/backup.py @@ -43,7 +43,7 @@ class Backup(base.CinderPersistentObject, base.CinderObject, # Version 1.7: Add parent VERSION = '1.7' - OPTIONAL_FIELDS = ('metadata',) + OPTIONAL_FIELDS = ('metadata', 'parent') fields = { 'id': fields.UUIDField(), @@ -156,6 +156,11 @@ def obj_load_attr(self, attrname): if not self._context: raise exception.OrphanedObjectError(method='obj_load_attr', objtype=self.obj_name()) + if attrname == 'parent': + if self.parent_id: + self.parent = self.get_by_id(self._context, self.parent_id) + else: + self.parent = None self.obj_reset_changes(fields=[attrname]) def obj_what_changed(self): @@ -212,7 +217,7 @@ def encode_record(self, **kwargs): # We don't want to export extra fields and we want to force lazy # loading, so we can't use dict(self) or self.obj_to_primitive record = {name: field.to_primitive(self, name, getattr(self, name)) - for name, field in self.fields.items()} + for name, field in self.fields.items() if name != 'parent'} # We must update kwargs instead of record to ensure we don't overwrite # "real" data from the backup kwargs.update(record) diff --git a/cinder/tests/unit/objects/test_backup.py b/cinder/tests/unit/objects/test_backup.py index 9f4e9027fd5..0fb756ef17d 100644 --- a/cinder/tests/unit/objects/test_backup.py +++ b/cinder/tests/unit/objects/test_backup.py @@ -143,6 +143,20 @@ def test_obj_field_metadata(self): metadata={'test_key': 'test_value'}) self.assertEqual({'test_key': 'test_value'}, backup.metadata) + @mock.patch('cinder.objects.backup.Backup.get_by_id', + return_value=None) + def test_obj_field_parent(self, mock_lzy_ld): + backup = objects.Backup(context=self.context, + parent_id=None) + self.assertIsNone(backup.parent) + + # Bug #1862635: should trigger a lazy load + backup = objects.Backup(context=self.context, + parent_id=fake.UUID5) + # need noqa here because of pyflakes issue #202 + _ = backup.parent # noqa + mock_lzy_ld.assert_called_once() + def test_import_record(self): utils.replace_obj_loader(self, objects.Backup) backup = objects.Backup(context=self.context, id=fake.BACKUP_ID, @@ -154,6 +168,24 @@ def test_import_record(self): # Make sure we don't lose data when converting from string self.assertDictEqual(self._expected_backup(backup), imported_backup) + @mock.patch('cinder.db.get_by_id', return_value=fake_backup) + def test_import_record_w_parent(self, backup_get): + full_backup = objects.Backup.get_by_id(self.context, fake.USER_ID) + self._compare(self, fake_backup, full_backup) + + utils.replace_obj_loader(self, objects.Backup) + incr_backup = objects.Backup(context=self.context, + id=fake.BACKUP2_ID, + parent=full_backup, + parent_id=full_backup['id'], + num_dependent_backups=0) + export_string = incr_backup.encode_record() + imported_backup = objects.Backup.decode_record(export_string) + + # Make sure we don't lose data when converting from string + self.assertDictEqual(self._expected_backup(incr_backup), + imported_backup) + def test_import_record_additional_info(self): utils.replace_obj_loader(self, objects.Backup) backup = objects.Backup(context=self.context, id=fake.BACKUP_ID, @@ -175,7 +207,7 @@ def test_import_record_additional_info(self): def _expected_backup(self, backup): record = {name: field.to_primitive(backup, name, getattr(backup, name)) - for name, field in backup.fields.items()} + for name, field in backup.fields.items() if name != 'parent'} return record def test_import_record_additional_info_cant_overwrite(self):