Merge lp:~ltrager/maas/lp1593991 into lp:~maas-committers/maas/trunk

[Lee Trager]

Validate power parameters in the node form.

Previously power parameters were hacked on after form validation and the node object was saved. This was done wherever a node form was used(API, websockets, RPC) Most likely this was done because the form was not picking up the power parameters. The form wasn't picking up power paramters because of three issues, we filter all fields before going into the form and were not accepting power parameters, django requires a prefix when dealing with subfields, and we weren't properly setting the initial fields for subitems.

The power parameters are now validated based on the fields given by each power driver. This not only solves lp:1593991 by ensuring a power_parameter is set but also gives more specific validation errors. For exammple, if a field is missing or if a given field is invalid for the power type.

I noticed that the power drivers were not setting any field to be required. I set the power address for each power type as required and kept all other fields as is. Please let me know if other fields should be listed as required.

[Blake Rouse (blake-rouse)]

Much cleaner thanks for taking this on. I do feel like Django forms just get dirty as so many different conditions can occur. Just on comment that should be addresses before landing, but not going to block you.

review: Approve

=== modified file 'src/maasserver/api/machines.py'
--- src/maasserver/api/machines.py	2016-10-12 02:26:47 +0000
+++ src/maasserver/api/machines.py	2016-10-20 20:55:55 +0000
@@ -34,7 +34,6 @@
     OwnerDataMixin,
     PowerMixin,
     PowersMixin,
-    store_node_power_parameters,
 )
 from maasserver.api.support import (
     admin_method,
@@ -796,8 +795,6 @@
     form = Form(data=altered_query_data, request=request)
     if form.is_valid():
         machine = form.save()
-        # Hack in the power parameters here.
-        store_node_power_parameters(machine, request)
         maaslog.info("%s: Enlisted new machine", machine.hostname)
         return machine
     else:

=== modified file 'src/maasserver/api/tests/test_enlistment.py'
--- src/maasserver/api/tests/test_enlistment.py	2016-09-22 02:53:33 +0000
+++ src/maasserver/api/tests/test_enlistment.py	2016-10-20 20:55:55 +0000
@@ -87,6 +87,7 @@
         architecture = make_usable_architecture(self)
         power_type = 'ipmi'
         power_parameters = {
+            "power_address": factory.make_ip_address(),
             "power_user": factory.make_name("power-user"),
             "power_pass": factory.make_name("power-pass"),
             }
@@ -95,14 +96,16 @@
             {
                 'hostname': hostname,
                 'architecture': architecture,
-                'power_type': 'manual',
+                'power_type': power_type,
                 'mac_addresses': factory.make_mac_address(),
                 'power_parameters': json.dumps(power_parameters),
-                'power_type': power_type,
             })
+        # Add the default values.
+        power_parameters['power_driver'] = 'LAN_2_0'
+        power_parameters['mac_address'] = ''
         self.assertEqual(http.client.OK, response.status_code)
         [machine] = Machine.objects.filter(hostname=hostname)
-        self.assertEqual(power_parameters, machine.power_parameters)
+        self.assertItemsEqual(power_parameters, machine.power_parameters)
         self.assertEqual(power_type, machine.power_type)
 
     def test_POST_create_creates_machine_with_arch_only(self):
@@ -454,23 +457,29 @@
         self.assertEqual([], machines_returned)
 
     def test_POST_simple_user_can_set_power_type_and_parameters(self):
-        new_power_address = factory.make_string()
+        new_power_address = factory.make_url()
+        new_power_id = factory.make_name('power_id')
         response = self.client.post(
             reverse('machines_handler'), {
                 'architecture': make_usable_architecture(self),
-                'power_type': 'manual',
+                'power_type': 'virsh',
                 'power_parameters': json.dumps(
-                    {"power_address": new_power_address}),
+                    {
+                        "power_address": new_power_address,
+                        "power_id": new_power_id,
+                    }),
                 'mac_addresses': ['AA:BB:CC:DD:EE:FF'],
                 })
-
         machine = Machine.objects.get(
             system_id=json_load_bytes(response.content)['system_id'])
-        self.assertEqual(
-            (http.client.OK, {"power_address": new_power_address},
-             'manual'),
-            (response.status_code, machine.power_parameters,
-             machine.power_type))
+        self.assertEqual(http.client.OK, response.status_code)
+        self.assertEqual('virsh', machine.power_type)
+        self.assertItemsEqual(
+            {
+                'power_pass': '',
+                'power_id': new_power_id,
+                'power_address': new_power_address,
+            }, machine.power_parameters)
 
     def test_POST_returns_limited_fields(self):
         response = self.client.post(

=== modified file 'src/maasserver/api/tests/test_machine.py'
--- src/maasserver/api/tests/test_machine.py	2016-10-18 08:00:37 +0000
+++ src/maasserver/api/tests/test_machine.py	2016-10-20 20:55:55 +0000
@@ -850,7 +850,7 @@
         # The api allows the updating of a Machine.
         machine = factory.make_Node(
             hostname='diane', owner=self.user,
-            architecture=make_usable_architecture(self))
+            architecture=make_usable_architecture(self), power_type='manual')
         response = self.client.put(
             self.get_machine_uri(machine), {'hostname': 'francis'})
         parsed_result = json_load_bytes(response.content)
@@ -867,7 +867,8 @@
         hostname = factory.make_name('hostname')
         arch = make_usable_architecture(self)
         machine = factory.make_Node(
-            hostname=hostname, owner=self.user, architecture=arch)
+            hostname=hostname, owner=self.user, architecture=arch,
+            power_type='manual')
         response = self.client.put(
             self.get_machine_uri(machine),
             {'architecture': arch})
