Merge lp:~jtv/maas/node-nodegroup-api-ui into lp:maas/trunk

[Jeroen T. Vermeulen]

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.

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

[John A Meinel (jameinel)]

I think the only missing test is to check that the field is not present when the node already exists.

Otherwise this looks good to me, but maybe we want someone with a stronger Django background to also give it a look.

review: Approve

[lp:~jtv/maas/node-nodegroup-api-ui]

1023. By Jeroen T. Vermeulen on 2012-09-21

Merge trunk, resolve conflicts.

1024. By Jeroen T. Vermeulen on 2012-09-21

Added tests, as per review call with jam.

[Raphaël Badin (rvb)]

Looks good. A few remarks ([0] is probably the most important one).

[0]

332 + def test_find_nodegroup_reports_if_not_found(self):
333 + self.assertRaises(
334 + NodeGroup.DoesNotExist,
335 + NodeGroupFormField().clean,
336 + factory.getRandomIPAddress())

form.clean should really fail with a ValidationError. This will allow the API to dtrt (return a bad request error).

[1]

145 - fields = (
146 - 'hostname',
147 - 'after_commissioning_action',
148 - 'architecture',
149 - 'distro_series',
150 + fields = NodeForm.Meta.fields + (

I'd rather have the field explicitly listed here, much easy to spot a missing field that way.

[2]

242 + <p>
243 + <label for="id_nodegroup">Node group</label>
244 + {{ node_form.nodegroup }}
245 + </p>

You can test this by adding a test similar to testFormContainsArchitectureChoice in src/maasserver/static/js/tests/test_node_add.js.

review: Approve

[lp:~jtv/maas/node-nodegroup-api-ui]

1025. By Jeroen T. Vermeulen on 2012-09-21

Merge trunk.

1026. By Jeroen T. Vermeulen on 2012-09-21

Review changes.

[Jeroen T. Vermeulen (jtv)]

Thanks for the review, gentlemen. I made the requested changes, except for two (after talking them over with Raphaël):

 - The fields change would be non-functional and thus a bit misleading in maintenance. Instead I documented the real meaning of Meta.fields, i.e. to say which fields the form should auto-generate from the model.

 - As originally envisioned, the javascript test turned out to be pointless because it only ends up testing the test's own HTML fixtures. It wouldn't be impossible to change, but not particularly cost-effective.

Jeroen

=== modified file 'src/maasserver/fields.py'
--- src/maasserver/fields.py	2012-09-21 04:56:49 +0000
+++ src/maasserver/fields.py	2012-09-21 09:08:18 +0000
@@ -22,12 +22,17 @@
     )
 import re
 
+from django.core.exceptions import ValidationError
 from django.core.validators import RegexValidator
 from django.db.models import (
     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 +59,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, bytes) and '.' not in value:
+            nodegroup_id = int(value)
+        else:
+            nodegroup = self.find_nodegroup(value)
+            if nodegroup is None:
+                raise ValidationError(
+                    "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 09:08:18 +0000
@@ -54,15 +54,18 @@
 from maasserver.enum import (
     ARCHITECTURE,
     ARCHITECTURE_CHOICES,
+    DISTRO_SERIES,
+    DISTRO_SERIES_CHOICES,
     NODE_AFTER_COMMISSIONING_ACTION,
     NODE_AFTER_COMMISSIONING_ACTION_CHOICES,
     NODEGROUP_STATUS,
     NODEGROUPINTERFACE_MANAGEMENT,
     NODEGROUPINTERFACE_MANAGEMENT_CHOICES,
-    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,
@@ -122,6 +133,9 @@
 
     class Meta:
         model = Node
+
+        # Fields that the form should generate automatically from the
+        # model:
         fields = (
             'hostname',
             'after_commissioning_action',
@@ -162,11 +176,10 @@
 
     class Meta:
         model = Node
-        fields = (
-            'hostname',
-            'after_commissioning_action',
-            'architecture',
-            'distro_series',
+
+        # Fields that the form should generate automatically from the
+        # model:
+        fields = NodeForm.Meta.fields + (
             'power_type',
             'power_parameters',
             )
@@ -288,10 +301,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 +362,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/migrations/0027_add_tag_table.py'
--- src/maasserver/migrations/0027_add_tag_table.py	2012-09-20 13:01:37 +0000
+++ src/maasserver/migrations/0027_add_tag_table.py	2012-09-21 09:08:18 +0000
@@ -1,8 +1,10 @@
 # encoding: utf-8
 import datetime
+
+from django.db import models
 from south.db import db
 from south.v2 import SchemaMigration
-from django.db import models
+
 
 class Migration(SchemaMigration):
 

=== modified file 'src/maasserver/models/tag.py'
--- src/maasserver/models/tag.py	2012-09-20 13:01:37 +0000
+++ src/maasserver/models/tag.py	2012-09-21 09:08:18 +0000
@@ -16,8 +16,8 @@
 
 from django.db.models import (
     CharField,
+    Manager,
     TextField,
-    Manager,
     )
 from maasserver import DefaultMeta
 from maasserver.models.cleansave import CleanSave

=== modified file 'src/maasserver/preseed.py'
--- src/maasserver/preseed.py	2012-09-20 09:38:18 +0000
+++ src/maasserver/preseed.py	2012-09-21 09:08:18 +0000
@@ -28,9 +28,9 @@
     NODE_STATUS,
     PRESEED_TYPE,
     )
+from maasserver.models import Config
 from maasserver.server_address import get_maas_facing_server_host
 from maasserver.utils import absolute_reverse
-from maasserver.models import Config
 import tempita
 
 

=== 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 09:08: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 09:08: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(
+            ValidationError,
+            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 09:08: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)
@@ -163,11 +176,44 @@
             macs,
             [mac.mac_address for mac in node.macaddress_set.all()])
 
+    def test_includes_nodegroup_field_for_new_node(self):
+        self.assertIn(
+            'nodegroup',
+            NodeWithMACAddressesForm(self.make_params()).fields)
+
+    def test_does_not_include_nodegroup_field_for_existing_node(self):
+        params = self.make_params()
+        node = factory.make_node()
+        self.assertNotIn(
+            'nodegroup',
+            NodeWithMACAddressesForm(params, instance=node).fields)
+
     def test_sets_nodegroup_to_master_by_default(self):
         self.assertEqual(
             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)
@@ -227,6 +273,7 @@
                 'after_commissioning_action',
                 'architecture',
                 'distro_series',
+                'nodegroup',
             ], list(form.fields))
 
     def test_NodeForm_changes_node(self):

=== modified file 'src/maasserver/tests/test_node.py'
--- src/maasserver/tests/test_node.py	2012-09-18 18:46:31 +0000
+++ src/maasserver/tests/test_node.py	2012-09-21 09:08:18 +0000
@@ -20,11 +20,11 @@
     ValidationError,
     )
 from maasserver.enum import (
+    DISTRO_SERIES,
     NODE_PERMISSION,
     NODE_STATUS,
     NODE_STATUS_CHOICES,
     NODE_STATUS_CHOICES_DICT,
-    DISTRO_SERIES,
     )
 from maasserver.exceptions import NodeStateViolation
 from maasserver.models import (

=== modified file 'src/maasserver/tests/test_tag.py'
--- src/maasserver/tests/test_tag.py	2012-09-20 13:16:13 +0000
+++ src/maasserver/tests/test_tag.py	2012-09-21 09:08:18 +0000
@@ -12,9 +12,7 @@
 __metaclass__ = type
 __all__ = []
 
-from maasserver.models import (
-    Node,
-    )
+from maasserver.models import Node
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase