launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/wol-bug-1036005 into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/wol-bug-1036005/+merge/119726

This branch changes the power parameters used with WoL.

The previous situation was really broken: the power parameters for WoL were reduced to only a CharField named 'power_address' and that was not used in the WoL template at all!  The only trick is that, if a node's power parameters are empty, the 'mac' power parameter was dynamically added to the parameters (in node.get_effective_power_parameters).

Since 'power_address' is simply not used at all in the template, I decided to simply remove it.  Of the two tools used by the script (etherwake and wakeonlan), only wakeonlan could use an IP address to send the magic packet to a specific IP address in place of the default [the limited broadcast address (255.255.255.255)]. But since the nodes are supposed to be on the same network as the worker, I'm not sure this is really useful.  Anyway, this isn't used in the script so I choose to remove it for now.

In case one wants to have the WoL command sent to a different MAC Address (given the trick in get_effective_power_parameters, I guess that's why we have the power parameters in the WoL case), I've added a 'mac_address' field to the power_parameter for WoL.  That field forces its content to be a MAC address.
-- 
https://code.launchpad.net/~rvb/maas/wol-bug-1036005/+merge/119726
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/wol-bug-1036005 into lp:maas.

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2012-08-10 14:55:04 +0000
+++ src/maasserver/models/node.py	2012-08-15 14:45:24 +0000
@@ -314,7 +314,7 @@
             power_params = node.get_effective_power_parameters()
             node_power_type = node.get_effective_power_type()
             if node_power_type == POWER_TYPE.WAKE_ON_LAN:
-                mac = power_params.get('mac')
+                mac = power_params.get('mac_address')
                 do_start = (mac != '' and mac is not None)
             else:
                 do_start = True
@@ -587,7 +587,7 @@
         if not self.power_parameters:
             primary_mac = self.get_primary_mac()
             if primary_mac is not None:
-                power_params['mac'] = primary_mac.mac_address
+                power_params['mac_address'] = primary_mac.mac_address
         return power_params
 
     def acquire(self, user, token=None):

=== modified file 'src/maasserver/power_parameters.py'
--- src/maasserver/power_parameters.py	2012-07-16 12:33:43 +0000
+++ src/maasserver/power_parameters.py	2012-08-15 14:45:24 +0000
@@ -32,6 +32,7 @@
 
 from django import forms
 from maasserver.config_forms import DictCharField
+from maasserver.fields import MACAddressFormField
 from provisioningserver.enum import POWER_TYPE
 
 
@@ -42,8 +43,8 @@
         DictCharField(
             [
                 (
-                    'power_address',
-                    forms.CharField(label="Address", required=False)),
+                    'mac_address',
+                    MACAddressFormField(label="MAC Address", required=False)),
             ],
             required=False,
             skip_check=True),

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-08-15 03:20:44 +0000
+++ src/maasserver/tests/test_api.py	2012-08-15 14:45:24 +0000
@@ -53,6 +53,7 @@
     NODE_STATUS_CHOICES_DICT,
     )
 from maasserver.exceptions import Unauthorized
+from maasserver.fields import mac_error_msg
 from maasserver.kernel_opts import (
     compose_enlistment_preseed_url,
     compose_preseed_url,
@@ -593,13 +594,13 @@
     def test_POST_new_sets_power_parameters_field(self):
         # The api allows the setting of a Node's power_parameters field.
         # Create a power_parameter valid for the selected power_type.
-        new_power_address = factory.getRandomString()
+        new_mac_address = factory.getRandomMACAddress()
         response = self.client.post(
             self.get_uri('nodes/'), {
                 'op': 'new',
                 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
                 'power_type': POWER_TYPE.WAKE_ON_LAN,
-                'power_parameters_power_address': new_power_address,
+                'power_parameters_mac_address': new_mac_address,
                 'mac_addresses': ['AA:BB:CC:DD:EE:FF'],
                 })
 
@@ -607,7 +608,7 @@
             system_id=json.loads(response.content)['system_id'])
         self.assertEqual(httplib.OK, response.status_code)
         self.assertEqual(
-            {'power_address': new_power_address},
+            {'mac_address': new_mac_address},
             reload_object(node).power_parameters)
 
     def test_POST_updates_power_parameters_rejects_unknown_param(self):
@@ -1127,16 +1128,35 @@
             owner=self.logged_in_user,
             power_type=POWER_TYPE.WAKE_ON_LAN)
         # Create a power_parameter valid for the selected power_type.
-        new_power_address = factory.getRandomString()
+        new_power_address = factory.getRandomMACAddress()
         response = self.client.put(
             self.get_node_uri(node),
-            {'power_parameters_power_address': new_power_address})
+            {'power_parameters_mac_address': new_power_address})
 
         self.assertEqual(httplib.OK, response.status_code)
         self.assertEqual(
-            {'power_address': new_power_address},
+            {'mac_address': new_power_address},
             reload_object(node).power_parameters)
 