@@ -890,7 +891,8 @@
         self.become_admin()
         machine = factory.make_Node(
             owner=self.user,
-            architecture=make_usable_architecture(self))
+            architecture=make_usable_architecture(self),
+            power_type='manual')
         field = factory.make_string()
         response = self.client.put(
             self.get_machine_uri(machine),
@@ -908,7 +910,11 @@
             power_type=original_power_type,
             architecture=make_usable_architecture(self))
         response = self.client.put(
-            self.get_machine_uri(machine), {'power_type': new_power_type})
+            self.get_machine_uri(machine),
+            {
+                'power_type': new_power_type,
+                'power_parameters_skip_check': 'true',
+            })
 
         self.assertEqual(http.client.OK, response.status_code)
         self.assertEqual(
@@ -932,7 +938,8 @@
         # provides the URI for this Machine.
         machine = factory.make_Node(
             hostname='diane', owner=self.user,
-            architecture=make_usable_architecture(self))
+            architecture=make_usable_architecture(self),
+            power_type='manual')
         response = self.client.put(
             self.get_machine_uri(machine), {'hostname': 'francis'})
         parsed_result = json_load_bytes(response.content)
@@ -948,7 +955,8 @@
         self.become_admin()
         machine = factory.make_Node(
             hostname='diane', owner=self.user,
-            architecture=make_usable_architecture(self))
+            architecture=make_usable_architecture(self),
+            power_type='manual')
         response = self.client.put(
             self.get_machine_uri(machine), {'hostname': '.'})
         parsed_result = json_load_bytes(response.content)
@@ -1001,8 +1009,8 @@
         self.become_admin()
         machine = factory.make_Node(
             owner=self.user,
-            power_type=factory.pick_power_type(),
-            architecture=make_usable_architecture(self))
+            architecture=make_usable_architecture(self),
+            power_type='manual')
         response = self.client.put(
             self.get_machine_uri(machine),
             {'cpu_count': 1, 'memory': 1024})
@@ -1109,7 +1117,8 @@
         self.become_admin()
         machine = factory.make_Node(
             owner=self.user,
-            architecture=make_usable_architecture(self))
+            architecture=make_usable_architecture(self),
+            power_parameters={})
         new_param = factory.make_string()
         new_value = factory.make_string()
         response = self.client.put(
@@ -1125,7 +1134,11 @@
 
     def test_PUT_updates_power_parameters_empty_string(self):
         self.become_admin()
-        power_parameters = {factory.make_string(): factory.make_string()}
+        power_parameters = {
+            'power_address': factory.make_url(),
+            'power_id': factory.make_name('power_id'),
+            'power_pass': factory.make_name('power_pass'),
+        }
         machine = factory.make_Node(
             owner=self.user,
             power_type='virsh',
@@ -1133,22 +1146,17 @@
             architecture=make_usable_architecture(self))
         response = self.client.put(
             self.get_machine_uri(machine),
-            {'power_parameters_power_id': ''})
+            {'power_parameters_power_pass': ''})
 
         self.assertEqual(http.client.OK, response.status_code)
         self.assertEqual(
-            {
-                'power_id': '',
-                'power_pass': '',
-                'power_address': '',
-            },
-            reload_object(machine).power_parameters)
+            '', reload_object(machine).power_parameters['power_pass'])
 
     def test_PUT_sets_zone(self):
         self.become_admin()
         new_zone = factory.make_Zone()
         machine = factory.make_Node(
-            architecture=make_usable_architecture(self))
+            architecture=make_usable_architecture(self), power_type='manual')
 
         response = self.client.put(
             self.get_machine_uri(machine), {'zone': new_zone.name})
@@ -1161,7 +1169,7 @@
         self.become_admin()
         new_name = factory.make_name()
         machine = factory.make_Node(
-            architecture=make_usable_architecture(self))
+            architecture=make_usable_architecture(self), power_type='manual')
         old_zone = machine.zone
 
         response = self.client.put(
@@ -1190,7 +1198,8 @@
         self.become_admin()
         zone = factory.make_Zone()
         machine = factory.make_Node(
-            zone=zone, architecture=make_usable_architecture(self))
+            zone=zone, architecture=make_usable_architecture(self),
+            power_type='manual')
 
         response = self.client.put(self.get_machine_uri(machine), {})
 
@@ -1226,7 +1235,8 @@
         self.become_admin()
         machine = factory.make_Node(
             owner=self.user,
-            architecture=make_usable_architecture(self))
+            architecture=make_usable_architecture(self),
+            power_type='manual')
         response = self.client.put(
             reverse('machine_handler', args=[machine.system_id]),
             {'swap_size': 5 * 1000 ** 3})  # Making sure we overflow 32 bits
