Drop rollback actions for set-XX and extend-XX rules actions

Rollback actions were designed to help with rerunning introspection
on the same node. However, rollback actions for these actions proved
to be confusing and were never properly documented at all.
Even worse, the rollback action for set-attribute actually makes
this command impossible to use with non-removable attributes
(e.g. /driver).

This change removes rollback actions from all rules.
We need to rethink how we handle rollback in rules later on.

Change-Id: I2260f4b463c5dc804edac642c86e0da153e163f6

ironic_inspector/plugins/rules.py

@@ -117,16 +117,6 @@ class SetAttributeAction(base.RuleActionPlugin):
         node_info.patch([{'op': 'add', 'path': params['path'],
                           'value': params['value']}])
 
-    def rollback(self, node_info, params, **kwargs):
-        try:
-            node_info.get_by_path(params['path'])
-        except KeyError:
-            LOG.debug('Field %s was not set, no need for rollback',
-                      params['path'], node_info=node_info)
-            return
-
-        node_info.patch([{'op': 'remove', 'path': params['path']}])
-
 
 class SetCapabilityAction(base.RuleActionPlugin):
     REQUIRED_PARAMS = {'name'}
@@ -136,9 +126,6 @@ class SetCapabilityAction(base.RuleActionPlugin):
         node_info.update_capabilities(
             **{params['name']: params.get('value')})
 
-    def rollback(self, node_info, params, **kwargs):
-        node_info.update_capabilities(**{params['name']: None})
-
 
 class ExtendAttributeAction(base.RuleActionPlugin):
     REQUIRED_PARAMS = {'path', 'value'}
@@ -153,9 +140,3 @@ class ExtendAttributeAction(base.RuleActionPlugin):
             return values
 
         node_info.replace_field(params['path'], _replace, default=[])
-
-    def rollback(self, node_info, params, **kwargs):
-        def _replace(values):
-            return [v for v in values if v != params['value']]
-
-        node_info.replace_field(params['path'], _replace, default=[])

ironic_inspector/test/functional.py

@@ -333,11 +333,6 @@ class Test(Base):
         self.call_continue(self.data)
         eventlet.greenthread.sleep(DEFAULT_SLEEP)
 
-        # clean up for second rule
-        self.cli.node.update.assert_any_call(
-            self.uuid,
-            [{'op': 'remove', 'path': '/extra/bar'}])
-        # applying first rule
         self.cli.node.update.assert_any_call(
             self.uuid,
             [{'op': 'add', 'path': '/extra/foo', 'value': 'bar'}])

ironic_inspector/test/test_plugins_rules.py

@@ -144,19 +144,6 @@ class TestSetAttributeAction(test_base.NodeTest):
                                              'path': '/extra/value',
                                              'value': 42}])
 
-    @mock.patch.object(node_cache.NodeInfo, 'patch')
-    def test_rollback_with_existing(self, mock_patch):
-        self.node.extra = {'value': 'value'}
-        self.act.rollback(self.node_info, self.params)
-        mock_patch.assert_called_once_with([{'op': 'remove',
-                                             'path': '/extra/value'}])
-
-    @mock.patch.object(node_cache.NodeInfo, 'patch')
-    def test_rollback_no_existing(self, mock_patch):
-        self.node.extra = {}
-        self.act.rollback(self.node_info, self.params)
-        self.assertFalse(mock_patch.called)
-
 
 class TestSetCapabilityAction(test_base.NodeTest):
     act = rules_plugins.SetCapabilityAction()
@@ -182,23 +169,6 @@ class TestSetCapabilityAction(test_base.NodeTest):
         new_caps = utils.capabilities_to_dict(patch[0]['value'])
         self.assertEqual({'cap1': 'val', 'x': 'y', 'answer': '42'}, new_caps)
 
-    @mock.patch.object(node_cache.NodeInfo, 'patch')
-    def test_rollback_with_existing(self, mock_patch):
-        self.node.properties = {'capabilities': 'foo:bar,cap1:val'}
-        self.act.rollback(self.node_info, self.params)
-        mock_patch.assert_called_once_with(
-            [{'op': 'add', 'path': '/properties/capabilities',
-              'value': 'foo:bar'}])
-
-    @mock.patch.object(node_cache.NodeInfo, 'patch')
-    def test_rollback_no_existing(self, mock_patch):
-        self.node.properties = {'capabilities': 'foo:bar'}
-        self.act.rollback(self.node_info, self.params)
-        # TODO(dtantsur): make sure it's not called at all
-        mock_patch.assert_called_once_with(
-            [{'op': 'add', 'path': '/properties/capabilities',
-              'value': 'foo:bar'}])
-
 
 class TestExtendAttributeAction(test_base.NodeTest):
     act = rules_plugins.ExtendAttributeAction()
@@ -228,17 +198,3 @@ class TestExtendAttributeAction(test_base.NodeTest):
         self.node.extra['value'] = [42]
         self.act.apply(self.node_info, params)
         self.assertFalse(mock_patch.called)
-
-    @mock.patch.object(node_cache.NodeInfo, 'patch')
-    def test_rollback_with_existing(self, mock_patch):
-        self.node.extra['value'] = [1, 42, 0]
-        self.act.rollback(self.node_info, self.params)
-
-        mock_patch.assert_called_once_with(
-            [{'op': 'replace', 'path': '/extra/value', 'value': [1, 0]}])
-
-    @mock.patch.object(node_cache.NodeInfo, 'patch')
-    def test_rollback_no_existing(self, mock_patch):
-        self.node.extra['value'] = [1, 0]
-        self.act.rollback(self.node_info, self.params)
-        self.assertFalse(mock_patch.called)

releasenotes/notes/no-rollback-e15bc7fee0134545.yaml

@@ -0,0 +1,10 @@
+---
+upgrade:
+  - Introspection rules actions 'set-attribute', 'set-capability' and
+    'extend-attribute' no longer have the opposite effect on nodes that do not
+    match a rule.
+fixes:
+  - Dropped rollback actions from 'set-attribute', 'set-capability' and
+    'extend-attribute' introspection rules actions, as they were confusing,
+    completely undocumented and broke some real world use cases
+    (e.g. setting driver field).