From 1f91b438d710547b9b8864c9d58695643f434c7e Mon Sep 17 00:00:00 2001 From: Goutham Pacha Ravi Date: Sat, 4 Apr 2020 16:57:22 -0700 Subject: [PATCH] Prevent share type deletion if linked to group types Share types can be associated with share group types. This relationship needs to be dropped in case share types are to be deleted, or we'll be leaving behind dead references in the database. Share groups cannot exist without share group types, and hence we don't need to check on existing groups when deleting share types; checking for existing group types will suffice. Change-Id: Id86a93f1923a493cefe73e2d90d5094005b42ae4 Closes-Bug: #1699836 Signed-off-by: Goutham Pacha Ravi (cherry picked from commit 470bab7c712b41fb22d2fd48ae6e43505525db31) (cherry picked from commit 18430ceafe2d5b46d496cf6ef33d5f1825cb82b8) (cherry picked from commit 30473c34810665dda1830d516951694ada09d198) (cherry picked from commit f08c8be4370e3ec23edb5dd7b7a76325e94dc650) --- manila/db/sqlalchemy/api.py | 28 +++++--- manila/exception.py | 4 +- manila/tests/db/sqlalchemy/test_api.py | 72 +++++++++++++++---- ...ve-share-group-types-83809532d06ef0dd.yaml | 6 ++ 4 files changed, 88 insertions(+), 22 deletions(-) create mode 100644 releasenotes/notes/bug-1699836-disallow-share-type-deletion-with-active-share-group-types-83809532d06ef0dd.yaml diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index 3a5e8d1653..c954080c4d 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -3908,18 +3908,25 @@ def share_type_destroy(context, id): session = get_session() with session.begin(): _share_type_get(context, id, session) - results = (model_query(context, models.ShareInstance, session=session, - read_deleted="no"). - filter_by(share_type_id=id).count()) - share_group_count = model_query( + shares_count = model_query( context, - models.ShareGroupShareTypeMapping, + models.ShareInstance, read_deleted="no", session=session, ).filter_by(share_type_id=id).count() - if results or share_group_count: - LOG.error('ShareType %s deletion failed, ShareType in use.', - id) + share_group_types_count = model_query( + context, + models.ShareGroupTypeShareTypeMapping, + read_deleted="no", + session=session, + ).filter_by(share_type_id=id).count() + if shares_count or share_group_types_count: + msg = ("Deletion of share type %(stype)s failed; it in use by " + "%(shares)d shares and %(gtypes)d share group types") + msg_args = {'stype': id, + 'shares': shares_count, + 'gtypes': share_group_types_count} + LOG.error(msg, msg_args) raise exception.ShareTypeInUse(share_type_id=id) (model_query(context, models.ShareTypeExtraSpecs, session=session). filter_by(share_type_id=id).soft_delete()) @@ -4780,6 +4787,11 @@ def share_group_type_destroy(context, type_id): ).filter_by( share_group_type_id=type_id, ).soft_delete() + model_query( + context, models.ShareGroupTypeShareTypeMapping, session=session + ).filter_by( + share_group_type_id=type_id, + ).soft_delete() model_query( context, models.ShareGroupTypes, session=session ).filter_by( diff --git a/manila/exception.py b/manila/exception.py index 024e57ea69..8448bfd477 100644 --- a/manila/exception.py +++ b/manila/exception.py @@ -628,8 +628,8 @@ class ShareGroupTypeSpecsNotFound(NotFound): class ShareTypeInUse(ManilaException): - message = _("Share Type %(share_type_id)s deletion is not allowed with " - "shares present with the type.") + message = _("Share Type %(share_type_id)s deletion is not allowed while " + "shares or share group types are associated with the type.") class IPAddressInUse(InUse): diff --git a/manila/tests/db/sqlalchemy/test_api.py b/manila/tests/db/sqlalchemy/test_api.py index e6e7a1b517..287dff5c69 100644 --- a/manila/tests/db/sqlalchemy/test_api.py +++ b/manila/tests/db/sqlalchemy/test_api.py @@ -1123,6 +1123,57 @@ class ShareGroupDatabaseAPITestCase(test.TestCase): self.assertEqual(constants.STATUS_AVAILABLE, member['status']) +@ddt.ddt +class ShareGroupTypeAPITestCase(test.TestCase): + + def setUp(self): + super(ShareGroupTypeAPITestCase, self).setUp() + self.ctxt = context.RequestContext( + user_id='user_id', project_id='project_id', is_admin=True) + + @ddt.data(True, False) + def test_share_type_destroy_in_use(self, used_by_groups): + share_type_1 = db_utils.create_share_type(name='fike') + share_type_2 = db_utils.create_share_type(name='bowman') + share_group_type_1 = db_utils.create_share_group_type( + name='orange', is_public=False, share_types=[share_type_1['id']], + group_specs={'dabo': 'allin', 'cadence': 'count'}, + override_defaults=True) + share_group_type_2 = db_utils.create_share_group_type( + name='regalia', share_types=[share_type_2['id']]) + if used_by_groups: + share_group_1 = db_utils.create_share_group( + share_group_type_id=share_group_type_1['id'], + share_types=[share_type_1['id']]) + share_group_2 = db_utils.create_share_group( + share_group_type_id=share_group_type_2['id'], + share_types=[share_type_2['id']]) + self.assertRaises(exception.ShareGroupTypeInUse, + db_api.share_group_type_destroy, + self.ctxt, share_group_type_1['id']) + self.assertRaises(exception.ShareGroupTypeInUse, + db_api.share_group_type_destroy, + self.ctxt, share_group_type_2['id']) + # Cleanup share groups + db_api.share_group_destroy(self.ctxt, share_group_1['id']) + db_api.share_group_destroy(self.ctxt, share_group_2['id']) + + # Let's cleanup share_group_type_1 and verify it is gone + self.assertIsNone(db_api.share_group_type_destroy( + self.ctxt, share_group_type_1['id'])) + self.assertDictMatch( + {}, db_api.share_group_type_specs_get( + self.ctxt, share_group_type_1['id'])) + self.assertRaises(exception.ShareGroupTypeNotFound, + db_api.share_group_type_get, + self.ctxt, share_group_type_1['id']) + + # share_group_type_2 must still be around + self.assertEqual(share_group_type_2['id'], + db_api.share_group_type_get( + self.ctxt, share_group_type_2['id'])['id']) + + @ddt.ddt class ShareSnapshotDatabaseAPITestCase(test.TestCase): @@ -2921,29 +2972,26 @@ 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.data({'used_by_shares': True, 'used_by_group_types': False}, + {'used_by_shares': False, 'used_by_group_types': True}, + {'used_by_shares': True, 'used_by_group_types': True}) @ddt.unpack def test_share_type_destroy_in_use(self, used_by_shares, - used_by_groups): + used_by_group_types): 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: + if used_by_group_types: 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( + 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, @@ -2955,13 +3003,13 @@ class ShareTypeAPITestCase(test.TestCase): # 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: + if used_by_group_types: 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.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'])) diff --git a/releasenotes/notes/bug-1699836-disallow-share-type-deletion-with-active-share-group-types-83809532d06ef0dd.yaml b/releasenotes/notes/bug-1699836-disallow-share-type-deletion-with-active-share-group-types-83809532d06ef0dd.yaml new file mode 100644 index 0000000000..5f8af0c50c --- /dev/null +++ b/releasenotes/notes/bug-1699836-disallow-share-type-deletion-with-active-share-group-types-83809532d06ef0dd.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Fixed `Launchpad bug 1699836 `_ by + preventing share type deletion when there are share group types + associated with them.