From 76c075269de50001c429e470d0b19b0bd9be71c2 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Thu, 30 Mar 2023 15:06:28 -0700 Subject: [PATCH] Enable vendor interfaces to be called as steps Adds the logic and testing to handle vendor interfaces to be able to be called as steps, as well as adds the ipmitool send_raw vendor passthru method to be able to be called as a step. Change-Id: I741a4173f1d150298008d3190e4c3998402a8b86 --- doc/source/admin/drivers/ipmitool.rst | 21 +++++++++ doc/source/contributor/deploy-steps.rst | 8 ++++ ironic/conductor/cleaning.py | 4 ++ ironic/conductor/deployments.py | 4 ++ ironic/conductor/steps.py | 2 + ironic/drivers/modules/fake.py | 13 ++++++ ironic/drivers/modules/ipmitool.py | 23 +++++++++- ironic/tests/unit/conductor/test_steps.py | 22 ++++++++-- .../unit/drivers/modules/test_ipmitool.py | 44 ++++++++++++++++++- ...rface-step-decorated-a673f608c5f5721a.yaml | 15 +++++++ 10 files changed, 150 insertions(+), 6 deletions(-) create mode 100644 releasenotes/notes/vendor-interface-step-decorated-a673f608c5f5721a.yaml 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 0a26efb4ce..ce9979b825 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.