Merge "Enable vendor interfaces to be called as steps"

doc/source/admin/drivers/ipmitool.rst

@@ -290,6 +290,27 @@ Example::
     ipmitool -I lanplus -H <BMC ADDRESS> -U <Username> -P <Password> \
         chassis bootparam get 5
 
+send_raw clean/deploy step
+~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The ``send_raw`` vendor passthru method is available to be invoked as a
+clean or deployment step should raw bytes need to be transmitted to the
+remote BMC in order to facilitate some sort of action or specific state.
+In this case, the raw bytes to be set are conveyed with a ``raw_bytes``
+argument on the requested clean or deploy step.
+
+Example::
+
+  {
+    "interface": "vendor",
+    "step": "send_raw",
+    "args": {
+      "raw_bytes": "0x00 0x00 0x00 0x00"
+    }
+  }
+
+
+
 .. _IPMItool: https://sourceforge.net/projects/ipmitool/
 .. _IPMI: https://en.wikipedia.org/wiki/Intelligent_Platform_Management_Interface
 .. _BMC: https://en.wikipedia.org/wiki/Intelligent_Platform_Management_Interface#Baseboard_management_controller

doc/source/contributor/deploy-steps.rst

@@ -160,6 +160,14 @@ the step.
    This approach only works for steps implemented on a ``deploy``
    interface that inherits agent deploy.
 
+.. warning::
+   Steps generally should have a return value of None **unless** the
+   a state is returned as part of an asyncrhonous workflow.
+
+   Please be mindful of this constraint when creating steps, as the
+   step runner **will** error if a value aside from None is returned
+   upon step completion.
+
 Execution on reboot
 ~~~~~~~~~~~~~~~~~~~
 

ironic/conductor/cleaning.py

@@ -237,6 +237,10 @@ def do_next_clean_step(task, step_index, disable_ramdisk=None):
             task.process_event('wait', target_state=target_state)
             return
         elif result is not None:
+            # NOTE(TheJulia): If your here debugging a step which fails,
+            # part of the constraint is that a value *cannot* be returned.
+            # to the runner. The step has to either succeed and return
+            # None, or raise an exception.
             msg = (_('While executing step %(step)s on node '
                      '%(node)s, step returned invalid value: %(val)s')
                    % {'step': step, 'node': node.uuid, 'val': result})

ironic/conductor/deployments.py

@@ -561,6 +561,10 @@ def execute_step_on_child_nodes(task, step):
                     # deploys to take place with the agent from a parent node
                     # being deployed.
                     continue
