launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #12290
[Merge] lp:~jtv/maas/node-nodegroup-api-ui into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/node-nodegroup-api-ui into lp:maas.
Commit message:
Allow node-group selection when creating a node in API or UI.
Node groups are selected by subnet: any IP address in a node group's subnet identifies that node group. It could be the node's IP, or the cluster manager's, or the broadcast address, or any other IP in the subnet. This requires a node group to have an interface (subnet) defined before nodes can be added to it, and an IP in the subnet must not be in any other node group's subnet.
Requested reviews:
MAAS Maintainers (maas-maintainers)
For more details, see:
https://code.launchpad.net/~jtv/maas/node-nodegroup-api-ui/+merge/125624
See commit message. Discussed with Raphaël, but then again with Julian which led to a different solution.
A custom field is required to enable the form to identify nodegroups by something other than “id.”
I hit some problems in testing because initialize_node_group reset the nodegroup to the default (the master). And so I had to work yet more node-form functionality into the WithMACAddressesMixin. All the ugliness is necessitated by a combination of limitations in Django's forms hierarchy and ORM: the mixin requires the Node object to be saved (so that MACAddress objects can reference it) before the main form object has fully prepared it for saving. And there seems to be no way for application code to customize field values prior to saving an object: you have to save the form first to get your object, and then you get to change its values. To mitigate the problem when the object isn't in a state that can be saved to the database yet, you can tell the form not to save its object to the database — but that breaks down when other objects need to reference it, as is the case here. The use of mixins for different aspects of a form is classic Django style, but it works with these other limitations to bite us in this weak spot. We may want to sweep some of these classes together to simplify things.
Jeroen
--
https://code.launchpad.net/~jtv/maas/node-nodegroup-api-ui/+merge/125624
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/maas/node-nodegroup-api-ui into lp:maas.
=== modified file 'src/maasserver/fields.py'
--- src/maasserver/fields.py 2012-09-18 14:37:28 +0000
+++ src/maasserver/fields.py 2012-09-21 03:31:18 +0000
@@ -27,7 +27,11 @@
Field,
SubfieldBase,
)
-from django.forms import RegexField
+from django.forms import (
+ ModelChoiceField,
+ RegexField,
+ )
+from maasserver.utils.orm import get_one
import psycopg2.extensions
from south.modelsinspector import add_introspection_rules
@@ -54,6 +58,81 @@
])
+class NodeGroupFormField(ModelChoiceField):
+ """Form field: reference to a :class:`NodeGroup`.
+
+ Node groups are identified by their subnets. More precisely: this
+ field will accept any IP as an identifier for the nodegroup whose subnet
+ contains the IP address.
+
+ Unless `queryset` is explicitly given, this field covers all NodeGroup
+ objects.
+ """
+
+ def __init__(self, **kwargs):
+ # Avoid circular imports.
+ from maasserver.models import NodeGroup
+
+ kwargs.setdefault('queryset', NodeGroup.objects.all())
+ super(NodeGroupFormField, self).__init__(**kwargs)
+
+ def label_from_instance(self, nodegroup):
+ """Django method: get human-readable choice label for nodegroup."""
+ interface = nodegroup.get_managed_interface()
+ if interface is None:
+ return nodegroup.name
+ else:
+ return "%s: %s" % (nodegroup.name, interface.ip)
+
+ def find_nodegroup(self, ip_address):
+ """Find the nodegroup whose subnet contains `ip_address`.
+
+ The matching nodegroup may have multiple interfaces on the subnet,
+ but there can be only one matching nodegroup.
+ """
+ # Avoid circular imports.
+ from maasserver.models import NodeGroup
+
+ return get_one(NodeGroup.objects.raw("""
+ SELECT *
+ FROM maasserver_nodegroup
+ WHERE id IN (
+ SELECT nodegroup_id
+ FROM maasserver_nodegroupinterface
+ WHERE (inet '%s' & subnet_mask) = (ip & subnet_mask)
+ )
+ """ % ip_address))
+
+ def clean(self, value):
+ """Django method: provide expected output for various inputs.
+
+ There seems to be no clear specification on what `value` can be.
+ This method accepts the types that we see in practice: raw bytes
+ containing an IP address, a :class:`NodeGroup`, or the nodegroup's
+ numerical id in text form.
+
+ If no nodegroup is indicated, defaults to the master.
+ """
+ # Avoid circular imports.
+ from maasserver.models import NodeGroup
+
+ if value in (None, '', b''):
+ nodegroup_id = NodeGroup.objects.ensure_master().id
+ elif isinstance(value, NodeGroup):
+ nodegroup_id = value.id
+ elif isinstance(value, unicode) and value.isnumeric():
+ nodegroup_id = int(value)
+ elif isinstance(value, str) and '.' not in value:
+ nodegroup_id = int(value)
+ else:
+ nodegroup = self.find_nodegroup(value)
+ if nodegroup is None:
+ raise NodeGroup.DoesNotExist(
+ "No known subnet contains %s." % value)
+ nodegroup_id = nodegroup.id
+ return super(NodeGroupFormField, self).clean(nodegroup_id)
+
+
class MACAddressFormField(RegexField):
def __init__(self, *args, **kwargs):
=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py 2012-09-19 14:07:52 +0000
+++ src/maasserver/forms.py 2012-09-21 03:31:18 +0000
@@ -62,7 +62,10 @@
DISTRO_SERIES,
DISTRO_SERIES_CHOICES,
)
-from maasserver.fields import MACAddressFormField
+from maasserver.fields import (
+ MACAddressFormField,
+ NodeGroupFormField,
+ )
from maasserver.models import (
Config,
MACAddress,
@@ -104,6 +107,14 @@
class NodeForm(ModelForm):
+
+ def __init__(self, *args, **kwargs):
+ super(NodeForm, self).__init__(*args, **kwargs)
+ if kwargs.get('instance') is None:
+ # Creating a new node. Offer choice of nodegroup.
+ self.fields['nodegroup'] = NodeGroupFormField(
+ required=False, empty_label="Default (master)")
+
after_commissioning_action = forms.TypedChoiceField(
label="After commissioning",
choices=NODE_AFTER_COMMISSIONING_ACTION_CHOICES, required=False,
@@ -162,11 +173,15 @@
class Meta:
model = Node
+<<<<<<< TREE
fields = (
'hostname',
'after_commissioning_action',
'architecture',
'distro_series',
+=======
+ fields = NodeForm.Meta.fields + (
+>>>>>>> MERGE-SOURCE
'power_type',
'power_parameters',
)
@@ -288,10 +303,18 @@
return []
-def initialize_node_group(node):
- """If `node` is not in a node group yet, enroll it in the master group."""
- if node.nodegroup_id is None:
+def initialize_node_group(node, form_value=None):
+ """If `node` is not in a node group yet, initialize it.
+
+ The initial value is `form_value` if given, or the master nodegroup
+ otherwise.
+ """
+ if node.nodegroup_id is not None:
+ return
+ if form_value is None:
node.nodegroup = NodeGroup.objects.ensure_master()
+ else:
+ node.nodegroup = form_value
class WithMACAddressesMixin:
@@ -341,7 +364,11 @@
# We have to save this node in order to attach MACAddress
# records to it. But its nodegroup must be initialized before
# we can do that.
- initialize_node_group(node)
+ # As a side effect, this prevents editing of the node group on
+ # an existing node. It's all horribly dependent on the order of
+ # calls in this class family, but Django doesn't seem to give us
+ # a good way around it.
+ initialize_node_group(node, self.cleaned_data.get('nodegroup'))
node.save()
for mac in self.cleaned_data['mac_addresses']:
node.add_mac_address(mac)
=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py 2012-09-20 13:40:39 +0000
+++ src/maasserver/models/node.py 2012-09-21 03:31:18 +0000
@@ -39,7 +39,6 @@
from maasserver.enum import (
ARCHITECTURE,
ARCHITECTURE_CHOICES,
- DISTRO_SERIES,
DISTRO_SERIES_CHOICES,
NODE_AFTER_COMMISSIONING_ACTION,
NODE_AFTER_COMMISSIONING_ACTION_CHOICES,
=== modified file 'src/maasserver/templates/maasserver/snippets.html'
--- src/maasserver/templates/maasserver/snippets.html 2012-09-18 18:04:52 +0000
+++ src/maasserver/templates/maasserver/snippets.html 2012-09-21 03:31:18 +0000
@@ -27,6 +27,10 @@
<label for="id_architecture">Architecture</label>
{{ node_form.architecture }}
</p>
+ <p>
+ <label for="id_nodegroup">Node group</label>
+ {{ node_form.nodegroup }}
+ </p>
{% if node_form.power_type %}
<p>
<label for="id_power_type">Power type</label>
=== modified file 'src/maasserver/tests/test_fields.py'
--- src/maasserver/tests/test_fields.py 2012-09-18 14:37:28 +0000
+++ src/maasserver/tests/test_fields.py 2012-09-21 03:31:18 +0000
@@ -12,10 +12,17 @@
__metaclass__ = type
__all__ = []
+from django.core.exceptions import ValidationError
from django.db import DatabaseError
-from django.core.exceptions import ValidationError
-from maasserver.fields import validate_mac
-from maasserver.models import MACAddress
+from maasserver.fields import (
+ NodeGroupFormField,
+ validate_mac,
+ )
+from maasserver.models import (
+ MACAddress,
+ NodeGroup,
+ NodeGroupInterface,
+ )
from maasserver.testing.factory import factory
from maasserver.testing.testcase import (
TestCase,
@@ -27,6 +34,85 @@
)
+class TestNodeGroupFormField(TestCase):
+
+ def test_label_from_instance_tolerates_missing_interface(self):
+ nodegroup = factory.make_node_group()
+ interface = nodegroup.get_managed_interface()
+ if interface is not None:
+ NodeGroupInterface.objects.filter(id=interface.id).delete()
+ self.assertEqual(
+ nodegroup.name,
+ NodeGroupFormField().label_from_instance(nodegroup))
+
+ def test_label_from_instance_shows_name_and_address(self):
+ nodegroup = factory.make_node_group()
+ self.assertEqual(
+ '%s: %s' % (nodegroup.name, nodegroup.get_managed_interface().ip),
+ NodeGroupFormField().label_from_instance(nodegroup))
+
+ def test_clean_defaults_to_master(self):
+ spellings_for_none = [None, '', b'']
+ field = NodeGroupFormField()
+ self.assertEqual(
+ [NodeGroup.objects.ensure_master()] * len(spellings_for_none),
+ [field.clean(spelling) for spelling in spellings_for_none])
+
+ def test_clean_accepts_nodegroup(self):
+ nodegroup = factory.make_node_group()
+ self.assertEqual(nodegroup, NodeGroupFormField().clean(nodegroup))
+
+ def test_clean_accepts_id_as_text(self):
+ nodegroup = factory.make_node_group()
+ self.assertEqual(
+ nodegroup,
+ NodeGroupFormField().clean("%s" % nodegroup.id))
+
+ def test_clean_finds_nodegroup_by_network_address(self):
+ nodegroup = factory.make_node_group(
+ ip='192.168.28.1', subnet_mask='255.255.255.0')
+ self.assertEqual(
+ nodegroup,
+ NodeGroupFormField().clean('192.168.28.0'))
+
+ def test_find_nodegroup_looks_up_nodegroup_by_controller_ip(self):
+ nodegroup = factory.make_node_group()
+ self.assertEqual(
+ nodegroup,
+ NodeGroupFormField().clean(nodegroup.get_managed_interface().ip))
+
+ def test_find_nodegroup_accepts_any_ip_in_nodegroup_subnet(self):
+ nodegroup = factory.make_node_group(
+ ip='192.168.41.1', subnet_mask='255.255.255.0')
+ self.assertEqual(
+ nodegroup,
+ NodeGroupFormField().clean('192.168.41.199'))
+
+ def test_find_nodegroup_reports_if_not_found(self):
+ self.assertRaises(
+ NodeGroup.DoesNotExist,
+ NodeGroupFormField().clean,
+ factory.getRandomIPAddress())
+
+ def test_find_nodegroup_reports_if_multiple_matches(self):
+ factory.make_node_group(ip='10.0.0.1', subnet_mask='255.0.0.0')
+ factory.make_node_group(ip='10.1.1.1', subnet_mask='255.255.255.0')
+ self.assertRaises(
+ NodeGroup.MultipleObjectsReturned,
+ NodeGroupFormField().clean, '10.1.1.2')
+
+ def test_find_nodegroup_handles_multiple_matches_on_same_nodegroup(self):
+ nodegroup = factory.make_node_group(
+ ip='10.0.0.1', subnet_mask='255.0.0.0')
+ NodeGroupInterface.objects.create(
+ nodegroup=nodegroup, ip='10.0.0.2', subnet_mask='255.0.0.0',
+ interface='eth71')
+ NodeGroupInterface.objects.create(
+ nodegroup=nodegroup, ip='10.0.0.3', subnet_mask='255.0.0.0',
+ interface='eth72')
+ self.assertEqual(nodegroup, NodeGroupFormField().clean('10.0.0.9'))
+
+
class TestMACAddressField(TestCase):
def test_mac_address_is_stored_normalized_and_loaded(self):
=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py 2012-09-18 18:18:21 +0000
+++ src/maasserver/tests/test_forms.py 2012-09-21 03:31:18 +0000
@@ -62,6 +62,7 @@
AcceptAndCommission,
Delete,
)
+from maasserver.testing import reload_object
from maasserver.testing.factory import factory
from maasserver.testing.testcase import TestCase
from provisioningserver.enum import POWER_TYPE_CHOICES
@@ -71,19 +72,27 @@
class TestHelpers(TestCase):
- def test_initialize_node_group_initializes_nodegroup_to_master(self):
- node = Node(
- NODE_STATUS.DECLARED,
- architecture=factory.getRandomEnum(ARCHITECTURE))
- initialize_node_group(node)
- self.assertEqual(NodeGroup.objects.ensure_master(), node.nodegroup)
-
def test_initialize_node_group_leaves_nodegroup_reference_intact(self):
preselected_nodegroup = factory.make_node_group()
node = factory.make_node(nodegroup=preselected_nodegroup)
initialize_node_group(node)
self.assertEqual(preselected_nodegroup, node.nodegroup)
+ def test_initialize_node_group_initializes_nodegroup_to_form_value(self):
+ node = Node(
+ NODE_STATUS.DECLARED,
+ architecture=factory.getRandomEnum(ARCHITECTURE))
+ nodegroup = factory.make_node_group()
+ initialize_node_group(node, nodegroup)
+ self.assertEqual(nodegroup, node.nodegroup)
+
+ def test_initialize_node_group_defaults_to_master(self):
+ node = Node(
+ NODE_STATUS.DECLARED,
+ architecture=factory.getRandomEnum(ARCHITECTURE))
+ initialize_node_group(node)
+ self.assertEqual(NodeGroup.objects.ensure_master(), node.nodegroup)
+
class NodeWithMACAddressesFormTest(TestCase):
@@ -96,15 +105,19 @@
query_dict[k] = v
return query_dict
- def make_params(self, mac_addresses=None, architecture=None):
+ def make_params(self, mac_addresses=None, architecture=None,
+ nodegroup=None):
if mac_addresses is None:
mac_addresses = [factory.getRandomMACAddress()]
if architecture is None:
architecture = factory.getRandomEnum(ARCHITECTURE)
- return self.get_QueryDict({
+ params = {
'mac_addresses': mac_addresses,
'architecture': architecture,
- })
+ }
+ if nodegroup is not None:
+ params['nodegroup'] = nodegroup
+ return self.get_QueryDict(params)
def test_NodeWithMACAddressesForm_valid(self):
architecture = factory.getRandomEnum(ARCHITECTURE)
@@ -168,6 +181,27 @@
NodeGroup.objects.ensure_master(),
NodeWithMACAddressesForm(self.make_params()).save().nodegroup)
+ def test_sets_nodegroup_on_new_node_if_requested(self):
+ nodegroup = factory.make_node_group(
+ ip_range_low='192.168.14.2', ip_range_high='192.168.14.254',
+ ip='192.168.14.1', subnet_mask='255.255.255.0')
+ form = NodeWithMACAddressesForm(
+ self.make_params(nodegroup=nodegroup.get_managed_interface().ip))
+ self.assertEqual(nodegroup, form.save().nodegroup)
+
+ def test_leaves_nodegroup_alone_if_unset_on_existing_node(self):
+ # Selecting a node group for a node is only supported on new
+ # nodes. You can't change it later.
+ original_nodegroup = factory.make_node_group()
+ node = factory.make_node(nodegroup=original_nodegroup)
+ factory.make_node_group(
+ ip_range_low='10.0.0.1', ip_range_high='10.0.0.2',
+ ip='10.0.0.1', subnet_mask='255.0.0.0')
+ form = NodeWithMACAddressesForm(
+ self.make_params(nodegroup='10.0.0.1'), instance=node)
+ form.save()
+ self.assertEqual(original_nodegroup, reload_object(node).nodegroup)
+
class TestOptionForm(ConfigForm):
field1 = forms.CharField(label="Field 1", max_length=10)
@@ -226,7 +260,11 @@
'hostname',
'after_commissioning_action',
'architecture',
+<<<<<<< TREE
'distro_series',
+=======
+ 'nodegroup',
+>>>>>>> MERGE-SOURCE
], list(form.fields))
def test_NodeForm_changes_node(self):
