From f08c8be4370e3ec23edb5dd7b7a76325e94dc650 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) --- 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 378fc3e37e..d9d0bcab95 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -3975,18 +3975,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()) @@ -4847,6 +4854,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 9faf598ff0..193fb2feca 100644 --- a/manila/exception.py +++ b/manila/exception.py @@ -632,8 +632,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 e2724903a6..e90a487b7e 100644 --- a/manila/tests/db/sqlalchemy/test_api.py +++ b/manila/tests/db/sqlalchemy/test_api.py @@ -1194,6 +1194,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): @@ -2994,29 +3045,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, @@ -3028,13 +3076,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.