From 4ff9e63707e2c4cf5869f28e3e86fd0606d2db9a Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Wed, 31 Jan 2018 18:45:47 +0100 Subject: [PATCH] Fix leftovers after backup abort When aborting a backup on any chunked driver we will be leaving chunks in the backend without Cinder knowing so and with no way of deleting them from Cinder. In this case the only way to delete them is going to the storage itself and deleting them manually. Another issue that will happen if we are using a temporary resource for the backup, be it a volume or a snapshot, is that it will not be cleaned up and will be left for us to manually issue the delete through the Cinder API. The first issue is caused by the chunked driver's assumption that the `refresh` method in an OVO will ignore the context's `read_deleted` configuration and always read the record, which is not true. And since it doesn't work when the record is deleted there will be leftovers if the status of the backup transitions to deleted during the processing of a chunk. The second issue is caused by the same thing, but in this case is when the backup manager refreshes the backup OVO to know the temporary resource it needs to clean up. This patches fixes the incorrect behavior of the backup abort mechanism to prevent leaving things behind. Closes-Bug: #1746559 Change-Id: Idcfdbf815f404982d26618710a291054f19be736 --- cinder/backup/chunkeddriver.py | 3 +- cinder/backup/manager.py | 37 ++++++++++++------- cinder/objects/base.py | 28 ++++++++++++++ .../unit/backup/drivers/test_backup_nfs.py | 32 ++++++++++++++++ cinder/tests/unit/backup/test_backup.py | 25 +++++++++++++ cinder/tests/unit/objects/test_base.py | 16 ++++++++ .../fix-abort-backup-df196e9dcb992586.yaml | 5 +++ 7 files changed, 131 insertions(+), 15 deletions(-) create mode 100644 releasenotes/notes/fix-abort-backup-df196e9dcb992586.yaml diff --git a/cinder/backup/chunkeddriver.py b/cinder/backup/chunkeddriver.py index a5436154244..6743af8f712 100644 --- a/cinder/backup/chunkeddriver.py +++ b/cinder/backup/chunkeddriver.py @@ -516,7 +516,8 @@ class ChunkedBackupDriver(driver.BackupDriver): # First of all, we check the status of this backup. If it # has been changed to delete or has been deleted, we cancel the # backup process to do forcing delete. - backup.refresh() + with backup.as_read_deleted(): + backup.refresh() if backup.status in (fields.BackupStatus.DELETING, fields.BackupStatus.DELETED): is_backup_canceled = True diff --git a/cinder/backup/manager.py b/cinder/backup/manager.py index 0cb3900b65b..56c6d95e18f 100644 --- a/cinder/backup/manager.py +++ b/cinder/backup/manager.py @@ -394,21 +394,29 @@ class BackupManager(manager.ThreadPoolManager): self.db.volume_update(context, volume_id, {'status': previous_status, 'previous_status': 'backing-up'}) - backup.status = fields.BackupStatus.AVAILABLE - backup.size = volume['size'] - if updates: - backup.update(updates) - backup.save() + # _run_backup method above updated the status for the backup, so it + # will reflect latest status, even if it is deleted + completion_msg = 'finished' + if backup.status in (fields.BackupStatus.DELETING, + fields.BackupStatus.DELETED): + completion_msg = 'aborted' + else: + backup.status = fields.BackupStatus.AVAILABLE + backup.size = volume['size'] - # Handle the num_dependent_backups of parent backup when child backup - # has created successfully. - if backup.parent_id: - parent_backup = objects.Backup.get_by_id(context, - backup.parent_id) - parent_backup.num_dependent_backups += 1 - parent_backup.save() - LOG.info('Create backup finished. backup: %s.', backup.id) + if updates: + backup.update(updates) + backup.save() + + # Handle the num_dependent_backups of parent backup when child + # backup has created successfully. + if backup.parent_id: + parent_backup = objects.Backup.get_by_id(context, + backup.parent_id) + parent_backup.num_dependent_backups += 1 + parent_backup.save() + LOG.info('Create backup %s. backup: %s.', completion_msg, backup.id) self._notify_about_backup_usage(context, backup, "create.end") def _run_backup(self, context, backup, volume): @@ -451,7 +459,8 @@ class BackupManager(manager.ThreadPoolManager): backup_device.is_snapshot, force=True, ignore_errors=True) finally: - backup = objects.Backup.get_by_id(context, backup.id) + with backup.as_read_deleted(): + backup.refresh() self._cleanup_temp_volumes_snapshots_when_backup_created( context, backup) return updates diff --git a/cinder/objects/base.py b/cinder/objects/base.py index 8ae5ff4d9b2..db453774782 100644 --- a/cinder/objects/base.py +++ b/cinder/objects/base.py @@ -297,6 +297,34 @@ class CinderPersistentObject(object): finally: self._context = original_context + @contextlib.contextmanager + def as_read_deleted(self, mode='yes'): + """Context manager to make OVO with modified read deleted context. + + This temporarily modifies the context embedded in an object to + have a different `read_deleted` parameter. + + Parameter mode accepts most of the same parameters as our `model_query` + DB method. We support 'yes', 'no', and 'only'. + + usage: + + with obj.as_read_deleted(): + obj.refresh() + if obj.status = 'deleted': + ... + """ + if self._context is None: + raise exception.OrphanedObjectError(method='as_read_deleted', + objtype=self.obj_name()) + + original_mode = self._context.read_deleted + self._context.read_deleted = mode + try: + yield + finally: + self._context.read_deleted = original_mode + @classmethod def _get_expected_attrs(cls, context, *args, **kwargs): return None diff --git a/cinder/tests/unit/backup/drivers/test_backup_nfs.py b/cinder/tests/unit/backup/drivers/test_backup_nfs.py index 4b29a47edd2..481ced6b4b4 100644 --- a/cinder/tests/unit/backup/drivers/test_backup_nfs.py +++ b/cinder/tests/unit/backup/drivers/test_backup_nfs.py @@ -220,6 +220,38 @@ class BackupNFSSwiftBasedTestCase(test.TestCase): backup = objects.Backup.get_by_id(self.ctxt, FAKE_BACKUP_ID) self.assertEqual(backup['container'], UPDATED_CONTAINER_NAME) + def test_backup_cancel(self): + """Test the backup abort mechanism when backup is force deleted.""" + count = set() + + def my_refresh(): + # This refresh method will abort the backup after 1 chunk + count.add(len(count) + 1) + if len(count) == 2: + backup.destroy() + original_refresh() + + volume_id = fake.VOLUME_ID + self._create_backup_db_entry(volume_id=volume_id, + container=None, + backup_id=FAKE_BACKUP_ID) + service = nfs.NFSBackupDriver(self.ctxt) + self.volume_file.seek(0) + backup = objects.Backup.get_by_id(self.ctxt, FAKE_BACKUP_ID) + original_refresh = backup.refresh + + # We cannot mock refresh method in backup object directly because + # mock will raise AttributeError on context manager exit. + with mock.patch('cinder.objects.base.CinderPersistentObject.refresh', + side_effect=my_refresh), \ + mock.patch.object(service, 'delete_object', + side_effect=service.delete_object) as delete: + # Driver shouldn't raise the NotFound exception + service.backup(backup, self.volume_file) + + # Ensure we called the delete_backup method when abort is detected + self.assertEqual(1, delete.call_count) + @mock.patch('cinder.backup.drivers.posix.PosixBackupDriver.' 'update_container_name', return_value='testcontainer1') diff --git a/cinder/tests/unit/backup/test_backup.py b/cinder/tests/unit/backup/test_backup.py index 21ac6cdd5ee..b528d0587ba 100644 --- a/cinder/tests/unit/backup/test_backup.py +++ b/cinder/tests/unit/backup/test_backup.py @@ -567,6 +567,31 @@ class BackupTestCase(BaseBackupTest): self.assertEqual(fields.BackupStatus.ERROR, backup['status']) self.assertTrue(mock_run_backup.called) + @mock.patch('cinder.backup.manager.BackupManager._run_backup') + def test_create_backup_aborted(self, run_backup_mock): + """Test error handling when abort occurs during backup creation.""" + def my_run_backup(*args, **kwargs): + backup.destroy() + with backup.as_read_deleted(): + original_refresh() + + run_backup_mock.side_effect = my_run_backup + vol_id = self._create_volume_db_entry(size=1) + backup = self._create_backup_db_entry(volume_id=vol_id) + original_refresh = backup.refresh + + self.backup_mgr.create_backup(self.ctxt, backup) + + self.assertTrue(run_backup_mock.called) + + vol = objects.Volume.get_by_id(self.ctxt, vol_id) + self.assertEqual('available', vol.status) + self.assertEqual('backing-up', vol['previous_status']) + # Make sure we didn't set the backup to available after it was deleted + with backup.as_read_deleted(): + backup.refresh() + self.assertEqual(fields.BackupStatus.DELETED, backup.status) + @mock.patch('cinder.backup.manager.BackupManager._run_backup', side_effect=FakeBackupException(str(uuid.uuid4()))) def test_create_backup_with_snapshot_error(self, mock_run_backup): diff --git a/cinder/tests/unit/objects/test_base.py b/cinder/tests/unit/objects/test_base.py index c5d6957f735..e38b2925eb0 100644 --- a/cinder/tests/unit/objects/test_base.py +++ b/cinder/tests/unit/objects/test_base.py @@ -59,6 +59,7 @@ class TestCinderObjectVersionHistory(test_objects.BaseObjectsTestCase): history.add, '1.0', {'Backup': '1.0'}) +@ddt.ddt class TestCinderObject(test_objects.BaseObjectsTestCase): """Tests methods from CinderObject.""" @@ -160,6 +161,21 @@ class TestCinderObject(test_objects.BaseObjectsTestCase): MyTestObject.cinder_ovo_cls_init.assert_called_once_with() + def test_as_read_deleted_default(self): + volume = objects.Volume(context=self.context) + self.assertEqual('no', volume._context.read_deleted) + with volume.as_read_deleted(): + self.assertEqual('yes', volume._context.read_deleted) + self.assertEqual('no', volume._context.read_deleted) + + @ddt.data('yes', 'no', 'only') + def test_as_read_deleted_modes(self, mode): + volume = objects.Volume(context=self.context) + self.assertEqual('no', volume._context.read_deleted) + with volume.as_read_deleted(mode=mode): + self.assertEqual(mode, volume._context.read_deleted) + self.assertEqual('no', volume._context.read_deleted) + class TestCinderComparableObject(test_objects.BaseObjectsTestCase): def test_comparable_objects(self): diff --git a/releasenotes/notes/fix-abort-backup-df196e9dcb992586.yaml b/releasenotes/notes/fix-abort-backup-df196e9dcb992586.yaml new file mode 100644 index 00000000000..e9bb3259f28 --- /dev/null +++ b/releasenotes/notes/fix-abort-backup-df196e9dcb992586.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + We no longer leave orphaned chunks on the backup backend or leave a + temporary volume/snapshot when aborting a backup.