Merge "Enhance and test exception safety in hooks"

nova/hooks.py

@@ -46,6 +46,7 @@ import functools
 
 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 @@ NS = 'nova.hooks'
 _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 @@ class HookManager(stevedore.hook.HookManager):
             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 @@ class HookManager(stevedore.hook.HookManager):
             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 @@ class SampleHookC(SampleHookA):
         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 @@ class MockEntryPoint(object):
         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 @@ class HookTestCase(test.BaseHookTestCase):
         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 @@ class HookTestCaseWithFunction(HookTestCase):
         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)