fix unmange share with manage_error status will lead to quota error

if we failed to manage a share, we don't need to commit the quota usages. so
we should skip quota usages cuts when delete or unmange the share with
status of "error_manage". and the size of error_manage share should be
zero.

Closes-Bug:#1883506

Change-Id: I5c81dd6780890c55c8c6a92491c3f4f507531fdb

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'

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)

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']:

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:

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(

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."""

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']))

releasenotes/notes/bug-1883506-fix-delete-manage-error-share-will-lead-to-quota-error-085fd3b7d15ae109.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Fixed `bug #1883506 <https://bugs.launchpad.net/manila/+bug/18835060>`_
+    that caused a quota error when delete or unmanage a share that failed to
+    manage.