+    def test_PUT_updates_power_parameters_accepts_only_mac_for_wol(self):
+        self.become_admin()
+        node = factory.make_node(
+            owner=self.logged_in_user,
+            power_type=POWER_TYPE.WAKE_ON_LAN)
+        # Create an invalid power_parameter for WoL (not a valid
+        # MAC address).
+        new_power_address = factory.getRandomString()
+        response = self.client.put(
+            self.get_node_uri(node),
+            {'power_parameters_mac_address': new_power_address})
+
+        self.assertEqual(
+            (
+                httplib.BAD_REQUEST,
+                {'power_parameters': ["MAC Address: %s" % mac_error_msg]},
+            ),
+            (response.status_code, json.loads(response.content)))
+
     def test_PUT_updates_power_parameters_rejects_unknown_param(self):
         self.become_admin()
         power_parameters = factory.getRandomString()
@@ -1250,11 +1270,11 @@
             power_parameters=factory.getRandomString())
         response = self.client.put(
             self.get_node_uri(node),
-            {'power_parameters_power_address': ''})
+            {'power_parameters_mac_address': ''})
 
         self.assertEqual(httplib.OK, response.status_code)
         self.assertEqual(
-            {'power_address': ''},
+            {'mac_address': ''},
             reload_object(node).power_parameters)
 
     def test_DELETE_deletes_node(self):

=== modified file 'src/maasserver/tests/test_node.py'
--- src/maasserver/tests/test_node.py	2012-08-10 14:55:04 +0000
+++ src/maasserver/tests/test_node.py	2012-08-15 14:45:24 +0000
@@ -210,7 +210,8 @@
         node = factory.make_node()
         mac = factory.getRandomMACAddress()
         node.add_mac_address(mac)
-        self.assertEqual(mac, node.get_effective_power_parameters()['mac'])
+        self.assertEqual(
+            mac, node.get_effective_power_parameters()['mac_address'])
 
     def test_get_effective_power_parameters_adds_no_mac_if_params_set(self):
         node = factory.make_node(power_parameters={'foo': 'bar'})
@@ -671,7 +672,7 @@
             (
                 len(self.celery.tasks),
                 self.celery.tasks[0]['task'].name,
-                self.celery.tasks[0]['kwargs']['mac'],
+                self.celery.tasks[0]['kwargs']['mac_address'],
             ))
 
     def test_start_nodes_sets_commissioning_profile(self):
@@ -721,7 +722,7 @@
         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))
+            power_parameters=dict(mac_address=preferred_mac))
         output = Node.objects.start_nodes([node.system_id], user)
 
         self.assertItemsEqual([node], output)
@@ -730,12 +731,12 @@
             (
                 len(self.celery.tasks),
                 self.celery.tasks[0]['task'].name,
-                self.celery.tasks[0]['kwargs']['mac'],
+                self.celery.tasks[0]['kwargs']['mac_address'],
             ))
 
     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.
+        # If node.power_params is set but doesn't have "mac_address" in it,
+        # then the node shouldn't be started.
         user = factory.make_user()
         node, mac = self.make_node_with_mac(
             user, power_type=POWER_TYPE.WAKE_ON_LAN,
@@ -744,11 +745,11 @@
         self.assertItemsEqual([], output)
         self.assertEqual([], self.celery.tasks)
 
-    def test_start_nodes_wakeonlan_ignores_empty_mac_parameter(self):
+    def test_start_nodes_wakeonlan_ignores_empty_mac_address_parameter(self):
         user = factory.make_user()
         node, mac = self.make_node_with_mac(
             user, power_type=POWER_TYPE.WAKE_ON_LAN,
-            power_parameters=dict(mac=""))
+            power_parameters=dict(mac_address=""))
         output = Node.objects.start_nodes([node.system_id], user)
         self.assertItemsEqual([], output)
         self.assertEqual([], self.celery.tasks)

=== modified file 'src/provisioningserver/power/templates/ether_wake.template'
--- src/provisioningserver/power/templates/ether_wake.template	2012-06-08 16:05:50 +0000
+++ src/provisioningserver/power/templates/ether_wake.template	2012-08-15 14:45:24 +0000
@@ -3,7 +3,7 @@
 # Control node power through WOL, via `wakeonlan` or `etherwake`.
 #
 
-mac={{mac}}
+mac_address={{mac_address}}
 power_change={{power_change}}
 
 if [ "${power_change}" != 'on' ]
@@ -12,10 +12,10 @@
     exit 1
 elif [ -x /usr/bin/wakeonlan ]
 then
-    /usr/bin/wakeonlan $mac
+    /usr/bin/wakeonlan $mac_address
 elif [ -x /usr/sbin/etherwake ]
 then
-    /usr/sbin/etherwake $mac
+    /usr/sbin/etherwake $mac_address
 else
     echo "No wakeonlan or etherwake program found." >&2
 fi

=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py	2012-08-13 05:41:02 +0000
+++ src/provisioningserver/tests/test_tasks.py	2012-08-15 14:45:24 +0000
@@ -145,7 +145,8 @@
             PowerActionFail, power_on.delay, POWER_TYPE.WAKE_ON_LAN)
 
     def test_ether_wake_power_on(self):
-        result = power_on.delay(POWER_TYPE.WAKE_ON_LAN, mac=arbitrary_mac)
+        result = power_on.delay(
+            POWER_TYPE.WAKE_ON_LAN, mac_address=arbitrary_mac)
         self.assertTrue(result.successful())
 
     def test_ether_wake_does_not_support_power_off(self):