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 <gouthampravi@gmail.com>
(cherry picked from commit 470bab7c71)
(cherry picked from commit 18430ceafe)
(cherry picked from commit 30473c3481)
(cherry picked from commit f08c8be437)

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(

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):

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']))

releasenotes/notes/bug-1699836-disallow-share-type-deletion-with-active-share-group-types-83809532d06ef0dd.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Fixed `Launchpad bug 1699836 <https://launchpad.net/bugs/1699836>`_ by
+    preventing share type deletion when there are share group types
+    associated with them.