@@ -1239,7 +1249,8 @@
         self.become_admin()
         machine = factory.make_Node(
             owner=self.user,
-            architecture=make_usable_architecture(self))
+            architecture=make_usable_architecture(self),
+            power_type='manual')
 
         response = self.client.put(
             reverse('machine_handler', args=[machine.system_id]),

=== modified file 'src/maasserver/api/tests/test_node.py'
--- src/maasserver/api/tests/test_node.py	2016-08-31 13:52:59 +0000
+++ src/maasserver/api/tests/test_node.py	2016-10-20 20:55:55 +0000
@@ -303,7 +303,7 @@
         self.assertEqual(
             http.client.OK, response.status_code, response.content)
         parsed_params = json_load_bytes(response.content)
-        self.assertEqual({}, parsed_params)
+        self.assertEqual(node.power_parameters, parsed_params)
 
     def test_power_parameters_requires_admin(self):
         node = factory.make_Node()

=== modified file 'src/maasserver/api/tests/test_rackcontroller.py'
--- src/maasserver/api/tests/test_rackcontroller.py	2016-06-08 20:20:40 +0000
+++ src/maasserver/api/tests/test_rackcontroller.py	2016-10-20 20:55:55 +0000
@@ -36,7 +36,8 @@
 
     def test_PUT_updates_rack_controller(self):
         self.become_admin()
-        rack = factory.make_RackController(owner=self.user)
+        rack = factory.make_RackController(
+            owner=self.user, power_type='manual')
         zone = factory.make_zone()
         response = self.client.put(
             self.get_rack_uri(rack), {'zone': zone.name})

=== modified file 'src/maasserver/api/tests/test_utils.py'
--- src/maasserver/api/tests/test_utils.py	2016-05-24 13:51:36 +0000
+++ src/maasserver/api/tests/test_utils.py	2016-10-20 20:55:55 +0000
@@ -7,6 +7,7 @@
 
 from collections import namedtuple
 
+from django.forms import CharField
 from django.http import QueryDict
 from maasserver.api.utils import (
     extract_bool,
@@ -15,6 +16,7 @@
     get_oauth_token,
     get_overridden_query_dict,
 )
+from maasserver.config_forms import DictCharField
 from maasserver.exceptions import Unauthorized
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
@@ -94,6 +96,24 @@
         results = get_overridden_query_dict(defaults, data, [key1])
         self.assertEqual([data_value2], results.getlist(key2))
 
+    def test_expands_dict_fields(self):
+        field_name = factory.make_name('field_name')
+        sub_fields = {
+            factory.make_name('sub_field'): CharField() for _ in range(3)
+        }
+        fields = {
+            field_name: DictCharField(sub_fields)
+        }
+        defaults = {
+            "%s_%s" % (field_name, field): factory.make_name('subfield')
+            for field in sub_fields.keys()
+        }
+        data = {field_name: DictCharField(fields)}
+        results = get_overridden_query_dict(defaults, data, fields)
+        expected = {key: [value] for key, value in defaults.items()}
+        expected.update(fields)
+        self.assertItemsEqual(expected, results)
+
 
 def make_fake_request(auth_header):
     """Create a very simple fake request, with just an auth header."""

=== modified file 'src/maasserver/api/utils.py'
--- src/maasserver/api/utils.py	2016-09-21 23:23:54 +0000
+++ src/maasserver/api/utils.py	2016-10-20 20:55:55 +0000
@@ -16,6 +16,7 @@
 
 from django.http import QueryDict
 from formencode.validators import Invalid
+from maasserver.config_forms import DictCharField
 from maasserver.exceptions import (
     MAASAPIValidationError,
     Unauthorized,
@@ -158,11 +159,23 @@
     """
     # Create a writable query dict.
     new_data = QueryDict('').copy()
+    # If the fields are a dict of django Fields see if one is a DictCharField.
+    # DictCharField must have their values prefixed with the DictField name in
+    # the returned data or defaults don't get carried.
+    if isinstance(fields, dict):
+        acceptable_fields = []
+        for field_name, field in fields.items():
+            acceptable_fields.append(field_name)
+            if isinstance(field, DictCharField):
+                for sub_field in field.names:
+                    acceptable_fields.append("%s_%s" % (field_name, sub_field))
+    else:
+        acceptable_fields = fields
     # Missing fields will be taken from the node's current values.  This
     # is to circumvent Django's ModelForm (form created from a model)
     # default behaviour that requires all the fields to be defined.
     for k, v in defaults.items():
-        if k in fields:
+        if k in acceptable_fields:
             new_data.setlist(k, listify(v))
     # We can't use update here because data is a QueryDict and 'update'
     # does not replaces the old values with the new as one would expect.

=== modified file 'src/maasserver/clusterrpc/power_parameters.py'
--- src/maasserver/clusterrpc/power_parameters.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/clusterrpc/power_parameters.py	2016-10-20 20:55:55 +0000
@@ -65,12 +65,16 @@
             json_field['name'], json_field['choices'])
         extra_parameters = {
             'choices': json_field['choices'],
-            'initial': json_field['default'],
             'error_messages': {
                 'invalid_choice': invalid_choice_message},
             }
     else:
         extra_parameters = {}
+
+    default = json_field.get('default')
+    if default is not None:
+        extra_parameters['initial'] = default
+
     form_field = field_class(
         label=json_field['label'], required=json_field['required'],
         **extra_parameters)
@@ -111,13 +115,20 @@
          'missing_packages': missing_packages})
 
 
-def get_power_type_parameters_from_json(json_power_type_parameters):
+def get_power_type_parameters_from_json(
+        json_power_type_parameters, initial_power_params=None,
+        skip_check=False):
     """Return power type parameters.
 
     :param json_power_type_parameters: Power type parameters expressed
         as a JSON string or as set of JSONSchema-verifiable objects.
         Will be validated using jsonschema.validate().
     :type json_power_type_parameters: JSON string or iterable.
+    :param initial_power_params: Power paramaters that were already set, any
+        field which matches will have its initial value set.
+    :type initial_power_params: dict
+    :param skip_check: Whether the field should be checked or not.
+    :type skip_check: bool
     :return: A dict of power parameters for all power types, indexed by
         power type name.
     """
@@ -127,19 +138,27 @@
         '': DictCharField(
             [], required=False, skip_check=True),
     }
+    if initial_power_params is None:
+        initial_power_params = []
     for power_type in json_power_type_parameters:
         fields = []
+        has_required_field = False
         for json_field in power_type['fields']:
+            field_name = json_field['name']
+            if field_name in initial_power_params:
+                json_field['default'] = initial_power_params[field_name]
+            has_required_field = has_required_field or json_field['required']
             fields.append((
                 json_field['name'], make_form_field(json_field)))
-        params = DictCharField(fields, required=False, skip_check=True)
+        params = DictCharField(
+            fields, required=has_required_field, skip_check=skip_check)
         power_parameters[power_type['name']] = params
     return power_parameters
 
 
-def get_power_type_parameters():
+def get_power_type_parameters(initial_power_params=None, skip_check=False):
     return get_power_type_parameters_from_json(
-        get_all_power_types_from_clusters())
+        get_all_power_types_from_clusters(), initial_power_params, skip_check)
 
 
 def get_power_type_choices():

=== modified file 'src/maasserver/clusterrpc/tests/test_power_parameters.py'
--- src/maasserver/clusterrpc/tests/test_power_parameters.py	2016-05-12 19:07:37 +0000
+++ src/maasserver/clusterrpc/tests/test_power_parameters.py	2016-10-20 20:55:55 +0000
@@ -20,6 +20,7 @@
 )
 from maasserver.config_forms import DictCharField
 from maasserver.fields import MACAddressFormField
+from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
 from maasserver.utils.forms import compose_invalid_choice_text
 from maastesting.matchers import MockCalledOnceWith
@@ -70,6 +71,42 @@
         for name, field in power_type_parameters.items():
             self.assertIsInstance(field, DictCharField)
 
+    def test__overrides_defaults(self):
+        name = factory.make_name('name')
+        field_name = factory.make_name('field_name')
+        new_default = factory.make_name('new default')
+        json_parameters = [{
+            'name': name,
+            'description': factory.make_name('description'),
+            'fields': [{
+                'name': field_name,
+                'label': factory.make_name('field label'),
+                'field_type': factory.make_name('field type'),
+                'default': factory.make_name('field default'),
+                'required': False,
+            }],
+        }]
+        power_type_parameters = get_power_type_parameters_from_json(
+            json_parameters, {field_name: new_default})
+        self.assertEqual(
+            new_default, power_type_parameters[name].fields[0].initial)
+
+    def test__manual_does_not_require_power_params(self):
+        json_parameters = [{
+            'name': 'manual',
+            'description': factory.make_name('description'),
+            'fields': [{
+                'name': factory.make_name('field name'),
+                'label': factory.make_name('field label'),
+                'field_type': factory.make_name('field type'),
+                'default': factory.make_name('field default'),
+                'required': False,
+            }],
+        }]
+        power_type_parameters = get_power_type_parameters_from_json(
+            json_parameters)
+        self.assertFalse(power_type_parameters['manual'].required)
+
 
 class TestMakeFormField(MAASServerTestCase):
     """Test that make_form_field() converts JSON fields to Django."""
@@ -137,6 +174,17 @@
             (json_field['label'], json_field['required']),
             (django_field.label, django_field.required))
 
+    def test__sets_initial_to_default(self):
+        json_field = {
+            'name': 'some_field',
+            'label': 'Some Field',
+            'field_type': 'string',
+            'required': False,
+            'default': 'some default',
+        }
+        django_field = make_form_field(json_field)
+        self.assertEquals(json_field['default'], django_field.initial)
+
 
 class TestMakeJSONField(MAASServerTestCase):
     """Test that make_json_field() creates JSON-verifiable fields."""

=== modified file 'src/maasserver/config_forms.py'
--- src/maasserver/config_forms.py	2015-12-01 18:12:59 +0000
+++ src/maasserver/config_forms.py	2016-10-20 20:55:55 +0000
@@ -186,6 +186,9 @@
                 field_value = value[index]
             except IndexError:
                 field_value = None
+            # Set the field_value to the default value if not set.
+            if field_value is None and field.initial not in (None, ''):
+                field_value = field.initial
             # Check the field's 'required' field instead of the global
             # 'required' field to allow subfields to be required or not.
             if field.required and field_value in validators.EMPTY_VALUES:

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2016-10-17 20:09:56 +0000
+++ src/maasserver/forms.py	2016-10-20 20:55:55 +0000
@@ -36,6 +36,7 @@
 
 from collections import Counter
 from functools import partial
+import json
 import pipes
 import re
 
@@ -241,6 +242,27 @@
         return power_type
 
     @staticmethod
+    def _get_power_parameters(form, data, machine):
+        if data is None:
+            data = {}
+
+        power_parameters = data.get(
+            'power_parameters', form.initial.get('power_parameters', {}))
+
+        if isinstance(power_parameters, str):
+            try:
+                power_parameters = json.loads(power_parameters)
+            except json.JSONDecodeError:
+                raise ValidationError("Failed to parse JSON power_parameters")
+
+        # Integrate the machines existing power_parameters if unset by form.
+        if machine:
+            for key, value in machine.power_parameters.items():
+                if power_parameters.get(key) is None:
+                    power_parameters[key] = value
+        return power_parameters
+
+    @staticmethod
     def set_up_power_type(form, data, machine=None):
         """Set up the 'power_type' and 'power_parameters' fields.
 
@@ -251,10 +273,18 @@
         choices = [BLANK_CHOICE] + get_power_type_choices()
         form.fields['power_type'] = forms.ChoiceField(
             required=False, choices=choices, initial=power_type)
-        form.fields['power_parameters'] = get_power_type_parameters()[
-            power_type]
-        if form.instance is not None and form.instance.power_type != '':
-            form.initial['power_type'] = form.instance.power_type
+        power_parameters = WithPowerMixin._get_power_parameters(
+            form, data, machine)
+        skip_check = (
+            form.data.get('power_parameters_%s' % SKIP_CHECK_NAME) == 'true')
+        form.fields['power_parameters'] = get_power_type_parameters(
+            power_parameters, skip_check=skip_check)[power_type]
+        if form.instance is not None:
+            if form.instance.power_type != '':
+                form.initial['power_type'] = form.instance.power_type
+            if form.instance.power_parameters != '':
+                for key, value in power_parameters.items():
+                    form.initial['power_parameters_%s' % key] = value
 
     @staticmethod
     def check_power_type(form, cleaned_data):
@@ -277,8 +307,9 @@
 
             # If power_type is not set and power_parameters_skip_check is not
             # on, reset power_parameters (set it to the empty string).
-            if cleaned_data.get('power_type', '') == '':
-                cleaned_data['power_parameters'] = ''
+            power_type = cleaned_data.get('power_type', '')
+            if power_type == '':
+                cleaned_data['power_parameters'] = {}
         return cleaned_data
 
     @staticmethod

=== modified file 'src/maasserver/rpc/nodes.py'
--- src/maasserver/rpc/nodes.py	2016-10-03 20:36:58 +0000
+++ src/maasserver/rpc/nodes.py	2016-10-20 20:55:55 +0000
@@ -225,10 +225,6 @@
     form = AdminMachineWithMACAddressesForm(data_query_dict)
     if form.is_valid():
         node = form.save()
-        # We have to explicitly save the power parameters; the form
-        # won't do it for us.
-        node.power_parameters = json.loads(power_parameters)
-        node.save()
         return node
     else:
         raise ValidationError(form.errors)

=== modified file 'src/maasserver/rpc/tests/test_nodes.py'
--- src/maasserver/rpc/tests/test_nodes.py	2016-10-03 20:36:58 +0000
+++ src/maasserver/rpc/tests/test_nodes.py	2016-10-20 20:55:55 +0000
@@ -7,7 +7,6 @@
 
 from datetime import timedelta
 import json
-from json import dumps
 from operator import attrgetter
 import random
 from random import randint
@@ -88,17 +87,13 @@
         mac_addresses = [
             factory.make_mac_address() for _ in range(3)]
         architecture = make_usable_architecture(self)
-        power_type = random.choice(self.power_types)['name']
-        power_parameters = dumps({})
 
-        node = create_node(
-            architecture, power_type, power_parameters,
-            mac_addresses)
+        node = create_node(architecture, 'manual', {}, mac_addresses)
 
         self.assertEqual(
             (
                 architecture,
-                power_type,
+                'manual',
                 {},
             ),
             (
@@ -122,17 +117,14 @@
             factory.make_mac_address() for _ in range(3)]
         architecture = make_usable_architecture(self)
         hostname = factory.make_hostname()
-        power_type = random.choice(self.power_types)['name']
-        power_parameters = dumps({})
 
         node = create_node(
-            architecture, power_type, power_parameters,
-            mac_addresses, hostname=hostname)
+            architecture, 'manual', {}, mac_addresses, hostname=hostname)
 
         self.assertEqual(
             (
                 architecture,
-                power_type,
+                'manual',
                 {},
                 hostname
             ),
@@ -158,7 +150,7 @@
             "Microsoft Windows",
             ])
         power_type = random.choice(self.power_types)['name']
-        power_parameters = dumps({})
+        power_parameters = {}
 
         with ExpectedException(ValidationError):
             create_node(
@@ -173,17 +165,15 @@
         architecture = make_usable_architecture(self)
         hostname = factory.make_hostname()
         domain = factory.make_Domain()
-        power_type = random.choice(self.power_types)['name']
-        power_parameters = dumps({})
 
         node = create_node(
-            architecture, power_type, power_parameters,
-            mac_addresses, domain=domain.name, hostname=hostname)
+            architecture, 'manual', {}, mac_addresses, domain=domain.name,
+            hostname=hostname)
 
         self.assertEqual(
             (
                 architecture,
-                power_type,
+                'manual',
                 {},
                 domain.id,
                 hostname,
@@ -207,7 +197,7 @@
             factory.make_mac_address() for _ in range(3)]
         architecture = make_usable_architecture(self)
         power_type = random.choice(self.power_types)['name']
-        power_parameters = dumps({})
+        power_parameters = {}
 
         with ExpectedException(ValidationError):
             create_node(
@@ -220,7 +210,7 @@
         self.assertRaises(
             ValidationError, create_node,
             architecture="spam/eggs", power_type="scrambled",
-            power_parameters=dumps({}),
+            power_parameters={},
             mac_addresses=[factory.make_mac_address()])
 
     def test__raises_error_if_node_already_exists(self):
@@ -229,8 +219,8 @@
         mac_addresses = [
             factory.make_mac_address() for _ in range(3)]
         architecture = make_usable_architecture(self)
-        power_type = random.choice(self.power_types)['name']
-        power_parameters = dumps({})
+        power_type = 'manual'
+        power_parameters = {}
 
         create_node(
             architecture, power_type, power_parameters,
@@ -245,20 +235,20 @@
         mac_addresses = [
             factory.make_mac_address() for _ in range(3)]
         architecture = make_usable_architecture(self)
-        power_type = random.choice(self.power_types)['name']
         power_parameters = {
-            factory.make_name('key'): factory.make_name('value')
-            for _ in range(3)
+            'power_address': factory.make_url(),
+            'power_pass': factory.make_name('power_pass'),
+            'power_id': factory.make_name('power_id'),
         }
 
         node = create_node(
-            architecture, power_type, dumps(power_parameters),
+            architecture, 'virsh', power_parameters,
             mac_addresses)
 
         # Reload the object from the DB so that we're sure its power
         # parameters are being persisted.
         node = reload_object(node)
-        self.assertEqual(power_parameters, node.power_parameters)
+        self.assertItemsEqual(power_parameters, node.power_parameters)
 
     def test__forces_generic_subarchitecture_if_missing(self):
         self.prepare_rack_rpc()
@@ -266,13 +256,9 @@
         mac_addresses = [
             factory.make_mac_address() for _ in range(3)]
         architecture = make_usable_architecture(self, subarch_name='generic')
-        power_type = random.choice(self.power_types)['name']
-        power_parameters = dumps({})
 
         arch, subarch = architecture.split('/')
-        node = create_node(
-            arch, power_type, power_parameters,
-            mac_addresses)
+        node = create_node(arch, 'manual', {}, mac_addresses)
 
         self.assertEqual(architecture, node.architecture)
 

=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2016-10-18 08:00:37 +0000
+++ src/maasserver/testing/factory.py	2016-10-20 20:55:55 +0000
@@ -381,8 +381,6 @@
             zone = self.make_Zone()
         if power_type is None:
             power_type = 'virsh'
-        if power_parameters is None:
-            power_parameters = {}
         if power_state is None:
             power_state = self.pick_enum(POWER_STATE)
         if power_state_updated is undefined:
@@ -443,7 +441,8 @@
             bmc_ip_address = self.pick_ip_in_Subnet(ip_address.subnet)
             node.power_parameters = {
                 "power_address": "qemu+ssh://user@%s/system" % (
-                    factory.ip_to_url_format(bmc_ip_address))
+                    factory.ip_to_url_format(bmc_ip_address)),
+                "power_id": factory.make_name("power_id"),
             }
             node.save()
 

=== modified file 'src/maasserver/tests/test_config_forms.py'
--- src/maasserver/tests/test_config_forms.py	2016-06-22 17:03:02 +0000
+++ src/maasserver/tests/test_config_forms.py	2016-10-20 20:55:55 +0000
@@ -165,6 +165,15 @@
             {'char_field': char_value, 'multi_field': None},
             form.cleaned_data)
 
+    def test_DictCharField_sets_default_value_for_subfields(self):
+        default_value = factory.make_name('default_value')
+        multi_field = DictCharField(
+            [('field_a', forms.CharField(
+                label='Field a', initial=default_value))],
+            required=False)
+        self.assertEquals(
+            default_value, multi_field.clean_sub_fields('')['field_a'])
+
 
 class TestUtilities(MAASServerTestCase):
 

=== modified file 'src/maasserver/tests/test_forms_controller.py'
--- src/maasserver/tests/test_forms_controller.py	2016-04-13 02:20:16 +0000
+++ src/maasserver/tests/test_forms_controller.py	2016-10-20 20:55:55 +0000
@@ -40,6 +40,7 @@
         form = ControllerForm(
             data={
                 'power_type': power_type,
+                'power_parameters_skip_check': 'true',
             },
             instance=rack)
         rack = form.save()
@@ -51,12 +52,12 @@
         form = ControllerForm(
             data={
                 'power_parameters_field': power_parameters_field,
-                'power_parameters_skip_check': True,
+                'power_parameters_skip_check': 'true',
             },
             instance=rack)
         rack = form.save()
         self.assertEqual(
-            {'field': power_parameters_field}, rack.power_parameters)
+            power_parameters_field, rack.power_parameters['field'])
 
     def test__sets_zone(self):
         rack = factory.make_RackController()
@@ -64,6 +65,7 @@
         form = ControllerForm(
             data={
                 'zone': zone.name,
+                'power_parameters_skip_check': 'true',
             },
             instance=rack)
         rack = form.save()

=== modified file 'src/maasserver/tests/test_forms_machine.py'
--- src/maasserver/tests/test_forms_machine.py	2016-09-22 02:53:33 +0000
+++ src/maasserver/tests/test_forms_machine.py	2016-10-20 20:55:55 +0000
@@ -373,7 +373,7 @@
                 'architecture': arch,
                 'power_type': power_type,
                 'power_parameters_field': power_parameters_field,
-                'power_parameters_skip_check': True,
+                'power_parameters_skip_check': 'true',
                 },
             instance=node)
         form.save()
