launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #08223
[Merge] lp:~rvb/maas/power-param-api2 into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/power-param-api2 into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~rvb/maas/power-param-api2/+merge/107384
This is the first branch in a series of branches to add proper support for power parameters in MAAS. The goal is to allow one to set arbitrary power parameters (for the cases where we will need to tweak power support by hand and create custom templates for power management) on a node but at the same time provide validation and meaningful errors messages for the common power management methods which require/allow well defined power parameters.
In this branch I create the basic structure to store the association between the power types and a list of power parameters as well as the node's method to use that structure and change the field power_parameters (and optionally validate that the given power parameter correspond.
Next step is to create the proper API methods to manipulate a node's power parameters (with validation but also with a way to avoid validation and store arbitrary data).
The I'll change the UI to (dynamically) add the proper fields to the UI when one will select a news power_type for a node.
Note that we will also need a way to have MAAS wide defaults for power parameters (same as what we do for power_type).
= Pre-imp =
I had a pre-imp with no less than 3 persons for this.
With Julian I discussed the whole design and how to best compromised the need for being able to set arbitrary power parameters.
With Gavin I discussed about how to structure the power_parameters module.
With Daviey I discussed about the power parameter for each of the currently supported power types. He agreed that the simplest way to list the parameters for each power type was to look at the commands in Cobbler power management's templates.
--
https://code.launchpad.net/~rvb/maas/power-param-api2/+merge/107384
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/power-param-api2 into lp:maas.
=== modified file 'src/maasserver/models/__init__.py'
--- src/maasserver/models/__init__.py 2012-05-25 12:36:59 +0000
+++ src/maasserver/models/__init__.py 2012-05-25 13:49:31 +0000
@@ -79,6 +79,7 @@
from maasserver.models.filestorage import FileStorage
from maasserver.models.sshkey import SSHKey
from maasserver.models.timestampedmodel import TimestampedModel
+from maasserver.power_parameters import validate_power_parameters
from metadataserver import nodeinituser
from piston.models import (
Consumer,
@@ -591,11 +592,22 @@
raise ValueError(
"Default power type is configured to the default, but "
"that means to use the configured default. It needs to "
- "be confirued to another, more useful value.")
+ "be configured to another, more useful value.")
else:
power_type = self.power_type
return power_type
+ def set_power_parameters(self, power_parameters, validate=True):
+ """Change node's power parameters and optionally validate them by
+ comparing them to the expected power parameters for the effective
+ power type.
+ """
+ if validate:
+ validate_power_parameters(
+ power_parameters, self.get_effective_power_type())
+ self.power_parameters = power_parameters
+ self.save()
+
def acquire(self, user, token=None):
"""Mark commissioned node as acquired by the given user and token."""
assert self.owner is None
=== added file 'src/maasserver/power_parameters.py'
--- src/maasserver/power_parameters.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/power_parameters.py 2012-05-25 13:49:31 +0000
@@ -0,0 +1,131 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Power parameters. Each possible value of a Node's power_type field can
+be associated with specific 'power parameters' wich will be used when
+powering up or down the node in question. These 'power parameters' will be
+stored as a JSON object in the Node's power parameter field. Even if we want
+to allow arbitrary power parameters to be set using the API for maximum
+flexibility, each value of power type is associated with a set of 'sensible'
+power parameters. That is used to validate data (but again, it is possible
+to bypass that validation step and store arbitrary power parameters) and by
+the UI to display the right power parameter fields that correspond to the
+selected power_type. The classes in this module are used to associate each
+power type with a set of power parameters.
+
+To define a new set of power parameters for a new power_type: create a new
+mapping between the new power type and a list of PowerParameter instances in
+`POWER_TYPE_PARAMETERS`.
+"""
+
+from __future__ import (
+ absolute_import,
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = [
+ 'POWER_TYPE_PARAMETERS',
+ 'validate_power_parameters',
+ ]
+
+from collections import namedtuple
+from operator import attrgetter
+
+from django.core.exceptions import ValidationError
+from provisioningserver.enum import POWER_TYPE
+
+
+PowerParameter = namedtuple(
+ 'PowerParameter',
+ [
+ # 'display' will be used in the UI as the title of the field.
+ 'display',
+ # 'name' is the actual name of this parameter as used in the JSON
+ # structure (power parameters are stored as JSON dicts).
+ 'name',
+ ])
+
+
+POWER_TYPE_PARAMETERS = {
+ POWER_TYPE.WAKE_ON_LAN:
+ [
+ PowerParameter(
+ display='Address',
+ name='power_address',
+ ),
+ ],
+ POWER_TYPE.VIRSH:
+ [
+ PowerParameter(
+ display='Driver',
+ name='driver',
+ ),
+ PowerParameter(
+ display='Username',
+ name='username',
+ ),
+ PowerParameter(
+ display='Address',
+ name='power_address',
+ ),
+ PowerParameter(
+ display='power_id',
+ name='power_id',
+ ),
+ ],
+ POWER_TYPE.IPMI:
+ [
+ PowerParameter(
+ display='Address',
+ name='power_address',
+ ),
+ PowerParameter(
+ display='User',
+ name='power_user',
+ ),
+ PowerParameter(
+ display='Password',
+ name='power_pass',
+ ),
+ ],
+ POWER_TYPE.IPMI_LAN:
+ [
+ PowerParameter(
+ display='User',
+ name='power_user',
+ ),
+ PowerParameter(
+ display='Password',
+ name='power_pass',
+ ),
+ PowerParameter(
+ display='power_id',
+ name='power_id',
+ ),
+ ]
+ }
+
+
+def validate_power_parameters(power_parameters, power_type):
+ """Validate that the given power parameters are valid:
+ - the given power_parameter argument must be a dictionnary.
+ - the keys of the given power_parameter argument must be a subset of
+ the possible parameters for this power type.
+ If one of these assertions is not true, raise a ValidationError.
+ """
+ if not isinstance(power_parameters, dict):
+ raise ValidationError(
+ "The given power parameters should be a dictionary.")
+ expected_power_parameters = map(attrgetter(
+ 'name'), POWER_TYPE_PARAMETERS.get(power_type, []))
+ if len(expected_power_parameters) != 0:
+ unknown_fields = set(
+ power_parameters).difference(expected_power_parameters)
+ if len(unknown_fields) != 0:
+ raise ValidationError(
+ "These field(s) are invalid for this power type: %s. "
+ "Allowed fields: %s." % (
+ ','.join(unknown_fields),
+ ','.join(expected_power_parameters)))
=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py 2012-05-25 12:43:32 +0000
+++ src/maasserver/tests/test_models.py 2012-05-25 13:49:31 +0000
@@ -284,6 +284,20 @@
node = factory.make_node(power_type=POWER_TYPE.DEFAULT)
self.assertEqual("", node.power_parameters)
+ def test_set_power_parameters_validates_power_parameters(self):
+ node = factory.make_node()
+ # power_parameter should be a dict, create a list instead.
+ power_parameter = [[factory.getRandomString()]]
+ self.assertRaises(
+ ValidationError, node.set_power_parameters, power_parameter)
+
+ def test_set_power_parameters_can_skip_validation(self):
+ node = factory.make_node()
+ # power_parameter should be a dict, create a list instead.
+ power_parameter = [[factory.getRandomString()]]
+ node.set_power_parameters(power_parameter, validate=False)
+ self.assertEqual(power_parameter, reload_object(node).power_parameters)
+
def test_acquire(self):
node = factory.make_node(status=NODE_STATUS.READY)
user = factory.make_user()
=== added file 'src/maasserver/tests/test_power_parameters.py'
--- src/maasserver/tests/test_power_parameters.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/test_power_parameters.py 2012-05-25 13:49:31 +0000
@@ -0,0 +1,87 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for power parameters."""
+
+from __future__ import (
+ absolute_import,
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = []
+
+from operator import attrgetter
+
+from django.core.exceptions import ValidationError
+from maasserver.power_parameters import (
+ POWER_TYPE_PARAMETERS,
+ PowerParameter,
+ validate_power_parameters,
+ )
+from maasserver.testing.factory import factory
+from maasserver.testing.testcase import TestCase
+from maasserver.utils import map_enum
+from maastesting.matchers import ContainsAll
+from provisioningserver.enum import POWER_TYPE
+from testtools.matchers import (
+ AllMatch,
+ IsInstance,
+ )
+
+
+class TestPowerParameterDeclaration(TestCase):
+
+ def test_POWER_TYPE_PARAMETERS_is_dict_with_power_type_keys(self):
+ power_types = set(map_enum(POWER_TYPE).values())
+ self.assertIsInstance(POWER_TYPE_PARAMETERS, dict)
+ self.assertThat(power_types, ContainsAll(POWER_TYPE_PARAMETERS))
+
+ def test_POWER_TYPE_PARAMETERS_values_are_PowerParameter(self):
+ params = sum(POWER_TYPE_PARAMETERS.values(), [])
+ self.assertThat(params, AllMatch(IsInstance(PowerParameter)))
+
+
+class TestPowerParameterHelpers(TestCase):
+
+ def test_validate_power_parameters_requires_dict(self):
+ exception = self.assertRaises(
+ ValidationError, validate_power_parameters,
+ factory.getRandomString(), factory.getRandomString())
+ self.assertEqual(
+ ["The given power parameters should be a dictionary."],
+ exception.messages)
+
+ def test_validate_power_parameters_rejects_unknown_field(self):
+ # If power_type is a known power type, the fields in the provided
+ # power_parameters dict are checked.
+ param = factory.getRandomString()
+ power_parameters = {param: factory.getRandomString()}
+ power_type = POWER_TYPE.WAKE_ON_LAN
+ expected_power_parameters = map(attrgetter(
+ 'name'), POWER_TYPE_PARAMETERS.get(power_type, []))
+ exception = self.assertRaises(
+ ValidationError, validate_power_parameters,
+ power_parameters, power_type)
+ self.assertEqual(
+ ["These field(s) are invalid for this power type: %s. "
+ "Allowed fields: %s." % (
+ param,
+ ','.join(expected_power_parameters))],
+ exception.messages)
+
+ def test_validate_power_parameters_validates_if_unknown_power_type(self):
+ # If power_type is not a known power type, no check of the fields in
+ # power_parameter is performed.
+ power_parameters = {
+ factory.getRandomString(): factory.getRandomString()}
+ power_type = factory.getRandomString()
+ self.assertIsNone(
+ validate_power_parameters(power_parameters, power_type))
+
+ def test_validate_power_parameters_validates_with_power_type_info(self):
+ power_parameters = {'power_address': factory.getRandomString()}
+ power_type = POWER_TYPE.WAKE_ON_LAN
+ self.assertIsNone(
+ validate_power_parameters(power_parameters, power_type))
