From e7d1bf3206d65fb8319dcf35491e5aec401c77da Mon Sep 17 00:00:00 2001 From: Ruby Loo Date: Wed, 7 Feb 2018 16:41:16 -0500 Subject: [PATCH] Handle 'timeout' parameter in power methods The PowerInterface methods set_power_state() and reboot() were enhanced to take a 'timeout' parameter [1]. In the Queens release [2] , the conductor assumes that all PowerInterfaces can support this timeout parameter. This patch changes the PowerInterfaces so that they accept a 'timeout' parameter for reboot() and set_power_state(). The PowerInterfaces log a warning if the parameter is specified when it isn't supported. [1] f15d5b9a37260b3876f9dadeb030412e6e1053b2 [2] 9e87cebc12102cbb3ae47366836dcd7c3e439828 Change-Id: I0904958f1a7f981204ba41bfbdc0083182c5622e --- ironic_staging_drivers/amt/power.py | 21 +++++++- ironic_staging_drivers/iboot/power.py | 20 +++++++- ironic_staging_drivers/libvirt/power.py | 19 +++++++- .../tests/unit/amt/test_power.py | 32 ++++++++++++- .../tests/unit/iboot/test_power.py | 48 ++++++++++++++++++- .../tests/unit/libvirt/test_power.py | 23 ++++++++- .../tests/unit/wol/test_power.py | 16 ++++++- ironic_staging_drivers/wol/power.py | 16 +++++-- 8 files changed, 179 insertions(+), 16 deletions(-) diff --git a/ironic_staging_drivers/amt/power.py b/ironic_staging_drivers/amt/power.py index e7d24ad..2879bff 100644 --- a/ironic_staging_drivers/amt/power.py +++ b/ironic_staging_drivers/amt/power.py @@ -237,29 +237,46 @@ class AMTPower(base.PowerInterface): return _power_status(task.node) @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 node. Turn the node power on or off. :param task: a TaskManager instance contains the target node. :param pstate: The desired power state of the node. + :param timeout: timeout (in seconds). Unsupported by this interface. :raises: PowerStateFailure if the power cannot set to pstate. :raises: AMTFailure. :raises: AMTConnectFailure. :raises: InvalidParameterValue """ + # TODO(rloo): Support timeouts! + if timeout is not None: + LOG.warning( + "The 'amt' Power Interface's 'set_power_state' method " + "doesn't support the 'timeout' parameter. Ignoring " + "timeout=%(timeout)s", + {'timeout': timeout}) + _set_and_wait(task, pstate) @task_manager.require_exclusive_lock - def reboot(self, task): + def reboot(self, task, timeout=None): """Cycle the power of the node :param task: a TaskManager instance contains the target node. + :param timeout: timeout (in seconds). Unsupported by this interface. :raises: PowerStateFailure if failed to reboot. :raises: AMTFailure. :raises: AMTConnectFailure. :raises: InvalidParameterValue """ + # TODO(rloo): Support timeouts! + if timeout is not None: + LOG.warning("The 'amt' Power Interface's 'reboot' method " + "doesn't support the 'timeout' parameter. Ignoring " + "timeout=%(timeout)s", + {'timeout': timeout}) + _set_and_wait(task, states.POWER_OFF) _set_and_wait(task, states.POWER_ON) diff --git a/ironic_staging_drivers/iboot/power.py b/ironic_staging_drivers/iboot/power.py index 3d20457..3688da9 100644 --- a/ironic_staging_drivers/iboot/power.py +++ b/ironic_staging_drivers/iboot/power.py @@ -242,12 +242,13 @@ class IBootPower(base.PowerInterface): return _power_status(driver_info) @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. :param task: a TaskManager instance containing the node to act on. :param pstate: The desired power state, one of ironic.common.states POWER_ON, POWER_OFF. + :param timeout: timeout (in seconds). Unsupported by this interface. :raises: InvalidParameterValue if iboot parameters are invalid or if an invalid power state was specified. :raises: MissingParameterValue if required iboot parameters are @@ -255,6 +256,14 @@ class IBootPower(base.PowerInterface): :raises: PowerStateFailure if the power couldn't be set to pstate. """ + # TODO(rloo): Support timeouts! + if timeout is not None: + LOG.warning( + "The 'iboot' Power Interface's 'set_power_state' method " + "doesn't support the 'timeout' parameter. Ignoring " + "timeout=%(timeout)s", + {'timeout': timeout}) + driver_info = _parse_driver_info(task.node) if pstate == states.POWER_ON: _switch(driver_info, True) @@ -268,10 +277,11 @@ class IBootPower(base.PowerInterface): _check_power_state(driver_info, pstate) @task_manager.require_exclusive_lock - def reboot(self, task): + def reboot(self, task, timeout=None): """Cycles the power to 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 iboot parameters are invalid. :raises: MissingParameterValue if required iboot parameters are missing. @@ -279,6 +289,12 @@ class IBootPower(base.PowerInterface): POWER_ON. """ + if timeout is not None: + LOG.warning("The 'iboot' Power Interface's 'reboot' method " + "doesn't support the 'timeout' parameter. Ignoring " + "timeout=%(timeout)s", + {'timeout': timeout}) + driver_info = _parse_driver_info(task.node) _switch(driver_info, False) _sleep_switch(CONF.iboot.reboot_delay) diff --git a/ironic_staging_drivers/libvirt/power.py b/ironic_staging_drivers/libvirt/power.py index 405e564..fb2712b 100644 --- a/ironic_staging_drivers/libvirt/power.py +++ b/ironic_staging_drivers/libvirt/power.py @@ -369,7 +369,7 @@ class LibvirtPower(base.PowerInterface): return _get_power_state(domain) @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 the task's node. @@ -377,6 +377,7 @@ class LibvirtPower(base.PowerInterface): :param task: a TaskManager instance containing the node to act on. :param pstate: Either POWER_ON or POWER_OFF from :class: `ironic.common.states`. + :param timeout: timeout (in seconds). Unsupported by this interface. :raises: InvalidParameterValue if any connection parameters are incorrect, or if the desired power state is invalid. :raises: MissingParameterValue when a required parameter is missing @@ -385,6 +386,13 @@ class LibvirtPower(base.PowerInterface): :raises: PowerStateFailure if it failed to set power state to pstate. :raises: LibvirtError if failed to connect to the Libvirt uri. """ + # TODO(rloo): Support timeouts! + if timeout is not None: + LOG.warning( + "The 'libvirt' Power Interface's 'set_power_state' method " + "doesn't support the 'timeout' parameter. Ignoring " + "timeout=%(timeout)s", + {'timeout': timeout}) domain = _get_domain_by_macs(task) if pstate == states.POWER_ON: @@ -400,12 +408,13 @@ class LibvirtPower(base.PowerInterface): raise ir_exc.PowerStateFailure(pstate=pstate) @task_manager.require_exclusive_lock - def reboot(self, task): + def reboot(self, task, timeout=None): """Cycles the power to the task's node. Power cycles a node. :param task: a TaskManager instance containing the node to act on. + :param timeout: timeout (in seconds). Unsupported by this interface. :raises: InvalidParameterValue if any connection parameters are incorrect. :raises: MissingParameterValue when a required parameter is missing @@ -414,6 +423,12 @@ class LibvirtPower(base.PowerInterface): :raises: PowerStateFailure if it failed to set power state to POWER_ON. :raises: LibvirtError if failed to connect to the Libvirt uri. """ + # TODO(rloo): Support timeouts! + if timeout is not None: + LOG.warning("The 'libvirt' Power Interface's 'reboot' method " + "doesn't support the 'timeout' parameter. Ignoring " + "timeout=%(timeout)s", + {'timeout': timeout}) domain = _get_domain_by_macs(task) diff --git a/ironic_staging_drivers/tests/unit/amt/test_power.py b/ironic_staging_drivers/tests/unit/amt/test_power.py index 0f60848..7e411a0 100644 --- a/ironic_staging_drivers/tests/unit/amt/test_power.py +++ b/ironic_staging_drivers/tests/unit/amt/test_power.py @@ -262,15 +262,17 @@ class AMTPowerTestCase(db_base.DbTestCase): task.driver.power.get_power_state(task)) mock_ps.assert_called_once_with(task.node) + @mock.patch.object(amt_power.LOG, 'warning') @mock.patch.object(amt_power, '_set_and_wait', spec_set=True, autospec=True) - def test_set_power_state(self, mock_saw): + def test_set_power_state(self, mock_saw, mock_log): with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: pstate = states.POWER_ON mock_saw.return_value = states.POWER_ON task.driver.power.set_power_state(task, pstate) mock_saw.assert_called_once_with(task, pstate) + self.assertFalse(mock_log.called) @mock.patch.object(amt_power, '_set_and_wait', spec_set=True, autospec=True) @@ -285,12 +287,38 @@ class AMTPowerTestCase(db_base.DbTestCase): task, pstate) mock_saw.assert_called_once_with(task, pstate) + @mock.patch.object(amt_power.LOG, 'warning') @mock.patch.object(amt_power, '_set_and_wait', spec_set=True, autospec=True) - def test_reboot(self, mock_saw): + def test_set_power_state_timeout(self, mock_saw, mock_log): + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + pstate = states.POWER_ON + mock_saw.return_value = states.POWER_ON + task.driver.power.set_power_state(task, pstate, timeout=12) + mock_saw.assert_called_once_with(task, pstate) + self.assertTrue(mock_log.called) + + @mock.patch.object(amt_power.LOG, 'warning') + @mock.patch.object(amt_power, '_set_and_wait', spec_set=True, + autospec=True) + def test_reboot(self, mock_saw, mock_log): with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: task.driver.power.reboot(task) calls = [mock.call(task, states.POWER_OFF), mock.call(task, states.POWER_ON)] mock_saw.assert_has_calls(calls) + self.assertFalse(mock_log.called) + + @mock.patch.object(amt_power.LOG, 'warning') + @mock.patch.object(amt_power, '_set_and_wait', spec_set=True, + autospec=True) + def test_reboot_timeout(self, mock_saw, mock_log): + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + task.driver.power.reboot(task, timeout=13) + calls = [mock.call(task, states.POWER_OFF), + mock.call(task, states.POWER_ON)] + mock_saw.assert_has_calls(calls) + self.assertTrue(mock_log.called) diff --git a/ironic_staging_drivers/tests/unit/iboot/test_power.py b/ironic_staging_drivers/tests/unit/iboot/test_power.py index 0f8a82f..06c7c75 100644 --- a/ironic_staging_drivers/tests/unit/iboot/test_power.py +++ b/ironic_staging_drivers/tests/unit/iboot/test_power.py @@ -276,9 +276,11 @@ class IBootDriverTestCase(db_base.DbTestCase): self.context, self.node.uuid, shared=True) as task: self.assertEqual(expected, task.driver.get_properties()) + @mock.patch.object(iboot_power.LOG, 'warning') @mock.patch.object(iboot_power, '_power_status', autospec=True) @mock.patch.object(iboot_power, '_switch', autospec=True) - def test_set_power_state_good(self, mock_switch, mock_power_status): + def test_set_power_state_good(self, mock_switch, mock_power_status, + mock_log): mock_power_status.return_value = states.POWER_ON with task_manager.acquire(self.context, self.node.uuid) as task: @@ -287,6 +289,23 @@ class IBootDriverTestCase(db_base.DbTestCase): # ensure functions were called with the valid parameters mock_switch.assert_called_once_with(self.info, True) mock_power_status.assert_called_once_with(self.info) + self.assertFalse(mock_log.called) + + @mock.patch.object(iboot_power.LOG, 'warning') + @mock.patch.object(iboot_power, '_power_status', autospec=True) + @mock.patch.object(iboot_power, '_switch', autospec=True) + def test_set_power_state_timeout(self, mock_switch, mock_power_status, + mock_log): + mock_power_status.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=22) + + # ensure functions were called with the valid parameters + mock_switch.assert_called_once_with(self.info, True) + mock_power_status.assert_called_once_with(self.info) + self.assertTrue(mock_log.called) @mock.patch.object(iboot_power, '_power_status', autospec=True) @mock.patch.object(iboot_power, '_switch', autospec=True) @@ -329,12 +348,13 @@ class IBootDriverTestCase(db_base.DbTestCase): task.driver.power.set_power_state, task, states.NOSTATE) + @mock.patch.object(iboot_power.LOG, 'warning') @mock.patch.object(iboot_power, '_sleep_switch', spec_set=types.FunctionType) @mock.patch.object(iboot_power, '_power_status', autospec=True) @mock.patch.object(iboot_power, '_switch', spec_set=types.FunctionType) def test_reboot_good(self, mock_switch, mock_power_status, - mock_sleep_switch): + mock_sleep_switch, mock_log): self.config(reboot_delay=3, group='iboot') manager = mock.MagicMock(spec_set=['switch', 'sleep']) mock_power_status.return_value = states.POWER_ON @@ -349,6 +369,30 @@ class IBootDriverTestCase(db_base.DbTestCase): task.driver.power.reboot(task) self.assertEqual(expected, manager.mock_calls) + self.assertFalse(mock_log.called) + + @mock.patch.object(iboot_power.LOG, 'warning') + @mock.patch.object(iboot_power, '_sleep_switch', + spec_set=types.FunctionType) + @mock.patch.object(iboot_power, '_power_status', autospec=True) + @mock.patch.object(iboot_power, '_switch', spec_set=types.FunctionType) + def test_reboot_good_timeout(self, mock_switch, mock_power_status, + mock_sleep_switch, mock_log): + self.config(reboot_delay=3, group='iboot') + manager = mock.MagicMock(spec_set=['switch', 'sleep']) + mock_power_status.return_value = states.POWER_ON + + manager.attach_mock(mock_switch, 'switch') + manager.attach_mock(mock_sleep_switch, 'sleep') + expected = [mock.call.switch(self.info, False), + mock.call.sleep(3), + mock.call.switch(self.info, True)] + + with task_manager.acquire(self.context, self.node.uuid) as task: + task.driver.power.reboot(task, timeout=12) + + self.assertEqual(expected, manager.mock_calls) + self.assertTrue(mock_log.called) @mock.patch.object(iboot_power, '_sleep_switch', spec_set=types.FunctionType) diff --git a/ironic_staging_drivers/tests/unit/libvirt/test_power.py b/ironic_staging_drivers/tests/unit/libvirt/test_power.py index dab7ea7..221778b 100644 --- a/ironic_staging_drivers/tests/unit/libvirt/test_power.py +++ b/ironic_staging_drivers/tests/unit/libvirt/test_power.py @@ -485,9 +485,11 @@ class LibvirtPowerTestCase(db_base.DbTestCase): get_domain_mock.assert_called_once_with(task) get_power_state.assert_called_once_with(domain) + @mock.patch.object(power.LOG, 'warning') @mock.patch.object(power, '_power_on', autospec=True) @mock.patch.object(power, '_get_domain_by_macs', autospec=True) - def test_set_power_state_on(self, get_domain_mock, power_on_mock): + def test_set_power_state_on(self, get_domain_mock, power_on_mock, + log_mock): domain = FakeLibvirtDomain() get_domain_mock.return_value = domain power_on_mock.return_value = states.POWER_ON @@ -498,6 +500,25 @@ class LibvirtPowerTestCase(db_base.DbTestCase): get_domain_mock.assert_called_once_with(task) power_on_mock.assert_called_once_with(domain) + self.assertFalse(log_mock.called) + + @mock.patch.object(power.LOG, 'warning') + @mock.patch.object(power, '_power_on', autospec=True) + @mock.patch.object(power, '_get_domain_by_macs', autospec=True) + def test_set_power_state_on_timeout(self, get_domain_mock, power_on_mock, + log_mock): + domain = FakeLibvirtDomain() + get_domain_mock.return_value = domain + power_on_mock.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=42) + + get_domain_mock.assert_called_once_with(task) + power_on_mock.assert_called_once_with(domain) + self.assertTrue(log_mock.called) @mock.patch.object(power, '_power_off', autospec=True) @mock.patch.object(power, '_get_domain_by_macs', autospec=True) diff --git a/ironic_staging_drivers/tests/unit/wol/test_power.py b/ironic_staging_drivers/tests/unit/wol/test_power.py index c65999c..39d8e88 100644 --- a/ironic_staging_drivers/tests/unit/wol/test_power.py +++ b/ironic_staging_drivers/tests/unit/wol/test_power.py @@ -158,12 +158,24 @@ class WakeOnLanDriverTestCase(db_base.DbTestCase): pstate = task.driver.power.get_power_state(task) self.assertEqual(states.POWER_OFF, pstate) + @mock.patch.object(wol_power.LOG, 'warning') @mock.patch.object(wol_power, '_send_magic_packets', autospec=True, spec_set=True) - def test_set_power_state_power_on(self, mock_magic): + def test_set_power_state_power_on(self, mock_magic, mock_log): with task_manager.acquire(self.context, self.node.uuid) as task: task.driver.power.set_power_state(task, states.POWER_ON) mock_magic.assert_called_once_with(task, '255.255.255.255', 9) + self.assertFalse(mock_log.called) + + @mock.patch.object(wol_power.LOG, 'warning') + @mock.patch.object(wol_power, '_send_magic_packets', autospec=True, + spec_set=True) + def test_set_power_state_power_on_timeout(self, mock_magic, mock_log): + with task_manager.acquire(self.context, self.node.uuid) as task: + task.driver.power.set_power_state(task, states.POWER_ON, + timeout=13) + mock_magic.assert_called_once_with(task, '255.255.255.255', 9) + self.assertTrue(mock_log.called) @mock.patch.object(wol_power.LOG, 'info', autospec=True, spec_set=True) @mock.patch.object(wol_power, '_send_magic_packets', autospec=True, @@ -193,7 +205,7 @@ class WakeOnLanDriverTestCase(db_base.DbTestCase): task.driver.power.reboot(task) mock_log.assert_called_once_with(mock.ANY, self.node.uuid) mock_power.assert_called_once_with(task.driver.power, task, - states.POWER_ON) + states.POWER_ON, timeout=None) def test_get_supported_power_states(self): with task_manager.acquire( diff --git a/ironic_staging_drivers/wol/power.py b/ironic_staging_drivers/wol/power.py index 8d8d7f4..7b37604 100644 --- a/ironic_staging_drivers/wol/power.py +++ b/ironic_staging_drivers/wol/power.py @@ -133,7 +133,7 @@ class WakeOnLanPower(base.PowerInterface): return states.POWER_OFF if pstate is states.NOSTATE else pstate @task_manager.require_exclusive_lock - def set_power_state(self, task, pstate): + def set_power_state(self, task, pstate, timeout=None): """Wakes the task's node on power on. Powering off is not supported. Wakes the task's node on. Wake-On-Lan does not support powering @@ -142,12 +142,21 @@ class WakeOnLanPower(base.PowerInterface): :param task: a TaskManager instance containing the node to act on. :param pstate: The desired power state, one of ironic.common.states POWER_ON, POWER_OFF. + :param timeout: timeout (in seconds). Unsupported by this interface. :raises: InvalidParameterValue if parameters are invalid. :raises: MissingParameterValue if required parameters are missing. :raises: WOLOperationError if an error occur when sending the magic packets """ + # TODO(rloo): Support timeouts! + if timeout is not None: + LOG.warning( + "The 'wol' Power Interface's 'set_power_state' method " + "doesn't support the 'timeout' parameter. Ignoring " + "timeout=%(timeout)s", + {'timeout': timeout}) + node = task.node params = _parse_parameters(task) if pstate == states.POWER_ON: @@ -163,13 +172,14 @@ class WakeOnLanPower(base.PowerInterface): 'pstate': pstate}) @task_manager.require_exclusive_lock - def reboot(self, task): + def reboot(self, task, timeout=None): """Not supported. Cycles the power to the task's node. This operation is not fully supported by the Wake-On-Lan driver. So this method will just try to power the task's node on. :param task: a TaskManager instance containing the node to act on. + :param timeout: timeout (in seconds). :raises: InvalidParameterValue if parameters are invalid. :raises: MissingParameterValue if required parameters are missing. :raises: WOLOperationError if an error occur when sending the @@ -179,7 +189,7 @@ class WakeOnLanPower(base.PowerInterface): LOG.info('Reboot called for node %s. Wake-On-Lan does ' 'not fully support this operation. Trying to ' 'power on the node.', task.node.uuid) - self.set_power_state(task, states.POWER_ON) + self.set_power_state(task, states.POWER_ON, timeout=timeout) def get_supported_power_states(self, task): """Get a list of the supported power states.