From 9e87cebc12102cbb3ae47366836dcd7c3e439828 Mon Sep 17 00:00:00 2001 From: Ruby Loo Date: Thu, 1 Feb 2018 16:48:15 -0500 Subject: [PATCH] Fix handling of 'timeout' parameter to power methods The PowerInterface methods set_power_state() and reboot() were enhanced to take a 'timeout' parameter [1]. To handle Interfaces that didn't support timeout, conductor.utils.node_power_action() used reflection.get_signature() to determine whether or not the node's PowerInterface's methods could handle a timeout parameter. It turns out that there was a bug with the ironic_lib.metrics.timer decorator [2], such that reflection.get_signature() did not return the method parameters. This means that for PowerInterfaces that had this decorator, the conductor would incorrectly think that 'timeout' was not supported, even if it were supported. Instead of trying to decide whether a PowerInterface supports 'timeout', the conductor now assumes that it does. This patch changes all in-tree PowerInterfaces so that they accept a 'timeout' parameter for reboot() and set_power_state(). For any out-of-tree implementations that don't accept a 'timeout' parameter, a TypeError exception will be raised. [1] f15d5b9a37260b3876f9dadeb030412e6e1053b2 [2] https://bugs.launchpad.net/ironic/+bug/1746730 Closes-Bug: #1746849 Change-Id: Iae28e94c34d4d69593644d0e5f4542558db9bb79 --- ironic/conductor/utils.py | 25 +------- ironic/drivers/modules/cimc/power.py | 23 +++++++- ironic/drivers/modules/drac/power.py | 20 ++++++- ironic/drivers/modules/ilo/power.py | 22 +++++-- ironic/drivers/modules/snmp.py | 19 ++++++- ironic/drivers/modules/ucs/power.py | 20 ++++++- ironic/drivers/modules/xclarity/power.py | 19 ++++++- ironic/tests/unit/conductor/test_manager.py | 6 +- ironic/tests/unit/conductor/test_utils.py | 57 +++++++++++++++++-- .../unit/drivers/modules/cimc/test_power.py | 48 +++++++++++++++- .../unit/drivers/modules/drac/test_power.py | 35 +++++++++++- .../unit/drivers/modules/ilo/test_power.py | 35 +++++++++++- .../tests/unit/drivers/modules/test_snmp.py | 33 +++++++++-- .../unit/drivers/modules/ucs/test_power.py | 53 ++++++++++++++++- .../drivers/modules/xclarity/test_power.py | 34 ++++++++++- ...ter-to-power-methods-5f632c936497685e.yaml | 13 +++++ 16 files changed, 404 insertions(+), 58 deletions(-) create mode 100644 releasenotes/notes/add-timeout-parameter-to-power-methods-5f632c936497685e.yaml diff --git a/ironic/conductor/utils.py b/ironic/conductor/utils.py index 4ecc6776be..057d4dcd5c 100644 --- a/ironic/conductor/utils.py +++ b/ironic/conductor/utils.py @@ -16,7 +16,6 @@ from oslo_config import cfg from oslo_log import log from oslo_service import loopingcall from oslo_utils import excutils -from oslo_utils import reflection from ironic.common import exception from ironic.common.i18n import _ @@ -226,32 +225,12 @@ def node_power_action(task, new_state, timeout=None): task.driver.storage.attach_volumes(task) if new_state != states.REBOOT: - if ('timeout' in reflection.get_signature( - task.driver.power.set_power_state).parameters): - task.driver.power.set_power_state(task, new_state, - timeout=timeout) - else: - # FIXME(naohirot): - # After driver composition, we should print power interface - # name here instead of driver. - LOG.warning( - "The set_power_state method of %(driver_name)s " - "doesn't support 'timeout' parameter.", - {'driver_name': node.driver}) - task.driver.power.set_power_state(task, new_state) - + task.driver.power.set_power_state(task, new_state, timeout=timeout) else: # TODO(TheJulia): We likely ought to consider toggling # volume attachments, although we have no mechanism to # really verify what cinder has connector wise. - if ('timeout' in reflection.get_signature( - task.driver.power.reboot).parameters): - task.driver.power.reboot(task, timeout=timeout) - else: - LOG.warning("The reboot method of %(driver_name)s " - "doesn't support 'timeout' parameter.", - {'driver_name': node.driver}) - task.driver.power.reboot(task) + task.driver.power.reboot(task, timeout=timeout) except Exception as e: with excutils.save_and_reraise_exception(): node['target_power_state'] = states.NOSTATE diff --git a/ironic/drivers/modules/cimc/power.py b/ironic/drivers/modules/cimc/power.py index 39a4ae4435..ccb01e76f0 100644 --- a/ironic/drivers/modules/cimc/power.py +++ b/ironic/drivers/modules/cimc/power.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +from oslo_log import log as logging from oslo_service import loopingcall from oslo_utils import importutils @@ -25,6 +26,8 @@ from ironic.drivers.modules.cimc import common imcsdk = importutils.try_import('ImcSdk') +LOG = logging.getLogger(__name__) + if imcsdk: CIMC_TO_IRONIC_POWER_STATE = { @@ -116,15 +119,24 @@ class Power(base.PowerInterface): states.ERROR) @task_manager.require_exclusive_lock - def set_power_state(self, task, pstate): + def set_power_state(self, task, pstate, timeout=None): """Set the power state of the task's node. :param task: a TaskManager instance containing the node to act on. :param pstate: Any power state from :mod:`ironic.common.states`. + :param timeout: timeout (in seconds). Unsupported by this interface. :raises: MissingParameterValue if a required parameter is missing. :raises: InvalidParameterValue if an invalid power state is passed :raises: CIMCException if there is an error communicating with CIMC """ + # TODO(rloo): Support timeouts! + if timeout is not None: + LOG.warning( + "The 'cimc' Power Interface's 'set_power_state' method " + "doesn't support the 'timeout' parameter. Ignoring " + "timeout=%(timeout)s", + {'timeout': timeout}) + if pstate not in IRONIC_TO_CIMC_POWER_STATE: msg = _("set_power_state called for %(node)s with " "invalid state %(state)s") @@ -150,16 +162,23 @@ class Power(base.PowerInterface): raise exception.PowerStateFailure(pstate=pstate) @task_manager.require_exclusive_lock - def reboot(self, task): + def reboot(self, task, timeout=None): """Perform a hard reboot of the task's node. If the node is already powered on then it shall reboot the node, if its off then the node will just be turned on. :param task: a TaskManager instance containing the node to act on. + :param timeout: timeout (in seconds). Unsupported by this interface. :raises: MissingParameterValue if a required parameter is missing. :raises: CIMCException if there is an error communicating with CIMC """ + # TODO(rloo): Support timeouts! + if timeout is not None: + LOG.warning("The 'cimc' Power Interface's 'reboot' method " + "doesn't support the 'timeout' parameter. Ignoring " + "timeout=%(timeout)s", + {'timeout': timeout}) current_power_state = self.get_power_state(task) if current_power_state == states.POWER_ON: diff --git a/ironic/drivers/modules/drac/power.py b/ironic/drivers/modules/drac/power.py index 663cd46603..fe02fe5159 100644 --- a/ironic/drivers/modules/drac/power.py +++ b/ironic/drivers/modules/drac/power.py @@ -149,27 +149,43 @@ class DracPower(base.PowerInterface): @METRICS.timer('DracPower.set_power_state') @task_manager.require_exclusive_lock - def set_power_state(self, task, power_state): + def set_power_state(self, task, power_state, timeout=None): """Set the power state of the node. :param task: a TaskManager instance containing the node to act on. :param power_state: a power state from :mod:`ironic.common.states`. + :param timeout: timeout (in seconds). Unsupported by this interface. :raises: InvalidParameterValue if required DRAC credentials are missing. :raises: DracOperationError on an error from python-dracclient. """ + # TODO(rloo): Support timeouts! + if timeout is not None: + LOG.warning( + "The 'idrac' Power Interface's 'set_power_state' method " + "doesn't support the 'timeout' parameter. Ignoring " + "timeout=%(timeout)s", + {'timeout': timeout}) + _set_power_state(task.node, power_state) @METRICS.timer('DracPower.reboot') @task_manager.require_exclusive_lock - def reboot(self, task): + def reboot(self, task, timeout=None): """Perform a reboot of the task's node. :param task: a TaskManager instance containing the node to act on. + :param timeout: timeout (in seconds). Unsupported by this interface. :raises: InvalidParameterValue if required DRAC credentials are missing. :raises: DracOperationError on an error from python-dracclient. """ + # TODO(rloo): Support timeouts! + if timeout is not None: + LOG.warning("The 'idrac' Power Interface's 'reboot' method " + "doesn't support the 'timeout' parameter. Ignoring " + "timeout=%(timeout)s", + {'timeout': timeout}) current_power_state = _get_power_state(task.node) if current_power_state == states.POWER_ON: diff --git a/ironic/drivers/modules/ilo/power.py b/ironic/drivers/modules/ilo/power.py index 56695a2681..a7edc4d514 100644 --- a/ironic/drivers/modules/ilo/power.py +++ b/ironic/drivers/modules/ilo/power.py @@ -200,30 +200,44 @@ class IloPower(base.PowerInterface): @METRICS.timer('IloPower.set_power_state') @task_manager.require_exclusive_lock - def set_power_state(self, task, power_state): + def set_power_state(self, task, power_state, timeout=None): """Turn the current power state on or off. :param task: a TaskManager instance. - :param node: The Node. :param power_state: The desired power state POWER_ON,POWER_OFF or REBOOT from :mod:`ironic.common.states`. + :param timeout: timeout (in seconds). Unsupported by this interface. :raises: InvalidParameterValue if an invalid power state was specified. :raises: IloOperationError on an error from IloClient library. :raises: PowerStateFailure if the power couldn't be set to power_state. """ + # TODO(rloo): Support timeouts! + if timeout is not None: + LOG.warning("The 'ilo' Power Interface's 'set_power_state' method " + "doesn't support the 'timeout' parameter. Ignoring " + "timeout=%(timeout)s", + {'timeout': timeout}) + _set_power_state(task, power_state) @METRICS.timer('IloPower.reboot') @task_manager.require_exclusive_lock - def reboot(self, task): + def reboot(self, task, timeout=None): """Reboot the node :param task: a TaskManager instance. - :param node: The Node. + :param timeout: timeout (in seconds). Unsupported by this interface. :raises: PowerStateFailure if the final state of the node is not POWER_ON. :raises: IloOperationError on an error from IloClient library. """ + # TODO(rloo): Support timeouts! + if timeout is not None: + LOG.warning("The 'ilo' Power Interface's 'reboot' method " + "doesn't support the 'timeout' parameter. Ignoring " + "timeout=%(timeout)s", + {'timeout': timeout}) + node = task.node current_pstate = _get_power_state(node) if current_pstate == states.POWER_ON: diff --git a/ironic/drivers/modules/snmp.py b/ironic/drivers/modules/snmp.py index f2313909bf..6a13273ac2 100644 --- a/ironic/drivers/modules/snmp.py +++ b/ironic/drivers/modules/snmp.py @@ -700,7 +700,7 @@ class SNMPPower(base.PowerInterface): return power_state @task_manager.require_exclusive_lock - def set_power_state(self, task, pstate): + def set_power_state(self, task, pstate, timeout=None): """Turn the power on or off. Set the power state of a node. @@ -708,6 +708,7 @@ class SNMPPower(base.PowerInterface): :param task: A instance of `ironic.manager.task_manager.TaskManager`. :param pstate: Either POWER_ON or POWER_OFF from :class: `ironic.common.states`. + :param timeout: timeout (in seconds). Unsupported by this interface. :raises: MissingParameterValue if required SNMP parameters are missing. :raises: InvalidParameterValue if SNMP parameters are invalid or `pstate` is invalid. @@ -715,6 +716,13 @@ class SNMPPower(base.PowerInterface): as requested after the timeout. :raises: SNMPFailure if an SNMP request fails. """ + # TODO(rloo): Support timeouts! + if timeout is not None: + LOG.warning( + "The 'snmp' Power Interface's 'set_power_state' method " + "doesn't support the 'timeout' parameter. Ignoring " + "timeout=%(timeout)s", + {'timeout': timeout}) driver = _get_driver(task.node) if pstate == states.POWER_ON: @@ -729,16 +737,23 @@ class SNMPPower(base.PowerInterface): raise exception.PowerStateFailure(pstate=pstate) @task_manager.require_exclusive_lock - def reboot(self, task): + def reboot(self, task, timeout=None): """Cycles the power to a node. :param task: A instance of `ironic.manager.task_manager.TaskManager`. + :param timeout: timeout (in seconds). Unsupported by this interface. :raises: MissingParameterValue if required SNMP parameters are missing. :raises: InvalidParameterValue if SNMP parameters are invalid. :raises: PowerStateFailure if the final power state of the node is not POWER_ON after the timeout. :raises: SNMPFailure if an SNMP request fails. """ + # TODO(rloo): Support timeouts! + if timeout is not None: + LOG.warning("The 'snmp' Power Interface's 'reboot' method " + "doesn't support the 'timeout' parameter. Ignoring " + "timeout=%(timeout)s", + {'timeout': timeout}) driver = _get_driver(task.node) state = driver.power_reset() diff --git a/ironic/drivers/modules/ucs/power.py b/ironic/drivers/modules/ucs/power.py index 44bf6d104e..c19d332626 100644 --- a/ironic/drivers/modules/ucs/power.py +++ b/ironic/drivers/modules/ucs/power.py @@ -120,7 +120,7 @@ class Power(base.PowerInterface): @task_manager.require_exclusive_lock @ucs_helper.requires_ucs_client - def set_power_state(self, task, pstate, helper=None): + def set_power_state(self, task, pstate, timeout=None, helper=None): """Turn the power on or off. Set the power state of a node. @@ -128,6 +128,7 @@ class Power(base.PowerInterface): :param task: instance of `ironic.manager.task_manager.TaskManager`. :param pstate: Either POWER_ON or POWER_OFF from :class: `ironic.common.states`. + :param timeout: timeout (in seconds). Unsupported by this interface. :param helper: ucs helper instance :raises: InvalidParameterValue if an invalid power state was specified. :raises: MissingParameterValue if required CiscoDriver parameters @@ -135,6 +136,13 @@ class Power(base.PowerInterface): :raises: UcsOperationError on error from UCS Client. :raises: PowerStateFailure if the desired power state couldn't be set. """ + # TODO(rloo): Support timeouts! + if timeout is not None: + LOG.warning( + "The 'ucsm' Power Interface's 'set_power_state' method " + "doesn't support the 'timeout' parameter. Ignoring " + "timeout=%(timeout)s", + {'timeout': timeout}) if pstate not in (states.POWER_ON, states.POWER_OFF): msg = _("set_power_state called with invalid power state " @@ -170,15 +178,23 @@ class Power(base.PowerInterface): @task_manager.require_exclusive_lock @ucs_helper.requires_ucs_client - def reboot(self, task, helper=None): + def reboot(self, task, timeout=None, helper=None): """Cycles the power to a node. :param task: a TaskManager instance. + :param timeout: timeout (in seconds). Unsupported by this interface. :param helper: ucs helper instance. :raises: UcsOperationError on error from UCS Client. :raises: PowerStateFailure if the final state of the node is not POWER_ON. """ + # TODO(rloo): Support timeouts! + if timeout is not None: + LOG.warning("The 'ucsm' Power Interface's 'reboot' method " + "doesn't support the 'timeout' parameter. Ignoring " + "timeout=%(timeout)s", + {'timeout': timeout}) + try: ucs_power_handle = ucs_power.UcsPower(helper) ucs_power_handle.reboot() diff --git a/ironic/drivers/modules/xclarity/power.py b/ironic/drivers/modules/xclarity/power.py index b4ed8a8c37..f1bdc70140 100644 --- a/ironic/drivers/modules/xclarity/power.py +++ b/ironic/drivers/modules/xclarity/power.py @@ -71,15 +71,23 @@ class XClarityPower(base.PowerInterface): @METRICS.timer('XClarityPower.set_power_state') @task_manager.require_exclusive_lock - def set_power_state(self, task, power_state): + def set_power_state(self, task, power_state, timeout=None): """Turn the current power state on or off. :param task: a TaskManager instance. :param power_state: The desired power state POWER_ON, POWER_OFF or REBOOT from :mod:`ironic.common.states`. + :param timeout: timeout (in seconds). Unsupported by this interface. :raises: InvalidParameterValue if an invalid power state was specified. :raises: XClarityError if XClarity fails setting the power state. """ + # TODO(rloo): Support timeouts! + if timeout is not None: + LOG.warning( + "The 'xclarity' Power Interface's 'set_power_state' method " + "doesn't support the 'timeout' parameter. Ignoring " + "timeout=%(timeout)s", + {'timeout': timeout}) if power_state == states.REBOOT: target_power_state = self.get_power_state(task) @@ -103,10 +111,17 @@ class XClarityPower(base.PowerInterface): @METRICS.timer('XClarityPower.reboot') @task_manager.require_exclusive_lock - def reboot(self, task): + def reboot(self, task, timeout=None): """Reboot the node :param task: a TaskManager instance. + :param timeout: timeout (in seconds). Unsupported by this interface. """ + # TODO(rloo): Support timeouts! + if timeout is not None: + LOG.warning("The 'xclarity' Power Interface's 'reboot' method " + "doesn't support the 'timeout' parameter. Ignoring " + "timeout=%(timeout)s", + {'timeout': timeout}) self.set_power_state(task, states.REBOOT) diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 59fc87c8c1..2025bb5330 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -201,7 +201,8 @@ class ChangeNodePowerStateTestCase(mgr_utils.ServiceSetUpMixin, self._stop_service() get_power_mock.assert_called_once_with(mock.ANY) - set_power_mock.assert_called_once_with(mock.ANY, new_state) + set_power_mock.assert_called_once_with( + mock.ANY, new_state, timeout=None) node.refresh() self.assertEqual(initial_state, node.power_state) self.assertIsNone(node.target_power_state) @@ -334,7 +335,8 @@ class ChangeNodePowerStateTestCase(mgr_utils.ServiceSetUpMixin, # Give async worker a chance to finish self._stop_service() - set_power_mock.assert_called_once_with(mock.ANY, states.POWER_ON) + set_power_mock.assert_called_once_with( + mock.ANY, states.POWER_ON, timeout=None) # 2 notifications should be sent: 1 .start and 1 .error self.assertEqual(2, mock_notif.call_count) diff --git a/ironic/tests/unit/conductor/test_utils.py b/ironic/tests/unit/conductor/test_utils.py index 711e42fdf6..f1d96618c6 100644 --- a/ironic/tests/unit/conductor/test_utils.py +++ b/ironic/tests/unit/conductor/test_utils.py @@ -20,6 +20,7 @@ from ironic.common import network from ironic.common import states from ironic.conductor import task_manager from ironic.conductor import utils as conductor_utils +from ironic.drivers import base as drivers_base from ironic import objects from ironic.objects import fields as obj_fields from ironic.tests import base as tests_base @@ -31,6 +32,25 @@ from ironic.tests.unit.objects import utils as obj_utils CONF = cfg.CONF +class TestPowerNoTimeout(drivers_base.PowerInterface): + """Missing 'timeout' parameter for get_power_state & reboot""" + + def get_properties(self): + return {} + + def validate(self, task): + pass + + def get_power_state(self, task): + return task.node.power_state + + def set_power_state(self, task, power_state, timeout=None): + task.node.power_state = power_state + + def reboot(self, task): + pass + + class NodeSetBootDeviceTestCase(db_base.DbTestCase): def test_node_set_boot_device_non_existent_device(self): @@ -185,7 +205,7 @@ class NodePowerActionTestCase(db_base.DbTestCase): self.assertFalse(get_power_mock.called) node.refresh() - reboot_mock.assert_called_once_with(mock.ANY) + reboot_mock.assert_called_once_with(mock.ANY, timeout=None) self.assertEqual(states.POWER_ON, node['power_state']) self.assertIsNone(node['target_power_state']) self.assertIsNone(node['last_error']) @@ -442,8 +462,8 @@ class NodePowerActionTestCase(db_base.DbTestCase): node.refresh() get_power_mock.assert_called_once_with(mock.ANY) - set_power_mock.assert_called_once_with(mock.ANY, - states.POWER_ON) + set_power_mock.assert_called_once_with( + mock.ANY, states.POWER_ON, timeout=None) self.assertEqual(states.POWER_OFF, node['power_state']) self.assertIsNone(node['target_power_state']) self.assertIsNotNone(node['last_error']) @@ -476,8 +496,8 @@ class NodePowerActionTestCase(db_base.DbTestCase): node.refresh() get_power_mock.assert_called_once_with(mock.ANY) - set_power_mock.assert_called_once_with(mock.ANY, - states.POWER_ON) + set_power_mock.assert_called_once_with( + mock.ANY, states.POWER_ON, timeout=None) self.assertEqual(states.POWER_OFF, node.power_state) self.assertIsNone(node.target_power_state) self.assertIsNotNone(node.last_error) @@ -700,6 +720,33 @@ class NodePowerActionTestCase(db_base.DbTestCase): 'baremetal.node.power_set.error', obj_fields.NotificationLevel.ERROR) + def test_node_power_action_reboot_no_timeout(self): + """Test node reboot using Power Interface with no timeout arg.""" + node = obj_utils.create_test_node(self.context, + uuid=uuidutils.generate_uuid(), + driver='fake-hardware', + console_interface='no-console', + inspect_interface='no-inspect', + raid_interface='no-raid', + rescue_interface='no-rescue', + vendor_interface='no-vendor', + power_state=states.POWER_ON) + self.config(enabled_boot_interfaces=['fake']) + self.config(enabled_deploy_interfaces=['fake']) + self.config(enabled_management_interfaces=['fake']) + self.config(enabled_power_interfaces=['fake']) + + task = task_manager.TaskManager(self.context, node.uuid) + task.driver.power = TestPowerNoTimeout() + self.assertRaisesRegex(TypeError, + 'unexpected keyword argument', + conductor_utils.node_power_action, + task, states.REBOOT) + node.refresh() + self.assertEqual(states.POWER_ON, node['power_state']) + self.assertIsNone(node['target_power_state']) + self.assertTrue('unexpected keyword argument' in node['last_error']) + class NodeSoftPowerActionTestCase(db_base.DbTestCase): diff --git a/ironic/tests/unit/drivers/modules/cimc/test_power.py b/ironic/tests/unit/drivers/modules/cimc/test_power.py index 4617c7d5cb..6ffaf67333 100644 --- a/ironic/tests/unit/drivers/modules/cimc/test_power.py +++ b/ironic/tests/unit/drivers/modules/cimc/test_power.py @@ -190,7 +190,8 @@ class PowerTestCase(test_common.CIMCBaseTestCase): handle.get_imc_managedobject.assert_called_with( None, None, params={"Dn": "sys/rack-unit-1"}) - def test_set_power_state_on_ok(self, mock_handle): + @mock.patch.object(power.LOG, 'warning') + def test_set_power_state_on_ok(self, mock_log, mock_handle): with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: with mock_handle(task) as handle: @@ -213,6 +214,7 @@ class PowerTestCase(test_common.CIMCBaseTestCase): handle.get_imc_managedobject.assert_called_with( None, None, params={"Dn": "sys/rack-unit-1"}) + self.assertFalse(mock_log.called) def test_set_power_state_on_fail(self, mock_handle): with task_manager.acquire(self.context, self.node.uuid, @@ -236,6 +238,33 @@ class PowerTestCase(test_common.CIMCBaseTestCase): handle.get_imc_managedobject.assert_called_with( None, None, params={"Dn": "sys/rack-unit-1"}) + @mock.patch.object(power.LOG, 'warning') + def test_set_power_state_on_timeout(self, mock_log, mock_handle): + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + with mock_handle(task) as handle: + mock_rack_unit = mock.MagicMock() + mock_rack_unit.get_attr.side_effect = [ + imcsdk.ComputeRackUnit.CONST_OPER_POWER_OFF, + imcsdk.ComputeRackUnit.CONST_OPER_POWER_ON + ] + handle.get_imc_managedobject.return_value = [mock_rack_unit] + + task.driver.power.set_power_state(task, states.POWER_ON, + timeout=10) + + handle.set_imc_managedobject.assert_called_once_with( + None, class_id="ComputeRackUnit", + params={ + imcsdk.ComputeRackUnit.ADMIN_POWER: + imcsdk.ComputeRackUnit.CONST_ADMIN_POWER_UP, + imcsdk.ComputeRackUnit.DN: "sys/rack-unit-1" + }) + + handle.get_imc_managedobject.assert_called_with( + None, None, params={"Dn": "sys/rack-unit-1"}) + self.assertTrue(mock_log.called) + def test_set_power_state_off_ok(self, mock_handle): with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: @@ -282,14 +311,17 @@ class PowerTestCase(test_common.CIMCBaseTestCase): handle.get_imc_managedobject.assert_called_with( None, None, params={"Dn": "sys/rack-unit-1"}) + @mock.patch.object(power.LOG, 'warning') @mock.patch.object(power.Power, "set_power_state", autospec=True) @mock.patch.object(power.Power, "get_power_state", autospec=True) - def test_reboot_on(self, mock_get_state, mock_set_state, mock_handle): + def test_reboot_on(self, mock_get_state, mock_set_state, mock_log, + mock_handle): with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: mock_get_state.return_value = states.POWER_ON task.driver.power.reboot(task) mock_set_state.assert_called_with(mock.ANY, task, states.REBOOT) + self.assertFalse(mock_log.called) @mock.patch.object(power.Power, "set_power_state", autospec=True) @mock.patch.object(power.Power, "get_power_state", autospec=True) @@ -299,3 +331,15 @@ class PowerTestCase(test_common.CIMCBaseTestCase): mock_get_state.return_value = states.POWER_OFF task.driver.power.reboot(task) mock_set_state.assert_called_with(mock.ANY, task, states.POWER_ON) + + @mock.patch.object(power.LOG, 'warning') + @mock.patch.object(power.Power, "set_power_state", autospec=True) + @mock.patch.object(power.Power, "get_power_state", autospec=True) + def test_reboot_on_timeout(self, mock_get_state, mock_set_state, mock_log, + mock_handle): + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + mock_get_state.return_value = states.POWER_ON + task.driver.power.reboot(task, timeout=30) + mock_set_state.assert_called_with(mock.ANY, task, states.REBOOT) + self.assertTrue(mock_log.called) diff --git a/ironic/tests/unit/drivers/modules/drac/test_power.py b/ironic/tests/unit/drivers/modules/drac/test_power.py index a507f1b286..d1edf36d81 100644 --- a/ironic/tests/unit/drivers/modules/drac/test_power.py +++ b/ironic/tests/unit/drivers/modules/drac/test_power.py @@ -71,7 +71,8 @@ class DracPowerTestCase(db_base.DbTestCase): mock_client.get_power_state.assert_called_once_with() - def test_set_power_state(self, mock_get_drac_client): + @mock.patch.object(drac_power.LOG, 'warning') + def test_set_power_state(self, mock_log, mock_get_drac_client): mock_client = mock_get_drac_client.return_value with task_manager.acquire(self.context, self.node.uuid, @@ -80,6 +81,7 @@ class DracPowerTestCase(db_base.DbTestCase): drac_power_state = drac_power.REVERSE_POWER_STATES[states.POWER_OFF] mock_client.set_power_state.assert_called_once_with(drac_power_state) + self.assertFalse(mock_log.called) def test_set_power_state_fail(self, mock_get_drac_client): mock_client = mock_get_drac_client.return_value @@ -95,7 +97,21 @@ class DracPowerTestCase(db_base.DbTestCase): drac_power_state = drac_power.REVERSE_POWER_STATES[states.POWER_OFF] mock_client.set_power_state.assert_called_once_with(drac_power_state) - def test_reboot_while_powered_on(self, mock_get_drac_client): + @mock.patch.object(drac_power.LOG, 'warning') + def test_set_power_state_timeout(self, mock_log, mock_get_drac_client): + mock_client = mock_get_drac_client.return_value + + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + task.driver.power.set_power_state(task, states.POWER_OFF, + timeout=11) + + drac_power_state = drac_power.REVERSE_POWER_STATES[states.POWER_OFF] + mock_client.set_power_state.assert_called_once_with(drac_power_state) + self.assertTrue(mock_log.called) + + @mock.patch.object(drac_power.LOG, 'warning') + def test_reboot_while_powered_on(self, mock_log, mock_get_drac_client): mock_client = mock_get_drac_client.return_value mock_client.get_power_state.return_value = drac_constants.POWER_ON @@ -105,6 +121,21 @@ class DracPowerTestCase(db_base.DbTestCase): drac_power_state = drac_power.REVERSE_POWER_STATES[states.REBOOT] mock_client.set_power_state.assert_called_once_with(drac_power_state) + self.assertFalse(mock_log.called) + + @mock.patch.object(drac_power.LOG, 'warning') + def test_reboot_while_powered_on_timeout(self, mock_log, + mock_get_drac_client): + mock_client = mock_get_drac_client.return_value + mock_client.get_power_state.return_value = drac_constants.POWER_ON + + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + task.driver.power.reboot(task, timeout=42) + + drac_power_state = drac_power.REVERSE_POWER_STATES[states.REBOOT] + mock_client.set_power_state.assert_called_once_with(drac_power_state) + self.assertTrue(mock_log.called) def test_reboot_while_powered_off(self, mock_get_drac_client): mock_client = mock_get_drac_client.return_value diff --git a/ironic/tests/unit/drivers/modules/ilo/test_power.py b/ironic/tests/unit/drivers/modules/ilo/test_power.py index 392a30ceab..52216db761 100644 --- a/ironic/tests/unit/drivers/modules/ilo/test_power.py +++ b/ironic/tests/unit/drivers/modules/ilo/test_power.py @@ -209,20 +209,35 @@ class IloPowerTestCase(db_base.DbTestCase): task.driver.power.get_power_state(task)) mock_get_power.assert_called_once_with(task.node) + @mock.patch.object(ilo_power.LOG, 'warning') @mock.patch.object(ilo_power, '_set_power_state', spec_set=True, autospec=True) - def test_set_power_state(self, mock_set_power): + def test_set_power_state(self, mock_set_power, mock_log): mock_set_power.return_value = states.POWER_ON with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: task.driver.power.set_power_state(task, states.POWER_ON) mock_set_power.assert_called_once_with(task, states.POWER_ON) + self.assertFalse(mock_log.called) + @mock.patch.object(ilo_power.LOG, 'warning') + @mock.patch.object(ilo_power, '_set_power_state', spec_set=True, + autospec=True) + def test_set_power_state_timeout(self, mock_set_power, mock_log): + mock_set_power.return_value = states.POWER_ON + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + task.driver.power.set_power_state(task, states.POWER_ON, + timeout=13) + mock_set_power.assert_called_once_with(task, states.POWER_ON) + self.assertTrue(mock_log.called) + + @mock.patch.object(ilo_power.LOG, 'warning') @mock.patch.object(ilo_power, '_set_power_state', spec_set=True, autospec=True) @mock.patch.object(ilo_power, '_get_power_state', spec_set=True, autospec=True) - def test_reboot(self, mock_get_power, mock_set_power): + def test_reboot(self, mock_get_power, mock_set_power, mock_log): with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: mock_get_power.return_value = states.POWER_ON @@ -230,3 +245,19 @@ class IloPowerTestCase(db_base.DbTestCase): task.driver.power.reboot(task) mock_get_power.assert_called_once_with(task.node) mock_set_power.assert_called_once_with(task, states.REBOOT) + self.assertFalse(mock_log.called) + + @mock.patch.object(ilo_power.LOG, 'warning') + @mock.patch.object(ilo_power, '_set_power_state', spec_set=True, + autospec=True) + @mock.patch.object(ilo_power, '_get_power_state', spec_set=True, + autospec=True) + def test_reboot_timeout(self, mock_get_power, mock_set_power, mock_log): + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + mock_get_power.return_value = states.POWER_ON + mock_set_power.return_value = states.POWER_ON + task.driver.power.reboot(task, timeout=123) + mock_get_power.assert_called_once_with(task.node) + mock_set_power.assert_called_once_with(task, states.REBOOT) + self.assertTrue(mock_log.called) diff --git a/ironic/tests/unit/drivers/modules/test_snmp.py b/ironic/tests/unit/drivers/modules/test_snmp.py index 0a127601f3..349268a409 100644 --- a/ironic/tests/unit/drivers/modules/test_snmp.py +++ b/ironic/tests/unit/drivers/modules/test_snmp.py @@ -1267,12 +1267,24 @@ class SNMPDriverTestCase(db_base.DbTestCase): task.driver.power.get_power_state, task) mock_driver.power_state.assert_called_once_with() - def test_set_power_state_on(self, mock_get_driver): + @mock.patch.object(snmp.LOG, 'warning') + def test_set_power_state_on(self, mock_log, mock_get_driver): mock_driver = mock_get_driver.return_value mock_driver.power_on.return_value = states.POWER_ON with task_manager.acquire(self.context, self.node.uuid) as task: task.driver.power.set_power_state(task, states.POWER_ON) mock_driver.power_on.assert_called_once_with() + self.assertFalse(mock_log.called) + + @mock.patch.object(snmp.LOG, 'warning') + def test_set_power_state_on_timeout(self, mock_log, mock_get_driver): + mock_driver = mock_get_driver.return_value + mock_driver.power_on.return_value = states.POWER_ON + with task_manager.acquire(self.context, self.node.uuid) as task: + task.driver.power.set_power_state(task, states.POWER_ON, + timeout=222) + mock_driver.power_on.assert_called_once_with() + self.assertTrue(mock_log.called) def test_set_power_state_off(self, mock_get_driver): mock_driver = mock_get_driver.return_value @@ -1305,7 +1317,7 @@ class SNMPDriverTestCase(db_base.DbTestCase): task, states.POWER_OFF) mock_driver.power_off.assert_called_once_with() - def test_set_power_state_on_timeout(self, mock_get_driver): + def test_set_power_state_on_error(self, mock_get_driver): mock_driver = mock_get_driver.return_value mock_driver.power_on.return_value = states.ERROR with task_manager.acquire(self.context, self.node.uuid) as task: @@ -1314,7 +1326,7 @@ class SNMPDriverTestCase(db_base.DbTestCase): task, states.POWER_ON) mock_driver.power_on.assert_called_once_with() - def test_set_power_state_off_timeout(self, mock_get_driver): + def test_set_power_state_off_error(self, mock_get_driver): mock_driver = mock_get_driver.return_value mock_driver.power_off.return_value = states.ERROR with task_manager.acquire(self.context, self.node.uuid) as task: @@ -1323,12 +1335,23 @@ class SNMPDriverTestCase(db_base.DbTestCase): task, states.POWER_OFF) mock_driver.power_off.assert_called_once_with() - def test_reboot(self, mock_get_driver): + @mock.patch.object(snmp.LOG, 'warning') + def test_reboot(self, mock_log, mock_get_driver): mock_driver = mock_get_driver.return_value mock_driver.power_reset.return_value = states.POWER_ON with task_manager.acquire(self.context, self.node.uuid) as task: task.driver.power.reboot(task) mock_driver.power_reset.assert_called_once_with() + self.assertFalse(mock_log.called) + + @mock.patch.object(snmp.LOG, 'warning') + def test_reboot_timeout(self, mock_log, mock_get_driver): + mock_driver = mock_get_driver.return_value + mock_driver.power_reset.return_value = states.POWER_ON + with task_manager.acquire(self.context, self.node.uuid) as task: + task.driver.power.reboot(task, timeout=1) + mock_driver.power_reset.assert_called_once_with() + self.assertTrue(mock_log.called) def test_reboot_snmp_failure(self, mock_get_driver): mock_driver = mock_get_driver.return_value @@ -1338,7 +1361,7 @@ class SNMPDriverTestCase(db_base.DbTestCase): task.driver.power.reboot, task) mock_driver.power_reset.assert_called_once_with() - def test_reboot_timeout(self, mock_get_driver): + def test_reboot_error(self, mock_get_driver): mock_driver = mock_get_driver.return_value mock_driver.power_reset.return_value = states.ERROR with task_manager.acquire(self.context, self.node.uuid) as task: diff --git a/ironic/tests/unit/drivers/modules/ucs/test_power.py b/ironic/tests/unit/drivers/modules/ucs/test_power.py index 3c16847f4b..9e76604cfb 100644 --- a/ironic/tests/unit/drivers/modules/ucs/test_power.py +++ b/ironic/tests/unit/drivers/modules/ucs/test_power.py @@ -137,13 +137,15 @@ class UcsPowerTestCase(db_base.DbTestCase): task) power.get_power_state.assert_called_with() + @mock.patch.object(ucs_power.LOG, 'warning') @mock.patch('ironic.drivers.modules.ucs.helper.ucs_helper', spec_set=True, autospec=True) @mock.patch('ironic.drivers.modules.ucs.power._wait_for_state_change', spec_set=True, autospec=True) @mock.patch('ironic.drivers.modules.ucs.power.ucs_power.UcsPower', spec_set=True, autospec=True) - def test_set_power_state(self, mock_power_helper, mock__wait, mock_helper): + def test_set_power_state(self, mock_power_helper, mock__wait, mock_helper, + mock_log): target_state = states.POWER_ON mock_power = mock_power_helper.return_value mock_power.get_power_state.side_effect = ['down', 'up'] @@ -157,6 +159,32 @@ class UcsPowerTestCase(db_base.DbTestCase): mock_power.set_power_state.assert_called_once_with('up') mock_power.get_power_state.assert_called_once_with() mock__wait.assert_called_once_with(target_state, mock_power) + self.assertFalse(mock_log.called) + + @mock.patch.object(ucs_power.LOG, 'warning') + @mock.patch('ironic.drivers.modules.ucs.helper.ucs_helper', + spec_set=True, autospec=True) + @mock.patch('ironic.drivers.modules.ucs.power._wait_for_state_change', + spec_set=True, autospec=True) + @mock.patch('ironic.drivers.modules.ucs.power.ucs_power.UcsPower', + spec_set=True, autospec=True) + def test_set_power_state_timeout(self, mock_power_helper, mock__wait, + mock_helper, mock_log): + target_state = states.POWER_ON + mock_power = mock_power_helper.return_value + mock_power.get_power_state.side_effect = ['down', 'up'] + mock_helper.generate_ucsm_handle.return_value = (True, mock.Mock()) + mock__wait.return_value = target_state + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + self.assertIsNone(self.interface.set_power_state(task, + target_state, + timeout=23)) + + mock_power.set_power_state.assert_called_once_with('up') + mock_power.get_power_state.assert_called_once_with() + mock__wait.assert_called_once_with(target_state, mock_power) + self.assertTrue(mock_log.called) @mock.patch('ironic.drivers.modules.ucs.helper.ucs_helper', spec_set=True, autospec=True) @@ -249,13 +277,15 @@ class UcsPowerTestCase(db_base.DbTestCase): mock_power.get_power_state.assert_called_once_with() mock__wait.assert_called_once_with(target_state, mock_power) + @mock.patch.object(ucs_power.LOG, 'warning') @mock.patch('ironic.drivers.modules.ucs.helper.ucs_helper', spec_set=True, autospec=True) @mock.patch('ironic.drivers.modules.ucs.power._wait_for_state_change', spec_set=True, autospec=True) @mock.patch('ironic.drivers.modules.ucs.power.ucs_power.UcsPower', spec_set=True, autospec=True) - def test_reboot(self, mock_power_helper, mock__wait, mock_helper): + def test_reboot(self, mock_power_helper, mock__wait, mock_helper, + mock_log): mock_helper.generate_ucsm_handle.return_value = (True, mock.Mock()) mock_power = mock_power_helper.return_value mock__wait.return_value = states.POWER_ON @@ -263,6 +293,25 @@ class UcsPowerTestCase(db_base.DbTestCase): shared=False) as task: self.assertIsNone(self.interface.reboot(task)) mock_power.reboot.assert_called_once_with() + self.assertFalse(mock_log.called) + + @mock.patch.object(ucs_power.LOG, 'warning') + @mock.patch('ironic.drivers.modules.ucs.helper.ucs_helper', + spec_set=True, autospec=True) + @mock.patch('ironic.drivers.modules.ucs.power._wait_for_state_change', + spec_set=True, autospec=True) + @mock.patch('ironic.drivers.modules.ucs.power.ucs_power.UcsPower', + spec_set=True, autospec=True) + def test_reboot_timeout(self, mock_power_helper, mock__wait, mock_helper, + mock_log): + mock_helper.generate_ucsm_handle.return_value = (True, mock.Mock()) + mock_power = mock_power_helper.return_value + mock__wait.return_value = states.POWER_ON + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + self.assertIsNone(self.interface.reboot(task, timeout=88)) + mock_power.reboot.assert_called_once_with() + self.assertTrue(mock_log.called) @mock.patch('ironic.drivers.modules.ucs.helper.ucs_helper', spec_set=True, autospec=True) diff --git a/ironic/tests/unit/drivers/modules/xclarity/test_power.py b/ironic/tests/unit/drivers/modules/xclarity/test_power.py index f695c4c29f..6cec8cd31f 100644 --- a/ironic/tests/unit/drivers/modules/xclarity/test_power.py +++ b/ironic/tests/unit/drivers/modules/xclarity/test_power.py @@ -90,13 +90,28 @@ class XClarityPowerDriverTestCase(db_base.DbTestCase): task.driver.power.get_power_state, task) + @mock.patch.object(power.LOG, 'warning') @mock.patch.object(power.XClarityPower, 'get_power_state', return_value=states.POWER_ON) - def test_set_power(self, mock_set_power_state, mock_get_xc_client): + def test_set_power(self, mock_set_power_state, mock_log, + mock_get_xc_client): with task_manager.acquire(self.context, self.node.uuid) as task: task.driver.power.set_power_state(task, states.POWER_ON) expected = task.driver.power.get_power_state(task) self.assertEqual(expected, states.POWER_ON) + self.assertFalse(mock_log.called) + + @mock.patch.object(power.LOG, 'warning') + @mock.patch.object(power.XClarityPower, 'get_power_state', + return_value=states.POWER_ON) + def test_set_power_timeout(self, mock_set_power_state, mock_log, + mock_get_xc_client): + with task_manager.acquire(self.context, self.node.uuid) as task: + task.driver.power.set_power_state(task, states.POWER_ON, + timeout=21) + expected = task.driver.power.get_power_state(task) + self.assertEqual(expected, states.POWER_ON) + self.assertTrue(mock_log.called) def test_set_power_fail(self, mock_xc_client): with task_manager.acquire(self.context, self.node.uuid) as task: @@ -111,3 +126,20 @@ class XClarityPowerDriverTestCase(db_base.DbTestCase): self.assertRaises(common.XClarityError, task.driver.power.set_power_state, task, states.POWER_OFF) + + @mock.patch.object(power.LOG, 'warning') + @mock.patch.object(power.XClarityPower, 'set_power_state') + def test_reboot(self, mock_set_power_state, mock_log, mock_get_xc_client): + with task_manager.acquire(self.context, self.node.uuid) as task: + task.driver.power.reboot(task) + mock_set_power_state.assert_called_with(task, states.REBOOT) + self.assertFalse(mock_log.called) + + @mock.patch.object(power.LOG, 'warning') + @mock.patch.object(power.XClarityPower, 'set_power_state') + def test_reboot_timeout(self, mock_set_power_state, mock_log, + mock_get_xc_client): + with task_manager.acquire(self.context, self.node.uuid) as task: + task.driver.power.reboot(task, timeout=55) + mock_set_power_state.assert_called_with(task, states.REBOOT) + self.assertTrue(mock_log.called) diff --git a/releasenotes/notes/add-timeout-parameter-to-power-methods-5f632c936497685e.yaml b/releasenotes/notes/add-timeout-parameter-to-power-methods-5f632c936497685e.yaml new file mode 100644 index 0000000000..0cf840311d --- /dev/null +++ b/releasenotes/notes/add-timeout-parameter-to-power-methods-5f632c936497685e.yaml @@ -0,0 +1,13 @@ +--- +fixes: + - | + Fixes an issue where the ``timeout`` that was specified for a node's + power state transition was not used for the *ipmitool*, *irmc* and + *oneview* Power Interfaces. + See `bug 1746849 `_ + for details. +other: + - | + The ironic-conductor expects that all PowerInterface's set_power_state() + and reboot() methods accept a ``timeout`` parameter. Any out-of-tree + implementations that don't, will cause TypeError exceptions to be raised.