From 1c91180de97135266b8d55e827bf0075eded8e5f Mon Sep 17 00:00:00 2001 From: wanghao Date: Fri, 20 Mar 2015 15:51:57 +0800 Subject: [PATCH] Add support for force-delete backups 1. Add _get and _delete method to BackupAdminController. 2. Add force=False arg to delete method in backup api. 3. If force=True, then allow the deletion of backup, which can be in any state. 4. For chunkeddriver based backup, check the status of backup from DB in backup loop, if it has changed to deleting, set the flag is_backup_canceled and clear the data to avoid leaving orphan object in the object store. 5. Add the flag: support_force_delete=False in BackupDriver. That indicates if the backup driver supports force deletion. It should be set to True if the driver that inherits from BackupDriver supports the force deletion function. 6. If the backup driver doesn't support this feature, and it will return 405 when calling API of force delete. DocImpact APIImpact Add os-force_delete in request body: *POST /v2/{tenant_id}/backups/{id}/action { "os-force_delete": {} } Implements: blueprint support-force-delete-backup Change-Id: Ia4d36520d2a092730cd1be56e338349e383f9e6b --- cinder/api/contrib/admin_actions.py | 6 +++ cinder/api/contrib/backups.py | 5 +- cinder/backup/api.py | 26 +++++++-- cinder/backup/chunkeddriver.py | 17 ++++++ cinder/backup/driver.py | 4 ++ cinder/backup/manager.py | 8 +++ cinder/backup/rpcapi.py | 6 +++ cinder/exception.py | 5 ++ .../unit/api/contrib/test_admin_actions.py | 54 +++++++++++++++++++ cinder/tests/unit/api/contrib/test_backups.py | 9 ++++ cinder/tests/unit/policy.json | 1 + cinder/tests/unit/test_backup.py | 19 +++++++ etc/cinder/policy.json | 1 + 13 files changed, 154 insertions(+), 7 deletions(-) diff --git a/cinder/api/contrib/admin_actions.py b/cinder/api/contrib/admin_actions.py index 9e6cb061391..b0f809ecba4 100644 --- a/cinder/api/contrib/admin_actions.py +++ b/cinder/api/contrib/admin_actions.py @@ -285,6 +285,12 @@ class BackupAdminController(AdminController): 'error' ]) + def _get(self, *args, **kwargs): + return self.backup_api.get(*args, **kwargs) + + def _delete(self, *args, **kwargs): + return self.backup_api.delete(*args, **kwargs) + @wsgi.action('os-reset_status') def _reset_status(self, req, id, body): """Reset status on the resource.""" diff --git a/cinder/api/contrib/backups.py b/cinder/api/contrib/backups.py index ee366f64ebb..d327fdb39bf 100644 --- a/cinder/api/contrib/backups.py +++ b/cinder/api/contrib/backups.py @@ -175,13 +175,14 @@ class BackupsController(wsgi.Controller): def delete(self, req, id): """Delete a backup.""" - LOG.debug('delete called for member %s', id) + LOG.debug('Delete called for member %s.', id) context = req.environ['cinder.context'] LOG.info(_LI('Delete backup with id: %s'), id, context=context) try: - self.backup_api.delete(context, id) + backup = self.backup_api.get(context, id) + self.backup_api.delete(context, backup) except exception.BackupNotFound as error: raise exc.HTTPNotFound(explanation=error.msg) except exception.InvalidBackup as error: diff --git a/cinder/backup/api.py b/cinder/backup/api.py index 10b1fcbeb7b..afb0fcc2f29 100644 --- a/cinder/backup/api.py +++ b/cinder/backup/api.py @@ -63,17 +63,33 @@ class API(base.Base): check_policy(context, 'get') return objects.Backup.get_by_id(context, backup_id) - def delete(self, context, backup_id): - """Make the RPC call to delete a volume backup.""" + def _check_support_to_force_delete(self, context, backup_host): + result = self.backup_rpcapi.check_support_to_force_delete(context, + backup_host) + return result + + def delete(self, context, backup, force=False): + """Make the RPC call to delete a volume backup. + + Call backup manager to execute backup delete or force delete operation. + :param context: running context + :param backup: the dict of backup that is got from DB. + :param force: indicate force delete or not + :raises: InvalidBackup + :raises: BackupDriverException + """ check_policy(context, 'delete') - backup = self.get(context, backup_id) - if backup['status'] not in ['available', 'error']: + if not force and backup.status not in ['available', 'error']: msg = _('Backup status must be available or error') raise exception.InvalidBackup(reason=msg) + if force and not self._check_support_to_force_delete(context, + backup.host): + msg = _('force delete') + raise exception.NotSupportedOperation(operation=msg) # Don't allow backup to be deleted if there are incremental # backups dependent on it. - deltas = self.get_all(context, {'parent_id': backup['id']}) + deltas = self.get_all(context, {'parent_id': backup.id}) if deltas and len(deltas): msg = _('Incremental backups exist for this backup.') raise exception.InvalidBackup(reason=msg) diff --git a/cinder/backup/chunkeddriver.py b/cinder/backup/chunkeddriver.py index 567590d9508..e7e5f40164b 100644 --- a/cinder/backup/chunkeddriver.py +++ b/cinder/backup/chunkeddriver.py @@ -98,6 +98,7 @@ class ChunkedBackupDriver(driver.BackupDriver): self.backup_compression_algorithm = CONF.backup_compression_algorithm self.compressor = \ self._get_compressor(CONF.backup_compression_algorithm) + self.support_force_delete = True # To create your own "chunked" backup driver, implement the following # abstract methods. @@ -457,7 +458,19 @@ class ChunkedBackupDriver(driver.BackupDriver): sha256_list = object_sha256['sha256s'] shaindex = 0 + is_backup_canceled = False while True: + # 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 = objects.Backup.get_by_id(self.context, backup.id) + if 'deleting' == backup.status or 'deleted' == backup.status: + is_backup_canceled = True + # To avoid the chunk left when deletion complete, need to + # clean up the object of chunk again. + self.delete(backup) + LOG.debug('Cancel the backup process of %s.', backup.id) + break data_offset = volume_file.tell() data = volume_file.read(self.chunk_size_bytes) if data == b'': @@ -528,6 +541,10 @@ class ChunkedBackupDriver(driver.BackupDriver): # Stop the timer. timer.stop() + # If backup has been cancelled we have nothing more to do + # but timer.stop(). + if is_backup_canceled: + return # All the data have been sent, the backup_percent reaches 100. self._send_progress_end(self.context, backup, object_meta) diff --git a/cinder/backup/driver.py b/cinder/backup/driver.py index 622e2ef4a52..ebbc25c8142 100644 --- a/cinder/backup/driver.py +++ b/cinder/backup/driver.py @@ -324,6 +324,10 @@ class BackupDriver(base.Base): super(BackupDriver, self).__init__(db_driver) self.context = context self.backup_meta_api = BackupMetadataAPI(context, db_driver) + # This flag indicates if backup driver supports force + # deletion. So it should be set to True if the driver that inherits + # from BackupDriver supports the force deletion function. + self.support_force_delete = False def get_metadata(self, volume_id): return self.backup_meta_api.get(volume_id) diff --git a/cinder/backup/manager.py b/cinder/backup/manager.py index 381905d6b3a..dd7fb6dc58e 100644 --- a/cinder/backup/manager.py +++ b/cinder/backup/manager.py @@ -706,3 +706,11 @@ class BackupManager(manager.SchedulerDependentManager): notifier = rpc.get_notifier('backupStatusUpdate') notifier.info(context, "backups.reset_status.end", notifier_info) + + def check_support_to_force_delete(self, context): + """Check if the backup driver supports force delete operation. + + :param context: running context + """ + backup_service = self.service.get_backup_driver(context) + return backup_service.support_force_delete diff --git a/cinder/backup/rpcapi.py b/cinder/backup/rpcapi.py index 75575bc6213..f6909cc12bf 100644 --- a/cinder/backup/rpcapi.py +++ b/cinder/backup/rpcapi.py @@ -99,3 +99,9 @@ class BackupAPI(object): 'host': backup.host}) cctxt = self.client.prepare(server=backup.host) return cctxt.cast(ctxt, 'reset_status', backup=backup, status=status) + + def check_support_to_force_delete(self, ctxt, host): + LOG.debug("Check if backup driver supports force delete " + "on host %(host)s.", {'host': host}) + cctxt = self.client.prepare(server=host) + return cctxt.call(ctxt, 'check_support_to_force_delete') diff --git a/cinder/exception.py b/cinder/exception.py index 56aa9a89d15..b84c3d990fe 100644 --- a/cinder/exception.py +++ b/cinder/exception.py @@ -966,3 +966,8 @@ class DotHillNotTargetPortal(CinderException): class MetadataAbsent(CinderException): message = _("There is no metadata in DB object.") + + +class NotSupportedOperation(Invalid): + message = _("Operation not supported: %(operation)s.") + code = 405 diff --git a/cinder/tests/unit/api/contrib/test_admin_actions.py b/cinder/tests/unit/api/contrib/test_admin_actions.py index 56c96d9c3bb..4441462b661 100644 --- a/cinder/tests/unit/api/contrib/test_admin_actions.py +++ b/cinder/tests/unit/api/contrib/test_admin_actions.py @@ -30,6 +30,7 @@ from cinder import db from cinder import exception from cinder import objects from cinder import test +from cinder.tests.unit.api.contrib import test_backups from cinder.tests.unit.api import fakes from cinder.tests.unit.api.v2 import stubs from cinder.tests.unit import cast_as_call @@ -953,3 +954,56 @@ class AdminActionsTest(test.TestCase): self.assertRaises(exc.HTTPBadRequest, vac.validate_update, {'status': 'creating'}) + + @mock.patch('cinder.backup.api.API._check_support_to_force_delete') + def _force_delete_backup_util(self, test_status, mock_check_support): + # admin context + ctx = context.RequestContext('admin', 'fake', True) + mock_check_support.return_value = True + # current status is dependent on argument: test_status. + id = test_backups.BackupsAPITestCase._create_backup(status=test_status) + req = webob.Request.blank('/v2/fake/backups/%s/action' % id) + req.method = 'POST' + req.headers['Content-Type'] = 'application/json' + req.body = jsonutils.dumps({'os-force_delete': {}}) + req.environ['cinder.context'] = ctx + res = req.get_response(app()) + + self.assertEqual(202, res.status_int) + self.assertEqual('deleting', + test_backups.BackupsAPITestCase. + _get_backup_attrib(id, 'status')) + db.backup_destroy(context.get_admin_context(), id) + + def test_delete_backup_force_when_creating(self): + self._force_delete_backup_util('creating') + + def test_delete_backup_force_when_deleting(self): + self._force_delete_backup_util('deleting') + + def test_delete_backup_force_when_restoring(self): + self._force_delete_backup_util('restoring') + + def test_delete_backup_force_when_available(self): + self._force_delete_backup_util('available') + + def test_delete_backup_force_when_error(self): + self._force_delete_backup_util('error') + + def test_delete_backup_force_when_error_deleting(self): + self._force_delete_backup_util('error_deleting') + + @mock.patch('cinder.backup.rpcapi.BackupAPI.check_support_to_force_delete', + return_value=False) + def test_delete_backup_force_when_not_supported(self, mock_check_support): + # admin context + ctx = context.RequestContext('admin', 'fake', True) + self.override_config('backup_driver', 'cinder.backup.drivers.ceph') + id = test_backups.BackupsAPITestCase._create_backup() + req = webob.Request.blank('/v2/fake/backups/%s/action' % id) + req.method = 'POST' + req.headers['Content-Type'] = 'application/json' + req.body = jsonutils.dumps({'os-force_delete': {}}) + req.environ['cinder.context'] = ctx + res = req.get_response(app()) + self.assertEqual(405, res.status_int) diff --git a/cinder/tests/unit/api/contrib/test_backups.py b/cinder/tests/unit/api/contrib/test_backups.py index 623fcd2f91c..f410997ecf1 100644 --- a/cinder/tests/unit/api/contrib/test_backups.py +++ b/cinder/tests/unit/api/contrib/test_backups.py @@ -1504,3 +1504,12 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual("Missing required element 'backup-record' in " "request body.", res_dict['badRequest']['message']) + + @mock.patch('cinder.backup.rpcapi.BackupAPI.check_support_to_force_delete', + return_value=False) + def test_force_delete_with_not_supported_operation(self, + mock_check_support): + backup_id = self._create_backup(status='available') + backup = self.backup_api.get(self.context, backup_id) + self.assertRaises(exception.NotSupportedOperation, + self.backup_api.delete, self.context, backup, True) diff --git a/cinder/tests/unit/policy.json b/cinder/tests/unit/policy.json index 4d54a74d119..1e3722f2d6b 100644 --- a/cinder/tests/unit/policy.json +++ b/cinder/tests/unit/policy.json @@ -37,6 +37,7 @@ "volume_extension:volume_admin_actions:reset_status": "rule:admin_api", "volume_extension:snapshot_admin_actions:reset_status": "rule:admin_api", "volume_extension:backup_admin_actions:reset_status": "rule:admin_api", + "volume_extension:backup_admin_actions:force_delete": "rule:admin_api", "volume_extension:volume_admin_actions:force_delete": "rule:admin_api", "volume_extension:snapshot_admin_actions:force_delete": "rule:admin_api", "volume_extension:volume_admin_actions:force_detach": "rule:admin_api", diff --git a/cinder/tests/unit/test_backup.py b/cinder/tests/unit/test_backup.py index 1d0da26fe42..d49a6a075bd 100644 --- a/cinder/tests/unit/test_backup.py +++ b/cinder/tests/unit/test_backup.py @@ -595,6 +595,25 @@ class BackupTestCase(BaseBackupTest): backup = db.backup_get(self.ctxt, imported_record.id) self.assertEqual(backup['status'], 'error') + def test_not_supported_driver_to_force_delete(self): + """Test force delete check method for not supported drivers.""" + self.override_config('backup_driver', 'cinder.backup.drivers.ceph') + self.backup_mgr = importutils.import_object(CONF.backup_manager) + result = self.backup_mgr.check_support_to_force_delete(self.ctxt) + self.assertFalse(result) + + @mock.patch('cinder.backup.drivers.nfs.NFSBackupDriver.' + '_init_backup_repo_path', return_value=None) + @mock.patch('cinder.backup.drivers.nfs.NFSBackupDriver.' + '_check_configuration', return_value=None) + def test_check_support_to_force_delete(self, mock_check_configuration, + mock_init_backup_repo_path): + """Test force delete check method for supported drivers.""" + self.override_config('backup_driver', 'cinder.backup.drivers.nfs') + self.backup_mgr = importutils.import_object(CONF.backup_manager) + result = self.backup_mgr.check_support_to_force_delete(self.ctxt) + self.assertTrue(result) + class BackupTestCaseWithVerify(BaseBackupTest): """Test Case for backups.""" diff --git a/etc/cinder/policy.json b/etc/cinder/policy.json index 42d157b2aa9..79581d0c690 100644 --- a/etc/cinder/policy.json +++ b/etc/cinder/policy.json @@ -40,6 +40,7 @@ "volume_extension:volume_admin_actions:force_delete": "rule:admin_api", "volume_extension:volume_admin_actions:force_detach": "rule:admin_api", "volume_extension:snapshot_admin_actions:force_delete": "rule:admin_api", + "volume_extension:backup_admin_actions:force_delete": "rule:admin_api", "volume_extension:volume_admin_actions:migrate_volume": "rule:admin_api", "volume_extension:volume_admin_actions:migrate_volume_completion": "rule:admin_api",