From 314d4ec1f442919fd46e407431a11715ca226c4a Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Wed, 10 Feb 2016 20:52:37 +0000 Subject: [PATCH] Conditionally restore display_name When restoring a volume from backup, if the target volume looks like it was auto-created as part of the restore and the backed up metadata contains a name, then apply the name to the target volume. However, if we are restoring to an existing volume and it has a non-default name or we do not have a name to restore, then do not modify the target volume metadata display_name or display_description. This is intended to be a less drastic solution than commit 7ee80f7 introduced back in Juno. Change-Id: I87a62577665b5bd2a82f3305fb715ba6b24d9a78 Closes-Bug: 1539667 --- cinder/backup/driver.py | 37 +++++++++++-- cinder/tests/unit/test_backup_driver_base.py | 58 ++++++++++++++++++-- 2 files changed, 86 insertions(+), 9 deletions(-) diff --git a/cinder/backup/driver.py b/cinder/backup/driver.py index df9e43634..81cb37440 100644 --- a/cinder/backup/driver.py +++ b/cinder/backup/driver.py @@ -145,17 +145,25 @@ class BackupMetadataAPI(base.Base): LOG.debug("No metadata type '%s' available", type_tag) @staticmethod - def _filter(metadata, fields): + def _filter(metadata, fields, excludes=None): """Returns set of metadata restricted to required fields. If fields is empty list, the full set is returned. + + :param metadata: master set of metadata + :param fields: list of fields we want to extract + :param excludes: fields to be exluded + :returns: filtered metadata """ if not fields: return metadata + if not excludes: + excludes = [] + subset = {} for field in fields: - if field in metadata: + if field in metadata and field not in excludes: subset[field] = metadata[field] else: LOG.debug("Excluding field '%s'", field) @@ -165,6 +173,7 @@ class BackupMetadataAPI(base.Base): def _restore_vol_base_meta(self, metadata, volume_id, fields): """Restore values to Volume object for provided fields.""" LOG.debug("Restoring volume base metadata") + excludes = [] # Ignore unencrypted backups. key = 'encryption_key_id' @@ -172,7 +181,22 @@ class BackupMetadataAPI(base.Base): self._restore_vol_encryption_meta(volume_id, metadata['volume_type_id']) - metadata = self._filter(metadata, fields) + # NOTE(dosaboy): if the target volume looks like it was auto-created + # as part of this restore operation and we have a name to restore + # then apply the name to the target volume. However, if that target + # volume already existed and it has a name or we do not have a name to + # restore, then ignore this key. This is intended to be a less drastic + # solution than commit 7ee80f7. + key = 'display_name' + if key in fields and key in metadata: + target_vol = self.db.volume_get(self.context, volume_id) + name = target_vol.get(key, '') + if (not metadata.get(key) or name and + not name.startswith('restore_backup_')): + excludes.append(key) + excludes.append('display_description') + + metadata = self._filter(metadata, fields, excludes=excludes) self.db.volume_update(self.context, volume_id, metadata) def _restore_vol_encryption_meta(self, volume_id, src_volume_type_id): @@ -252,7 +276,10 @@ class BackupMetadataAPI(base.Base): Empty field list indicates that all backed up fields should be restored. """ - return {self.TYPE_TAG_VOL_META: + return {self.TYPE_TAG_VOL_BASE_META: + (self._restore_vol_base_meta, + ['display_name', 'display_description']), + self.TYPE_TAG_VOL_META: (self._restore_vol_meta, []), self.TYPE_TAG_VOL_GLANCE_META: (self._restore_vol_glance_meta, [])} @@ -269,7 +296,7 @@ class BackupMetadataAPI(base.Base): """ return {self.TYPE_TAG_VOL_BASE_META: (self._restore_vol_base_meta, - ['encryption_key_id']), + ['display_name', 'display_description', 'encryption_key_id']), self.TYPE_TAG_VOL_META: (self._restore_vol_meta, []), self.TYPE_TAG_VOL_GLANCE_META: diff --git a/cinder/tests/unit/test_backup_driver_base.py b/cinder/tests/unit/test_backup_driver_base.py index 23cad6606..e3dfe7e1b 100644 --- a/cinder/tests/unit/test_backup_driver_base.py +++ b/cinder/tests/unit/test_backup_driver_base.py @@ -26,6 +26,7 @@ from cinder import exception from cinder import objects from cinder import test from cinder.tests.unit.backup import fake_service +from cinder.volume import volume_types _backup_db_fields = ['id', 'user_id', 'project_id', 'volume_id', 'host', 'availability_zone', @@ -96,6 +97,7 @@ class BackupMetadataAPITestCase(test.TestCase): super(BackupMetadataAPITestCase, self).setUp() self.ctxt = context.get_admin_context() self.volume_id = str(uuid.uuid4()) + self.backup_id = str(uuid.uuid4()) self.volume_display_name = 'vol-1' self.volume_display_description = 'test vol' self._create_volume_db_entry(self.volume_id, 1, @@ -162,15 +164,46 @@ class BackupMetadataAPITestCase(test.TestCase): def test_v1_restore_factory(self): fact = self.bak_meta_api._v1_restore_factory() - keys = [self.bak_meta_api.TYPE_TAG_VOL_META, + keys = [self.bak_meta_api.TYPE_TAG_VOL_BASE_META, + self.bak_meta_api.TYPE_TAG_VOL_META, self.bak_meta_api.TYPE_TAG_VOL_GLANCE_META] self.assertEqual(set([]), set(keys).symmetric_difference(set(fact.keys()))) meta_container = {self.bak_meta_api.TYPE_TAG_VOL_BASE_META: - {'display_name': 'vol-2', - 'display_description': 'description'}, + {'display_name': 'my-backed-up-volume', + 'display_description': 'backed up description'}, + self.bak_meta_api.TYPE_TAG_VOL_META: {}, + self.bak_meta_api.TYPE_TAG_VOL_GLANCE_META: {}} + + # Emulate restore to new volume + volume_id = str(uuid.uuid4()) + vol_name = 'restore_backup_%s' % (self.backup_id) + self._create_volume_db_entry(volume_id, 1, vol_name, 'fake volume') + + for f in fact: + func = fact[f][0] + fields = fact[f][1] + func(meta_container[f], volume_id, fields) + + vol = db.volume_get(self.ctxt, volume_id) + self.assertEqual('my-backed-up-volume', vol['display_name']) + self.assertEqual('backed up description', vol['display_description']) + + def test_v1_restore_factory_no_restore_name(self): + fact = self.bak_meta_api._v1_restore_factory() + + keys = [self.bak_meta_api.TYPE_TAG_VOL_BASE_META, + self.bak_meta_api.TYPE_TAG_VOL_META, + self.bak_meta_api.TYPE_TAG_VOL_GLANCE_META] + + self.assertEqual(set([]), + set(keys).symmetric_difference(set(fact.keys()))) + + meta_container = {self.bak_meta_api.TYPE_TAG_VOL_BASE_META: + {'display_name': 'my-backed-up-volume', + 'display_description': 'backed up description'}, self.bak_meta_api.TYPE_TAG_VOL_META: {}, self.bak_meta_api.TYPE_TAG_VOL_GLANCE_META: {}} for f in fact: @@ -193,10 +226,27 @@ class BackupMetadataAPITestCase(test.TestCase): self.assertEqual(set([]), set(keys).symmetric_difference(set(fact.keys()))) + volume_types.create(self.ctxt, 'faketype') + vol_type = volume_types.get_volume_type_by_name(self.ctxt, 'faketype') + + meta_container = {self.bak_meta_api.TYPE_TAG_VOL_BASE_META: + {'encryption_key_id': '123', + 'volume_type_id': vol_type.get('id'), + 'display_name': 'vol-2', + 'display_description': 'description'}, + self.bak_meta_api.TYPE_TAG_VOL_META: {}, + self.bak_meta_api.TYPE_TAG_VOL_GLANCE_META: {}} + for f in fact: func = fact[f][0] fields = fact[f][1] - func({}, self.volume_id, fields) + func(meta_container[f], self.volume_id, fields) + + vol = db.volume_get(self.ctxt, self.volume_id) + self.assertEqual(self.volume_display_name, vol['display_name']) + self.assertEqual(self.volume_display_description, + vol['display_description']) + self.assertEqual('123', vol['encryption_key_id']) def test_restore_vol_glance_meta(self): # Fields is an empty list for _restore_vol_glance_meta method.