Allow hooks to have dependencies on other hooks

Also refactored hooks and got rid of compatibility layer with old
stevedore.

Change-Id: I81f21df7ebad4df893539ec5f0a03064e7c0a263
Closes-Bug: #1681751

CONTRIBUTING.rst

@@ -192,6 +192,12 @@ Writing a Plugin
       updated on a node.  Please refer to the docstring for details
       and examples.
 
+  You can optionally define the following attribute:
+
+  ``dependencies``
+      a list of entry point names of the hooks this hook depends on. These
+      hooks are expected to be enabled before the current hook.
+
   Make your plugin a setuptools entry point under
   ``ironic_inspector.hooks.processing`` namespace and enable it in the
   configuration file (``processing.processing_hooks`` option).

doc/source/usage.rst

@@ -138,7 +138,8 @@ Plugins
 **ironic-inspector** heavily relies on plugins for data processing. Even the
 standard functionality is largely based on plugins. Set ``processing_hooks``
 option in the configuration file to change the set of plugins to be run on
-introspection data. Note that order does matter in this option.
+introspection data. Note that order does matter in this option, especially
+for hooks that have dependencies on other hooks.
 
 These are plugins that are enabled by default and should not be disabled,
 unless you understand what you're doing:

ironic_inspector/main.py

@@ -448,16 +448,12 @@ class Service(object):
         db.init()
 
         try:
-            hooks = [ext.name for ext in
-                     plugins_base.processing_hooks_manager()]
-        except KeyError as exc:
-            # callback function raises MissingHookError derived from KeyError
-            # on missing hook
-            LOG.critical('Hook(s) %s failed to load or was not found',
-                         str(exc))
+            hooks = plugins_base.validate_processing_hooks()
+        except Exception as exc:
+            LOG.critical(str(exc))
             sys.exit(1)
 
-        LOG.info('Enabled processing hooks: %s', hooks)
+        LOG.info('Enabled processing hooks: %s', [h.name for h in hooks])
 
         if CONF.firewall.manage_firewall:
             firewall.init()

ironic_inspector/plugins/base.py

@@ -31,6 +31,12 @@ LOG = log.getLogger(__name__)
 class ProcessingHook(object):  # pragma: no cover
     """Abstract base class for introspection data processing hooks."""
 
