Make share service understand driver init failure

Recent change [1] made share manager ignoring all driver setup errors
and continue run process. So, if driver init failed and we do not have
errors in "stats reporting" then manager/driver will report that
it is OK and scheduler will consider it alive when it is not indeed.

So, add attr 'initialized' to driver base class that will be used by
share manager to get to know is it allowed to call driver methods or
not.
Manila-share service will still run, but will be marked as 'down'.

[1] I5c4c551da9d3576ea49118ef09655d2939990cb2

Change-Id: I15d1f2aaf1d7cec6e8afb93fc32e46a877261d4e
Closes-Bug: #1502809

manila/exception.py

@@ -672,3 +672,7 @@ class InvalidConsistencyGroup(Invalid):
 
 class InvalidCGSnapshot(Invalid):
     message = _("Invalid CGSnapshot: %(reason)s")
+
+
+class DriverNotInitialized(ManilaException):
+    message = _("Share driver '%(driver)s' not initialized.")

manila/share/driver.py

@@ -221,6 +221,7 @@ class ShareDriver(object):
         """
         super(ShareDriver, self).__init__()
         self.configuration = kwargs.get('configuration', None)
+        self.initialized = False
         self._stats = {}
 
         self.pools = []

manila/share/manager.py

@@ -210,12 +210,21 @@ class ShareManager(manager.SchedulerDependentManager):
         try:
             self.driver.do_setup(ctxt)
             self.driver.check_for_setup_error()
-        except Exception:
-            LOG.exception(_LE("Failed to initialize driver %s"),
-                          self.driver.__class__.__name__)
+        except Exception as e:
+            LOG.exception(
+                _LE("Error encountered during initialization of driver "
+                    "'%(name)s' on '%(host)s' host. %(exc)s"), {
+                        "name": self.driver.__class__.__name__,
+                        "host": self.host,
+                        "exc": e,
+                }
+            )
+            self.driver.initialized = False
             # we don't want to continue since we failed
             # to initialize the driver correctly.
             return
+        else:
+            self.driver.initialized = True
 
         share_instances = self.db.share_instances_get_all_by_host(ctxt,
                                                                   self.host)
@@ -524,6 +533,7 @@ class ShareManager(manager.SchedulerDependentManager):
         return self.driver.get_driver_migration_info(ctxt, share_instance,
                                                      share_server)
 
+    @utils.require_driver_initialized
     def migrate_share(self, ctxt, share_id, host, force_host_copy=False):
         """Migrates a share from current host to another host."""
         LOG.debug("Entered migrate_share method for share %s." % share_id)
@@ -680,6 +690,7 @@ class ShareManager(manager.SchedulerDependentManager):
         return self.db.share_instance_get(context, id, with_share_data=True)
 
     @add_hooks
+    @utils.require_driver_initialized
     def create_share_instance(self, context, share_instance_id,
                               request_spec=None, filter_properties=None,
                               snapshot_id=None):
@@ -785,6 +796,7 @@ class ShareManager(manager.SchedulerDependentManager):
             )
 
     @add_hooks
+    @utils.require_driver_initialized
     def manage_share(self, context, share_id, driver_options):
         context = context.elevated()
         share_ref = self.db.share_get(context, share_id)
@@ -860,6 +872,7 @@ class ShareManager(manager.SchedulerDependentManager):
                                            user_id, resource, usage)
 
     @add_hooks
+    @utils.require_driver_initialized
     def unmanage_share(self, context, share_id):
         context = context.elevated()
         share_ref = self.db.share_get(context, share_id)
@@ -918,6 +931,7 @@ class ShareManager(manager.SchedulerDependentManager):
                               'deleted': True})
 
     @add_hooks
+    @utils.require_driver_initialized
     def delete_share_instance(self, context, share_instance_id):
         """Delete a share instance."""
         context = context.elevated()
@@ -950,6 +964,7 @@ class ShareManager(manager.SchedulerDependentManager):
                 self.delete_share_server(context, share_server)
 
     @periodic_task.periodic_task(spacing=600)
+    @utils.require_driver_initialized
     def delete_free_share_servers(self, ctxt):
         if not (self.driver.driver_handles_share_servers and
                 self.configuration.automatic_share_server_cleanup):
@@ -973,6 +988,7 @@ class ShareManager(manager.SchedulerDependentManager):
                               share_instance, share_server)
 
     @add_hooks
+    @utils.require_driver_initialized
     def create_snapshot(self, context, share_id, snapshot_id):
         """Create snapshot for share."""
         snapshot_ref = self.db.share_snapshot_get(context, snapshot_id)
@@ -1008,6 +1024,7 @@ class ShareManager(manager.SchedulerDependentManager):
         return snapshot_id
 
     @add_hooks
+    @utils.require_driver_initialized
     def delete_snapshot(self, context, snapshot_id):
         """Delete share snapshot."""
         context = context.elevated()
@@ -1053,6 +1070,7 @@ class ShareManager(manager.SchedulerDependentManager):
                 QUOTAS.commit(context, reservations, project_id=project_id)
 
     @add_hooks
+    @utils.require_driver_initialized
     def allow_access(self, context, share_instance_id, access_id):
         """Allow access to some share instance."""
         access_mapping = self.db.share_instance_access_get(context, access_id,
@@ -1083,6 +1101,7 @@ class ShareManager(manager.SchedulerDependentManager):
                   'share_instance_id': share_instance_id})
 
     @add_hooks
+    @utils.require_driver_initialized
     def deny_access(self, context, share_instance_id, access_id):
         """Deny access to some share."""
         access_ref = self.db.share_access_get(context, access_id)
@@ -1109,6 +1128,7 @@ class ShareManager(manager.SchedulerDependentManager):
                     context, access_mapping['id'], access_mapping.STATE_ERROR)
 
     @periodic_task.periodic_task(spacing=CONF.periodic_interval)
+    @utils.require_driver_initialized
     def _report_driver_status(self, context):
         LOG.info(_LI('Updating share status'))
         share_stats = self.driver.get_share_stats(refresh=True)
@@ -1123,6 +1143,7 @@ class ShareManager(manager.SchedulerDependentManager):
         self.update_service_capabilities(share_stats)
 
     @periodic_task.periodic_task(spacing=CONF.periodic_hooks_interval)
+    @utils.require_driver_initialized
     def _execute_periodic_hook(self, context):
         """Executes periodic-based hooks."""
         # TODO(vponomaryov): add also access rules and share servers
@@ -1143,6 +1164,7 @@ class ShareManager(manager.SchedulerDependentManager):
                     for server in share_servers)
 
     @add_hooks
+    @utils.require_driver_initialized
     def publish_service_capabilities(self, context):
         """Collect driver status and then publish it."""
         self._report_driver_status(context)
@@ -1290,6 +1312,7 @@ class ShareManager(manager.SchedulerDependentManager):
                 reason=msg % network_info['segmentation_id'])
 
     @add_hooks
+    @utils.require_driver_initialized
     def delete_share_server(self, context, share_server):
 
         @utils.synchronized(
@@ -1348,6 +1371,7 @@ class ShareManager(manager.SchedulerDependentManager):
                 "between 10 minutes and 1 hour.")
 
     @add_hooks
+    @utils.require_driver_initialized
     def extend_share(self, context, share_id, new_size, reservations):
         context = context.elevated()
         share = self.db.share_get(context, share_id)
@@ -1384,6 +1408,7 @@ class ShareManager(manager.SchedulerDependentManager):
         LOG.info(_LI("Extend share completed successfully."), resource=share)
 
     @add_hooks
+    @utils.require_driver_initialized
     def shrink_share(self, context, share_id, new_size):
         context = context.elevated()
         share = self.db.share_get(context, share_id)
@@ -1439,6 +1464,7 @@ class ShareManager(manager.SchedulerDependentManager):
 
         LOG.info(_LI("Shrink share completed successfully."), resource=share)
 
+    @utils.require_driver_initialized
     def create_consistency_group(self, context, cg_id):
         context = context.elevated()
         group_ref = self.db.consistency_group_get(context, cg_id)
@@ -1546,6 +1572,7 @@ class ShareManager(manager.SchedulerDependentManager):
 
         return group_ref['id']
 
+    @utils.require_driver_initialized
     def delete_consistency_group(self, context, cg_id):
         context = context.elevated()
         group_ref = self.db.consistency_group_get(context, cg_id)
@@ -1587,6 +1614,7 @@ class ShareManager(manager.SchedulerDependentManager):
 
         # TODO(ameade): Add notification for delete.end
 
+    @utils.require_driver_initialized
     def create_cgsnapshot(self, context, cgsnapshot_id):
         context = context.elevated()
         snap_ref = self.db.cgsnapshot_get(context, cgsnapshot_id)
@@ -1643,6 +1671,7 @@ class ShareManager(manager.SchedulerDependentManager):
 
         return snap_ref['id']
 
+    @utils.require_driver_initialized
     def delete_cgsnapshot(self, context, cgsnapshot_id):
         context = context.elevated()
         snap_ref = self.db.cgsnapshot_get(context, cgsnapshot_id)

manila/tests/share/test_manager.py

@@ -53,6 +53,7 @@ class ShareManagerTestCase(test.TestCase):
         self.mock_object(self.share_manager.driver, 'do_setup')
         self.mock_object(self.share_manager.driver, 'check_for_setup_error')
         self.context = context.get_admin_context()
+        self.share_manager.driver.initialized = True
 
     def test_share_manager_instance(self):
         fake_service_name = "fake_service"
@@ -129,6 +130,7 @@ class ShareManagerTestCase(test.TestCase):
 
         self.share_manager.init_host()
 
+        self.assertTrue(self.share_manager.driver.initialized)
         self.share_manager.db.share_instances_get_all_by_host.\
             assert_called_once_with(utils.IsAMatcher(context.RequestContext),
                                     self.share_manager.host)
@@ -137,25 +139,53 @@ class ShareManagerTestCase(test.TestCase):
         self.share_manager.driver.check_for_setup_error.\
             assert_called_once_with()
 
-    def test_init_host_with_driver_do_setup_fail(self):
+    @ddt.data(
+        "migrate_share",
+        "create_share_instance",
+        "manage_share",
+        "unmanage_share",
+        "delete_share_instance",
+        "delete_free_share_servers",
+        "create_snapshot",
+        "delete_snapshot",
+        "allow_access",
+        "deny_access",
+        "_report_driver_status",
+        "_execute_periodic_hook",
+        "publish_service_capabilities",
+        "delete_share_server",
+        "extend_share",
+        "shrink_share",
+        "create_consistency_group",
+        "delete_consistency_group",
+        "create_cgsnapshot",
+        "delete_cgsnapshot",
+    )
+    def test_call_driver_when_its_init_failed(self, method_name):
         self.mock_object(self.share_manager.driver, 'do_setup',
                          mock.Mock(side_effect=Exception()))
-        self.mock_object(manager.LOG, 'exception')
-
         self.share_manager.init_host()
 
-        manager.LOG.exception.assert_called_with(
-            mock.ANY, self.share_manager.driver.__class__.__name__)
+        self.assertRaises(
+            exception.DriverNotInitialized,
+            getattr(self.share_manager, method_name),
+            'foo', 'bar', 'quuz'
+        )
 
-    def test_init_host_with_driver_check_for_setup_error_fail(self):
-        self.mock_object(self.share_manager.driver, 'check_for_setup_error',
+    @ddt.data("do_setup", "check_for_setup_error")
+    def test_init_host_with_driver_failure(self, method_name):
+        self.mock_object(self.share_manager.driver, method_name,
                          mock.Mock(side_effect=Exception()))
         self.mock_object(manager.LOG, 'exception')
+        self.share_manager.driver.initialized = False
 
         self.share_manager.init_host()
 
-        manager.LOG.exception.assert_called_with(
-            mock.ANY, self.share_manager.driver.__class__.__name__)
+        manager.LOG.exception.assert_called_once_with(
+            mock.ANY, {'name': self.share_manager.driver.__class__.__name__,
+                       'host': self.share_manager.host,
+                       'exc': mock.ANY})
+        self.assertFalse(self.share_manager.driver.initialized)
 
     def _setup_init_mocks(self, setup_access_rules=True):
         instances = [
@@ -1686,6 +1716,7 @@ class ShareManagerTestCase(test.TestCase):
         data = dict(DEFAULT=dict(automatic_share_server_cleanup=False))
         with test_utils.create_temp_config_with_opts(data):
             share_manager = manager.ShareManager()
+            share_manager.driver.initialized = True
             share_manager.delete_free_share_servers(self.context)
             self.assertFalse(db.share_server_get_all_unused_deletable.called)
 
@@ -1697,6 +1728,7 @@ class ShareManagerTestCase(test.TestCase):
         data = dict(DEFAULT=dict(driver_handles_share_servers=False))
         with test_utils.create_temp_config_with_opts(data):
             share_manager = manager.ShareManager()
+            share_manager.driver.initialized = True
             share_manager.delete_free_share_servers(self.context)
             self.assertFalse(db.share_server_get_all_unused_deletable.called)
             self.assertFalse(share_manager.delete_share_server.called)

manila/tests/test_utils.py

@@ -798,3 +798,34 @@ class TestRetryDecorator(test.TestCase):
 
             self.assertRaises(ValueError, raise_unexpected_error)
             self.assertFalse(mock_sleep.called)
+
+
+@ddt.ddt
+class RequireDriverInitializedTestCase(test.TestCase):
+
+    @ddt.data(True, False)
+    def test_require_driver_initialized(self, initialized):
+
+        class FakeDriver(object):
+            @property
+            def initialized(self):
+                return initialized
+
+        class FakeException(Exception):
+            pass
+
+        class FakeManager(object):
+            driver = FakeDriver()
+
+            @utils.require_driver_initialized
+            def call_me(self):
+                raise FakeException(
+                    "Should be raised only if manager.driver.initialized "
+                    "('%s') is equal to 'True'." % initialized)
+
+        if initialized:
+            expected_exception = FakeException
+        else:
+            expected_exception = exception.DriverNotInitialized
+
+        self.assertRaises(expected_exception, FakeManager().call_me)

manila/utils.py

@@ -19,6 +19,7 @@
 
 import contextlib
 import errno
+import functools
 import inspect
 import os
 import pyclbr
@@ -600,3 +601,14 @@ def retry(exception, interval=1, retries=10, backoff_rate=2,
         return _wrapper
 
     return _decorator
+
+
+def require_driver_initialized(func):
+    @functools.wraps(func)
+    def wrapper(self, *args, **kwargs):
+        # we can't do anything if the driver didn't init
+        if not self.driver.initialized:
+            driver_name = self.driver.__class__.__name__
+            raise exception.DriverNotInitialized(driver=driver_name)
+        return func(self, *args, **kwargs)
+    return wrapper