Enhance and test exception safety in hooks

This patch adds exception handling when calling pre and
post functions of hooks. Exceptions will be caught and
logged, but the hook chain and function call will be
allowed to proceed.

Partially implements: bp instance-network-info-hook

Change-Id: I45e76cd156c244ff57f77db103a9a579f14ad4f1

nova/hooks.py

@@ -46,6 +46,7 @@ def post(self, f, *args, **kwards):
 
 import stevedore
 
+from nova.openstack.common.gettextutils import _LE
 from nova.openstack.common import log as logging
 
 LOG = logging.getLogger(__name__)
@@ -54,6 +55,14 @@ def post(self, f, *args, **kwards):
 _HOOKS = {}  # hook name => hook manager
 
 
+class FatalHookException(Exception):
+    """Exception which should be raised by hooks to indicate that normal
+    execution of the hooked function should be terminated. Raised exception
+    will be logged and reraised.
+    """
+    pass
+
+
 class HookManager(stevedore.hook.HookManager):
     def __init__(self, name):
         # invoke_on_load creates an instance of the Hook class
@@ -66,10 +75,19 @@ def run_pre(self, name, args, kwargs, f=None):
             if pre:
                 LOG.debug("Running %(name)s pre-hook: %(obj)s",
                           {'name': name, 'obj': obj})
-                if f:
-                    pre(f, *args, **kwargs)
-                else:
-                    pre(*args, **kwargs)
+                try:
+                    if f:
+                        pre(f, *args, **kwargs)
+                    else:
+                        pre(*args, **kwargs)
+                except FatalHookException:
+                    msg = _LE("Fatal Exception running %(name)s "
+                              "pre-hook: %(obj)s")
+                    LOG.exception(msg, {'name': name, 'obj': obj})
+                    raise
+                except Exception:
+                    msg = _LE("Exception running %(name)s pre-hook: %(obj)s")
+                    LOG.exception(msg, {'name': name, 'obj': obj})
 
     def run_post(self, name, rv, args, kwargs, f=None):
         for e in reversed(self.extensions):
@@ -78,10 +96,19 @@ def run_post(self, name, rv, args, kwargs, f=None):
             if post:
                 LOG.debug("Running %(name)s post-hook: %(obj)s",
                           {'name': name, 'obj': obj})
-                if f:
-                    post(f, rv, *args, **kwargs)
-                else:
-                    post(rv, *args, **kwargs)
+                try:
+                    if f:
+                        post(f, rv, *args, **kwargs)
+                    else:
+                        post(rv, *args, **kwargs)
+                except FatalHookException:
+                    msg = _LE("Fatal Exception running %(name)s "
+                              "post-hook: %(obj)s")
+                    LOG.exception(msg, {'name': name, 'obj': obj})
+                    raise
+                except Exception:
+                    msg = _LE("Exception running %(name)s post-hook: %(obj)s")
+                    LOG.exception(msg, {'name': name, 'obj': obj})
 
 
 def add_hook(name, pass_function=False):

nova/tests/test_hooks.py

@@ -50,6 +50,22 @@ def post(self, f, rv, *args, **kwargs):
         self._add_called("post" + f.__name__, kwargs)
 
 
+class SampleHookExceptionPre(SampleHookA):
+    name = "epre"
+    exception = Exception()
+
+    def pre(self, f, *args, **kwargs):
+        raise self.exception
+
+
+class SampleHookExceptionPost(SampleHookA):
+    name = "epost"
+    exception = Exception()
+
+    def post(self, f, rv, *args, **kwargs):
+        raise self.exception
+
+
 class MockEntryPoint(object):
 
     def __init__(self, cls):
@@ -59,7 +75,20 @@ def load(self):
         return self.cls
 
 
-class HookTestCase(test.BaseHookTestCase):
+class MockedHookTestCase(test.BaseHookTestCase):
+    def _mock_load_plugins(self, iload, *iargs, **ikwargs):
+        return []
+
+    def setUp(self):
+        super(MockedHookTestCase, self).setUp()
+
+        hooks.reset()
+
+        self.stubs.Set(stevedore.extension.ExtensionManager, '_load_plugins',
+                       self._mock_load_plugins)
+
+
+class HookTestCase(MockedHookTestCase):
     def _mock_load_plugins(self, iload, *iargs, **ikwargs):
         return [
             stevedore.extension.Extension('test_hook',
@@ -95,7 +124,7 @@ def test_order_of_execution(self):
         self.assertEqual(['prea', 'preb', 'postb'], called_order)
 
 
-class HookTestCaseWithFunction(HookTestCase):
+class HookTestCaseWithFunction(MockedHookTestCase):
     def _mock_load_plugins(self, iload, *iargs, **ikwargs):
         return [
             stevedore.extension.Extension('function_hook',
@@ -118,3 +147,59 @@ def test_order_of_execution(self):
         called_order = []
         self._hooked(42, called=called_order)
         self.assertEqual(['pre_hookedc', 'post_hookedc'], called_order)
+
+
+class HookFailPreTestCase(MockedHookTestCase):
+    def _mock_load_plugins(self, iload, *iargs, **ikwargs):
+        return [
+            stevedore.extension.Extension('fail_pre',
+                MockEntryPoint(SampleHookExceptionPre),
+                SampleHookExceptionPre, SampleHookExceptionPre()),
+        ]
+
+    @hooks.add_hook('fail_pre', pass_function=True)
+    def _hooked(self, a, b=1, c=2, called=None):
+        return 42
+
+    def test_hook_fail_should_still_return(self):
+        self.assertEqual(42, self._hooked(1))
+
+        mgr = hooks._HOOKS['fail_pre']
+        self.assert_has_hook('fail_pre', self._hooked)
+        self.assertEqual(1, len(mgr.extensions))
+        self.assertEqual(SampleHookExceptionPre, mgr.extensions[0].plugin)
+
+    def test_hook_fail_should_raise_fatal(self):
+        self.stubs.Set(SampleHookExceptionPre, 'exception',
+                       hooks.FatalHookException())
+
+        self.assertRaises(hooks.FatalHookException,
+                          self._hooked, 1)
+
+
+class HookFailPostTestCase(MockedHookTestCase):
+    def _mock_load_plugins(self, iload, *iargs, **ikwargs):
+        return [
+            stevedore.extension.Extension('fail_post',
+                MockEntryPoint(SampleHookExceptionPost),
+                SampleHookExceptionPost, SampleHookExceptionPost()),
+        ]
+
+    @hooks.add_hook('fail_post', pass_function=True)
+    def _hooked(self, a, b=1, c=2, called=None):
+        return 42
+
+    def test_hook_fail_should_still_return(self):
+        self.assertEqual(42, self._hooked(1))
+
+        mgr = hooks._HOOKS['fail_post']
+        self.assert_has_hook('fail_post', self._hooked)
+        self.assertEqual(1, len(mgr.extensions))
+        self.assertEqual(SampleHookExceptionPost, mgr.extensions[0].plugin)
+
+    def test_hook_fail_should_raise_fatal(self):
+        self.stubs.Set(SampleHookExceptionPost, 'exception',
+                       hooks.FatalHookException())
+
+        self.assertRaises(hooks.FatalHookException,
+                          self._hooked, 1)