From 4b6cfcf69023dc7a4b004c59513c50d92b979152 Mon Sep 17 00:00:00 2001 From: Goutham Pacha Ravi Date: Fri, 15 Mar 2019 00:07:32 -0700 Subject: [PATCH] Destroy type quotas when a share type is deleted Upon share type deletion, we were leaving behind share type quotas, usages and reservations. These couldn't be cleaned up without resorting to manual intervention. Cleanup quotas when a share type is being destroyed, since the resources that they pertained to are gone too. Closes-Bug: #1811680 Change-Id: I04b1fe88808596aa8c05429b964190d654587f9a --- manila/db/api.py | 9 +- manila/db/sqlalchemy/api.py | 51 +++--- manila/quota.py | 4 +- manila/tests/db/sqlalchemy/test_api.py | 156 ++++++++++++++++++ manila/tests/db_utils.py | 12 ++ manila/tests/test_quota.py | 4 +- ...-deleting-share-type-a18f2e00a65fe922.yaml | 7 + 7 files changed, 214 insertions(+), 29 deletions(-) create mode 100644 releasenotes/notes/bug-1811680-destroy-quotas-usages-reservations-when-deleting-share-type-a18f2e00a65fe922.yaml diff --git a/manila/db/api.py b/manila/db/api.py index 213c697bf6..893bb757c4 100644 --- a/manila/db/api.py +++ b/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, - share_type_id): - """Destroy all quotas associated with a given project and user.""" - return IMPL.quota_destroy_all_by_project_and_share_type( - context, project_id, share_type_id) +def quota_destroy_all_by_share_type(context, share_type_id, project_id=None): + """Destroy all quotas associated with a given share type and project.""" + return IMPL.quota_destroy_all_by_share_type( + context, share_type_id, project_id=project_id) def quota_destroy_all_by_project(context, project_id): diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index 3401b21068..787918240e 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -1206,34 +1206,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, - share_type_id): +def quota_destroy_all_by_share_type(context, share_type_id, project_id=None): + """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( - project_id=project_id, - ).filter_by( - share_type_id=share_type_id, - ).soft_delete(synchronize_session=False) + ).filter_by(share_type_id=share_type_id) - model_query( + share_type_quota_usages = model_query( context, models.QuotaUsage, session=session, read_deleted="no", - ).filter_by( - project_id=project_id, - ).filter_by( - share_type_id=share_type_id, - ).soft_delete(synchronize_session=False) + ).filter_by(share_type_id=share_type_id) - model_query( + share_type_quota_reservations = model_query( context, models.Reservation, session=session, read_deleted="no", - ).filter_by( - project_id=project_id, - ).filter_by( - share_type_id=share_type_id, - ).soft_delete(synchronize_session=False) + ).filter_by(share_type_id=share_type_id) + + if project_id is not None: + share_type_quotas = share_type_quotas.filter_by( + 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 @@ -4070,6 +4078,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, diff --git a/manila/quota.py b/manila/quota.py index 670f4aa5c8..72ca8b101f 100644 --- a/manila/quota.py +++ b/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( - context, project_id, share_type_id) + db.quota_destroy_all_by_share_type(context, share_type_id, + project_id=project_id) def expire(self, context): """Expire reservations. diff --git a/manila/tests/db/sqlalchemy/test_api.py b/manila/tests/db/sqlalchemy/test_api.py index 5e6d8c054f..d2040ed613 100644 --- a/manila/tests/db/sqlalchemy/test_api.py +++ b/manila/tests/db/sqlalchemy/test_api.py @@ -3082,6 +3082,7 @@ class PurgeDeletedTest(test.TestCase): self.assertEqual(0, s_row + type_row) +@ddt.ddt class ShareTypeAPITestCase(test.TestCase): def setUp(self): @@ -3089,6 +3090,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() diff --git a/manila/tests/db_utils.py b/manila/tests/db_utils.py index a1fffdbda1..d935b325d7 100644 --- a/manila/tests/db_utils.py +++ b/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 = { diff --git a/manila/tests/test_quota.py b/manila/tests/test_quota.py index 8277ce41e1..512a800ff6 100644 --- a/manila/tests/test_quota.py +++ b/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') diff --git a/releasenotes/notes/bug-1811680-destroy-quotas-usages-reservations-when-deleting-share-type-a18f2e00a65fe922.yaml b/releasenotes/notes/bug-1811680-destroy-quotas-usages-reservations-when-deleting-share-type-a18f2e00a65fe922.yaml new file mode 100644 index 0000000000..6378cd7aa7 --- /dev/null +++ b/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 + `_ for details regarding + the bug that prevented this cleanup prior.