diff --git a/doc/source/admin/drivers/ipmitool.rst b/doc/source/admin/drivers/ipmitool.rst index 9840e812db..6f9f0440f5 100644 --- a/doc/source/admin/drivers/ipmitool.rst +++ b/doc/source/admin/drivers/ipmitool.rst @@ -290,6 +290,27 @@ Example:: ipmitool -I lanplus -H -U -P \ 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 diff --git a/doc/source/contributor/deploy-steps.rst b/doc/source/contributor/deploy-steps.rst index 03856d231a..a30535cb81 100644 --- a/doc/source/contributor/deploy-steps.rst +++ b/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 ~~~~~~~~~~~~~~~~~~~ diff --git a/ironic/conductor/cleaning.py b/ironic/conductor/cleaning.py index 01c2aac37e..1e79f971b3 100644 --- a/ironic/conductor/cleaning.py +++ b/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}) diff --git a/ironic/conductor/deployments.py b/ironic/conductor/deployments.py index ba49b4b902..9324ebabdb 100644 --- a/ironic/conductor/deployments.py +++ b/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, diff --git a/ironic/conductor/steps.py b/ironic/conductor/steps.py index 19953338d8..1acb556172 100644 --- a/ironic/conductor/steps.py +++ b/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, diff --git a/ironic/drivers/modules/fake.py b/ironic/drivers/modules/fake.py index 5823fb3796..c2d92a7f16 100644 --- a/ironic/drivers/modules/fake.py +++ b/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.""" diff --git a/ironic/drivers/modules/ipmitool.py b/ironic/drivers/modules/ipmitool.py index 0085cf2f3e..faaa38c24d 100644 --- a/ironic/drivers/modules/ipmitool.py +++ b/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') diff --git a/ironic/tests/unit/conductor/test_steps.py b/ironic/tests/unit/conductor/test_steps.py index 2aae428688..2a743959fa 100644 --- a/ironic/tests/unit/conductor/test_steps.py +++ b/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( diff --git a/ironic/tests/unit/drivers/modules/test_ipmitool.py b/ironic/tests/unit/drivers/modules/test_ipmitool.py index 016b9d6ed4..1b63ebcdec 100644 --- a/ironic/tests/unit/drivers/modules/test_ipmitool.py +++ b/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') diff --git a/releasenotes/notes/vendor-interface-step-decorated-a673f608c5f5721a.yaml b/releasenotes/notes/vendor-interface-step-decorated-a673f608c5f5721a.yaml new file mode 100644 index 0000000000..adb7d01f09 --- /dev/null +++ b/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.