Remove rollback support from introspection rules

It was deprecated more than a year ago.

Change-Id: Ibf12960ac7208acb290f2f48de4cec6fef299dc5
Related-Bug: #1686942

ironic_inspector/plugins/base.py

@@ -137,17 +137,6 @@ class RuleActionPlugin(WithValidation):  # pragma: no cover
         :raises: utils.Error on failure
         """
 
-    def rollback(self, node_info, params, **kwargs):
-        """Rollback action effects from previous run on a failed match.
-
-        Default implementation does nothing.
-
-        :param node_info: NodeInfo object
-        :param params: parameters as a dictionary
-        :param kwargs: used for extensibility without breaking existing plugins
-        :raises: utils.Error on failure
-        """
-
 
 _HOOKS_MGR = None
 _NOT_FOUND_HOOK_MGR = None
@@ -239,10 +228,4 @@ def rule_actions_manager():
         _ACTIONS_MGR = stevedore.ExtensionManager(
             'ironic_inspector.rules.actions',
             invoke_on_load=True)
-        for act in _ACTIONS_MGR:
-            # a trick to detect if function was overridden
-            if "rollback" in act.obj.__class__.__dict__:
-                LOG.warning('Defining "rollback" for introspection rules '
-                            'actions is deprecated (action "%s")',
-                            act.name)
     return _ACTIONS_MGR

ironic_inspector/rules.py

@@ -182,20 +182,13 @@ class IntrospectionRule(object):
                  node_info=node_info, data=data)
         return True
 
-    def apply_actions(self, node_info, rollback=False, data=None):
+    def apply_actions(self, node_info, data=None):
         """Run actions on a node.
 
         :param node_info: NodeInfo instance
