Fix delete when share not found in update_access

Whenever drivers throw exceptions during update_access,
share cannot be deleted, not even with force-delete.
Some drivers already do not throw exception in delete_share
in such cases, but update_access should still throw exception
if share is not found when allow_access or deny_access
are called.
This patch adds possibility for driver throwing a
ShareResourceNotFound exception to let the manager
know share does not exist in backend anymore.
Drivers that already handle this case in delete_share
(by not throwing exception) only need to change update_access.
Additionally, adding possibility of share being completely
deletable if force-delete is specified in API call.

Closes-bug: #1550377
Change-Id: Iccce421f60234bc031f01370319a8918104b099b

manila/share/access.py

@@ -17,7 +17,6 @@ from oslo_log import log
 import six
 
 from manila.common import constants
-from manila import exception
 from manila.i18n import _LI
 
 LOG = log.getLogger(__name__)
@@ -86,14 +85,12 @@ class ShareInstanceAccess(object):
                 self._update_access_fallback(add_rules, context, delete_rules,
                                              remove_rules, share_instance,
                                              share_server)
-        except Exception as e:
-            error = six.text_type(e)
-            LOG.exception(error)
+        except Exception:
             self.db.share_instance_update_access_status(
                 context,
                 share_instance['id'],
                 constants.STATUS_ERROR)
-            raise exception.ManilaException(message=error)
+            raise
 
         # NOTE(ganso): remove rules after maintenance is complete
         if remove_rules:

manila/share/api.py

@@ -641,7 +641,8 @@ class API(base.Base):
              'terminated_at': timeutils.utcnow()}
         )
 
-        self.share_rpcapi.delete_share_instance(context, share_instance)
+        self.share_rpcapi.delete_share_instance(context, share_instance,
+                                                force=force)
 
         # NOTE(u_glide): 'updated_at' timestamp is used to track last usage of
         # share server. This is required for automatic share servers cleanup

manila/share/manager.py

@@ -1685,7 +1685,7 @@ class ShareManager(manager.SchedulerDependentManager):
 
     @add_hooks
     @utils.require_driver_initialized
-    def delete_share_instance(self, context, share_instance_id):
+    def delete_share_instance(self, context, share_instance_id, force=False):
         """Delete a share instance."""
         context = context.elevated()
         share_instance = self._get_share_instance(context, share_instance_id)
@@ -1698,14 +1698,44 @@ class ShareManager(manager.SchedulerDependentManager):
                 delete_rules="all",
                 share_server=share_server
             )
+        except exception.ShareResourceNotFound:
+            LOG.warning(_LW("Share instance %s does not exist in the "
+                            "backend."), share_instance_id)
+        except Exception:
+            with excutils.save_and_reraise_exception() as exc_context:
+                if force:
+                    msg = _LE("The driver was unable to delete access rules "
+                              "for the instance: %s. Will attempt to delete "
+                              "the instance anyway.")
+                    LOG.error(msg, share_instance_id)
+                    exc_context.reraise = False
+                else:
+                    self.db.share_instance_update(
+                        context,
+                        share_instance_id,
+                        {'status': constants.STATUS_ERROR_DELETING})
+
+        try:
             self.driver.delete_share(context, share_instance,
                                      share_server=share_server)
+        except exception.ShareResourceNotFound:
+            LOG.warning(_LW("Share instance %s does not exist in the "
+                            "backend."), share_instance_id)
         except Exception:
-            with excutils.save_and_reraise_exception():
-                self.db.share_instance_update(
-                    context,
-                    share_instance_id,
-                    {'status': constants.STATUS_ERROR_DELETING})
+            with excutils.save_and_reraise_exception() as exc_context:
+                if force:
+                    msg = _LE("The driver was unable to delete the share "
+                              "instance: %s on the backend. Since this "
+                              "operation is forced, the instance will be "
+                              "deleted from Manila's database. A cleanup on "
+                              "the backend may be necessary.")
+                    LOG.error(msg, share_instance_id)
+                    exc_context.reraise = False
+                else:
+                    self.db.share_instance_update(
+                        context,
+                        share_instance_id,
+                        {'status': constants.STATUS_ERROR_DELETING})
 
         self.db.share_instance_delete(context, share_instance_id)
         LOG.info(_LI("Share instance %s: deleted successfully."),

manila/share/rpcapi.py

@@ -109,12 +109,13 @@ class ShareAPI(object):
                           'unmanage_snapshot',
                           snapshot_id=snapshot['id'])
 
