Merge "Destroy type quotas when a share type is deleted" into stable/rocky

manila/db/api.py

@@ -280,11 +280,10 @@ def quota_destroy_all_by_project_and_user(context, project_id, user_id):
                                                       project_id, user_id)
 
 
-def quota_destroy_all_by_project_and_share_type(context, project_id,
+def quota_destroy_all_by_share_type(context, share_type_id, project_id=None):
-                                                share_type_id):
+    """Destroy all quotas associated with a given share type and project."""
-    """Destroy all quotas associated with a given project and user."""
+    return IMPL.quota_destroy_all_by_share_type(
-    return IMPL.quota_destroy_all_by_project_and_share_type(
+        context, share_type_id, project_id=project_id)
-        context, project_id, share_type_id)
 
 
 def quota_destroy_all_by_project(context, project_id):

manila/db/sqlalchemy/api.py

@@ -1205,34 +1205,42 @@ def quota_destroy_all_by_project_and_user(context, project_id, user_id):
 
 
 @require_admin_context
-def quota_destroy_all_by_project_and_share_type(context, project_id,
+def quota_destroy_all_by_share_type(context, share_type_id, project_id=None):
-                                                share_type_id):
+    """Soft deletes all quotas, usages and reservations.
+
+    :param context: request context for queries, updates and logging
+    :param share_type_id: ID of the share type to filter the quotas, usages
+        and reservations under.
+    :param project_id: ID of the project to filter the quotas, usages and
+        reservations under. If not provided, share type quotas for all
+        projects will be acted upon.
+    """
     session = get_session()
     with session.begin():
-        model_query(
+        share_type_quotas = model_query(
             context, models.ProjectShareTypeQuota, session=session,
             read_deleted="no",
-        ).filter_by(
+        ).filter_by(share_type_id=share_type_id)
-            project_id=project_id,
-        ).filter_by(
-            share_type_id=share_type_id,
-        ).soft_delete(synchronize_session=False)
 
-        model_query(
+        share_type_quota_usages = model_query(
             context, models.QuotaUsage, session=session, read_deleted="no",
-        ).filter_by(
+        ).filter_by(share_type_id=share_type_id)
-            project_id=project_id,
-        ).filter_by(
-            share_type_id=share_type_id,
-        ).soft_delete(synchronize_session=False)
 
-        model_query(
+        share_type_quota_reservations = model_query(
             context, models.Reservation, session=session, read_deleted="no",
-        ).filter_by(
+        ).filter_by(share_type_id=share_type_id)
-            project_id=project_id,
+
-        ).filter_by(
+        if project_id is not None:
-            share_type_id=share_type_id,
+            share_type_quotas = share_type_quotas.filter_by(
-        ).soft_delete(synchronize_session=False)
+                project_id=project_id)
+            share_type_quota_usages = share_type_quota_usages.filter_by(
+                project_id=project_id)
+            share_type_quota_reservations = (
+                share_type_quota_reservations.filter_by(project_id=project_id))
+
+        share_type_quotas.soft_delete(synchronize_session=False)
+        share_type_quota_usages.soft_delete(synchronize_session=False)
+        share_type_quota_reservations.soft_delete(synchronize_session=False)
 
 
 @require_admin_context
@@ -3980,6 +3988,9 @@ def share_type_destroy(context, id):
         (model_query(context, models.ShareTypes, session=session).
             filter_by(id=id).soft_delete())
 
