Merge lp:~allenap/maas/anon-power-setting into lp:maas/trunk

[Gavin Panella]

Allows power parameters to be supplied at enlistment.

[Julian Edwards (julian-edwards)]

review: Approve

[MAAS Lander (maas-lander)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/maas-merger-quantal/117/console reported an error when processing this lp:~allenap/maas/anon-power-setting branch.
Not merging it.

[MAAS Lander (maas-lander)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/maas-merger-quantal/119/console reported an error when processing this lp:~allenap/maas/anon-power-setting branch.
Not merging it.

[lp:~allenap/maas/anon-power-setting]

1173. By Gavin Panella on 2012-10-05

Rewrite store_node_power_parameters; it was fairly broken, and was untested.

1174. By Gavin Panella on 2012-10-05

Fix test fallout.

[Raphaël Badin (rvb)]

Looks good, thanks for spotting the problem and fixing this Gavin!

[0]

def store_node_power_parameters(node, request):

I wonder if it would not be more clear to change the signature of this method to store_node_power_parameters(node, power_type, power_parameters) because there is no real reason to pass the whole request around. Maybe store_node_power_parameters(node, data) (when data=request.POST) ?

[1]

+ self.assertEqual(power_type, self.node.power_type)
+ self.assertEqual("", self.node.power_parameters)
+ self.save.assert_called_once_with()

I think it would be cleaner to test the behavior here and:
- not patch the save method
- test that reload_object(node).power_type == the_new_power_type and reload_object(node).power_parameters == ""

Same remark in a few other places.

[2]

+ self.assertRaises(
+ MAASAPIBadRequest, store_node_power_parameters,
+ self.node, self.request)
+ self.save.assert_has_calls([])

Again, instead of 'self.save.assert_has_calls([])', I'd test that the node has not been changed. But maybe it's a matter of taste :).

[3]

+ power_types = map_enum(POWER_TYPE).values()
+ if power_type in power_types:
+ node.power_type = power_type
+ else:
+ raise MAASAPIBadRequest("Bad power_type '%s'" % power_type)

51 + except ValueError:
52 + raise MAASAPIBadRequest("Failed to parse JSON power_parameters")

This should probably be a ValidationError({'power_type': [error_message]}) / ValidationError({'power_parameters': [error_message]})

Rarg, all of this is kinda ugly, that's exactly why we should use forms to take care of validation. Django forms are not perfect, but they are still much better than doing all of this manually. Of course, this can be refined later ( "# Hack in the power parameters here." -> that's really what it says in the tin :)).

[4]

194 + [node] = Node.objects.filter(hostname=hostname)

I'd use "node = Node.objects.get(hostname=hostname)": is (a little bit) more clear.

review: Approve

[Andres Rodriguez (andreserl)]

Wouldn't it be easier leave the metadataserver api method as it is (to not break commissioning), and simply allow the power parameters to be passed on enlistment the same way it is with the rest of the parameters?

[Gavin Panella (allenap)]

On 5 October 2012 17:09, Andres Rodriguez <email address hidden> wrote:
> Wouldn't it be easier leave the metadataserver api method as it is (to not break commissioning), and simply allow the power parameters to be passed on enlistment the same way it is with the rest of the parameters?

That's kind of what store_node_power_parameters() does, but if there's
a better way of doing it I'm interested to know; this is not an area
where I have much expertise.

As to leaving the commissioning thing alone, I have mixed feelings. On
one hand it's broken, by validating against the wrong list of choices,
and on the other I don't want to accidentally break something that's
working.

Are there any tests for the commissioning scripts, the ones that run
on the node? They should prevent us from breaking this contract.

[Andres Rodriguez (andreserl)]

What do you mean by evaluating against the wrong list of choices? It evaluates against the correct power types, which is what we need t evaluate. This has been tested correctly (passing incorrect power_type).

No there are no tests for the commissioning scripts.. not an easy thing to do. IMHO much more complex than adding tests for maas-import-pxe-files.