@@ -393,6 +393,7 @@
             data={
                 'hostname': hostname,
                 'architecture': arch,
+                'power_parameters_skip_check': 'true',
                 },
             instance=node)
         node = form.save()
@@ -407,6 +408,7 @@
             data={
                 'hostname': hostname,
                 'architecture': arch,
+                'power_parameters_skip_check': 'true',
                 },
             instance=node)
         node = form.save()
@@ -422,6 +424,7 @@
                 'hostname': hostname,
                 'architecture': arch,
                 'power_type': power_type,
+                'power_parameters_skip_check': 'true',
                 },
             instance=node)
         node = form.save()

=== modified file 'src/maasserver/websockets/handlers/machine.py'
--- src/maasserver/websockets/handlers/machine.py	2016-10-17 20:09:56 +0000
+++ src/maasserver/websockets/handlers/machine.py	2016-10-20 20:55:55 +0000
@@ -237,6 +237,7 @@
         new_params["hostname"] = params.get("hostname")
         new_params["architecture"] = params.get("architecture")
         new_params["power_type"] = params.get("power_type")
+        new_params["power_parameters"] = params.get("power_parameters")
         if "zone" in params:
             new_params["zone"] = params["zone"]["name"]
         if "domain" in params:
@@ -259,13 +260,8 @@
         if not reload_object(self.user).is_superuser:
             raise HandlerPermissionError()
 