+                # NOTE(TheJulia): If your here debugging a step which fails,
+                # part of the constraint is that a value *cannot* be returned.
+                # to the runner. The step has to either succeed and return
+                # None, or raise an exception.
                 msg = (_('While executing step %(step)s on child node '
                          '%(node)s, step returned invalid value: %(val)s')
                        % {'step': step, 'node': child_task.node.uuid,

ironic/conductor/steps.py

@@ -30,6 +30,7 @@ CLEANING_INTERFACE_PRIORITY = {
     # by which interface is implementing the clean step. The clean step of the
     # interface with the highest value here, will be executed first in that
     # case.
+    'vendor': 6,
     'power': 5,
     'management': 4,
     'deploy': 3,
@@ -44,6 +45,7 @@ DEPLOYING_INTERFACE_PRIORITY = {
     # TODO(rloo): If we think it makes sense to have the interface priorities
     # the same for cleaning & deploying, replace the two with one e.g.
     # 'INTERFACE_PRIORITIES'.
+    'vendor': 6,
     'power': 5,
     'management': 4,
     'deploy': 3,

ironic/drivers/modules/fake.py

@@ -181,6 +181,9 @@ class FakeVendorA(base.VendorInterface):
                 raise exception.MissingParameterValue(_(
                     "Parameter 'bar' not passed to method 'first_method'."))
 
+    # NOTE(TheJulia): As an example, it is advisable to assign
+    # parameters from **kwargs, and then perform handling on
+    # the needful necessary from there.
     @base.passthru(['POST'],
                    description=_("Test if the value of bar is baz"))
     def first_method(self, task, http_method, bar):
@@ -221,6 +224,16 @@ class FakeVendorB(base.VendorInterface):
         sleep(CONF.fake.vendor_delay)
         return True if bar == 'woof' else False
 
+    @base.clean_step(priority=1)
+    @base.passthru(['POST'],
+                   description=_("Test pass-through to wait."))
+    def log_passthrough(self, task, **kwargs):
+        LOG.debug('Test method test_passhtrough_method called with '
+                  'arguments %s.', kwargs)
+        sleep(CONF.fake.vendor_delay)
+        # NOTE(TheJulia): Step methods invoked via an API *cannot*
+        # have return values
+
 
 class FakeConsole(base.ConsoleInterface):
     """Example implementation of a simple console interface."""

ironic/drivers/modules/ipmitool.py

@@ -1379,23 +1379,42 @@ class VendorPassthru(base.VendorInterface):
     def __init__(self):
         _constructor_checks(driver=self.__class__.__name__)
 
+    _send_raw_step_args = {
+        'raw_bytes': {
+            'description': (
+                'A string of raw bytes to convey the request to transmit '
+                'to the remote BMC.'
+            ),
+            'required': True
+        }
+    }
+
     @METRICS.timer('VendorPassthru.send_raw')
+    @base.deploy_step(priority=0,
+                      argsinfo=_send_raw_step_args)
+    @base.clean_step(priority=0, abortable=False,
+                     argsinfo=_send_raw_step_args,
+                     requires_ramdisk=False)
     @base.passthru(['POST'],
                    description=_("Send raw bytes to the BMC. Required "
                                  "argument: 'raw_bytes' - a string of raw "
                                  "bytes (e.g. '0x00 0x01')."))
     @task_manager.require_exclusive_lock
-    def send_raw(self, task, http_method, raw_bytes):
+    def send_raw(self, task, **kwargs):
         """Send raw bytes to the BMC. Bytes should be a string of bytes.
 
         :param task: a TaskManager instance.
-        :param http_method: the HTTP method used on the request.
         :param raw_bytes: a string of raw bytes to send, e.g. '0x00 0x01'
+                          supplied as a kwargument.
         :raises: IPMIFailure on an error from ipmitool.
         :raises: MissingParameterValue if a required parameter is missing.
         :raises:  InvalidParameterValue when an invalid value is specified.
 
         """
+        raw_bytes = kwargs.get('raw_bytes')
+        if not raw_bytes:
+            raise exception.MissingParameterValue(
+                'Argument raw_bytes is missing.')
         send_raw(task, raw_bytes)
 
     @METRICS.timer('VendorPassthru.bmc_reset')

ironic/tests/unit/conductor/test_steps.py

@@ -581,9 +581,14 @@ class NodeCleaningStepsTestCase(db_base.DbTestCase):
         self.deploy_erase = {
             'step': 'erase_disks', 'priority': 20, 'interface': 'deploy',
             'abortable': True}
+        self.vendor_action = {
+            'abortable': False, 'argsinfo': None, 'interface': 'vendor',
+            'priority': 1, 'requires_ramdisk': True,
+            'step': 'log_passthrough'}
+
         # Automated cleaning should be executed in this order
         self.clean_steps = [self.deploy_erase, self.power_update,
-                            self.deploy_update]
+                            self.deploy_update, self.vendor_action]
         # Manual clean step
         self.deploy_raid = {
             'step': 'build_raid', 'priority': 0, 'interface': 'deploy',
@@ -614,6 +619,8 @@ class NodeCleaningStepsTestCase(db_base.DbTestCase):
 
         self.assertEqual(self.clean_steps, steps)
 
+    @mock.patch('ironic.drivers.modules.fake.FakeVendorB.get_clean_steps',
+                lambda self, task: [])
     @mock.patch('ironic.drivers.modules.fake.FakeBIOS.get_clean_steps',
                 lambda self, task: [])
     @mock.patch('ironic.drivers.modules.fake.FakeDeploy.get_clean_steps',
@@ -661,6 +668,8 @@ class NodeCleaningStepsTestCase(db_base.DbTestCase):
 
         self.assertEqual(self.clean_steps, steps)
 
+    @mock.patch('ironic.drivers.modules.fake.FakeVendorB.get_clean_steps',
+                lambda self, task: [])
     @mock.patch('ironic.drivers.modules.fake.FakeDeploy.get_clean_steps',
                 autospec=True)
     @mock.patch('ironic.drivers.modules.fake.FakePower.get_clean_steps',
@@ -766,7 +775,13 @@ class NodeCleaningStepsTestCase(db_base.DbTestCase):
                                     {'interface': 'power', 'priority': 10,
                                      'step': 'update_firmware'},
                                     {'interface': 'deploy', 'priority': 10,
-                                     'step': 'update_firmware'}]
+                                     'step': 'update_firmware'},
+                                    {'abortable': False,
+                                     'argsinfo': None,
+                                     'interface': 'vendor',
+                                     'priority': 1,
+                                     'requires_ramdisk': True,
+                                     'step': 'log_passthrough'}]
 
         self.assertEqual(expected_step_priorities, steps)
 
@@ -781,7 +796,8 @@ class NodeCleaningStepsTestCase(db_base.DbTestCase):
         cfg.CONF.set_override('clean_step_priority_override',
                               ["deploy.erase_disks:0",
                                "power.update_firmware:0",
-                               "deploy.update_firmware:0", ],
+                               "deploy.update_firmware:0",
+                               "vendor.log_passthrough:0", ],
                               'conductor')
 
         node = obj_utils.create_test_node(

ironic/tests/unit/drivers/modules/test_ipmitool.py

@@ -2208,6 +2208,37 @@ class IPMIToolDriverTestCase(Base):
                               http_method='POST',
                               raw_bytes='0x00 0x01')
 
+    def test_send_raw_bytes_is_in_step_list(self):
+        with task_manager.acquire(self.context,
+                                  self.node.uuid) as task:
+            steps = task.driver.vendor.get_clean_steps(task)
+            self.assertEqual('send_raw', steps[0]['step'])
+            self.assertEqual('vendor', steps[0]['interface'])
+            self.assertIn('raw_bytes', steps[0]['argsinfo'])
+            steps = task.driver.vendor.get_deploy_steps(task)
+            self.assertEqual('send_raw', steps[0]['step'])
+            self.assertEqual('vendor', steps[0]['interface'])
+            self.assertIn('raw_bytes', steps[0]['argsinfo'])
+
+    @mock.patch.object(ipmi, '_exec_ipmitool', autospec=True)
+    def test_send_raw_bytes_from_clean_step(self, ipmitool_mock):
+        step = {
+            'priority': 10,
+            'interface': 'vendor',
+            'step': 'send_raw',
+            'args': {'raw_bytes': '0x00 0x00 0x00 0x00'},
+            'reboot_requested': False
+        }
+        ipmitool_mock.return_value = ('', '')
+        self.node.save()
+
+        with task_manager.acquire(self.context, self.node['uuid'],
+                                  shared=False) as task:
+            task.driver.vendor.execute_clean_step(task, step)
+            ipmitool_mock.assert_called_once_with(
+                self.info,
+                'raw 0x00 0x00 0x00 0x00')
+
     @mock.patch.object(ipmi, '_exec_ipmitool', autospec=True)
     def test__bmc_reset_ok(self, mock_exec):
         mock_exec.return_value = [None, None]
@@ -2391,7 +2422,8 @@ class IPMIToolDriverTestCase(Base):
                               task, method='send_raw')
 
     @mock.patch.object(ipmi.VendorPassthru, 'send_raw', autospec=True)
-    def test_vendor_passthru_call_send_raw_bytes(self, raw_bytes_mock):
+    def test_vendor_passthru_call_send_raw_bytes_with_http_method(
+            self, raw_bytes_mock):
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=False) as task:
             self.vendor.send_raw(task, http_method='POST',
@@ -2400,6 +2432,16 @@ class IPMIToolDriverTestCase(Base):
                 self.vendor, task, http_method='POST',
                 raw_bytes='0x00 0x01')
 