[Gavin Panella (allenap)]

> What do you mean by evaluating against the wrong list of choices? It evaluates
> against the correct power types, which is what we need t evaluate. This has
> been tested correctly (passing incorrect power_type).

It checks that the given power type, converted to upper-case, is a
member of map_enum(POWER_TYPES). The result of map_enum(POWER_TYPES)
is an attribute-name->enum-value mapping, so it's checking that the
given parameter matches one of the enum's attribute names, i.e. a
Python variable name. It should be checking (without changing case)
against map_enum(POWER_TYPES).values().

> No there are no tests for the commissioning scripts.. not an easy thing to do.
> IMHO much more complex than adding tests for maas-import-pxe-files.

No, granted, an integration test of that would be a beast. We could
add unit tests for individual parts of those scripts though, for
example the part that sends the commissioning result, but I don't
think we'll have time for that before releasing 12.10.

[Julian Edwards (julian-edwards)]

On Friday 05 October 2012 21:50:27 you wrote:
> It checks that the given power type, converted to upper-case, is a
> member of map_enum(POWER_TYPES). The result of map_enum(POWER_TYPES)
> is an attribute-name->enum-value mapping, so it's checking that the
> given parameter matches one of the enum's attribute names, i.e. a
> Python variable name. It should be checking (without changing case)
> against map_enum(POWER_TYPES).values().

What's the practical difference? (Given what map_enum is doing)

> > No there are no tests for the commissioning scripts.. not an easy thing to
> > do. IMHO much more complex than adding tests for maas-import-pxe-files.
> No, granted, an integration test of that would be a beast. We could
> add unit tests for individual parts of those scripts though, for
> example the part that sends the commissioning result, but I don't
> think we'll have time for that before releasing 12.10.

Sadly no, this will go down as tech debt.

[Gavin Panella (allenap)]

On 8 October 2012 00:51, Julian Edwards <...> wrote:
> On Friday 05 October 2012 21:50:27 you wrote:
>> It checks that the given power type, converted to upper-case, is a
>> member of map_enum(POWER_TYPES). The result of map_enum(POWER_TYPES)
>> is an attribute-name->enum-value mapping, so it's checking that the
>> given parameter matches one of the enum's attribute names, i.e. a
>> Python variable name. It should be checking (without changing case)
>> against map_enum(POWER_TYPES).values().
>
> What's the practical difference?  (Given what map_enum is doing)

It's looking in the keys:

  >>> map_enum(POWER_TYPE).keys()
  ['DEFAULT', 'IPMI', 'VIRSH', 'WAKE_ON_LAN']

but it should be looking in the values:

  >>> map_enum(POWER_TYPE).values()
  [u'', u'ipmi', u'virsh', u'ether_wake']

[Julian Edwards (julian-edwards)]

On Monday 08 October 2012 08:52:43 you wrote:
> On 8 October 2012 00:51, Julian Edwards <...> wrote:
> > On Friday 05 October 2012 21:50:27 you wrote:
> >> It checks that the given power type, converted to upper-case, is a
> >> member of map_enum(POWER_TYPES). The result of map_enum(POWER_TYPES)
> >> is an attribute-name->enum-value mapping, so it's checking that the
> >> given parameter matches one of the enum's attribute names, i.e. a
> >> Python variable name. It should be checking (without changing case)
> >> against map_enum(POWER_TYPES).values().
> >
> > What's the practical difference? (Given what map_enum is doing)
>
> It's looking in the keys:
>
> >>> map_enum(POWER_TYPE).keys()
> ['DEFAULT', 'IPMI', 'VIRSH', 'WAKE_ON_LAN']
>
> but it should be looking in the values:
>
> >>> map_enum(POWER_TYPE).values()
> [u'', u'ipmi', u'virsh', u'ether_wake']

