diff --git a/manila/common/constants.py b/manila/common/constants.py index 185d2a1091..2d230fb503 100644 --- a/manila/common/constants.py +++ b/manila/common/constants.py @@ -28,6 +28,7 @@ STATUS_INACTIVE = 'inactive' STATUS_MANAGING = 'manage_starting' STATUS_MANAGE_ERROR = 'manage_error' STATUS_UNMANAGING = 'unmanage_starting' +STATUS_MANAGE_ERROR_UNMANAGING = 'manage_error_unmanage_starting' STATUS_UNMANAGE_ERROR = 'unmanage_error' STATUS_UNMANAGED = 'unmanaged' STATUS_EXTENDING = 'extending' diff --git a/manila/scheduler/manager.py b/manila/scheduler/manager.py index 3a5957e5a2..dd85b261b6 100644 --- a/manila/scheduler/manager.py +++ b/manila/scheduler/manager.py @@ -129,12 +129,13 @@ class SchedulerManager(manager.Manager): """Ensure that the host exists and can accept the share.""" def _manage_share_set_error(self, context, ex, request_spec): - # NOTE(nidhimittalhada): set size as 1 because design expects size - # to be set, it also will allow us to handle delete/unmanage - # operations properly with this errored share according to quotas. + # NOTE(haixin) if failed to scheduler backend for manage share, + # and we do not commit quota usages here, so we should set size 0 + # because we don't know the real size of the size, and we will + # skip quota cuts when unmanage share with manage_error status. self._set_share_state_and_notify( 'manage_share', - {'status': constants.STATUS_MANAGE_ERROR, 'size': 1}, + {'status': constants.STATUS_MANAGE_ERROR, 'size': 0}, context, ex, request_spec) share_ref = db.share_get(context, share_id) diff --git a/manila/share/api.py b/manila/share/api.py index a43c2905b9..f6a11afe48 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -916,7 +916,12 @@ class API(base.Base): self._check_is_share_busy(share) - update_data = {'status': constants.STATUS_UNMANAGING, + if share['status'] == constants.STATUS_MANAGE_ERROR: + update_status = constants.STATUS_MANAGE_ERROR_UNMANAGING + else: + update_status = constants.STATUS_UNMANAGING + + update_data = {'status': update_status, 'terminated_at': timeutils.utcnow()} share_ref = self.db.share_update(context, share['id'], update_data) @@ -1128,7 +1133,6 @@ class API(base.Base): _("Share still has %d dependent share group snapshot " "members.") % share_group_snapshot_members_count) raise exception.InvalidShare(reason=msg) - self._check_is_share_busy(share) for share_instance in share.instances: if share_instance['host']: diff --git a/manila/share/manager.py b/manila/share/manager.py index acf78d89d7..38f5887fae 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -2589,6 +2589,8 @@ class ShareManager(manager.SchedulerDependentManager): ) if not share_update.get('size'): + # NOTE(haixin)if failed to get real size of share, will not + # commit quota usages. msg = _("Driver cannot calculate share size.") raise exception.InvalidShare(reason=msg) @@ -2639,12 +2641,12 @@ class ShareManager(manager.SchedulerDependentManager): self.db.share_update(context, share_id, share_update) except Exception: - # NOTE(vponomaryov): set size as 1 because design expects size - # to be set, it also will allow us to handle delete/unmanage - # operations properly with this errored share according to quotas. + # NOTE(haixin) we should set size 0 because we don't know the real + # size of the size, and we will skip quota cuts when + # delete/unmanage share. self.db.share_update( context, share_id, - {'status': constants.STATUS_MANAGE_ERROR, 'size': 1}) + {'status': constants.STATUS_MANAGE_ERROR, 'size': 0}) raise @add_hooks @@ -2767,30 +2769,34 @@ class ShareManager(manager.SchedulerDependentManager): ("Share can not be unmanaged: %s."), e) return - deltas = { - 'project_id': project_id, - 'shares': -1, - 'gigabytes': -share_ref['size'], - 'share_type_id': share_instance['share_type_id'], - } - # NOTE(carloss): while unmanaging a share, a share will not contain - # replicas other than the active one. So there is no need to - # recalculate the amount of share replicas to be deallocated. - if supports_replication: - deltas.update({'share_replicas': -1, - 'replica_gigabytes': -share_ref['size']}) - try: - reservations = QUOTAS.reserve(context, **deltas) - QUOTAS.commit( - context, reservations, project_id=project_id, - share_type_id=share_instance['share_type_id'], - ) - except Exception as e: - # Note(imalinovskiy): - # Quota reservation errors here are not fatal, because - # unmanage is administrator API and he/she could update user - # quota usages later if it's required. - LOG.warning("Failed to update quota usages: %s.", e) + # NOTE(haixin) we will skip quota cuts when unmanag share with + # 'error_manage' status, because we have not commit quota usages when + # we failed to manage the share. + if share_ref['status'] != constants.STATUS_MANAGE_ERROR_UNMANAGING: + deltas = { + 'project_id': project_id, + 'shares': -1, + 'gigabytes': -share_ref['size'], + 'share_type_id': share_instance['share_type_id'], + } + # NOTE(carloss): while unmanaging a share, a share will not + # contain replicas other than the active one. So there is no need + # to recalculate the amount of share replicas to be deallocated. + if supports_replication: + deltas.update({'share_replicas': -1, + 'replica_gigabytes': -share_ref['size']}) + try: + reservations = QUOTAS.reserve(context, **deltas) + QUOTAS.commit( + context, reservations, project_id=project_id, + share_type_id=share_instance['share_type_id'], + ) + except Exception as e: + # Note(imalinovskiy): + # Quota reservation errors here are not fatal, because + # unmanage is administrator API and he/she could update user + # quota usages later if it's required. + LOG.warning("Failed to update quota usages: %s.", e) if self.configuration.safe_get('unmanage_remove_access_rules'): try: diff --git a/manila/tests/api/v1/test_share_unmanage.py b/manila/tests/api/v1/test_share_unmanage.py index a7027e67cb..209fdbefde 100644 --- a/manila/tests/api/v1/test_share_unmanage.py +++ b/manila/tests/api/v1/test_share_unmanage.py @@ -52,9 +52,9 @@ class ShareUnmanageTest(test.TestCase): self.mock_policy_check = self.mock_object( policy, 'check_policy', mock.Mock(return_value=True)) - def test_unmanage_share(self): - share = dict(status=constants.STATUS_AVAILABLE, id='foo_id', - instance={}) + @ddt.data(constants.STATUS_AVAILABLE, constants.STATUS_MANAGE_ERROR) + def test_unmanage_share(self, status): + share = dict(status=status, id='foo_id', instance={}) self.mock_object(share_api.API, 'get', mock.Mock(return_value=share)) self.mock_object(share_api.API, 'unmanage', mock.Mock()) self.mock_object( diff --git a/manila/tests/scheduler/test_manager.py b/manila/tests/scheduler/test_manager.py index 38919a4682..80114fcf6d 100644 --- a/manila/tests/scheduler/test_manager.py +++ b/manila/tests/scheduler/test_manager.py @@ -353,7 +353,7 @@ class SchedulerManagerTestCase(test.TestCase): {'share_id': share_id}, None) db_update.assert_called_once_with( self.context, share_id, - {'status': constants.STATUS_MANAGE_ERROR, 'size': 1}) + {'status': constants.STATUS_MANAGE_ERROR, 'size': 0}) def test_create_share_replica_exception_path(self): """Test 'raisable' exceptions for create_share_replica.""" diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py index d3bc756a11..c77a602e20 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -2631,7 +2631,7 @@ class ShareManagerTestCase(test.TestCase): self.share_manager.db.share_update.assert_called_once_with( utils.IsAMatcher(context.RequestContext), share_id, - {'status': constants.STATUS_MANAGE_ERROR, 'size': 1}) + {'status': constants.STATUS_MANAGE_ERROR, 'size': 0}) (self.share_manager._get_extra_specs_from_share_type. assert_called_once_with( mock.ANY, share['instance']['share_type_id'])) @@ -2739,7 +2739,7 @@ class ShareManagerTestCase(test.TestCase): assert_called_once_with(mock.ANY, driver_options)) self.share_manager.db.share_update.assert_called_once_with( utils.IsAMatcher(context.RequestContext), share_id, - {'status': constants.STATUS_MANAGE_ERROR, 'size': 1}) + {'status': constants.STATUS_MANAGE_ERROR, 'size': 0}) (self.share_manager._get_extra_specs_from_share_type. assert_called_once_with( mock.ANY, share['instance']['share_type_id'])) @@ -2772,7 +2772,7 @@ class ShareManagerTestCase(test.TestCase): assert_called_once_with(mock.ANY, driver_options)) self.share_manager.db.share_update.assert_called_once_with( mock.ANY, share_id, - {'status': constants.STATUS_MANAGE_ERROR, 'size': 1}) + {'status': constants.STATUS_MANAGE_ERROR, 'size': 0}) (self.share_manager._get_extra_specs_from_share_type. assert_called_once_with( mock.ANY, share['instance']['share_type_id'])) diff --git a/releasenotes/notes/bug-1883506-fix-delete-manage-error-share-will-lead-to-quota-error-085fd3b7d15ae109.yaml b/releasenotes/notes/bug-1883506-fix-delete-manage-error-share-will-lead-to-quota-error-085fd3b7d15ae109.yaml new file mode 100644 index 0000000000..b455c920e7 --- /dev/null +++ b/releasenotes/notes/bug-1883506-fix-delete-manage-error-share-will-lead-to-quota-error-085fd3b7d15ae109.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Fixed `bug #1883506 `_ + that caused a quota error when delete or unmanage a share that failed to + manage.