+    @mock.patch.object(ipmi.VendorPassthru, 'send_raw', autospec=True)
+    def test_vendor_passthru_call_send_raw_bytes(self, raw_bytes_mock):
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=False) as task:
+            self.vendor.send_raw(task,
+                                 raw_bytes='0x00 0x01')
+            raw_bytes_mock.assert_called_once_with(
+                self.vendor, task,
+                raw_bytes='0x00 0x01')
+
     def test_vendor_passthru_validate__bmc_reset_good(self):
         with task_manager.acquire(self.context, self.node.uuid) as task:
             self.vendor.validate(task, method='bmc_reset')

releasenotes/notes/vendor-interface-step-decorated-a673f608c5f5721a.yaml

@@ -0,0 +1,15 @@
+---
+features:
+  - |
+    Methods in vendor interfaces may now be decroated with ``clean_step`` and
+    ``deploy_step`` decorators.
+  - |
+    The ``ipmitool`` vendor interface's ``send_raw`` method can now be called
+    as a part of cleaning or deployment steps with an "raw_bytes" argument
+    matching how it can be called with the vendor passthru interface.
+other:
+  - |
+    The ``ipmitool`` vendor passthrough interface method no longer requires a
+    ``http_method`` parameter. This is optional in the code base, but included
+    on all API initiated vendor passthru method calls.
+    The value was not utilized.