Right. As we discussed in the hangout, it's ok to just change this as
fortunately the only thing using it so far is the impi stuff where the key is
the same as the value.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-10-05 08:04:58 +0000
+++ src/maasserver/api.py	2012-10-05 14:13:24 +0000
@@ -75,6 +75,7 @@
     "TagsHandler",
     "pxeconfig",
     "render_api_docs",
+    "store_node_power_parameters",
     ]
 
 from base64 import b64decode
@@ -158,6 +159,7 @@
     compose_preseed_url,
     )
 from maasserver.server_address import get_maas_facing_server_address
+from maasserver.utils import map_enum
 from maasserver.utils.orm import get_one
 from piston.handler import (
     AnonymousBaseHandler,
@@ -167,6 +169,7 @@
 from piston.models import Token
 from piston.resource import Resource
 from piston.utils import rc
+from provisioningserver.enum import POWER_TYPE
 from provisioningserver.kernel_opts import KernelParameters
 
 
@@ -418,6 +421,31 @@
     )
 
 
+def store_node_power_parameters(node, request):
+    """Store power parameters in request.
+
+    The parameters should be JSON, passed with key `power_parameters`.
+    """
+    power_type = request.POST.get("power_type", None)
+    if power_type is None:
+        return
+
+    power_types = map_enum(POWER_TYPE).values()
+    if power_type in power_types:
+        node.power_type = power_type
+    else:
+        raise MAASAPIBadRequest("Bad power_type '%s'" % power_type)
+
+    power_parameters = request.POST.get("power_parameters", None)
+    if power_parameters and not power_parameters.isspace():
+        try:
+            node.power_parameters = json.loads(power_parameters)
+        except ValueError:
+            raise MAASAPIBadRequest("Failed to parse JSON power_parameters")
+
+    node.save()
+
+
 class NodeHandler(OperationsHandler):
     """Manage individual Nodes."""
     create = None  # Disable create.
@@ -586,7 +614,10 @@
     Form = get_node_create_form(request.user)
     form = Form(altered_query_data)
     if form.is_valid():
-        return form.save()
+        node = form.save()
+        # Hack in the power parameters here.
+        store_node_power_parameters(node, request)
+        return node
     else:
         raise ValidationError(form.errors)
 

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-10-05 08:04:58 +0000
+++ src/maasserver/tests/test_api.py	2012-10-05 14:13:24 +0000
@@ -45,6 +45,7 @@
     extract_oauth_key_from_auth_header,
     get_oauth_token,
     get_overrided_query_dict,
+    store_node_power_parameters,
     )
 from maasserver.enum import (
     ARCHITECTURE,
@@ -57,7 +58,10 @@
     NODEGROUP_STATUS,
     NODEGROUPINTERFACE_MANAGEMENT,
     )
