launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/initialize-node-nodegroup into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/initialize-node-nodegroup/+merge/117377

This may look like a simple change, and perhaps even a good one.  In reality it was neither.  The forms that save a new node make up a mesh of related classes.  One of these, a mix-in related to MAC addresses, absolutely needs to save its model object and so can't support the commit=False argument.  And it needs to come early in the form's base classes, so this is where a node object is really created.  Thus it was in this mixin's overridden save() method that I needed to ensure initialization of Node.nodegroup.  Anywhere else and the node would already have been written to the database without this field initialized, which wouldn't let us add a NOT NULL constraint.

I tried reordering the forms' base classes, but this broke tests in a way that I couldn't fathom at the time.  It had come to a point where I urgently needed to get past the practical problem and get that NOT NULL constraint into place before I could consider the bigger picture again.

Another approach that might possibly have worked would have been to add yet another mixin, even earlier in the inheritance chains, that merely ensured initialization of this field before the MAC-related mixin even came into play.  I shirked from doing that because it would have increased the reliance on ordering of parent classes even further.  Instead, I ended up relying on tests to ensure the desired behaviour.  My next branch will add NOT NULL constraints, at which point any violation will be swiftly and mercilessly punished.  But yes, it's ugly.  Once we get this working I'd be happy to consider nicer arrangements.


Jeroen

PS — this will conflict with trunk initially, since some of the code changes adjoin a docstring change I made in another branch that is landing as I write this.
-- 
https://code.launchpad.net/~jtv/maas/initialize-node-nodegroup/+merge/117377
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/initialize-node-nodegroup into lp:maas.

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2012-07-31 04:43:48 +0000
+++ src/maasserver/forms.py	2012-07-31 06:28:22 +0000
@@ -59,6 +59,7 @@
     Config,
     MACAddress,
     Node,
+    NodeGroup,
     SSHKey,
     )
 from maasserver.node_action import compile_node_actions
@@ -267,6 +268,12 @@
         return []
 
 