-        # Create the object, then save the power parameters because the
-        # form will not save this information.
         data = super(NodeHandler, self).create(params)
         node_obj = Node.objects.get(system_id=data['system_id'])
-        node_obj.power_type = params.get("power_type", '')
-        node_obj.power_parameters = params.get("power_parameters", {})
-        node_obj.save()
 
         # Start the commissioning process right away, which has the
         # desired side effect of initializing the node's power state.
@@ -279,16 +275,13 @@
         if not reload_object(self.user).is_superuser:
             raise HandlerPermissionError()
 
-        # Update the node with the form. The form will not update the
-        # power_type or power_parameters, so we perform that here.
         data = super(NodeHandler, self).update(params)
         node_obj = Node.objects.get(system_id=data['system_id'])
-        node_obj.power_type = params.get("power_type", '')
-        node_obj.power_parameters = params.get("power_parameters", {})
 
         # Update the tags for the node and disks.
         self.update_tags(node_obj, params['tags'])
         node_obj.save()
+
         return self.full_dehydrate(node_obj)
 
     def mount_special(self, params):

=== modified file 'src/maasserver/websockets/handlers/tests/test_machine.py'
--- src/maasserver/websockets/handlers/tests/test_machine.py	2016-10-17 20:09:56 +0000
+++ src/maasserver/websockets/handlers/tests/test_machine.py	2016-10-20 20:55:55 +0000
@@ -1145,7 +1145,7 @@
     def test_update_raises_validation_error_for_invalid_architecture(self):
         user = factory.make_admin()
         handler = MachineHandler(user, {})