+    dependencies = []
+    """An ordered list of hooks that must be enabled before this one.
+
+    The items here should be entry point names, not classes.
+    """
+
     def before_processing(self, introspection_data, **kwargs):
         """Hook to run before any other data processing.
 
@@ -151,8 +157,8 @@ _ACTIONS_MGR = None
 
 def missing_entrypoints_callback(names):
     """Raise MissingHookError with comma-separated list of missing hooks"""
-    missing_names = ', '.join(names)
-    raise MissingHookError(missing_names)
+    error = _('The following hook(s) are missing or failed to load: %s')
+    raise RuntimeError(error % ', '.join(names))
 
 
 def processing_hooks_manager(*args):
@@ -175,6 +181,35 @@ def processing_hooks_manager(*args):
     return _HOOKS_MGR
 
 
+def validate_processing_hooks():
+    """Validate the enabled processing hooks.
+
+    :raises: MissingHookError on missing or failed to load hooks
+    :raises: RuntimeError on validation failure
+    :returns: the list of hooks passed validation
+    """
+    hooks = [ext for ext in processing_hooks_manager()]
+    enabled = set()
+    errors = []
+    for hook in hooks:
+        deps = getattr(hook.obj, 'dependencies', ())
+        missing = [d for d in deps if d not in enabled]
+        if missing:
+            errors.append('Hook %(hook)s requires the following hooks to be '
+                          'enabled before it: %(deps)s. The following hooks '
+                          'are missing: %(missing)s.' %
+                          {'hook': hook.name,
+                           'deps': ', '.join(deps),
+                           'missing': ', '.join(missing)})
+        enabled.add(hook.name)
+
+    if errors:
+        raise RuntimeError("Some hooks failed to load due to dependency "
+                           "problems:\n%s" % "\n".join(errors))
+
+    return hooks
+
+
 def node_not_found_hook_manager(*args):
     global _NOT_FOUND_HOOK_MGR
     if _NOT_FOUND_HOOK_MGR is None:
@@ -211,7 +246,3 @@ def rule_actions_manager():
                             'actions is deprecated (action "%s")',
                             act.name)
     return _ACTIONS_MGR
-
-
-class MissingHookError(KeyError):
-    """Exception when hook is not found when processing it."""

ironic_inspector/test/unit/test_main.py

@@ -710,7 +710,8 @@ class TestInit(test_base.BaseTest):
         plugins_base._HOOKS_MGR = None
 
         self.assertRaises(SystemExit, self.service.init)
-        mock_log.assert_called_once_with(mock.ANY, "'foo!'")
+        mock_log.assert_called_once_with(
+            'The following hook(s) are missing or failed to load: foo!')
 
 
 class TestCreateSSLContext(test_base.BaseTest):

ironic_inspector/test/unit/test_plugins_base.py

@@ -11,6 +11,10 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import collections
+
+import mock
+
 from ironic_inspector.plugins import base
 from ironic_inspector.test import base as test_base
 
@@ -42,3 +46,47 @@ class TestWithValidation(test_base.BaseTest):
     def test_unexpected(self):
         self.assertRaisesRegex(ValueError, 'unexpected parameter\(s\): foo',
                                self.test.validate, {'foo': 'bar', 'x': 42})
+
+
+fake_ext = collections.namedtuple('Extension', ['name', 'obj'])
+
+
+@mock.patch.object(base, 'processing_hooks_manager', autospec=True)
+class TestValidateProcessingHooks(test_base.BaseTest):
+    def test_ok(self, mock_mgr):
+        mock_mgr.return_value = [
+            fake_ext(name='1', obj=mock.Mock(dependencies=[])),
+            fake_ext(name='2', obj=mock.Mock(dependencies=['1'])),
+            fake_ext(name='3', obj=mock.Mock(dependencies=['2', '1'])),
+        ]
+
+        hooks = base.validate_processing_hooks()
+        self.assertEqual(mock_mgr.return_value, hooks)
+        mock_mgr.assert_called_once_with()
+
+    def test_broken_dependencies(self, mock_mgr):
+        mock_mgr.return_value = [
+            fake_ext(name='2', obj=mock.Mock(dependencies=['1'])),
+            fake_ext(name='3', obj=mock.Mock(dependencies=['2', '1'])),
+        ]
+
+        self.assertRaisesRegex(RuntimeError, "missing: 1",
+                               base.validate_processing_hooks)
+
+    def test_self_dependency(self, mock_mgr):
+        mock_mgr.return_value = [
+            fake_ext(name='1', obj=mock.Mock(dependencies=['1'])),
+        ]
+
+        self.assertRaisesRegex(RuntimeError, "missing: 1",
+                               base.validate_processing_hooks)
+
+    def test_wrong_dependencies_order(self, mock_mgr):
+        mock_mgr.return_value = [
+            fake_ext(name='2', obj=mock.Mock(dependencies=['1'])),
+            fake_ext(name='1', obj=mock.Mock(dependencies=[])),
+            fake_ext(name='3', obj=mock.Mock(dependencies=['2', '1'])),
+        ]
+
+        self.assertRaisesRegex(RuntimeError, "missing: 1",
+                               base.validate_processing_hooks)

releasenotes/notes/hook-deps-83a867c7af0300e4.yaml

@@ -0,0 +1,6 @@
+---
+features:
+  - |
+    Processing hooks can now define dependencies on other processing hooks.
+    **ironic-inspector** start up fails when required hooks are not enabled
+    before the hook that requires them.