diff --git a/cinder/backup/chunkeddriver.py b/cinder/backup/chunkeddriver.py index 751e0024a8f..685c5386411 100644 --- a/cinder/backup/chunkeddriver.py +++ b/cinder/backup/chunkeddriver.py @@ -559,7 +559,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 f6791ef7d30..b935f72c488 100644 --- a/cinder/backup/manager.py +++ b/cinder/backup/manager.py @@ -405,21 +405,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): @@ -471,7 +479,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 428129a1613..10a1c058cca 100644 --- a/cinder/objects/base.py +++ b/cinder/objects/base.py @@ -298,6 +298,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 3f05fd0a9a2..5d1a887b01d 100644 --- a/cinder/tests/unit/backup/drivers/test_backup_nfs.py +++ b/cinder/tests/unit/backup/drivers/test_backup_nfs.py @@ -250,6 +250,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 38ea0482469..a3b559fe651 100644 --- a/cinder/tests/unit/backup/test_backup.py +++ b/cinder/tests/unit/backup/test_backup.py @@ -588,6 +588,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.