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.