Merge lp:~julian-edwards/maas/node-power-monitor-crash into lp:~maas-committers/maas/trunk

Proposed by Julian Edwards
Status: Merged
Approved by: Julian Edwards
Approved revision: no longer in the source branch.
Merged at revision: 3026
Proposed branch: lp:~julian-edwards/maas/node-power-monitor-crash
Merge into: lp:~maas-committers/maas/trunk
Diff against target: 50 lines (+29/-0)
2 files modified
src/provisioningserver/rpc/power.py (+3/-0)
src/provisioningserver/rpc/tests/test_power.py (+26/-0)
To merge this branch: bzr merge lp:~julian-edwards/maas/node-power-monitor-crash
Reviewer Review Type Date Requested Status
Raphaël Badin (community) Approve
Review via email: mp+235085@code.launchpad.net

Commit message

Ensure crazy power template results are treated as errors in the power monitoring service. Crazy means not an "on" or a "off". Previously, it would halt the whole scan.

To post a comment you must log in.
Revision history for this message
Raphaël Badin (rvb) wrote :

Looks good but wouldn't the code be better placed directly inside perform_power_query()?

@synchronous
def perform_power_query(system_id, hostname, power_type, context):
    """Issue the given `power_query` command.

    No exception handling is performed here, this allows
    `get_power_state` to perform multiple queries and only
    log the final error.
    """
    action = PowerAction(power_type)
    power_state = action.execute(power_change='query', **context)
    if power_state in ("on", "off"):
        return power_state
    else:
        raise PowerActionFail(power_state)

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/provisioningserver/rpc/power.py'
2--- src/provisioningserver/rpc/power.py 2014-09-12 13:19:39 +0000
3+++ src/provisioningserver/rpc/power.py 2014-09-18 06:44:43 +0000
4@@ -227,6 +227,9 @@
5 try:
6 power_state = yield deferToThread(
7 perform_power_query, system_id, hostname, power_type, context)
8+ if power_state not in ("on", "off"):
9+ # This is considered an error.
10+ raise PowerActionFail(power_state)
11 except PowerActionFail as e:
12 # Hold the error so if failure after retries, we can
13 # log the reason.
14
15=== modified file 'src/provisioningserver/rpc/tests/test_power.py'
16--- src/provisioningserver/rpc/tests/test_power.py 2014-09-12 13:19:39 +0000
17+++ src/provisioningserver/rpc/tests/test_power.py 2014-09-18 06:44:43 +0000
18@@ -527,6 +527,32 @@
19 system_id, hostname, err_msg))
20 )
21
22+ def test_get_power_state_marks_node_broken_if_template_returns_crap(self):
23+ system_id = factory.make_name('system_id')
24+ hostname = factory.make_name('hostname')
25+ power_type = random.choice(power.QUERY_POWER_TYPES)
26+ template_return_gibberish = factory.make_name('gibberish')
27+ context = {
28+ factory.make_name('context-key'): factory.make_name('context-val')
29+ }
30+ self.patch(power, 'pause')
31+ power_action, execute = self.patch_power_action(
32+ return_value=template_return_gibberish)
33+ _, markNodeBroken, io = self.patch_rpc_methods()
34+ d = power.get_power_state(
35+ system_id, hostname, power_type, context)
36+ io.flush()
37+ self.assertEqual('error', extract_result(d))
38+
39+ self.assertThat(
40+ markNodeBroken,
41+ MockCalledOnceWith(
42+ ANY,
43+ system_id=system_id,
44+ error_description="Node could not be queried %s (%s) %s" % (
45+ system_id, hostname, template_return_gibberish))
46+ )
47+
48 def test_get_power_state_pauses_inbetween_retries(self):
49 system_id = factory.make_name('system_id')
50 hostname = factory.make_name('hostname')