Merge "Fix replica quotas allocation during share migration"

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(

manila/share/api.py

@@ -1531,6 +1531,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,
@@ -1615,6 +1715,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']

manila/share/manager.py

@@ -1228,6 +1228,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'])
@@ -1329,7 +1332,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:
 
@@ -1388,6 +1392,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.
@@ -1651,6 +1659,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,
@@ -1736,6 +1748,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:
@@ -1746,6 +1761,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})
@@ -1841,6 +1859,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
@@ -1905,6 +1929,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'])
 

manila/share/share_types.py

@@ -29,9 +29,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"
@@ -459,3 +461,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)

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):
 

manila/tests/share/test_api.py

@@ -3374,6 +3374,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},
@@ -3434,6 +3520,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,
@@ -3459,6 +3546,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'
@@ -3516,6 +3606,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,
@@ -3582,6 +3673,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,

manila/tests/share/test_manager.py

@@ -5986,6 +5986,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',
@@ -6044,6 +6047,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):
 
@@ -6118,6 +6126,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'])
@@ -6163,6 +6174,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):

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)

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 <https://bugs.launchpad.net/manila/+bug/1910752>`_
+    for more details.