launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/maas/pass-power-params into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~julian-edwards/maas/pass-power-params/+merge/107163

Make start_nodes() use Node.power_parameters instead of assuming everything needs a MAC address.

Some small points to note:
 1. Wake on lan is a special case since we store MAC addresses outside of the power parameters.  I've basically made power_parameters take precedence if it's set, so that you can override which MAC gets used.

 2. test_start_nodes_ignores_uneditable_nodes was broken as it also assumed everything used MACs.

 3. test_stop_nodes_ignores_uneditable_nodes was also broken (although not apparent yet, but I fixed it anyway).

 4. I added an empty virsh template so we can use another power type in tests.  It can be properly filled later.
-- 
https://code.launchpad.net/~julian-edwards/maas/pass-power-params/+merge/107163
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/maas/pass-power-params into lp:maas.

=== modified file 'src/maasserver/models/__init__.py'
--- src/maasserver/models/__init__.py	2012-05-24 05:33:41 +0000
+++ src/maasserver/models/__init__.py	2012-05-24 07:15:24 +0000
@@ -352,12 +352,23 @@
         processed_nodes = []
         for node in nodes:
             NodeUserData.objects.set_user_data(node, user_data)
-            # For now, use the first registered MAC address.
-            if node.macaddress_set.exists():
-                mac = node.macaddress_set.all().order_by(
-                    'created')[0].mac_address
-                power_on.delay(node.power_type, mac=mac)
-                processed_nodes.append(node)
+            # Wake on LAN is a special case, deal with it first.
+            if node.power_type == POWER_TYPE.WAKE_ON_LAN:
+                # If power_parameters is set, use it.  Otherwise, use the
+                # first registered MAC address.
+                mac = None
+                if node.power_parameters:
+                    mac = node.power_parameters.get("mac", None)
+                elif node.macaddress_set.exists():
+                    mac = node.macaddress_set.all().order_by(
+                        'created')[0].mac_address
+                if mac is not None and mac != "":
+                    power_on.delay(node.power_type, mac=mac)
+                    processed_nodes.append(node)
+            else:
+                if node.power_parameters:
+                    power_on.delay(node.power_type, **node.power_parameters)
+                    processed_nodes.append(node)
         return processed_nodes
 
 

=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py	2012-05-24 06:00:57 +0000
+++ src/maasserver/tests/test_models.py	2012-05-24 07:15:24 +0000
@@ -663,8 +663,11 @@
         self.assertEqual('stop', power_status[node.system_id])
 
     def test_stop_nodes_ignores_uneditable_nodes(self):
-        nodes = [self.make_node(factory.make_user()) for counter in range(3)]
-        ids = [node.system_id for node in nodes]
+        nodes = [
+            self.make_node_with_mac(
+                factory.make_user(), power_type=POWER_TYPE.WAKE_ON_LAN)
+            for counter in range(3)]
+        ids = [node.system_id for node, mac in nodes]
         stoppable_node = nodes[0]
         self.assertItemsEqual(
             [stoppable_node],
@@ -687,6 +690,63 @@
                 fixture.tasks[0]['kwargs']['mac'],
             ))
 
+    def test_start_nodes_wakeonlan_prefers_power_parameters(self):
+        # If power_parameters is set we should prefer it to sifting
+        # through related MAC addresses.
+        fixture = self.useFixture(CeleryFixture())
+
+        user = factory.make_user()
+        preferred_mac = factory.getRandomMACAddress()
+        node, mac = self.make_node_with_mac(
+            user, power_type=POWER_TYPE.WAKE_ON_LAN,
+            power_parameters=dict(mac=preferred_mac))
+        output = Node.objects.start_nodes([node.system_id], user)
+
+        self.assertItemsEqual([node], output)
+        self.assertEqual(
+            (1, 'provisioningserver.tasks.power_on', preferred_mac),
+            (
+                len(fixture.tasks),
+                fixture.tasks[0]['task'].name,
+                fixture.tasks[0]['kwargs']['mac'],
+            ))
+
+    def test_start_nodes_wakeonlan_ignores_invalid_parameters(self):
+        # If node.power_params is set but doesn't have "mac" in it, then
+        # the node shouldn't be started.
+        fixture = self.useFixture(CeleryFixture())
+        user = factory.make_user()
+        node, mac = self.make_node_with_mac(
+            user, power_type=POWER_TYPE.WAKE_ON_LAN,
+            power_parameters=dict(jarjar="binks"))
+        output = Node.objects.start_nodes([node.system_id], user)
+        self.assertItemsEqual([], output)
+
+    def test_start_nodes_other_power_type(self):
+        # wakeonlan tests, above, are a special case. Test another type
+        # here that exclusively uses power_parameters from the node.
+        fixture = self.useFixture(CeleryFixture())
+        user = factory.make_user()
+        params = dict(
+            driver="qemu",
+            address="localhost",
+            user="nobody")
+        node, mac = self.make_node_with_mac(
+            user, power_type=POWER_TYPE.VIRSH, power_parameters=params)
+        output = Node.objects.start_nodes([node.system_id], user)
+
+        self.assertItemsEqual([node], output)
+        self.assertEqual(
+            (1, 'provisioningserver.tasks.power_on',
+                "qemu", "localhost", "nobody"),
+            (
+                len(fixture.tasks),
+                fixture.tasks[0]['task'].name,
+                fixture.tasks[0]['kwargs']['driver'],
+                fixture.tasks[0]['kwargs']['address'],
+                fixture.tasks[0]['kwargs']['user'],
+            ))
+
     def test_start_nodes_ignores_nodes_without_mac(self):
         user = factory.make_user()
         node = self.make_node(user)
@@ -697,7 +757,8 @@
     def test_start_nodes_ignores_uneditable_nodes(self):
         nodes = [
             self.make_node_with_mac(
-                factory.make_user())[0] for counter in range(3)]
+                factory.make_user(), power_type=POWER_TYPE.WAKE_ON_LAN)[0]
+                for counter in range(3)]
         ids = [node.system_id for node in nodes]
         startable_node = nodes[0]
         self.assertItemsEqual(

=== added file 'src/provisioningserver/power/templates/virsh.template'
--- src/provisioningserver/power/templates/virsh.template	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/power/templates/virsh.template	2012-05-24 07:15:24 +0000
@@ -0,0 +1,1 @@
+# FIXME: Doing nothing yet.