-        node = factory.make_Node(interface=True)
+        node = factory.make_Node(interface=True, power_type='manual')
         node_data = self.dehydrate_node(node, handler)
         arch = factory.make_name("arch")
         node_data["architecture"] = arch
@@ -1195,7 +1195,8 @@
         user = factory.make_admin()
         handler = MachineHandler(user, {})
         architecture = make_usable_architecture(self)
-        node = factory.make_Node(interface=True, architecture=architecture)
+        node = factory.make_Node(
+            interface=True, architecture=architecture, power_type='manual')
         tags = [
             factory.make_Tag(definition='').name
             for _ in range(3)
@@ -1209,7 +1210,8 @@
         user = factory.make_admin()
         handler = MachineHandler(user, {})
         architecture = make_usable_architecture(self)
-        node = factory.make_Node(interface=True, architecture=architecture)
+        node = factory.make_Node(
+            interface=True, architecture=architecture, power_type='manual')
         tags = []
         for _ in range(3):
             tag = factory.make_Tag(definition='')
@@ -1226,7 +1228,8 @@
         user = factory.make_admin()
         handler = MachineHandler(user, {})
         architecture = make_usable_architecture(self)
-        node = factory.make_Node(interface=True, architecture=architecture)
+        node = factory.make_Node(
+            interface=True, architecture=architecture, power_type='manual')
         tag_name = factory.make_name("tag")
         node_data = self.dehydrate_node(node, handler)
         node_data["tags"].append(tag_name)

=== modified file 'src/maasserver/websockets/tests/test_base.py'
--- src/maasserver/websockets/tests/test_base.py	2016-09-27 14:24:19 +0000
+++ src/maasserver/websockets/tests/test_base.py	2016-10-20 20:55:55 +0000
@@ -523,7 +523,7 @@
 
     def test_update_with_form_updates_node(self):
         arch = make_usable_architecture(self)
-        node = factory.make_Node(architecture=arch)
+        node = factory.make_Node(architecture=arch, power_type='manual')
         hostname = factory.make_name("hostname")
         handler = self.make_nodes_handler(
             fields=['hostname'], form=AdminMachineForm)
@@ -539,7 +539,7 @@
 
     def test_update_with_form_uses_form_from_get_form_class(self):
         arch = make_usable_architecture(self)
-        node = factory.make_Node(architecture=arch)
+        node = factory.make_Node(architecture=arch, power_type='manual')
         hostname = factory.make_name("hostname")
         handler = self.make_nodes_handler(fields=['hostname'])
         self.patch(

=== modified file 'src/provisioningserver/power/schema.py'
--- src/provisioningserver/power/schema.py	2016-09-27 18:38:53 +0000
+++ src/provisioningserver/power/schema.py	2016-10-20 20:55:55 +0000
@@ -238,9 +238,10 @@
         'name': 'virsh',
         'description': 'Virsh (virtual systems)',
         'fields': [
-            make_json_field('power_address', "Power address"),
+            make_json_field('power_address', "Power address", required=True),
             make_json_field(
-                'power_id', "Power ID", scope=POWER_PARAMETER_SCOPE.NODE),
+                'power_id', "Power ID", scope=POWER_PARAMETER_SCOPE.NODE,
+                required=True),
             make_json_field(
                 'power_pass', "Power password (optional)",
                 required=False, field_type='password'),
@@ -258,10 +259,11 @@
             make_json_field(
                 'power_uuid', "VM UUID (if known)", required=False,
                 scope=POWER_PARAMETER_SCOPE.NODE),
-            make_json_field('power_address', "VMware hostname"),
-            make_json_field('power_user', "VMware username"),
+            make_json_field('power_address', "VMware hostname", required=True),
+            make_json_field('power_user', "VMware username", required=True),
             make_json_field(
-                'power_pass', "VMware password", field_type='password'),
+                'power_pass', "VMware password", field_type='password',
+                required=True),
             make_json_field(
                 'power_port', "VMware API port (optional)", required=False),
             make_json_field(
@@ -273,9 +275,10 @@
         'name': 'fence_cdu',
         'description': 'Sentry Switch CDU',
         'fields': [
-            make_json_field('power_address', "Power address"),
+            make_json_field('power_address', "Power address", required=True),
             make_json_field(
-                'power_id', "Power ID", scope=POWER_PARAMETER_SCOPE.NODE),
+                'power_id', "Power ID", scope=POWER_PARAMETER_SCOPE.NODE,
+                required=True),
             make_json_field('power_user', "Power user"),
             make_json_field(
                 'power_pass', "Power password", field_type='password'),
@@ -288,8 +291,9 @@
         'fields': [
             make_json_field(
                 'power_driver', "Power driver", field_type='choice',
-                choices=IPMI_DRIVER_CHOICES, default=IPMI_DRIVER.LAN_2_0),
-            make_json_field('power_address', "IP address"),
+                choices=IPMI_DRIVER_CHOICES, default=IPMI_DRIVER.LAN_2_0,
+                required=True),
+            make_json_field('power_address', "IP address", required=True),
             make_json_field('power_user', "Power user"),
             make_json_field(
                 'power_pass', "Power password", field_type='password'),
@@ -302,13 +306,13 @@
         'name': 'moonshot',
         'description': 'HP Moonshot - iLO4 (IPMI)',
         'fields': [
-            make_json_field('power_address', "Power address"),
+            make_json_field('power_address', "Power address", required=True),
             make_json_field('power_user', "Power user"),
             make_json_field(
                 'power_pass', "Power password", field_type='password'),
             make_json_field(
                 'power_hwaddress', "Power hardware address",
-                scope=POWER_PARAMETER_SCOPE.NODE),
+                scope=POWER_PARAMETER_SCOPE.NODE, required=True),
         ],
         'ip_extractor': make_ip_extractor('power_address'),
     },
