From aae7bc723591728c6be2a500ca44af8e6a032268 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 (cherry picked from commit 4b6cfcf69023dc7a4b004c59513c50d92b979152) --- 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 cbc63fdcc0..d20f6dd92d 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 8d1aaf924f..5b4c9ec912 100644 --- a/manila/db/sqlalchemy/api.py +++ b/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, - 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 @@ -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, 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 97ff7d9ac2..e0d2aa43b1 100644 --- a/manila/tests/db/sqlalchemy/test_api.py +++ b/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() 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.