From 4b4fbd35da26c7d697ddf18d3f0487f9ea817224 Mon Sep 17 00:00:00 2001 From: TommyLike Date: Thu, 26 Jul 2018 11:08:15 +0800 Subject: [PATCH] Consume quota when importing backup resource Now cinder will validate&consume backup quotas when importing new backup resource. Change-Id: I1825600dc6e01499c8fc4ede05400a180672e130 Closes-Bug:#1783525 --- cinder/backup/api.py | 68 +++++++++++++------ cinder/tests/unit/api/contrib/test_backups.py | 48 +++++++++++-- cinder/tests/unit/backup/test_backup.py | 24 +++++++ ...t-backup-quota-issue-8yh69hd19u7tuu23.yaml | 3 + 4 files changed, 117 insertions(+), 26 deletions(-) create mode 100644 releasenotes/notes/fix-import-backup-quota-issue-8yh69hd19u7tuu23.yaml diff --git a/cinder/backup/api.py b/cinder/backup/api.py index d7206c1af1d..923c2883b54 100644 --- a/cinder/backup/api.py +++ b/cinder/backup/api.py @@ -485,6 +485,8 @@ class API(base.Base): :raises InvalidBackup: :raises InvalidInput: """ + reservations = None + backup = None # Deserialize string backup record into a dictionary backup_record = objects.Backup.decode_record(backup_url) @@ -493,6 +495,22 @@ class API(base.Base): msg = _('Provided backup record is missing an id') raise exception.InvalidInput(reason=msg) + # Since we use size to reserve&commit quota, size is another required + # field. + if 'size' not in backup_record: + msg = _('Provided backup record is missing size attribute') + raise exception.InvalidInput(reason=msg) + + try: + reserve_opts = {'backups': 1, + 'backup_gigabytes': backup_record['size']} + reservations = QUOTAS.reserve(context, **reserve_opts) + except exception.OverQuota as e: + quota_utils.process_reserve_over_quota( + context, e, + resource='backups', + size=backup_record['size']) + kwargs = { 'user_id': context.user_id, 'project_id': context.project_id, @@ -504,29 +522,37 @@ class API(base.Base): } try: - # Try to get the backup with that ID in all projects even among - # deleted entries. - backup = objects.BackupImport.get_by_id( - context.elevated(read_deleted='yes'), - backup_record['id'], - project_only=False) + try: + # Try to get the backup with that ID in all projects even among + # deleted entries. + backup = objects.BackupImport.get_by_id( + context.elevated(read_deleted='yes'), + backup_record['id'], + project_only=False) - # If record exists and it's not deleted we cannot proceed with the - # import - if backup.status != fields.BackupStatus.DELETED: - msg = _('Backup already exists in database.') - raise exception.InvalidBackup(reason=msg) - - # Otherwise we'll "revive" delete backup record - backup.update(kwargs) - backup.save() - - except exception.BackupNotFound: - # If record doesn't exist create it with the specific ID - backup = objects.BackupImport(context=context, - id=backup_record['id'], **kwargs) - backup.create() + # If record exists and it's not deleted we cannot proceed + # with the import + if backup.status != fields.BackupStatus.DELETED: + msg = _('Backup already exists in database.') + raise exception.InvalidBackup(reason=msg) + # Otherwise we'll "revive" delete backup record + backup.update(kwargs) + backup.save() + QUOTAS.commit(context, reservations) + except exception.BackupNotFound: + # If record doesn't exist create it with the specific ID + backup = objects.BackupImport(context=context, + id=backup_record['id'], **kwargs) + backup.create() + QUOTAS.commit(context, reservations) + except Exception: + with excutils.save_and_reraise_exception(): + try: + if backup and 'id' in backup: + backup.destroy() + finally: + QUOTAS.rollback(context, reservations) return backup def import_record(self, context, backup_service, backup_url): diff --git a/cinder/tests/unit/api/contrib/test_backups.py b/cinder/tests/unit/api/contrib/test_backups.py index 5986e12d789..d30019bbf20 100644 --- a/cinder/tests/unit/api/contrib/test_backups.py +++ b/cinder/tests/unit/api/contrib/test_backups.py @@ -37,6 +37,7 @@ from cinder import exception from cinder.i18n import _ from cinder import objects from cinder.objects import fields +from cinder import quota from cinder import test from cinder.tests.unit.api import fakes from cinder.tests.unit import fake_constants as fake @@ -2053,17 +2054,25 @@ class BackupsAPITestCase(test.TestCase): # request is not authorized self.assertEqual(http_client.FORBIDDEN, res.status_int) + @mock.patch.object(quota.QUOTAS, 'commit') + @mock.patch.object(quota.QUOTAS, 'rollback') + @mock.patch.object(quota.QUOTAS, 'reserve') @mock.patch('cinder.backup.api.API._list_backup_hosts') @mock.patch('cinder.backup.rpcapi.BackupAPI.import_record') def test_import_record_volume_id_specified_json(self, _mock_import_record_rpc, - _mock_list_services): + _mock_list_services, + mock_reserve, + mock_rollback, + mock_commit): utils.replace_obj_loader(self, objects.Backup) + mock_reserve.return_value = "fake_reservation" project_id = fake.PROJECT_ID backup_service = 'fake' ctx = context.RequestContext(fake.USER_ID, project_id, is_admin=True) backup = objects.Backup(ctx, id=fake.BACKUP_ID, user_id=fake.USER_ID, project_id=project_id, + size=1, status=fields.BackupStatus.AVAILABLE) backup_url = backup.encode_record() _mock_import_record_rpc.return_value = None @@ -2091,19 +2100,31 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual(ctx.user_id, db_backup.user_id) self.assertEqual(backup_api.IMPORT_VOLUME_ID, db_backup.volume_id) self.assertEqual(fields.BackupStatus.CREATING, db_backup.status) + mock_reserve.assert_called_with( + ctx, backups=1, backup_gigabytes=1) + mock_commit.assert_called_with(ctx, "fake_reservation") + @mock.patch.object(quota.QUOTAS, 'commit') + @mock.patch.object(quota.QUOTAS, 'rollback') + @mock.patch.object(quota.QUOTAS, 'reserve') @mock.patch('cinder.backup.api.API._list_backup_hosts') @mock.patch('cinder.backup.rpcapi.BackupAPI.import_record') def test_import_record_volume_id_exists_deleted(self, _mock_import_record_rpc, - _mock_list_services): + _mock_list_services, + mock_reserve, + mock_rollback, + mock_commit, + ): ctx = context.RequestContext(fake.USER_ID, fake.PROJECT_ID, is_admin=True) + mock_reserve.return_value = 'fake_reservation' utils.replace_obj_loader(self, objects.Backup) # Original backup belonged to a different user_id and project_id backup = objects.Backup(ctx, id=fake.BACKUP_ID, user_id=fake.USER2_ID, project_id=fake.PROJECT2_ID, + size=1, status=fields.BackupStatus.AVAILABLE) backup_url = backup.encode_record() @@ -2136,6 +2157,8 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual(ctx.user_id, db_backup.user_id) self.assertEqual(backup_api.IMPORT_VOLUME_ID, db_backup.volume_id) self.assertEqual(fields.BackupStatus.CREATING, db_backup.status) + mock_reserve.assert_called_with(ctx, backups=1, backup_gigabytes=1) + mock_commit.assert_called_with(ctx, "fake_reservation") backup_del.destroy() @@ -2188,12 +2211,19 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual("Invalid input received: Can't parse backup record.", res_dict['badRequest']['message']) + @mock.patch.object(quota.QUOTAS, 'commit') + @mock.patch.object(quota.QUOTAS, 'rollback') + @mock.patch.object(quota.QUOTAS, 'reserve') @mock.patch('cinder.backup.api.API._list_backup_hosts') def test_import_backup_with_existing_backup_record(self, - _mock_list_services): + _mock_list_services, + mock_reserve, + mock_rollback, + mock_commit): ctx = context.RequestContext(fake.USER_ID, fake.PROJECT_ID, is_admin=True) - backup = utils.create_backup(self.context, fake.VOLUME_ID) + mock_reserve.return_value = "fake_reservation" + backup = utils.create_backup(self.context, fake.VOLUME_ID, size=1) backup_service = 'fake' backup_url = backup.encode_record() _mock_list_services.return_value = ['no-match1', 'no-match2'] @@ -2212,12 +2242,20 @@ class BackupsAPITestCase(test.TestCase): res_dict['badRequest']['code']) self.assertEqual('Invalid backup: Backup already exists in database.', res_dict['badRequest']['message']) - + mock_reserve.assert_called_with( + ctx, backups=1, backup_gigabytes=1) + mock_rollback.assert_called_with(ctx, "fake_reservation") backup.destroy() + @mock.patch.object(quota.QUOTAS, 'commit') + @mock.patch.object(quota.QUOTAS, 'rollback') + @mock.patch.object(quota.QUOTAS, 'reserve') @mock.patch('cinder.backup.api.API._list_backup_hosts') @mock.patch('cinder.backup.rpcapi.BackupAPI.import_record') def test_import_backup_with_missing_backup_services(self, + mock_reserve, + mock_rollback, + mock_commit, _mock_import_record, _mock_list_services): ctx = context.RequestContext(fake.USER_ID, fake.PROJECT_ID, diff --git a/cinder/tests/unit/backup/test_backup.py b/cinder/tests/unit/backup/test_backup.py index efe4b78b3e7..b5df7fd908c 100644 --- a/cinder/tests/unit/backup/test_backup.py +++ b/cinder/tests/unit/backup/test_backup.py @@ -35,6 +35,7 @@ from cinder import db from cinder import exception from cinder import objects from cinder.objects import fields +from cinder import quota from cinder import test from cinder.tests import fake_driver from cinder.tests.unit import fake_constants as fake @@ -2128,3 +2129,26 @@ class BackupAPITestCase(BaseBackupTest): self.api.restore(self.ctxt, backup.id, volume_id) backup = objects.Backup.get_by_id(self.ctxt, backup.id) self.assertEqual(volume_id, backup.restore_volume_id) + + @mock.patch.object(objects.Backup, 'decode_record') + @mock.patch.object(quota.QUOTAS, 'commit') + @mock.patch.object(quota.QUOTAS, 'rollback') + @mock.patch.object(quota.QUOTAS, 'reserve') + def test__get_import_backup_invalid_backup( + self, mock_reserve, mock_rollback, mock_commit, mock_decode): + + backup = self._create_backup_db_entry(size=1, + status='available') + mock_decode.return_value = {'id': backup.id, + 'project_id': backup.project_id, + 'user_id': backup.user_id, + 'volume_id': backup.volume_id, + 'size': 1} + mock_reserve.return_value = 'fake_reservation' + + self.assertRaises(exception.InvalidBackup, + self.api._get_import_backup, + self.ctxt, 'fake_backup_url') + mock_reserve.assert_called_with( + self.ctxt, backups=1, backup_gigabytes=1) + mock_rollback.assert_called_with(self.ctxt, "fake_reservation") diff --git a/releasenotes/notes/fix-import-backup-quota-issue-8yh69hd19u7tuu23.yaml b/releasenotes/notes/fix-import-backup-quota-issue-8yh69hd19u7tuu23.yaml new file mode 100644 index 00000000000..53ac6ac4d56 --- /dev/null +++ b/releasenotes/notes/fix-import-backup-quota-issue-8yh69hd19u7tuu23.yaml @@ -0,0 +1,3 @@ +--- +fixes: + - Cinder will now consume quota when importing new backup resource.