Extend conditions and actions

Conditions: ``field`` supports new format, it allow to comparison
data both from inspection and node info:

     {'field': 'node://ironic/style/path', 'op': 'eq', 'value': 'val'}
     {'field': 'data://introspection/path', 'op': 'eq', 'value': 'val'}

Actions: ``value`` supports fetching data from introspection, it's
using python string formatting notation:

     {'action': 'set-attribute', 'path': '/driver_info/ipmi_address',
      'value': '{data[inventory][bmc_address]}'}

Related-Bug: #1524753
Change-Id: Ie05f82e7a29fba2f743217f0893c085fd843cd5b

doc/source/usage.rst

@@ -74,6 +74,16 @@ A condition is represented by an object with fields:
 ``field`` a `JSON path <http://goessner.net/articles/JsonPath/>`_ to the field
 in the introspection data to use in comparison.
 
+Starting with the Mitaka release, you can also apply conditions to ironic node
+field. Prefix field with schema (``data://`` or ``node://``) to distinguish
+between values from introspection data and node. Both schemes use JSON path::
+
+    {'field': 'node://property.path', 'op': 'eq', 'value': 'val'}
+    {'field': 'data://introspection.path', 'op': 'eq', 'value': 'val'}
+
+if scheme (node or data) is missing, condition compares data with
+introspection data.
+
 ``multiple`` how to treat situations where the ``field`` query returns multiple
 results (e.g. the field contains a list), available options are:
 
@@ -110,6 +120,13 @@ Default available actions include:
   value as a list and appends value to it. If optional ``unique`` parameter is
   set to ``True``, nothing will be added if given value is already in a list.
 
+Starting from Mitaka release, ``value`` field in actions supports fetching data
+from introspection, it's using `python string formatting notation
+<https://docs.python.org/2/library/string.html#formatspec>`_ ::
+
+        {'action': 'set-attribute', 'path': '/driver_info/ipmi_address',
+                   'value': '{data[inventory][bmc_address]}'}
+
 .. _setting-ipmi-creds:
 
 Setting IPMI Credentials

ironic_inspector/plugins/base.py

@@ -116,6 +116,9 @@ class RuleConditionPlugin(WithValidation):  # pragma: no cover
 class RuleActionPlugin(WithValidation):  # pragma: no cover
     """Abstract base class for rule action plugins."""
 
+    FORMATTED_PARAMS = []
+    """List of params will be formatted with python format."""
+
     @abc.abstractmethod
     def apply(self, node_info, params, **kwargs):
         """Run action on successful rule match.

ironic_inspector/plugins/rules.py

@@ -113,6 +113,8 @@ class SetAttributeAction(base.RuleActionPlugin):
     REQUIRED_PARAMS = {'path', 'value'}
     # TODO(dtantsur): proper validation of path
 
+    FORMATTED_PARAMS = ['value']
+
     def apply(self, node_info, params, **kwargs):
         node_info.patch([{'op': 'add', 'path': params['path'],
                           'value': params['value']}])
@@ -132,6 +134,8 @@ class SetCapabilityAction(base.RuleActionPlugin):
     REQUIRED_PARAMS = {'name'}
     OPTIONAL_PARAMS = {'value'}
 
+    FORMATTED_PARAMS = ['value']
+
     def apply(self, node_info, params, **kwargs):
         node_info.update_capabilities(
             **{params['name']: params.get('value')})
@@ -145,6 +149,8 @@ class ExtendAttributeAction(base.RuleActionPlugin):
     OPTIONAL_PARAMS = {'unique'}
     # TODO(dtantsur): proper validation of path
 
+    FORMATTED_PARAMS = ['value']
+
     def apply(self, node_info, params, **kwargs):
         def _replace(values):
             value = params['value']

ironic_inspector/rules.py

@@ -131,7 +131,14 @@ class IntrospectionRule(object):
                   node_info=node_info, data=data)
         ext_mgr = plugins_base.rule_conditions_manager()
         for cond in self._conditions:
-            field_values = jsonpath.parse(cond.field).find(data)
+            scheme, path = _parse_path(cond.field)
+
+            if scheme == 'node':
+                source_data = node_info.node().dict()
+            elif scheme == 'data':
+                source_data = data
+
+            field_values = jsonpath.parse(path).find(source_data)
             field_values = [x.value for x in field_values]
             cond_ext = ext_mgr[cond.op].obj
 
@@ -172,7 +179,7 @@ class IntrospectionRule(object):
 
         :param node_info: NodeInfo instance
         :param rollback: if True, rollback actions are executed
-        :param data: introspection data (only used for logging)
+        :param data: introspection data
         """
         if rollback:
             method = 'rollback'
@@ -185,11 +192,26 @@ class IntrospectionRule(object):
 
         ext_mgr = plugins_base.rule_actions_manager()
         for act in self._actions:
+            ext = ext_mgr[act.action].obj
+            for formatted_param in ext.FORMATTED_PARAMS:
+                value = act.params.get(formatted_param)
+                if not value:
+                    continue
+
+                # NOTE(aarefiev): verify provided value with introspection
+                # data format specifications.
+                # TODO(aarefiev): simple verify on import rule time.
+                try:
+                    act.params[formatted_param] = value.format(data=data)
+                except KeyError as e:
+                    raise utils.Error(_('Invalid formatting variable key '
+                                        'provided: %s') % e,
+                                      node_info=node_info, data=data)
+
             LOG.debug('Running %(what)s action `%(action)s %(params)s`',
                       {'action': act.action, 'params': act.params,
                        'what': method},
                       node_info=node_info, data=data)
-            ext = ext_mgr[act.action].obj
             getattr(ext, method)(node_info, act.params)
 
         LOG.debug('Successfully applied %s',
@@ -197,6 +219,27 @@ class IntrospectionRule(object):
                   node_info=node_info, data=data)
 
 
+def _parse_path(path):
+    """Parse path, extract scheme and path.
+
+     Parse path with 'node' and 'data' scheme, which links on
+     introspection data and node info respectively. If scheme is
+     missing in path, default is 'data'.
+
+    :param path: data or node path
+    :return: tuple (scheme, path)
+    """
+    try:
+        index = path.index('://')
+    except ValueError:
+        scheme = 'data'
+        path = path
+    else:
+        scheme = path[:index]
+        path = path[index + 3:]
+    return scheme, path
+
+
 def create(conditions_json, actions_json, uuid=None,
            description=None):
     """Create a new rule in database.
@@ -235,8 +278,15 @@ def create(conditions_json, actions_json, uuid=None,
     conditions = []
     for cond_json in conditions_json:
         field = cond_json['field']
+
+        scheme, path = _parse_path(field)
+
+        if scheme not in ('node', 'data'):
+            raise utils.Error(_('Unsupported scheme for field: %s, valid '
+                                'values are node:// or data://') % scheme)
+        # verify field as JSON path
         try:
-            jsonpath.parse(field)
+            jsonpath.parse(path)
         except Exception as exc:
             raise utils.Error(_('Unable to parse field JSON path %(field)s: '
                                 '%(error)s') % {'field': field, 'error': exc})
@@ -364,7 +414,7 @@ def apply(node_info, data):
     if to_rollback:
         LOG.debug('Running rollback actions', node_info=node_info, data=data)
         for rule in to_rollback:
-            rule.apply_actions(node_info, rollback=True)
+            rule.apply_actions(node_info, rollback=True, data=data)
     else:
         LOG.debug('No rollback actions to apply',
                   node_info=node_info, data=data)
@@ -372,7 +422,7 @@ def apply(node_info, data):
     if to_apply:
         LOG.debug('Running actions', node_info=node_info, data=data)
         for rule in to_apply:
-            rule.apply_actions(node_info, rollback=False)
+            rule.apply_actions(node_info, rollback=False, data=data)
     else:
         LOG.debug('No actions to apply', node_info=node_info, data=data)
 

ironic_inspector/test/base.py

@@ -88,15 +88,23 @@ class NodeTest(BaseTest):
         self.uuid = uuidutils.generate_uuid()
         self.bmc_address = '1.2.3.4'
         self.macs = ['11:22:33:44:55:66', '66:55:44:33:22:11']
-        self.node = mock.Mock(driver='pxe_ipmitool',
+        fake_node = {
-                              driver_info={'ipmi_address': self.bmc_address},
+            'driver': 'pxe_ipmitool',
-                              properties={'cpu_arch': 'i386', 'local_gb': 40},
+            'driver_info': {'ipmi_address': self.bmc_address},
-                              uuid=self.uuid,
+            'properties': {'cpu_arch': 'i386', 'local_gb': 40},
-                              power_state='power on',
+            'uuid': self.uuid,
-                              provision_state='inspecting',
+            'power_state': 'power on',
-                              extra={},
+            'provision_state': 'inspecting',
-                              instance_uuid=None,
+            'extra': {},
-                              maintenance=False)
+            'instance_uuid': None,
+            'maintenance': False
+        }
+        mock_to_dict = mock.Mock(return_value=fake_node)
+
+        self.node = mock.Mock(**fake_node)
+        self.node.dict = mock_to_dict
+
         self.ports = []
         self.node_info = node_cache.NodeInfo(uuid=self.uuid, started_at=0,
                                              node=self.node, ports=self.ports)
+        self.node_info.node = mock.Mock(return_value=self.node)

ironic_inspector/test/functional.py

@@ -342,6 +342,49 @@ class Test(Base):
             self.uuid,
             [{'op': 'add', 'path': '/extra/foo', 'value': 'bar'}])
 
+    def test_conditions_scheme_actions_path(self):
+        rules = [
+            {
+                'conditions': [
+                    {'field': 'node://properties.local_gb', 'op': 'eq',
+                     'value': 40},
+                    {'field': 'node://driver_info.ipmi_address', 'op': 'eq',
+                     'value': self.bmc_address},
+                ],
+                'actions': [
+                    {'action': 'set-attribute', 'path': '/extra/foo',
+                     'value': 'bar'}
+                ]
+            },
+            {
+                'conditions': [
+                    {'field': 'data://inventory.cpu.count', 'op': 'eq',
+                     'value': self.data['inventory']['cpu']['count']},
+                ],
+                'actions': [
+                    {'action': 'set-attribute',
+                     'path': '/driver_info/ipmi_address',
+                     'value': '{data[inventory][bmc_address]}'}
+                ]
+            }
+        ]
+        for rule in rules:
+            self.call_add_rule(rule)
+
+        self.call_introspect(self.uuid)
+        eventlet.greenthread.sleep(DEFAULT_SLEEP)
+        self.call_continue(self.data)
+        eventlet.greenthread.sleep(DEFAULT_SLEEP)
+
+        self.cli.node.update.assert_any_call(
+            self.uuid,
+            [{'op': 'add', 'path': '/extra/foo', 'value': 'bar'}])
+
+        self.cli.node.update.assert_any_call(
+            self.uuid,
+            [{'op': 'add', 'path': '/driver_info/ipmi_address',
+              'value': self.data['inventory']['bmc_address']}])
+
     def test_root_device_hints(self):
         self.node.properties['root_device'] = {'size': 20}
 

ironic_inspector/test/test_rules.py

@@ -18,14 +18,13 @@ import mock
 from oslo_utils import uuidutils
 
 from ironic_inspector import db
-from ironic_inspector import node_cache
 from ironic_inspector.plugins import base as plugins_base
 from ironic_inspector import rules
 from ironic_inspector.test import base as test_base
 from ironic_inspector import utils
 
 
-class BaseTest(test_base.BaseTest):
+class BaseTest(test_base.NodeTest):
     def setUp(self):
         super(BaseTest, self).setUp()
         self.uuid = uuidutils.generate_uuid()
@@ -41,7 +40,6 @@ class BaseTest(test_base.BaseTest):
             'memory_mb': 1024,
             'local_gb': 42,
         }
-        self.node_info = node_cache.NodeInfo(uuid=self.uuid, started_at=42)
 
 
 class TestCreateRule(BaseTest):
@@ -321,6 +319,38 @@ class TestCheckConditionsMultiple(BaseTest):
                           data)
 
 
+class TestCheckConditionsSchemePath(BaseTest):
+    def test_conditions_data_path(self):
+        self.data_set = [
+            ([{'op': 'eq', 'field': 'data://memory_mb', 'value': 1024}],
+             True),
+            ([{'op': 'gt', 'field': 'data://local_gb', 'value': 42}],
+             False)
+        ]
+
+        for condition, res in self.data_set:
+            rule = rules.create(conditions_json=condition,
+                                actions_json=self.actions_json)
+            self.assertIs(res,
+                          rule.check_conditions(self.node_info, self.data),
+                          self.data)
+
+    def test_conditions_node_path(self):
+        self.node_set = [
+            ([{'op': 'eq', 'field': 'node://driver_info.ipmi_address',
+               'value': self.bmc_address}],
+             True),
+            ([{'op': 'eq', 'field': 'node://driver', 'value': 'fake'}],
+             False)
+        ]
+
+        for condition, res in self.node_set:
+            rule = rules.create(conditions_json=condition,
+                                actions_json=self.actions_json)
+            self.assertIs(res,
+                          rule.check_conditions(self.node_info, self.data))
+
+
 @mock.patch.object(plugins_base, 'rule_actions_manager', autospec=True)
 class TestApplyActions(BaseTest):
     def setUp(self):
@@ -330,12 +360,13 @@ class TestApplyActions(BaseTest):
         self.rule = rules.create(conditions_json=self.conditions_json,
                                  actions_json=self.actions_json)
         self.act_mock = mock.Mock(spec=plugins_base.RuleActionPlugin)
+        self.act_mock.FORMATTED_PARAMS = ['value']
         self.ext_mock = mock.Mock(spec=['obj'], obj=self.act_mock)
 
     def test_apply(self, mock_ext_mgr):
         mock_ext_mgr.return_value.__getitem__.return_value = self.ext_mock
 
-        self.rule.apply_actions(self.node_info)
+        self.rule.apply_actions(self.node_info, data=self.data)
 
         self.act_mock.apply.assert_any_call(self.node_info,
                                             {'message': 'boom!'})
@@ -344,6 +375,33 @@ class TestApplyActions(BaseTest):
                          self.act_mock.apply.call_count)
         self.assertFalse(self.act_mock.rollback.called)
 
+    def test_apply_data_format_value(self, mock_ext_mgr):
+        self.rule = rules.create(actions_json=[
+            {'action': 'set-attribute',
+             'path': '/driver_info/ipmi_address',
+             'value': '{data[memory_mb]}'}],
+            conditions_json=self.conditions_json
+        )
+        mock_ext_mgr.return_value.__getitem__.return_value = self.ext_mock
+
+        self.rule.apply_actions(self.node_info, data=self.data)
+
+        self.assertEqual(1, self.act_mock.apply.call_count)
+        self.assertFalse(self.act_mock.rollback.called)
+
+    def test_apply_data_format_value_fail(self, mock_ext_mgr):
+        self.rule = rules.create(
+            actions_json=[
+                {'action': 'set-attribute',
+                 'path': '/driver_info/ipmi_address',
+                 'value': '{data[inventory][bmc_address]}'}],
+            conditions_json=self.conditions_json
+        )
+        mock_ext_mgr.return_value.__getitem__.return_value = self.ext_mock
+
+        self.assertRaises(utils.Error, self.rule.apply_actions,
+                          self.node_info, data=self.data)
+
     def test_rollback(self, mock_ext_mgr):
         mock_ext_mgr.return_value.__getitem__.return_value = self.ext_mock
 
@@ -380,7 +438,7 @@ class TestApply(BaseTest):
             rule.check_conditions.assert_called_once_with(self.node_info,
                                                           self.data)
             rule.apply_actions.assert_called_once_with(
-                self.node_info, rollback=bool(idx))
+                self.node_info, rollback=bool(idx), data=self.data)
 
     def test_actions(self, mock_get_all):
         mock_get_all.return_value = self.rules
@@ -393,7 +451,7 @@ class TestApply(BaseTest):
             rule.check_conditions.assert_called_once_with(self.node_info,
                                                           self.data)
             rule.apply_actions.assert_called_once_with(
-                self.node_info, rollback=bool(idx))
+                self.node_info, rollback=bool(idx), data=self.data)
 
     def test_no_rollback(self, mock_get_all):
         mock_get_all.return_value = self.rules
@@ -406,7 +464,7 @@ class TestApply(BaseTest):
             rule.check_conditions.assert_called_once_with(self.node_info,
                                                           self.data)
             rule.apply_actions.assert_called_once_with(
-                self.node_info, rollback=False)
+                self.node_info, rollback=False, data=self.data)
 
     def test_only_rollback(self, mock_get_all):
         mock_get_all.return_value = self.rules
@@ -419,4 +477,4 @@ class TestApply(BaseTest):
             rule.check_conditions.assert_called_once_with(self.node_info,
                                                           self.data)
             rule.apply_actions.assert_called_once_with(
-                self.node_info, rollback=True)
+                self.node_info, rollback=True, data=self.data)

releasenotes/notes/extend-rules-9a9d38701e970611.yaml

@@ -0,0 +1,5 @@
+---
+features:
+  - Conditions now support comparing fields from node info;
+  - Actions support formatting to fetch values from introspection data.
+    See http://docs.openstack.org/developer/ironic-inspector/usage.html#introspection-rules