-        :param rollback: if True, rollback actions are executed
         :param data: introspection data
         """
-        if rollback:
+        LOG.debug('Running actions for rule "%s"', self.description,
-            method = 'rollback'
-        else:
-            method = 'apply'
-
-        LOG.debug('Running %(what)s actions for rule "%(rule)s"',
-                  {'what': method, 'rule': self.description},
                   node_info=node_info, data=data)
 
         ext_mgr = plugins_base.rule_actions_manager()
@@ -213,25 +206,16 @@ class IntrospectionRule(object):
                 try:
                     act.params[formatted_param] = value.format(data=data)
                 except KeyError as e:
-                    if rollback:
+                    raise utils.Error(_('Invalid formatting variable key '
-                        LOG.warning('Invalid formatting variable key provided:'
+                                        'provided: %s') % e,
-                                    ' %(key)s, skipping rollback for action '
+                                      node_info=node_info, data=data)
-                                    '%(action)s',
-                                    {'key': e, 'action': act.action})
-                        break
-                    else:
-                        raise utils.Error(_('Invalid formatting variable key '
-                                            'provided: %s') % e,
-                                          node_info=node_info, data=data)
-            else:
-                LOG.debug('Running %(what)s action `%(action)s %(params)s`',
-                          {'action': act.action, 'params': act.params,
-                           'what': method},
-                          node_info=node_info, data=data)
-                getattr(ext, method)(node_info, act.params)
 
-        LOG.debug('Successfully applied %s',
+            LOG.debug('Running action `%(action)s %(params)s`',
-                  'rollback actions' if rollback else 'actions',
+                      {'action': act.action, 'params': act.params},
+                      node_info=node_info, data=data)
+            ext.apply(node_info, act.params)
+
+        LOG.debug('Successfully applied actions',
                   node_info=node_info, data=data)
 
 
@@ -425,26 +409,15 @@ def apply(node_info, data):
     LOG.debug('Applying custom introspection rules',
               node_info=node_info, data=data)
 
-    to_rollback = []
     to_apply = []
     for rule in rules:
         if rule.check_conditions(node_info, data):
             to_apply.append(rule)
-        else:
-            to_rollback.append(rule)
-
-    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, data=data)
-    else:
-        LOG.debug('No rollback actions to apply',
-                  node_info=node_info, data=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, data=data)
+            rule.apply_actions(node_info, data=data)
     else:
         LOG.debug('No actions to apply', node_info=node_info, data=data)
 

ironic_inspector/test/unit/test_rules.py

@@ -390,7 +390,6 @@ class TestApplyActions(BaseTest):
         self.act_mock.apply.assert_any_call(self.node_info, {})
         self.assertEqual(len(self.actions_json),
                          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=[
@@ -404,7 +403,6 @@ class TestApplyActions(BaseTest):
         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(
@@ -431,36 +429,6 @@ class TestApplyActions(BaseTest):
         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_rollback(self, mock_ext_mgr):
-        mock_ext_mgr.return_value.__getitem__.return_value = self.ext_mock
-
-        self.rule.apply_actions(self.node_info, rollback=True)
-
-        self.act_mock.rollback.assert_any_call(self.node_info,
-                                               {'message': 'boom!'})
-        self.act_mock.rollback.assert_any_call(self.node_info, {})
-        self.assertEqual(len(self.actions_json),
-                         self.act_mock.rollback.call_count)
-        self.assertFalse(self.act_mock.apply.called)
-
-    @mock.patch.object(rules.LOG, 'warning', autospec=True)
-    def test_rollback_ignores_formatting_failures(self, mock_log,
-                                                  mock_ext_mgr):
-        self.rule = rules.create(
-            actions_json=[
-                {'action': 'set-attribute',
-                 'path': '/driver_info/ipmi_address',
-                 'value': '{data[foo][bar]}'}],
-            conditions_json=self.conditions_json
-        )
-        mock_ext_mgr.return_value.__getitem__.return_value = self.ext_mock
-
-        self.rule.apply_actions(self.node_info, rollback=True, data=self.data)
-
-        self.assertTrue(mock_log.called)
-        self.assertFalse(self.act_mock.rollback.called)
 
 
 @mock.patch.object(rules, 'get_all', autospec=True)
@@ -475,7 +443,7 @@ class TestApply(BaseTest):
 
         rules.apply(self.node_info, self.data)
 
-    def test_no_actions(self, mock_get_all):
+    def test_apply(self, mock_get_all):
         mock_get_all.return_value = self.rules
         for idx, rule in enumerate(self.rules):
             rule.check_conditions.return_value = not bool(idx)
@@ -485,44 +453,8 @@ class TestApply(BaseTest):
         for idx, rule in enumerate(self.rules):
             rule.check_conditions.assert_called_once_with(self.node_info,
                                                           self.data)
-            rule.apply_actions.assert_called_once_with(
+            if rule.check_conditions.return_value:
-                self.node_info, rollback=bool(idx), data=self.data)
+                rule.apply_actions.assert_called_once_with(
-
+                    self.node_info, data=self.data)
-    def test_actions(self, mock_get_all):
+            else:
-        mock_get_all.return_value = self.rules
+                self.assertFalse(rule.apply_actions.called)
-        for idx, rule in enumerate(self.rules):
-            rule.check_conditions.return_value = not bool(idx)
-
-        rules.apply(self.node_info, self.data)
-
-        for idx, rule in enumerate(self.rules):
-            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), data=self.data)
-
-    def test_no_rollback(self, mock_get_all):
-        mock_get_all.return_value = self.rules
-        for rule in self.rules:
-            rule.check_conditions.return_value = True
-
-        rules.apply(self.node_info, self.data)
-
-        for rule in self.rules:
-            rule.check_conditions.assert_called_once_with(self.node_info,
-                                                          self.data)
-            rule.apply_actions.assert_called_once_with(
-                self.node_info, rollback=False, data=self.data)
-
-    def test_only_rollback(self, mock_get_all):
-        mock_get_all.return_value = self.rules
-        for rule in self.rules:
-            rule.check_conditions.return_value = False
-
-        rules.apply(self.node_info, self.data)
-
-        for rule in self.rules:
-            rule.check_conditions.assert_called_once_with(self.node_info,
-                                                          self.data)
-            rule.apply_actions.assert_called_once_with(
-                self.node_info, rollback=True, data=self.data)

releasenotes/notes/rollback-removal-a03a989e2e9f776b.yaml

@@ -0,0 +1,4 @@
+---
+upgrade:
+  - |
+    Support for rollback actions in introspection rules was removed.