-from maasserver.exceptions import Unauthorized
+from maasserver.exceptions import (
+    MAASAPIBadRequest,
+    Unauthorized,
+    )
 from maasserver.fields import mac_error_msg
 from maasserver.forms import DEFAULT_ZONE_NAME
 from maasserver.models import (
@@ -221,6 +225,71 @@
         self.assertEqual([data_value], results.getlist(key))
 
 
+class TestStoreNodeParameters(TestCase):
+    """Tests for `store_node_power_parameters`."""
+
+    def setUp(self):
+        super(TestStoreNodeParameters, self).setUp()
+        self.node = factory.make_node()
+        self.save = self.patch(self.node, "save")
+        self.request = Mock()
+
+    def test_power_type_not_given(self):
+        # When power_type is not specified, nothing happens.
+        self.request.POST = {}
+        store_node_power_parameters(self.node, self.request)
+        self.assertEqual(POWER_TYPE.DEFAULT, self.node.power_type)
+        self.assertEqual("", self.node.power_parameters)
+        self.save.assert_has_calls([])
+
+    def test_power_type_set_but_no_parameters(self):
+        # When power_type is valid, it is set. However, if power_parameters is
+        # not specified, the node's power_parameters is left alone, and the
+        # node is saved.
+        power_type = factory.getRandomChoice(POWER_TYPE_CHOICES)
+        self.request.POST = {"power_type": power_type}
+        store_node_power_parameters(self.node, self.request)
+        self.assertEqual(power_type, self.node.power_type)
+        self.assertEqual("", self.node.power_parameters)
+        self.save.assert_called_once_with()
+
+    def test_power_type_set_with_parameters(self):
+        # When power_type is valid, and power_parameters is valid JSON, both
+        # fields are set on the node, and the node is saved.
+        power_type = factory.getRandomChoice(POWER_TYPE_CHOICES)
+        power_parameters = {"foo": [1, 2, 3]}
+        self.request.POST = {
+            "power_type": power_type,
+            "power_parameters": json.dumps(power_parameters),
+            }
+        store_node_power_parameters(self.node, self.request)
+        self.assertEqual(power_type, self.node.power_type)
+        self.assertEqual(power_parameters, self.node.power_parameters)
+        self.save.assert_called_once_with()
+
+    def test_power_type_set_with_invalid_parameters(self):
+        # When power_type is valid, but power_parameters is invalid JSON, the
+        # node is not saved, and an exception is raised.
+        power_type = factory.getRandomChoice(POWER_TYPE_CHOICES)
+        self.request.POST = {
+            "power_type": power_type,
+            "power_parameters": "Not JSON.",
+            }
+        self.assertRaises(
+            MAASAPIBadRequest, store_node_power_parameters,
+            self.node, self.request)
+        self.save.assert_has_calls([])
+
+    def test_invalid_power_type(self):
+        # When power_type is invalid, the node is not saved, and an exception
+        # is raised.
+        self.request.POST = {"power_type": factory.make_name("bogus")}
+        self.assertRaises(
+            MAASAPIBadRequest, store_node_power_parameters,
+            self.node, self.request)
+        self.save.assert_has_calls([])
+
+
 class MultipleUsersScenarios:
     """A mixin that uses testscenarios to repeat a testcase as different
     users.
@@ -289,6 +358,32 @@
         [diane] = Node.objects.filter(hostname='diane')
         self.assertEqual(architecture, diane.architecture)
 
+    def test_POST_new_creates_node_with_power_parameters(self):
+        # We're setting power parameters so we disable start_commissioning to
+        # prevent anything from attempting to issue power instructions.
+        self.patch(Node, "start_commissioning")
+        hostname = factory.make_name("hostname")
+        architecture = factory.getRandomChoice(ARCHITECTURE_CHOICES)
+        power_type = POWER_TYPE.IPMI
+        power_parameters = {
+            "power_user": factory.make_name("power-user"),
+            "power_pass": factory.make_name("power-pass"),
+            }
+        response = self.client.post(
+            self.get_uri('nodes/'),
+            {
+                'op': 'new',
+                'hostname': hostname,
+                'architecture': architecture,
+                'mac_addresses': factory.getRandomMACAddress(),
+                'power_parameters': json.dumps(power_parameters),
+                'power_type': power_type,
+            })
+        self.assertEqual(httplib.OK, response.status_code)
+        [node] = Node.objects.filter(hostname=hostname)
+        self.assertEqual(power_parameters, node.power_parameters)
+        self.assertEqual(power_type, node.power_type)
+
     def test_POST_new_creates_node_with_arch_only(self):
         architecture = factory.getRandomChoice(
             [choice for choice in ARCHITECTURE_CHOICES
@@ -600,23 +695,25 @@
         nodes_returned = json.loads(response.content)
         self.assertEqual([], nodes_returned)
 
-    def test_POST_simple_user_cannot_set_power_type_and_parameters(self):
+    def test_POST_simple_user_can_set_power_type_and_parameters(self):
         new_power_address = factory.getRandomString()
         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': json.dumps(
+                    {"power_address": new_power_address}),
                 'mac_addresses': ['AA:BB:CC:DD:EE:FF'],
                 })
 
         node = Node.objects.get(
             system_id=json.loads(response.content)['system_id'])
         self.assertEqual(
-                (httplib.OK, '', POWER_TYPE.DEFAULT),
-                (response.status_code, node.power_parameters,
-                    node.power_type))
+            (httplib.OK, {"power_address": new_power_address},
+             POWER_TYPE.WAKE_ON_LAN),
+            (response.status_code, node.power_parameters,
+             node.power_type))
 
     def test_POST_returns_limited_fields(self):
         response = self.client.post(

=== modified file 'src/metadataserver/api.py'
--- src/metadataserver/api.py	2012-10-04 09:53:04 +0000
+++ src/metadataserver/api.py	2012-10-05 14:13:24 +0000
@@ -18,8 +18,6 @@
     'VersionIndexHandler',
     ]
 
-import json
-
 from django.conf import settings
 from django.core.exceptions import PermissionDenied
 from django.http import HttpResponse
@@ -29,6 +27,7 @@
     get_mandatory_param,
     operation,
     OperationsHandler,
+    store_node_power_parameters,
     )
 from maasserver.enum import (
     NODE_STATUS,
@@ -49,7 +48,6 @@
     get_enlist_userdata,
     get_preseed,
     )
-from maasserver.utils import map_enum
 from maasserver.utils.orm import get_one
 from metadataserver.models import (
     NodeCommissionResult,
@@ -57,7 +55,6 @@
     NodeUserData,
     )
 from piston.utils import rc
-from provisioningserver.enum import POWER_TYPE
 
 
 class UnknownMetadataVersion(MAASAPINotFound):
@@ -188,25 +185,6 @@
             contents = raw_content.decode('utf-8')
             NodeCommissionResult.objects.store_data(node, name, contents)
 
-    def _store_power_parameters(self, node, request):
-        """Store power parameters passed back in commissioning result."""
-        type = request.POST.get("power_type", None)
-        if type is None:
-            return
-
-        params = request.POST.get("power_parameters", None)
-
-        type_dict = map_enum(POWER_TYPE)
-        if type.upper() not in type_dict:
-            raise MAASAPIBadRequest("Bad power_type '%s'" % type)
-        node.power_type = type_dict[type.upper()]
-
-        try:
-            node.power_parameters = json.loads(params)
-        except ValueError:
-            raise MAASAPIBadRequest("Failed to parse json power_parameters")
-        node.save()
-
     @operation(idempotent=False)
     def signal(self, request, version=None, mac=None):
         """Signal commissioning status.
@@ -243,7 +221,7 @@
             return rc.ALL_OK
 
         self._store_commissioning_results(node, request)
-        self._store_power_parameters(node, request)
+        store_node_power_parameters(node, request)
 
         target_status = self.signaling_statuses.get(status)
         if target_status in (None, node.status):

=== modified file 'src/metadataserver/tests/test_api.py'
--- src/metadataserver/tests/test_api.py	2012-10-04 11:20:46 +0000
+++ src/metadataserver/tests/test_api.py	2012-10-05 14:13:24 +0000
@@ -524,7 +524,7 @@
             power_user=factory.getRandomString(),
             power_pass=factory.getRandomString())
         response = self.call_signal(
-            client, power_type="IPMI", power_parameters=json.dumps(params))
+            client, power_type="ipmi", power_parameters=json.dumps(params))
         self.assertEqual(httplib.OK, response.status_code, response.content)
         node = reload_object(node)
         self.assertEqual(
@@ -552,7 +552,7 @@
         response = self.call_signal(
             client, power_type="ipmi", power_parameters="badjson")
         self.assertEqual(
-            (httplib.BAD_REQUEST, "Failed to parse json power_parameters"),
+            (httplib.BAD_REQUEST, "Failed to parse JSON power_parameters"),
             (response.status_code, response.content))
 
     def test_api_retrieves_node_metadata_by_mac(self):