@@ -317,14 +321,16 @@
         'description': 'SeaMicro 15000',
         'fields': [
             make_json_field(
-                'system_id', "System ID", scope=POWER_PARAMETER_SCOPE.NODE),
-            make_json_field('power_address', "Power address"),
+                'system_id', "System ID", scope=POWER_PARAMETER_SCOPE.NODE,
+                required=True),
+            make_json_field('power_address', "Power address", required=True),
             make_json_field('power_user', "Power user"),
             make_json_field(
                 'power_pass', "Power password", field_type='password'),
             make_json_field(
                 'power_control', "Power control type", field_type='choice',
-                choices=SM15K_POWER_CONTROL_CHOICES, default='ipmi'),
+                choices=SM15K_POWER_CONTROL_CHOICES, default='ipmi',
+                required=True),
         ],
         'ip_extractor': make_ip_extractor('power_address'),
     },
@@ -334,7 +340,7 @@
         'fields': [
             make_json_field(
                 'power_pass', "Power password", field_type='password'),
-            make_json_field('power_address', "Power address")
+            make_json_field('power_address', "Power address", required=True)
         ],
         'ip_extractor': make_ip_extractor('power_address'),
     },
@@ -343,8 +349,9 @@
         'description': 'Digital Loggers, Inc. PDU',
         'fields': [
             make_json_field(
-                'outlet_id', "Outlet ID", scope=POWER_PARAMETER_SCOPE.NODE),
-            make_json_field('power_address', "Power address"),
+                'outlet_id', "Outlet ID", scope=POWER_PARAMETER_SCOPE.NODE,
+                required=True),
+            make_json_field('power_address', "Power address", required=True),
             make_json_field('power_user', "Power user"),
             make_json_field(
                 'power_pass', "Power password", field_type='password'),
