From eb07839bd40387ac9fdd60a67b3e537e751835b0 Mon Sep 17 00:00:00 2001
From: waleedm <waleedm@nvidia.com>
Date: Mon, 13 Jun 2022 11:42:39 +0000
Subject: [PATCH] Fix passing kwargs in clean steps

Pass kwargs to dispatch_to_managers method in execute_clean_step

Change-Id: Ida4ed4646659b2ee3f8f92b0a4d73c0266dd5a99
Story: 2010123
Task: 45705
---
 doc/source/contributor/hardware_managers.rst  | 36 +++++++++++++++++++
 ironic_python_agent/extensions/clean.py       |  4 ++-
 .../tests/unit/extensions/test_clean.py       | 28 +++++++++++++++
 .../notes/bug-2010123-d4c38d8f6606d0e0.yaml   |  6 ++++
 4 files changed, 73 insertions(+), 1 deletion(-)
 create mode 100644 releasenotes/notes/bug-2010123-d4c38d8f6606d0e0.yaml

diff --git a/doc/source/contributor/hardware_managers.rst b/doc/source/contributor/hardware_managers.rst
index b388dc581..ee3d63576 100644
--- a/doc/source/contributor/hardware_managers.rst
+++ b/doc/source/contributor/hardware_managers.rst
@@ -107,6 +107,42 @@ message, and no further managers will be called. An example step:
         else:
             raise errors.IncompatibleHardwareMethodError()
 
+If the step has args, you need to add them to argsinfo and provide the
+function with extra parameters.
+
+.. code-block:: python
+
+    def get_clean_steps(self, node, ports):
+        return [
+            {
+                # A function on the custom hardware manager
+                'step': 'upgrade_firmware',
+                # An integer priority. Largest priorities are executed first
+                'priority': 10,
+                # Should always be the deploy interface
+                'interface': 'deploy',
+                # Arguments that can be required or optional.
+                'argsinfo': {
+                    'firmware_url': {
+                        'description': 'Url for firmware',
+                        'required': True,
+                    },
+                }
+                # Request the node to be rebooted out of band by Ironic when
+                # the step completes successfully
+                'reboot_requested': False
+            }
+        ]
+
+.. code-block:: python
+
+    def upgrade_firmware(self, node, ports, firmware_url):
+        if self._device_exists():
+            # Do the upgrade
+            return 'upgraded firmware'
+        else:
+            raise errors.IncompatibleHardwareMethodError()
+
 .. note::
 
     If two managers return steps with the same `step` key, the priority will
diff --git a/ironic_python_agent/extensions/clean.py b/ironic_python_agent/extensions/clean.py
index 1b6275341..a195bb94d 100644
--- a/ironic_python_agent/extensions/clean.py
+++ b/ironic_python_agent/extensions/clean.py
@@ -72,8 +72,10 @@ class CleanExtension(base.BaseAgentExtension):
             msg = 'Malformed clean_step, no "step" key: %s' % step
             LOG.error(msg)
             raise ValueError(msg)
+        kwargs.update(step.get('args') or {})
         try:
-            result = hardware.dispatch_to_managers(step['step'], node, ports)
+            result = hardware.dispatch_to_managers(step['step'], node, ports,
+                                                   **kwargs)
         except (errors.RESTError, il_exc.IronicException):
             LOG.exception('Error performing clean step %s', step['step'])
             raise
diff --git a/ironic_python_agent/tests/unit/extensions/test_clean.py b/ironic_python_agent/tests/unit/extensions/test_clean.py
index 50299ce39..335f1f458 100644
--- a/ironic_python_agent/tests/unit/extensions/test_clean.py
+++ b/ironic_python_agent/tests/unit/extensions/test_clean.py
@@ -191,6 +191,34 @@ class TestCleanExtension(base.IronicAgentTest):
         self.assertEqual(expected_result, async_result.command_result)
         mock_cache_node.assert_called_once_with(self.node)
 
+    @mock.patch('ironic_python_agent.hardware.dispatch_to_managers',
+                autospec=True)
+    @mock.patch('ironic_python_agent.hardware.check_versions',
+                autospec=True)
+    def test_execute_clean_step_with_args(self, mock_version, mock_dispatch,
+                                          mock_cache_node):
+        result = 'cleaned'
+        mock_dispatch.return_value = result
+
+        step = self.step['GenericHardwareManager'][0]
+        step['args'] = {'foo': 'bar'}
+        expected_result = {
+            'clean_step': step,
+            'clean_result': result
+        }
+        async_result = self.agent_extension.execute_clean_step(
+            step=self.step['GenericHardwareManager'][0],
+            node=self.node, ports=self.ports,
+            clean_version=self.version)
+        async_result.join()
+
+        mock_version.assert_called_once_with(self.version)
+        mock_dispatch.assert_called_once_with(
+            self.step['GenericHardwareManager'][0]['step'],
+            self.node, self.ports, foo='bar')
+        self.assertEqual(expected_result, async_result.command_result)
+        mock_cache_node.assert_called_once_with(self.node)
+
     @mock.patch('ironic_python_agent.hardware.check_versions',
                 autospec=True)
     def test_execute_clean_step_no_step(self, mock_version, mock_cache_node):
diff --git a/releasenotes/notes/bug-2010123-d4c38d8f6606d0e0.yaml b/releasenotes/notes/bug-2010123-d4c38d8f6606d0e0.yaml
new file mode 100644
index 000000000..5e3fada2a
--- /dev/null
+++ b/releasenotes/notes/bug-2010123-d4c38d8f6606d0e0.yaml
@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Fixes an issue where arguments were not passed to clean steps when
+    a manual cleaning operation was being executed. The arguments are
+    now passed in appropriately.
\ No newline at end of file