+    # Destroy any quotas, usages and reservations for the share type:
+    quota_destroy_all_by_share_type(context, id)
+
 
 def _share_type_access_query(context, session=None):
     return model_query(context, models.ShareTypeProjects, session=session,

manila/quota.py

@@ -581,8 +581,8 @@ class DbQuotaDriver(object):
         :param share_type_id: The UUID of the share type.
         """
 
-        db.quota_destroy_all_by_project_and_share_type(
+        db.quota_destroy_all_by_share_type(context, share_type_id,
-            context, project_id, share_type_id)
+                                           project_id=project_id)
 
     def expire(self, context):
         """Expire reservations.

manila/tests/db/sqlalchemy/test_api.py

@@ -2933,6 +2933,7 @@ class PurgeDeletedTest(test.TestCase):
         self.assertEqual(0, s_row + type_row)
 
 
+@ddt.ddt
 class ShareTypeAPITestCase(test.TestCase):
 
     def setUp(self):
@@ -2940,6 +2941,161 @@ class ShareTypeAPITestCase(test.TestCase):
         self.ctxt = context.RequestContext(
             user_id='user_id', project_id='project_id', is_admin=True)
 
+    @ddt.data({'used_by_shares': True, 'used_by_groups': False},
+              {'used_by_shares': False, 'used_by_groups': True},
+              {'used_by_shares': True, 'used_by_groups': True})
+    @ddt.unpack
+    def test_share_type_destroy_in_use(self, used_by_shares,
+                                       used_by_groups):
+        share_type_1 = db_utils.create_share_type(
+            name='orange', extra_specs={'somekey': 'someval'})
+        share_type_2 = db_utils.create_share_type(name='regalia')
+        if used_by_shares:
+            share_1 = db_utils.create_share(share_type_id=share_type_1['id'])
+            db_utils.create_share(share_type_id=share_type_2['id'])
+        if used_by_groups:
+            group_type_1 = db_utils.create_share_group_type(
+                name='crimson', share_types=[share_type_1['id']])
+            group_type_2 = db_utils.create_share_group_type(
+                name='tide', share_types=[share_type_2['id']])
+            share_group_1 = db_utils.create_share_group(
+                share_group_type_id=group_type_1['id'],
+                share_types=[share_type_1['id']])
+            db_utils.create_share_group(
+                share_group_type_id=group_type_2['id'],
+                share_types=[share_type_2['id']])
+
+        self.assertRaises(exception.ShareTypeInUse,
+                          db_api.share_type_destroy,
+                          self.ctxt, share_type_1['id'])
+        self.assertRaises(exception.ShareTypeInUse,
+                          db_api.share_type_destroy,
+                          self.ctxt, share_type_2['id'])
+
+        # Let's cleanup share_type_1 and verify it is gone
+        if used_by_shares:
+            db_api.share_instance_delete(self.ctxt, share_1.instance.id)
+        if used_by_groups:
+            db_api.share_group_destroy(self.ctxt, share_group_1['id'])
+            db_api.share_group_type_destroy(self.ctxt,
+                                            group_type_1['id'])
+
+        self.assertIsNone(db_api.share_type_destroy(
+            self.ctxt, share_type_1['id']))
+        self.assertDictMatch(
+            {}, db_api.share_type_extra_specs_get(
+                self.ctxt, share_type_1['id']))
+        self.assertRaises(exception.ShareTypeNotFound,
+                          db_api.share_type_get,
+                          self.ctxt, share_type_1['id'])
+
+        # share_type_2 must still be around
+        self.assertEqual(
+            share_type_2['id'],
+            db_api.share_type_get(self.ctxt, share_type_2['id'])['id'])
+
+    @ddt.data({'usages': False, 'reservations': False},
+              {'usages': False, 'reservations': True},
+              {'usages': True, 'reservations': False})
+    @ddt.unpack
+    def test_share_type_destroy_quotas_and_reservations(self, usages,
+                                                        reservations):
+        share_type = db_utils.create_share_type(name='clemsontigers')
+        shares_quota = db_api.quota_create(
+            self.ctxt, "fake-project-id", 'shares', 10,
+            share_type_id=share_type['id'])
+        snapshots_quota = db_api.quota_create(
+            self.ctxt, "fake-project-id", 'snapshots', 30,
+            share_type_id=share_type['id'])
+
+        if reservations:
+            resources = {
+                'shares': quota.ReservableResource('shares', '_sync_shares'),
+                'snapshots': quota.ReservableResource(
+                    'snapshots', '_sync_snapshots'),
+            }
+            project_quotas = {
+                'shares': shares_quota.hard_limit,
+                'snapshots': snapshots_quota.hard_limit,
+            }
+            user_quotas = {
+                'shares': shares_quota.hard_limit,
+                'snapshots': snapshots_quota.hard_limit,
+            }
+            deltas = {'shares': 1, 'snapshots': 3}
+            expire = timeutils.utcnow() + datetime.timedelta(seconds=86400)
+            reservation_uuids = db_api.quota_reserve(
+                self.ctxt, resources, project_quotas, user_quotas,
+                project_quotas, deltas, expire, False, 30,
+                project_id='fake-project-id', share_type_id=share_type['id'])
+
+            db_session = db_api.get_session()
+            q_reservations = db_api._quota_reservations_query(
+                db_session, self.ctxt, reservation_uuids).all()
+            # There should be 2 "user" reservations and 2 "share-type"
+            # quota reservations
+            self.assertEqual(4, len(q_reservations))
+            q_share_type_reservations = [qr for qr in q_reservations
+                                         if qr['share_type_id'] is not None]
+            # There should be exactly two "share type" quota reservations
+            self.assertEqual(2, len(q_share_type_reservations))
+            for q_reservation in q_share_type_reservations:
+                self.assertEqual(q_reservation['share_type_id'],
+                                 share_type['id'])
+
+        if usages:
+            db_api.quota_usage_create(self.ctxt, 'fake-project-id',
+                                      'fake-user-id', 'shares', 3, 2, False,
+                                      share_type_id=share_type['id'])
+            db_api.quota_usage_create(self.ctxt, 'fake-project-id',
+                                      'fake-user-id', 'snapshots', 2, 2, False,
+                                      share_type_id=share_type['id'])
+            q_usages = db_api.quota_usage_get_all_by_project_and_share_type(
+                self.ctxt, 'fake-project-id', share_type['id'])
+            self.assertEqual(3, q_usages['shares']['in_use'])
+            self.assertEqual(2, q_usages['shares']['reserved'])
+            self.assertEqual(2, q_usages['snapshots']['in_use'])
+            self.assertEqual(2, q_usages['snapshots']['reserved'])
+
+        # Validate that quotas exist
+        share_type_quotas = db_api.quota_get_all_by_project_and_share_type(
+            self.ctxt,  'fake-project-id', share_type['id'])
+        expected_quotas = {
+            'project_id': 'fake-project-id',
+            'share_type_id': share_type['id'],
+            'shares': 10,
+            'snapshots': 30,
+        }
+        self.assertDictMatch(expected_quotas, share_type_quotas)
+
+        db_api.share_type_destroy(self.ctxt, share_type['id'])
+
+        self.assertRaises(exception.ShareTypeNotFound,
+                          db_api.share_type_get,
+                          self.ctxt, share_type['id'])
+        # Quotas must be gone
+        share_type_quotas = db_api.quota_get_all_by_project_and_share_type(
+            self.ctxt, 'fake-project-id', share_type['id'])
+        self.assertEqual({'project_id': 'fake-project-id',
+                          'share_type_id': share_type['id']},
+                         share_type_quotas)
+
+        # Check usages and reservations
+        if usages:
+            q_usages = db_api.quota_usage_get_all_by_project_and_share_type(
+                self.ctxt, 'fake-project-id', share_type['id'])
+            expected_q_usages = {'project_id': 'fake-project-id',
+                                 'share_type_id': share_type['id']}
+            self.assertDictMatch(expected_q_usages, q_usages)
+        if reservations:
+            q_reservations = db_api._quota_reservations_query(
+                db_session, self.ctxt, reservation_uuids).all()
+            # just "user" quota reservations should be left, since we didn't
+            # clean them up.
+            self.assertEqual(2, len(q_reservations))
+            for q_reservation in q_reservations:
+                self.assertIsNone(q_reservation['share_type_id'])
+
     def test_share_type_get_by_name_or_id_found_by_id(self):
         share_type = db_utils.create_share_type()
 

manila/tests/db_utils.py

@@ -234,6 +234,18 @@ def create_share_type(**kwargs):
     return _create_db_row(db.share_type_create, share_type, kwargs)
 
 
+def create_share_group_type(**kwargs):
+    """Create a share group type object"""
+
+    share_group_type = {
+        'name': 'fake_group_type',
+        'is_public': True,
+    }
+
+    return _create_db_row(db.share_group_type_create, share_group_type,
+                          kwargs)
+
+
 def create_share_network(**kwargs):
     """Create a share network object."""
     net = {

manila/tests/test_quota.py

@@ -440,14 +440,14 @@ class DbQuotaDriverTestCase(test.TestCase):
 
     def test_destroy_all_by_project_and_share_type(self):
         mock_destroy_all = self.mock_object(
-            quota.db, 'quota_destroy_all_by_project_and_share_type')
+            quota.db, 'quota_destroy_all_by_share_type')
 
         result = self.driver.destroy_all_by_project_and_share_type(
             self.ctxt, self.project_id, self.share_type_id)
 
         self.assertIsNone(result)
         mock_destroy_all.assert_called_once_with(
-            self.ctxt, self.project_id, self.share_type_id)
+            self.ctxt, self.share_type_id, project_id=self.project_id)
 
     def test_expire(self):
         self.mock_object(quota.db, 'reservation_expire')

releasenotes/notes/bug-1811680-destroy-quotas-usages-reservations-when-deleting-share-type-a18f2e00a65fe922.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Share type quotas, usages and reservations will now be correctly cleaned
+    up if a share type has been deleted. See `launchpad bug #1811680
+    <https://bugs.launchpad.net/manila/+bug/1811680>`_ for details regarding
+    the bug that prevented this cleanup prior.