conductor.utils.node_power_action() doesn't always use timeout

[Ruby Loo]

The PowerInterface methods set_power_state() and reboot() were enhanced to take a 'timeout' parameter [1]. For backwards compatibility and to support Interfaces that didn't support timeout, conductor.utils.node_power_action() used reflection.get_signature() to determine whether or not the node's driver's PowerInterface's methods could handle a timeout parameter.

The problem is 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 think that 'timeout' was not supported, even if it were supported.

At the time of filing this bug, these PowerInterfaces support 'timeout':
- FakePower
- ipmitool
- irmc
- oneview
- redfish

Of the above interfaces, ipmitool, irmc, and oneview use the METRICS decorator, so the 'timeout' parameter was being ignored, even though it should have been used :-(

[1] https://review.openstack.org/#/c/216730/
[2] https://bugs.launchpad.net/ironic/+bug/1746730

[Ruby Loo (rloo)]

We had a discussion on irc about this [1] - [2]. Among other things, we decided that we'd add a timeout parameter to all the in-tree PowerInterfaces. Out-of-tree PowerInterfaces will cause a TypeError exception to be raised.

[1] http://eavesdrop.openstack.org/irclogs/%23openstack-ironic/%23openstack-ironic.2018-02-01.log.html#t2018-02-01T15:51:18
[2] http://eavesdrop.openstack.org/irclogs/%23openstack-ironic/%23openstack-ironic.2018-02-01.log.html#t2018-02-01T17:18:33

[Ruby Loo (rloo)]

Patch to address this: https://review.openstack.org/#/c/540150/

[Hironori Shiina (shiina-hironori)]

FWIW, it seems that 'timeout' works with most power interfaces fortunately.
Reflection can see 'timeout' parameter if a method is wrapped by another decorator with six.wraps.

This is a quick check with the interactive interpreter:

$ python
Python 2.7.12 (default, Dec 4 2017, 14:50:18)
[GCC 5.4.0 20160609] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from ironic.drivers.modules import ipmitool
>>> from oslo_utils import reflection
>>> power = ipmitool.IPMIPower()
>>> reflection.get_signature(power.set_power_state).parameters
OrderedDict([('task', <Parameter at 0x7fb6b43dad08 'task'>), ('power_state', <Parameter at 0x7fb6b43dad60 'power_state'>), ('timeout', <Parameter at 0x7fb6b43dadb8 'timeout'>)])
>>> reflection.get_signature(power.reboot).parameters
OrderedDict([('task', <Parameter at 0x7fb6b43f3628 'task'>), ('timeout', <Parameter at 0x7fb6b43f3680 'timeout'>)])

IPMIPower.set_power_interface is wrapped by task_manager.require_exclusive_lock[1] and this decorator uses six.wraps[2]. Other interfaces also use task_manager.require_exclusive_lock.

This just works by chance. I agree to fix this issue.

[1] https://github.com/openstack/ironic/blob/master/ironic/drivers/modules/ipmitool.py#L784
[2] https://github.com/openstack/ironic/blob/master/ironic/conductor/task_manager.py#L134

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/540150
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=9e87cebc12102cbb3ae47366836dcd7c3e439828
Submitter: Zuul
Branch: master

commit 9e87cebc12102cbb3ae47366836dcd7c3e439828
Author: Ruby Loo <email address hidden>
Date: Thu Feb 1 16:48:15 2018 -0500

    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

[Ruby Loo (rloo)]

Hironori, I just saw your comment #3. I'm glad you looked into it! Thanks. So that means that the timeout parameter was being used. We've landed the patch already, so I think I'll just update the release note to indicate that there was no bug that needed to be fixed.

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.openstack.org/541961

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/541961
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=b631c0f4eed447a03cb18762595669c1fae687d3
Submitter: Zuul
Branch: master

commit b631c0f4eed447a03cb18762595669c1fae687d3
Author: Ruby Loo <email address hidden>
Date: Wed Feb 7 20:09:17 2018 -0500

    [reno] timeout parameter worked

    We thought that the 'timeout' parameter that was specified for a
    node's power state transition wasn't working for ipmitool, irmc,
    and oneview power interfaces, but it turns out that it was working.
    This updates the release note to reflect that.

    Change-Id: Icc439179a3790f109d42c5014b11defbfd6c2127
    Related-Bug: #1746849

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/ironic 10.1.0 release.