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 2bd9b10377..bab8100521 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.