@@ -355,7 +362,7 @@
         'name': 'wedge',
         'description': "Facebook's Wedge",
         'fields': [
-            make_json_field('power_address', "IP address"),
+            make_json_field('power_address', "IP address", required=True),
             make_json_field('power_user', "Power user"),
             make_json_field(
                 'power_pass', "Power password", field_type='password'),
@@ -367,8 +374,9 @@
         'description': "Cisco UCS Manager",
         'fields': [
             make_json_field(
-                'uuid', "Server UUID", scope=POWER_PARAMETER_SCOPE.NODE),
-            make_json_field('power_address', "URL for XML API"),
+                'uuid', "Server UUID", scope=POWER_PARAMETER_SCOPE.NODE,
+                required=True),
+            make_json_field('power_address', "URL for XML API", required=True),
             make_json_field('power_user', "API user"),
             make_json_field(
                 'power_pass', "API password", field_type='password'),
@@ -380,7 +388,8 @@
         'name': 'mscm',
         'description': "HP Moonshot - iLO Chassis Manager",
         'fields': [
-            make_json_field('power_address', "IP for MSCM CLI API"),
+            make_json_field(
+                'power_address', "IP for MSCM CLI API", required=True),
             make_json_field('power_user', "MSCM CLI API user"),
             make_json_field(
                 'power_pass', "MSCM CLI API password", field_type='password'),
@@ -388,7 +397,7 @@
                 'node_id',
                 "Node ID - Must adhere to cXnY format "
                 "(X=cartridge number, Y=node number).",
-                scope=POWER_PARAMETER_SCOPE.NODE),
+                scope=POWER_PARAMETER_SCOPE.NODE, required=True),
         ],
         'ip_extractor': make_ip_extractor('power_address'),
     },
@@ -396,14 +405,14 @@
         'name': 'msftocs',
         'description': "Microsoft OCS - Chassis Manager",
         'fields': [
-            make_json_field('power_address', "Power address"),
+            make_json_field('power_address', "Power address", required=True),
             make_json_field('power_port', "Power port"),
             make_json_field('power_user', "Power user"),
             make_json_field(
                 'power_pass', "Power password", field_type='password'),
             make_json_field(
                 'blade_id', "Blade ID (Typically 1-24)",
-                scope=POWER_PARAMETER_SCOPE.NODE),
+                scope=POWER_PARAMETER_SCOPE.NODE, required=True),
         ],
         'ip_extractor': make_ip_extractor('power_address'),
     },
@@ -411,10 +420,10 @@
         'name': 'apc',
         'description': "American Power Conversion (APC) PDU",
         'fields': [
-            make_json_field('power_address', "IP for APC PDU"),
+            make_json_field('power_address', "IP for APC PDU", required=True),
             make_json_field(
                 'node_outlet', "APC PDU node outlet number (1-16)",
-                scope=POWER_PARAMETER_SCOPE.NODE),
+                scope=POWER_PARAMETER_SCOPE.NODE, required=True),
             make_json_field(
                 'power_on_delay', "Power ON outlet delay (seconds)",
                 default='5'),
@@ -425,16 +434,16 @@
         'name': 'hmc',
         'description': "IBM Hardware Management Console (HMC)",
         'fields': [
-            make_json_field('power_address', "IP for HMC"),
+            make_json_field('power_address', "IP for HMC", required=True),
             make_json_field('power_user', "HMC username"),
             make_json_field(
                 'power_pass', "HMC password", field_type='password'),
             make_json_field(
                 'server_name', "HMC Managed System server name",
-                scope=POWER_PARAMETER_SCOPE.NODE),
+                scope=POWER_PARAMETER_SCOPE.NODE, required=True),
             make_json_field(
                 'lpar', "HMC logical partition",
-                scope=POWER_PARAMETER_SCOPE.NODE),
+                scope=POWER_PARAMETER_SCOPE.NODE, required=True),
         ],
         'ip_extractor': make_ip_extractor('power_address'),
     },
@@ -442,11 +451,13 @@
         'name': 'nova',
         'description': 'OpenStack Nova',
         'fields': [
-            make_json_field('nova_id', "Host UUID"),
-            make_json_field('os_tenantname', "Tenant name"),
-            make_json_field('os_username', "Username"),
-            make_json_field('os_password', "Password", field_type='password'),
-            make_json_field('os_authurl', "Auth URL"),
+            make_json_field('nova_id', "Host UUID", required=True),
+            make_json_field('os_tenantname', "Tenant name", required=True),
+            make_json_field('os_username', "Username", required=True),
+            make_json_field(
+                'os_password', "Password", field_type='password',
+                required=True),
+            make_json_field('os_authurl', "Auth URL", required=True),
         ],
     },
 ]