From 6a6aa0f146034ffa0d9ebb4c84c7d7485af5b007 Mon Sep 17 00:00:00 2001 From: silvacarloss Date: Mon, 1 Mar 2021 15:35:11 -0300 Subject: [PATCH] Fix replica quotas allocation during share migration Fixed the issue in the share migration where the replica quotas were not being accounted. Manila now tries to allocate share replica quotas for the migrated share if the destination share type has a `replication_type`. If the migration fails or get cancelled, the quotas will be reverted. Change-Id: Ie0a08a61c79dc8ea2a7a240363e94b26a80e64a4 Closes-Bug: 1910752 --- manila/scheduler/manager.py | 4 + manila/share/api.py | 102 ++++++++++++++++++ manila/share/manager.py | 29 ++++- manila/share/share_types.py | 47 ++++++++ manila/tests/scheduler/test_manager.py | 11 ++ manila/tests/share/test_api.py | 92 ++++++++++++++++ manila/tests/share/test_manager.py | 14 +++ manila/tests/share/test_share_types.py | 55 ++++++++++ ...n-replication-quotas-eaa013b743d721cd.yaml | 7 ++ 9 files changed, 360 insertions(+), 1 deletion(-) create mode 100644 releasenotes/notes/bug-1910752-fix-migration-replication-quotas-eaa013b743d721cd.yaml diff --git a/manila/scheduler/manager.py b/manila/scheduler/manager.py index 2bac36d2de..f982473f04 100644 --- a/manila/scheduler/manager.py +++ b/manila/scheduler/manager.py @@ -39,8 +39,10 @@ from manila.message import message_field from manila import quota from manila import rpc from manila.share import rpcapi as share_rpcapi +from manila.share import share_types LOG = log.getLogger(__name__) +QUOTAS = quota.QUOTAS scheduler_driver_opt = cfg.StrOpt('scheduler_driver', default='manila.scheduler.drivers.' @@ -179,6 +181,8 @@ class SchedulerManager(manager.Manager): 'migrate_share_to_host', {'task_state': constants.TASK_STATE_MIGRATION_ERROR}, context, ex, request_spec) + share_types.revert_allocated_share_type_quotas_during_migration( + context, share_ref, new_share_type_id) try: tgt_host = self.driver.host_passes_filters( diff --git a/manila/share/api.py b/manila/share/api.py index d8325b39c6..a3a770abdc 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -1395,6 +1395,106 @@ class API(base.Base): return snapshot + def _modify_quotas_for_share_migration(self, context, share, + new_share_type): + """Consume quotas for share migration. + + If a share migration was requested and a new share type was provided, + quotas must be consumed from this share type. If no quotas are + available for shares, gigabytes, share replicas or replica gigabytes, + an error will be thrown. + """ + + new_share_type_id = new_share_type['id'] + + if new_share_type_id == share['share_type_id']: + return + + new_type_extra_specs = self.get_share_attributes_from_share_type( + new_share_type) + new_type_replication_type = new_type_extra_specs.get( + 'replication_type', None) + + deltas = {} + + # NOTE(carloss): If a new share type with a replication type was + # specified, there is need to allocate quotas in the new share type. + # We won't remove the current consumed quotas, since both share + # instances will co-exist until the migration gets completed, + # cancelled or it fails. + if new_type_replication_type: + deltas['share_replicas'] = 1 + deltas['replica_gigabytes'] = share['size'] + + deltas.update({ + 'share_type_id': new_share_type_id, + 'shares': 1, + 'gigabytes': share['size'] + }) + + try: + reservations = QUOTAS.reserve( + context, project_id=share['project_id'], + user_id=share['user_id'], **deltas) + QUOTAS.commit( + context, reservations, project_id=share['project_id'], + user_id=share['user_id'], share_type_id=new_share_type_id) + except exception.OverQuota as e: + overs = e.kwargs['overs'] + usages = e.kwargs['usages'] + quotas = e.kwargs['quotas'] + + def _consumed(name): + return (usages[name]['reserved'] + usages[name]['in_use']) + + if 'replica_gigabytes' in overs: + LOG.warning("Replica gigabytes quota exceeded " + "for %(s_pid)s, tried to migrate " + "%(s_size)sG share (%(d_consumed)dG of " + "%(d_quota)dG already consumed).", { + 's_pid': context.project_id, + 's_size': share['size'], + 'd_consumed': _consumed( + 'replica_gigabytes'), + 'd_quota': quotas['replica_gigabytes']}) + msg = _("Failed while migrating a share with replication " + "support. Maximum number of allowed " + "replica gigabytes is exceeded.") + raise exception.ShareReplicaSizeExceedsAvailableQuota( + message=msg) + + if 'share_replicas' in overs: + LOG.warning("Quota exceeded for %(s_pid)s, " + "unable to migrate share-replica (%(d_consumed)d " + "of %(d_quota)d already consumed).", { + 's_pid': context.project_id, + 'd_consumed': _consumed('share_replicas'), + 'd_quota': quotas['share_replicas']}) + msg = _( + "Failed while migrating a share with replication " + "support. Maximum number of allowed share-replicas " + "is exceeded.") + raise exception.ShareReplicasLimitExceeded(msg) + + if 'gigabytes' in overs: + LOG.warning("Quota exceeded for %(s_pid)s, " + "tried to migrate " + "%(s_size)sG share (%(d_consumed)dG of " + "%(d_quota)dG already consumed).", { + 's_pid': context.project_id, + 's_size': share['size'], + 'd_consumed': _consumed('gigabytes'), + 'd_quota': quotas['gigabytes']}) + raise exception.ShareSizeExceedsAvailableQuota() + if 'shares' in overs: + LOG.warning("Quota exceeded for %(s_pid)s, " + "tried to migrate " + "share (%(d_consumed)d shares " + "already consumed).", { + 's_pid': context.project_id, + 'd_consumed': _consumed('shares')}) + raise exception.ShareLimitExceeded(allowed=quotas['shares']) + def migration_start( self, context, share, dest_host, force_host_assisted_migration, preserve_metadata, writable, nondisruptive, preserve_snapshots, @@ -1479,6 +1579,8 @@ class API(base.Base): " given share %s has extra_spec " "'driver_handles_share_servers' as True.") % share['id'] raise exception.InvalidInput(reason=msg) + self._modify_quotas_for_share_migration(context, share, + new_share_type) else: share_type = {} share_type_id = share_instance['share_type_id'] diff --git a/manila/share/manager.py b/manila/share/manager.py index e64c34c411..96b0ef8df4 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -1194,6 +1194,9 @@ class ShareManager(manager.SchedulerDependentManager): # NOTE(ganso): Cleaning up error'ed destination share instance from # database. It is assumed that driver cleans up leftovers in # backend when migration fails. + share_types.revert_allocated_share_type_quotas_during_migration( + context, src_share_instance, + src_share_instance['share_type_id']) self._migration_delete_instance(context, dest_share_instance['id']) self._restore_migrating_snapshots_status( context, src_share_instance['id']) @@ -1295,7 +1298,8 @@ class ShareManager(manager.SchedulerDependentManager): def migration_driver_continue(self, context): """Invokes driver to continue migration of shares.""" - instances = self.db.share_instances_get_all_by_host(context, self.host) + instances = self.db.share_instances_get_all_by_host( + context, self.host, with_share_data=True) for instance in instances: @@ -1354,6 +1358,10 @@ class ShareManager(manager.SchedulerDependentManager): except Exception: + (share_types. + revert_allocated_share_type_quotas_during_migration( + context, src_share_instance, + dest_share_instance['share_type_id'])) # NOTE(ganso): Cleaning up error'ed destination share # instance from database. It is assumed that driver cleans # up leftovers in backend when migration fails. @@ -1617,6 +1625,10 @@ class ShareManager(manager.SchedulerDependentManager): src_share_instance['id'], dest_share_instance['id']) + share_types.revert_allocated_share_type_quotas_during_migration( + context, dest_share_instance, src_share_instance['share_type_id'], + allow_deallocate_from_current_type=True) + self._migration_delete_instance(context, src_share_instance['id']) def _migration_complete_instance(self, context, share_ref, @@ -1702,6 +1714,9 @@ class ShareManager(manager.SchedulerDependentManager): self.db.share_update( context, share_ref['id'], {'task_state': constants.TASK_STATE_MIGRATION_ERROR}) + # NOTE(carloss): No need to deallocate quotas allocated during + # the migration request, since both share instances still exist + # even they are set to an error state. raise exception.ShareMigrationFailed(reason=msg) else: try: @@ -1712,6 +1727,9 @@ class ShareManager(manager.SchedulerDependentManager): msg = _("Host-assisted migration completion failed for" " share %s.") % share_ref['id'] LOG.exception(msg) + # NOTE(carloss): No need to deallocate quotas allocated during + # the migration request, since both source and destination + # instances will still exist self.db.share_update( context, share_ref['id'], {'task_state': constants.TASK_STATE_MIGRATION_ERROR}) @@ -1807,6 +1825,12 @@ class ShareManager(manager.SchedulerDependentManager): src_share_instance['id'], dest_share_instance['id']) + # NOTE(carloss): Won't revert allocated quotas for the share type here + # because the delete_instance_and_wait method will end up calling the + # delete_share_instance method here in the share manager. When the + # share instance deletion is requested in the share manager, Manila + # itself will take care of deallocating the existing quotas for the + # share instance helper.delete_instance_and_wait(src_share_instance) @utils.require_driver_initialized @@ -1871,6 +1895,9 @@ class ShareManager(manager.SchedulerDependentManager): context, share_ref['id'], {'task_state': constants.TASK_STATE_MIGRATION_CANCELLED}) + share_types.revert_allocated_share_type_quotas_during_migration( + context, src_share_instance, dest_share_instance['share_type_id']) + LOG.info("Share Migration for share %s" " was cancelled.", share_ref['id']) diff --git a/manila/share/share_types.py b/manila/share/share_types.py index 3eff44ce91..5233ca7ee5 100644 --- a/manila/share/share_types.py +++ b/manila/share/share_types.py @@ -30,9 +30,11 @@ from manila import context from manila import db from manila import exception from manila.i18n import _ +from manila import quota CONF = cfg.CONF LOG = log.getLogger(__name__) +QUOTAS = quota.QUOTAS MIN_SIZE_KEY = "provisioning:min_share_size" MAX_SIZE_KEY = "provisioning:max_share_size" @@ -460,3 +462,48 @@ def provision_filter_on_size(context, share_type, size): ) % {'req_size': size_int, 'max_size': max_size, 'sha_type': share_type['name']} raise exception.InvalidInput(reason=msg) + + +def revert_allocated_share_type_quotas_during_migration( + context, share, share_type_id, + allow_deallocate_from_current_type=False): + + # If both new share type and share's share type ID, there is no need + # to revert quotas because new quotas weren't allocated, as share + # type changes weren't identified, unless it is a migration that was + # successfully completed + if ((share_type_id == share['share_type_id']) + and not allow_deallocate_from_current_type): + return + + new_share_type = get_share_type(context, share_type_id) + new_type_extra_specs = new_share_type.get('extra_specs', None) + + new_type_replication_type = None + if new_type_extra_specs: + new_type_replication_type = new_type_extra_specs.get( + 'replication_type', None) + + deltas = {} + + if new_type_replication_type: + deltas['share_replicas'] = -1 + deltas['replica_gigabytes'] = -share['size'] + + deltas.update({ + 'share_type_id': new_share_type['id'], + 'shares': -1, + 'gigabytes': -share['size'] + }) + + try: + reservations = QUOTAS.reserve( + context, project_id=share['project_id'], + user_id=share['user_id'], **deltas) + except Exception: + LOG.exception("Failed to update usages for share_replicas and " + "replica_gigabytes.") + else: + QUOTAS.commit( + context, reservations, project_id=share['project_id'], + user_id=share['user_id'], share_type_id=share_type_id) diff --git a/manila/tests/scheduler/test_manager.py b/manila/tests/scheduler/test_manager.py index c0273b9c5f..3924281bfc 100644 --- a/manila/tests/scheduler/test_manager.py +++ b/manila/tests/scheduler/test_manager.py @@ -33,6 +33,7 @@ from manila.scheduler.drivers import base from manila.scheduler.drivers import filter from manila.scheduler import manager from manila.share import rpcapi as share_rpcapi +from manila.share import share_types from manila import test from manila.tests import db_utils from manila.tests import fake_share as fakes @@ -281,6 +282,9 @@ class SchedulerManagerTestCase(test.TestCase): self.mock_object(base.Scheduler, 'host_passes_filters', mock.Mock(return_value=host)) + self.mock_object( + share_types, + 'revert_allocated_share_type_quotas_during_migration') self.assertRaises( TypeError, self.manager.migrate_share_to_host, @@ -293,6 +297,8 @@ class SchedulerManagerTestCase(test.TestCase): share_rpcapi.ShareAPI.migration_start.assert_called_once_with( self.context, share, host.host, False, True, True, False, True, 'fake_net_id', 'fake_type_id') + (share_types.revert_allocated_share_type_quotas_during_migration. + assert_called_once_with(self.context, share, 'fake_type_id')) @ddt.data(exception.NoValidHost(reason='fake'), TypeError) def test_migrate_share_to_host_exception(self, exc): @@ -307,6 +313,9 @@ class SchedulerManagerTestCase(test.TestCase): mock.Mock(side_effect=exc)) self.mock_object(db, 'share_update') self.mock_object(db, 'share_instance_update') + self.mock_object( + share_types, + 'revert_allocated_share_type_quotas_during_migration') capture = (exception.NoValidHost if isinstance(exc, exception.NoValidHost) else TypeError) @@ -325,6 +334,8 @@ class SchedulerManagerTestCase(test.TestCase): db.share_instance_update.assert_called_once_with( self.context, share.instance['id'], {'status': constants.STATUS_AVAILABLE}) + (share_types.revert_allocated_share_type_quotas_during_migration. + assert_called_once_with(self.context, share, 'fake_type_id')) def test_manage_share(self): diff --git a/manila/tests/share/test_api.py b/manila/tests/share/test_api.py index a0765554a4..c59001d071 100644 --- a/manila/tests/share/test_api.py +++ b/manila/tests/share/test_api.py @@ -3222,6 +3222,92 @@ class ShareAPITestCase(test.TestCase): self.assertEqual(fake_el, out) + @ddt.data(True, False) + def test__modify_quotas_for_share_migration(self, new_replication_type): + extra_specs = ( + {'replication_type': 'readable'} if new_replication_type else {}) + share = db_utils.create_share() + share_type = db_utils.create_share_type(extra_specs=extra_specs) + + expected_deltas = { + 'project_id': share['project_id'], + 'user_id': share['user_id'], + 'shares': 1, + 'gigabytes': share['size'], + 'share_type_id': share_type['id'] + } + + if new_replication_type: + expected_deltas.update({ + 'share_replicas': 1, + 'replica_gigabytes': share['size'], + }) + reservations = 'reservations' + + mock_specs_get = self.mock_object( + self.api, 'get_share_attributes_from_share_type', + mock.Mock(return_value=extra_specs)) + mock_reserve = self.mock_object( + quota.QUOTAS, 'reserve', mock.Mock(return_value=reservations)) + mock_commit = self.mock_object(quota.QUOTAS, 'commit') + + self.api._modify_quotas_for_share_migration( + self.context, share, share_type) + + mock_specs_get.assert_called_once_with(share_type) + mock_reserve.assert_called_once_with( + self.context, **expected_deltas) + mock_commit.assert_called_once_with( + self.context, reservations, project_id=share['project_id'], + user_id=share['user_id'], share_type_id=share_type['id']) + + @ddt.data( + ('replica_gigabytes', exception.ShareReplicaSizeExceedsAvailableQuota), + ('share_replicas', exception.ShareReplicasLimitExceeded), + ('gigabytes', exception.ShareSizeExceedsAvailableQuota) + ) + @ddt.unpack + def test__modify_quotas_for_share_migration_reservation_failed( + self, over_resource, expected_exception): + extra_specs = {'replication_type': 'readable'} + share = db_utils.create_share() + share_type = db_utils.create_share_type(extra_specs=extra_specs) + expected_deltas = { + 'project_id': share['project_id'], + 'user_id': share['user_id'], + 'share_replicas': 1, + 'shares': 1, + 'gigabytes': share['size'], + 'replica_gigabytes': share['size'], + 'share_type_id': share_type['id'] + } + usages = { + over_resource: { + 'reserved': 'fake', + 'in_use': 'fake' + } + } + quotas = { + over_resource: 'fake' + } + + effect_exc = exception.OverQuota( + overs=[over_resource], usages=usages, quotas=quotas) + mock_specs_get = self.mock_object( + self.api, 'get_share_attributes_from_share_type', + mock.Mock(return_value=extra_specs)) + mock_reserve = self.mock_object( + quota.QUOTAS, 'reserve', mock.Mock(side_effect=effect_exc)) + + self.assertRaises( + expected_exception, + self.api._modify_quotas_for_share_migration, + self.context, share, share_type + ) + + mock_specs_get.assert_called_once_with(share_type) + mock_reserve.assert_called_once_with(self.context, **expected_deltas) + @ddt.data({'share_type': True, 'share_net': True, 'dhss': True}, {'share_type': False, 'share_net': True, 'dhss': True}, {'share_type': False, 'share_net': False, 'dhss': True}, @@ -3281,6 +3367,7 @@ class ShareAPITestCase(test.TestCase): self.mock_object(db_api, 'share_update') self.mock_object(db_api, 'service_get_by_args', mock.Mock(return_value=service)) + self.mock_object(share_api.API, '_modify_quotas_for_share_migration') if share_type: self.api.migration_start(self.context, share, host, False, True, @@ -3306,6 +3393,9 @@ class ShareAPITestCase(test.TestCase): db_api.share_instance_update.assert_called_once_with( self.context, share.instance['id'], {'status': constants.STATUS_MIGRATING}) + if share_type: + (share_api.API._modify_quotas_for_share_migration. + assert_called_once_with(self.context, share, fake_type_2)) def test_migration_start_with_new_share_type_limit(self): host = 'fake2@backend#pool' @@ -3363,6 +3453,7 @@ class ShareAPITestCase(test.TestCase): self.mock_object(db_api, 'share_update') self.mock_object(db_api, 'service_get_by_args', mock.Mock(return_value=service)) + self.mock_object(share_api.API, '_modify_quotas_for_share_migration') self.assertRaises(exception.InvalidShare, self.api.migration_start, @@ -3429,6 +3520,7 @@ class ShareAPITestCase(test.TestCase): host='fake@backend#pool', share_type_id=fake_type['id']) self.mock_object(utils, 'validate_service_host') + self.mock_object(share_api.API, '_modify_quotas_for_share_migration') self.assertRaises( exception.InvalidInput, self.api.migration_start, self.context, diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py index 54c8abfa1a..629c4c7768 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -5885,6 +5885,9 @@ class ShareManagerTestCase(test.TestCase): self.mock_object(self.share_manager, '_migration_delete_instance') self.mock_object(migration_api.ShareMigrationHelper, 'apply_new_access_rules') + self.mock_object( + share_types, + 'revert_allocated_share_type_quotas_during_migration') self.mock_object( self.share_manager.db, 'share_snapshot_instance_get_all_with_filters', @@ -5943,6 +5946,11 @@ class ShareManagerTestCase(test.TestCase): el_create.assert_called_once_with(self.context, el) else: el_create.assert_not_called() + (share_types. + revert_allocated_share_type_quotas_during_migration. + assert_called_once_with( + self.context, dest_instance, src_instance['share_type_id'], + allow_deallocate_from_current_type=True)) def test__migration_complete_host_assisted(self): @@ -6017,6 +6025,9 @@ class ShareManagerTestCase(test.TestCase): self.mock_object(self.share_manager.driver, 'migration_cancel') self.mock_object(helper, 'cleanup_new_instance') self.mock_object(self.share_manager, '_reset_read_only_access_rules') + self.mock_object( + share_types, + 'revert_allocated_share_type_quotas_during_migration') self.share_manager.migration_cancel( self.context, instance_1['id'], instance_2['id']) @@ -6062,6 +6073,9 @@ class ShareManagerTestCase(test.TestCase): self.share_manager.db.share_instance_update.assert_has_calls( share_instance_update_calls) + (share_types.revert_allocated_share_type_quotas_during_migration. + assert_called_once_with( + self.context, instance_1, instance_2['share_type_id'])) @ddt.data(True, False) def test__reset_read_only_access_rules(self, supress_errors): diff --git a/manila/tests/share/test_share_types.py b/manila/tests/share/test_share_types.py index 243190a82f..02f72b1519 100644 --- a/manila/tests/share/test_share_types.py +++ b/manila/tests/share/test_share_types.py @@ -29,8 +29,10 @@ from manila.common import constants from manila import context from manila import db from manila import exception +from manila import quota from manila.share import share_types from manila import test +from manila.tests import db_utils def create_share_type_dict(extra_specs=None): @@ -581,3 +583,56 @@ class ShareTypesTestCase(test.TestCase): share_types.provision_filter_on_size(self.context, type4, "24") share_types.provision_filter_on_size(self.context, type4, "99") share_types.provision_filter_on_size(self.context, type4, "30") + + @ddt.data(True, False) + def test__revert_allocated_share_type_quotas_during_migration( + self, failed_on_reservation): + fake_type_id = 'fake_1' + extra_specs = {'replication_type': 'readable'} + source_instance = db_utils.create_share() + dest_instance = db_utils.create_share( + share_type_id=fake_type_id) + share_type = { + 'name': 'fake_share_type', + 'extra_specs': extra_specs, + 'is_public': True, + 'id': 'fake_type_id' + } + + expected_deltas = { + 'project_id': dest_instance['project_id'], + 'user_id': dest_instance['user_id'], + 'share_replicas': -1, + 'replica_gigabytes': -dest_instance['size'], + 'share_type_id': share_type['id'], + 'shares': -1, + 'gigabytes': -dest_instance['size'], + } + reservations = 'reservations' + reservation_action = ( + mock.Mock(side_effect=exception.ManilaException(message='fake')) + if failed_on_reservation else mock.Mock(return_value=reservations)) + + mock_type_get = self.mock_object( + share_types, 'get_share_type', mock.Mock(return_value=share_type)) + mock_reserve = self.mock_object( + quota.QUOTAS, 'reserve', reservation_action) + mock_commit = self.mock_object(quota.QUOTAS, 'commit') + mock_log = self.mock_object(share_types.LOG, 'exception') + + share_types.revert_allocated_share_type_quotas_during_migration( + self.context, source_instance, share_type['id'], dest_instance) + + if not failed_on_reservation: + mock_commit.assert_called_once_with( + self.context, reservations, + project_id=dest_instance['project_id'], + user_id=dest_instance['user_id'], + share_type_id=share_type['id']) + else: + mock_log.assert_called_once() + + mock_type_get.assert_called_once_with( + self.context, share_type['id']) + mock_reserve.assert_called_once_with( + self.context, **expected_deltas) diff --git a/releasenotes/notes/bug-1910752-fix-migration-replication-quotas-eaa013b743d721cd.yaml b/releasenotes/notes/bug-1910752-fix-migration-replication-quotas-eaa013b743d721cd.yaml new file mode 100644 index 0000000000..741524e076 --- /dev/null +++ b/releasenotes/notes/bug-1910752-fix-migration-replication-quotas-eaa013b743d721cd.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Fixed the issue of not accounting replica quotas while triggering the + migration for a given share. Please refer to the + `Launchpad bug #1910752 `_ + for more details.