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

[Julian Edwards]

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.

[Raphaël Badin (rvb)]

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

=== modified file 'src/provisioningserver/rpc/power.py'
--- src/provisioningserver/rpc/power.py	2014-09-12 13:19:39 +0000
+++ src/provisioningserver/rpc/power.py	2014-09-18 06:44:43 +0000
@@ -227,6 +227,9 @@
         try:
             power_state = yield deferToThread(
                 perform_power_query, system_id, hostname, power_type, context)
+            if power_state not in ("on", "off"):
+                # This is considered an error.
+                raise PowerActionFail(power_state)
         except PowerActionFail as e:
             # Hold the error so if failure after retries, we can
             # log the reason.

=== modified file 'src/provisioningserver/rpc/tests/test_power.py'
--- src/provisioningserver/rpc/tests/test_power.py	2014-09-12 13:19:39 +0000
+++ src/provisioningserver/rpc/tests/test_power.py	2014-09-18 06:44:43 +0000
@@ -527,6 +527,32 @@
                     system_id, hostname, err_msg))
         )
 
+    def test_get_power_state_marks_node_broken_if_template_returns_crap(self):
+        system_id = factory.make_name('system_id')
+        hostname = factory.make_name('hostname')
+        power_type = random.choice(power.QUERY_POWER_TYPES)
+        template_return_gibberish = factory.make_name('gibberish')
+        context = {
+            factory.make_name('context-key'): factory.make_name('context-val')
+        }
+        self.patch(power, 'pause')
+        power_action, execute = self.patch_power_action(
+            return_value=template_return_gibberish)
+        _, markNodeBroken, io = self.patch_rpc_methods()
+        d = power.get_power_state(
+            system_id, hostname, power_type, context)
+        io.flush()
+        self.assertEqual('error', extract_result(d))
+
+        self.assertThat(
+            markNodeBroken,
+            MockCalledOnceWith(
+                ANY,
+                system_id=system_id,
+                error_description="Node could not be queried %s (%s) %s" % (
+                    system_id, hostname, template_return_gibberish))
+        )
+
     def test_get_power_state_pauses_inbetween_retries(self):
         system_id = factory.make_name('system_id')
         hostname = factory.make_name('hostname')