+def initialize_node_group(node):
+    """If `node` is not in a node group yet, enroll it in the master group."""
+    if node.nodegroup is None:
+        node.nodegroup = NodeGroup.objects.ensure_master()
+
+
 class WithMACAddressesMixin:
     """A form mixin which dynamically adds a MultipleMACAddressField to the
     list of fields.  This mixin also overrides the 'save' method to persist
@@ -306,11 +313,20 @@
         return data
 
     def save(self):
+<<<<<<< TREE
         """Save the form's data to the database.
 
         This implementation of `save` does not support the `commit` argument.
         """
         node = super(WithMACAddressesMixin, self).save()
+=======
+        node = super(WithMACAddressesMixin, self).save(commit=False)
+        # 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)
+        node.save()
+>>>>>>> MERGE-SOURCE
         for mac in self.cleaned_data['mac_addresses']:
             node.add_mac_address(mac)
         if self.cleaned_data['hostname'] == "":

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-07-30 15:42:41 +0000
+++ src/maasserver/tests/test_api.py	2012-07-31 06:28:22 +0000
@@ -54,6 +54,7 @@
     DHCPLease,
     MACAddress,
     Node,
+    NodeGroup,
     )
 from maasserver.models.user import (
     create_auth_token,
@@ -283,6 +284,20 @@
             ['aa:bb:cc:dd:ee:ff', '22:bb:cc:dd:ee:ff'],
             [mac.mac_address for mac in diane.macaddress_set.all()])
 
+    def test_POST_new_initializes_nodegroup_to_master_by_default(self):
+        hostname = factory.make_name('host')
+        self.client.post(
+            self.get_uri('nodes/'),
+            {
+                'op': 'new',
+                'hostname': hostname,
+                'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
+                'mac_addresses': [factory.getRandomMACAddress()],
+            })
+        self.assertEqual(
+            NodeGroup.objects.ensure_master(),
+            Node.objects.get(hostname=hostname).nodegroup)
+
     def test_POST_with_no_hostname_auto_populates_hostname(self):
         architecture = factory.getRandomChoice(ARCHITECTURE_CHOICES)
         response = self.client.post(

=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2012-07-31 04:23:03 +0000
+++ src/maasserver/tests/test_forms.py	2012-07-31 06:28:22 +0000
@@ -34,6 +34,7 @@
     get_node_create_form,
     get_node_edit_form,
     HostnameFormField,
+    initialize_node_group,
     MACAddressForm,
     NewUserCreationForm,
     NodeActionForm,
@@ -46,6 +47,8 @@
 from maasserver.models import (
     Config,
     MACAddress,
+    Node,
+    NodeGroup,
     )
 from maasserver.models.config import DEFAULT_CONFIG
 from maasserver.node_action import (
@@ -58,6 +61,22 @@
 from testtools.testcase import ExpectedException
 
 
+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)
+
+
 class NodeWithMACAddressesFormTest(TestCase):
 
     def get_QueryDict(self, params):
@@ -69,29 +88,35 @@
                 query_dict[k] = v
         return query_dict
 
+    def make_params(self, mac_addresses=None, architecture=None):
+        if mac_addresses is None:
+            mac_addresses = [factory.getRandomMACAddress()]
+        if architecture is None:
+            architecture = factory.getRandomEnum(ARCHITECTURE)
+        return self.get_QueryDict({
+            'mac_addresses': mac_addresses,
+            'architecture': architecture,
+        })
+
     def test_NodeWithMACAddressesForm_valid(self):
-
+        architecture = factory.getRandomEnum(ARCHITECTURE)
         form = NodeWithMACAddressesForm(
-            self.get_QueryDict({
-                'mac_addresses': ['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f'],
-                'architecture': ARCHITECTURE.i386,
-                }))
+            self.make_params(
+                mac_addresses=['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f'],
+                architecture=architecture))
 
         self.assertTrue(form.is_valid())
         self.assertEqual(
             ['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f'],
             form.cleaned_data['mac_addresses'])
-        self.assertEqual(ARCHITECTURE.i386, form.cleaned_data['architecture'])
+        self.assertEqual(architecture, form.cleaned_data['architecture'])
 
     def test_NodeWithMACAddressesForm_simple_invalid(self):
         # If the form only has one (invalid) MAC address field to validate,
         # the error message in form.errors['mac_addresses'] is the
         # message from the field's validation error.
         form = NodeWithMACAddressesForm(
-            self.get_QueryDict({
-                'mac_addresses': ['invalid'],
-                'architecture': ARCHITECTURE.i386,
-                }))
+            self.make_params(mac_addresses=['invalid']))
 
         self.assertFalse(form.is_valid())
         self.assertEqual(['mac_addresses'], list(form.errors))
@@ -104,10 +129,7 @@
         # if one or more fields are invalid, a single error message is
         # present in form.errors['mac_addresses'] after validation.
         form = NodeWithMACAddressesForm(
-            self.get_QueryDict({
-                'mac_addresses': ['invalid_1', 'invalid_2'],
-                'architecture': ARCHITECTURE.i386,
-                }))
+            self.make_params(mac_addresses=['invalid_1', 'invalid_2']))
 
         self.assertFalse(form.is_valid())
         self.assertEqual(['mac_addresses'], list(form.errors))
@@ -118,26 +140,26 @@
     def test_NodeWithMACAddressesForm_empty(self):
         # Empty values in the list of MAC addresses are simply ignored.
         form = NodeWithMACAddressesForm(
-            self.get_QueryDict({
-                'mac_addresses': ['aa:bb:cc:dd:ee:ff', ''],
-                'architecture': ARCHITECTURE.i386,
-                }))
+            self.make_params(
+                mac_addresses=[factory.getRandomMACAddress(), '']))
 
         self.assertTrue(form.is_valid())
 
     def test_NodeWithMACAddressesForm_save(self):
-        form = NodeWithMACAddressesForm(
-            self.get_QueryDict({
-                'mac_addresses': ['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f'],
-                'architecture': ARCHITECTURE.i386,
-                }))
+        macs = ['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f']
+        form = NodeWithMACAddressesForm(self.make_params(mac_addresses=macs))
         node = form.save()
 
         self.assertIsNotNone(node.id)  # The node is persisted.
         self.assertSequenceEqual(
-            ['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f'],
+            macs,
             [mac.mac_address for mac in node.macaddress_set.all()])
 
+    def test_sets_nodegroup_to_master_by_default(self):
+        self.assertEqual(
+            NodeGroup.objects.ensure_master(),
+            NodeWithMACAddressesForm(self.make_params()).save().nodegroup)
+
 
 class TestOptionForm(ConfigForm):
     field1 = forms.CharField(label="Field 1", max_length=10)