-    def delete_share_instance(self, context, share_instance):
+    def delete_share_instance(self, context, share_instance, force=False):
         host = utils.extract_host(share_instance['host'])
         call_context = self.client.prepare(server=host, version='1.4')
         call_context.cast(context,
                           'delete_share_instance',
-                          share_instance_id=share_instance['id'])
+                          share_instance_id=share_instance['id'],
+                          force=force)
 
     def migration_start(self, context, share, dest_host, force_host_copy,
                         notify):

manila/tests/share/test_api.py

@@ -1168,7 +1168,7 @@ class ShareAPITestCase(test.TestCase):
              'terminated_at': self.dt_utc}
         )
         self.api.share_rpcapi.delete_share_instance.assert_called_once_with(
-            self.context, instance
+            self.context, instance, force=force
         )
         db_api.share_server_update(
             self.context,

manila/tests/share/test_manager.py

@@ -1563,27 +1563,32 @@ class ShareManagerTestCase(test.TestCase):
         self.share_manager._setup_server.assert_called_once_with(
             utils.IsAMatcher(context.RequestContext), fake_server)
 
-    def test_create_delete_share_instance_error(self):
+    @ddt.data(True, False)
+    def test_create_delete_share_instance_error(self, exception_update_access):
         """Test share can be created and deleted with error."""
 
-        def _raise_not_found(self, *args, **kwargs):
-            raise exception.NotFound()
+        def _raise_exception(self, *args, **kwargs):
+            raise exception.ManilaException('fake')
 
         self.mock_object(self.share_manager.driver, "create_share",
-                         mock.Mock(side_effect=_raise_not_found))
+                         mock.Mock(side_effect=_raise_exception))
         self.mock_object(self.share_manager.driver, "delete_share",
-                         mock.Mock(side_effect=_raise_not_found))
+                         mock.Mock(side_effect=_raise_exception))
+        if exception_update_access:
+            self.mock_object(
+                self.share_manager.access_helper, "update_access_rules",
+                mock.Mock(side_effect=_raise_exception))
 
         share = db_utils.create_share()
         share_id = share['id']
-        self.assertRaises(exception.NotFound,
+        self.assertRaises(exception.ManilaException,
                           self.share_manager.create_share_instance,
                           self.context,
                           share.instance['id'])
 
         shr = db.share_get(self.context, share_id)
         self.assertEqual(constants.STATUS_ERROR, shr['status'])
-        self.assertRaises(exception.NotFound,
+        self.assertRaises(exception.ManilaException,
                           self.share_manager.delete_share_instance,
                           self.context,
                           share.instance['id'])
@@ -1594,10 +1599,11 @@ class ShareManagerTestCase(test.TestCase):
             utils.IsAMatcher(context.RequestContext),
             utils.IsAMatcher(models.ShareInstance),
             share_server=None)
-        self.share_manager.driver.delete_share.assert_called_once_with(
-            utils.IsAMatcher(context.RequestContext),
-            utils.IsAMatcher(models.ShareInstance),
-            share_server=None)
+        if not exception_update_access:
+            self.share_manager.driver.delete_share.assert_called_once_with(
+                utils.IsAMatcher(context.RequestContext),
+                utils.IsAMatcher(models.ShareInstance),
+                share_server=None)
 
     def test_create_share_instance_update_availability_zone(self):
         share = db_utils.create_share(availability_zone=None)
@@ -2064,7 +2070,11 @@ class ShareManagerTestCase(test.TestCase):
             security_services=[jsonutils.loads(
                 backend_details['security_service_ldap'])])
 
