launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/power-off into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/power-off/+merge/107716

As discussed with Julian, this adds a Celery task power_on.  Given how little the power methods I saw in Cobbler varied between the on/off actions, I chose to use the same templates for both actions.  It does risk adding a bit of complexity in shell script where it's harder to test, but it avoids the complexity and consistency hazards of having separate sets of templates for the two actions.

I made the wake-on-LAN template a bit easier to read along the way, but also, I made it fail with a (somewhat) helpful error message when you try to use it to power off a node.  We may find other ways to deal with that, or end up punting to humans, but I felt this was better than a silent no-op.

You'll see two very similar tests.  Don't be angry.  I think I can explain.  The PowerAction test is a unit test.  It tests a corner case: you can't use wake-on-LAN to shut down a machine.  The task test is an integration test: it's the only really testable behaviour we have right now for the power_off action, and so that's what I ended up testing.

I have my doubts about what else goes on in that test, actually: where does the magical hard-coded MAC address come from?  Was it chosen to match a hard-coded address somewhere else?  Was it hand-vetted to be safe, so that the tests won't accidentally mess with one specific machine somewhere in the world?  Or is it an arbitrary address that somebody thought was clearer than factory.getRandomMACAddress?  I don't know.  I just copied it in cargo-cult fashion and hoped that whatever reasons there were for this, will apply to my branch as well.  That's what generally happens in cases like this, in my experience.  I may take some time to find out more and fix things up separately if appropriate.

Finally, note that an exception in a “delayed” task in a test gets propagated into the test.  So the way to check for one in a test is self.assertRaises, not a check on the task's success code.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/power-off/+merge/107716
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/power-off into lp:maas.

=== modified file 'src/provisioningserver/power/poweraction.py'
--- src/provisioningserver/power/poweraction.py	2012-05-21 15:51:33 +0000
+++ src/provisioningserver/power/poweraction.py	2012-05-29 06:34:21 +0000
@@ -45,7 +45,7 @@
         basedir = POWER_TEMPLATES_DIR
         self.path = os.path.join(basedir, power_type + ".template")
         if not os.path.exists(self.path):
-            raise UnknownPowerType
+            raise UnknownPowerType(power_type)
 
         self.power_type = power_type
 

=== modified file 'src/provisioningserver/power/templates/ether_wake.template'
--- src/provisioningserver/power/templates/ether_wake.template	2012-05-16 06:53:48 +0000
+++ src/provisioningserver/power/templates/ether_wake.template	2012-05-29 06:34:21 +0000
@@ -1,6 +1,16 @@
-set mac = %(mac)s
-if [ -x /usr/bin/wakeonlan ]; then
-    /usr/bin/wakeonlan $mac
-elif [ -x /usr/sbin/etherwake ]; then
-    /usr/sbin/etherwake $mac
+if [ '%(power_change)s' != 'on' ]
+then
+    echo "There is no way to power down a node through etherwake." >&2
+    exit 1
+elif [ -x /usr/bin/wakeonlan ]
+then
+    /usr/bin/wakeonlan %(mac)s
+elif [ -x /usr/sbin/etherwake ]
+then
+    /usr/sbin/etherwake %(mac)s
+else
+    echo "No wakeonlan or etherwake program found." >&2
+    exit 1
 fi
+
+exit 0

=== modified file 'src/provisioningserver/power/tests/test_poweraction.py'
--- src/provisioningserver/power/tests/test_poweraction.py	2012-05-21 15:51:33 +0000
+++ src/provisioningserver/power/tests/test_poweraction.py	2012-05-29 06:34:21 +0000
@@ -104,3 +104,9 @@
         exception = self.assertRaises(PowerActionFail, self.run_action, path)
         self.assertEqual(
             "ether_wake failed with return code 127", exception.message)
+
+    def test_wake_on_lan_cannot_shut_down_node(self):
+        pa = PowerAction(POWER_TYPE.WAKE_ON_LAN)
+        self.assertRaises(
+            PowerActionFail,
+            pa.execute, power_change='off', mac=factory.getRandomMACAddress())

=== modified file 'src/provisioningserver/tasks.py'
--- src/provisioningserver/tasks.py	2012-05-23 14:43:16 +0000
+++ src/provisioningserver/tasks.py	2012-05-29 06:34:21 +0000
@@ -11,7 +11,8 @@
 
 __metaclass__ = type
 __all__ = [
-    'power_on'
+    'power_off',
+    'power_on',
     ]
 
 
@@ -22,8 +23,17 @@
     )
 
 
-@task
-def power_on(power_type, **kwargs):
+def issue_power_action(power_type, power_change, **kwargs):
+    """Issue a power action to a node.
+
+    :param power_type: The node's power type.  Must have a corresponding
+        power template.
+    :param power_change: The change to request: 'on' or 'off'.
+    :param **kwargs: Keyword arguments are passed on to :class:`PowerAction`.
+    """
+    assert power_change in ('on', 'off'), (
+        "Unknown power change keyword: %s" % power_change)
+    kwargs['power_change'] = power_change
     try:
         pa = PowerAction(power_type)
         pa.execute(**kwargs)
@@ -35,3 +45,15 @@
         raise
 
     # TODO: signal to webapp that it worked.
+
+
+@task
+def power_on(power_type, **kwargs):
+    """Turn a node on."""
+    issue_power_action(power_type, 'on', **kwargs)
+
+
+@task
+def power_off(power_type, **kwargs):
+    """Turn a node off."""
+    issue_power_action(power_type, 'off', **kwargs)

=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py	2012-05-23 14:43:16 +0000
+++ src/provisioningserver/tests/test_tasks.py	2012-05-29 06:34:21 +0000
@@ -16,7 +16,10 @@
 from maastesting.testcase import TestCase
 from provisioningserver.enum import POWER_TYPE
 from provisioningserver.power.poweraction import PowerActionFail
-from provisioningserver.tasks import power_on
+from provisioningserver.tasks import (
+    power_off,
+    power_on,
+    )
 from testresources import FixtureResource
 
 
@@ -37,3 +40,8 @@
         mac = "AA:BB:CC:DD:EE:FF"
         result = power_on.delay(POWER_TYPE.WAKE_ON_LAN, mac=mac)
         self.assertTrue(result.successful())
+
+    def test_ether_wake_does_not_support_power_off(self):
+        mac = "AA:BB:CC:DD:EE:FF"
+        self.assertRaises(
+            PowerActionFail, power_off.delay, POWER_TYPE.WAKE_ON_LAN, mac=mac)