-    def test_delete_share_instance_last_on_server(self):
+    @ddt.data({'force': True, 'side_effect': 'update_access'},
+              {'force': True, 'side_effect': 'delete_share'},
+              {'force': False, 'side_effect': None})
+    @ddt.unpack
+    def test_delete_share_instance_last_on_server(self, force, side_effect):
         share_net = db_utils.create_share_network()
         share_srv = db_utils.create_share_server(
             share_network_id=share_net['id'],
@@ -2074,12 +2084,21 @@ class ShareManagerTestCase(test.TestCase):
                                       share_server_id=share_srv['id'])
 
         self.share_manager.driver = mock.Mock()
+        if side_effect == 'update_access':
+            self.mock_object(
+                self.share_manager.access_helper, 'update_access_rules',
+                mock.Mock(side_effect=Exception('fake')))
+        if side_effect == 'delete_share':
+            self.mock_object(self.share_manager.driver, 'delete_share',
+                             mock.Mock(side_effect=Exception('fake')))
+        self.mock_object(manager.LOG, 'error')
         manager.CONF.delete_share_server_with_last_share = True
-        self.share_manager.delete_share_instance(self.context,
-                                                 share.instance['id'])
+        self.share_manager.delete_share_instance(
+            self.context, share.instance['id'], force=force)
         self.share_manager.driver.teardown_server.assert_called_once_with(
             server_details=share_srv.get('backend_details'),
             security_services=[])
+        self.assertEqual(force, manager.LOG.error.called)
 
     def test_delete_share_instance_last_on_server_deletion_disabled(self):
         share_net = db_utils.create_share_network()
@@ -2113,6 +2132,52 @@ class ShareManagerTestCase(test.TestCase):
                                                  share.instance['id'])
         self.assertFalse(self.share_manager.driver.teardown_network.called)
 
+    @ddt.data('update_access', 'delete_share')
+    def test_delete_share_instance_not_found(self, side_effect):
+        share_net = db_utils.create_share_network()
+        share_srv = db_utils.create_share_server(
+            share_network_id=share_net['id'],
+            host=self.share_manager.host)
+        share = db_utils.create_share(share_network_id=share_net['id'],
+                                      share_server_id=share_srv['id'])
+        access = db_utils.create_access(share_id=share['id'])
+        db_utils.create_share(share_network_id=share_net['id'],
+                              share_server_id=share_srv['id'])
+
+        manager.CONF.delete_share_server_with_last_share = False
+
+        self.mock_object(db, 'share_server_get',
+                         mock.Mock(return_value=share_srv))
+        self.mock_object(db, 'share_instance_get',
+                         mock.Mock(return_value=share.instance))
+        self.mock_object(db, 'share_access_get_all_for_instance',
+                         mock.Mock(return_value=[access]))
+        self.share_manager.driver = mock.Mock()
+        self.share_manager.access_helper.driver = mock.Mock()
+        if side_effect == 'update_access':
+            self.mock_object(
+                self.share_manager.access_helper.driver, 'update_access',
+                mock.Mock(side_effect=exception.ShareResourceNotFound(
+                    share_id=share['id'])))
+        if side_effect == 'delete_share':
+            self.mock_object(
+                self.share_manager.driver, 'delete_share',
+                mock.Mock(side_effect=exception.ShareResourceNotFound(
+                    share_id=share['id'])))
+
+        self.mock_object(manager.LOG, 'warning')
+
+        self.share_manager.delete_share_instance(self.context,
+                                                 share.instance['id'])
+        self.assertFalse(self.share_manager.driver.teardown_network.called)
+
+        (self.share_manager.access_helper.driver.update_access.
+            assert_called_once_with(utils.IsAMatcher(
+                context.RequestContext), share.instance, [], add_rules=[],
+                delete_rules=[access], share_server=share_srv))
+
+        self.assertTrue(manager.LOG.warning.called)
+
     def test_allow_deny_access(self):
         """Test access rules to share can be created and deleted."""
         self.mock_object(share_access.LOG, 'info')

manila/tests/share/test_rpcapi.py

@@ -163,7 +163,8 @@ class ShareRpcAPITestCase(test.TestCase):
         self._test_share_api('delete_share_instance',
                              rpc_method='cast',
                              version='1.4',
-                             share_instance=self.fake_share)
+                             share_instance=self.fake_share,
+                             force=False)
 
     def test_allow_access(self):
         self._test_share_api('allow_access',