launchpad-reviewers team mailing list archive

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

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

This is the outcome of a pre-imp with Raphaël.  As soon as we create the master nodegroup, all existing nodes that do not have a nodegroup yet should be assigned to the newly-created master.

A later branch will ensure creation of the master nodegroup from a South migration, and can then make Node.nodegroup NOT NULL.

You'll also see me refer to the nodegroup name “master” here and there.  We may change how that works.  I'll write up a separate branch that hides this a bit, so that we get more flexibility to change it.  If nothing else, it'll help us recognize the code that will need to follow suit.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/init-node-nodegroup/+merge/116811
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/init-node-nodegroup into lp:maas.

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2012-07-23 05:00:54 +0000
+++ src/maasserver/models/node.py	2012-07-26 09:16:25 +0000
@@ -49,7 +49,6 @@
 from maasserver.fields import JSONObjectField
 from maasserver.models.cleansave import CleanSave
 from maasserver.models.config import Config
-from maasserver.models.nodegroup import NodeGroup
 from maasserver.models.timestampedmodel import TimestampedModel
 from maasserver.utils import get_db_state
 from piston.models import Token
@@ -57,7 +56,10 @@
     POWER_TYPE,
     POWER_TYPE_CHOICES,
     )
-from provisioningserver.tasks import power_on, power_off
+from provisioningserver.tasks import (
+    power_off,
+    power_on,
+    )
 
 
 def generate_node_system_id():
@@ -382,7 +384,7 @@
     netboot = BooleanField(default=True)
 
     nodegroup = ForeignKey(
-        NodeGroup, default=None, blank=True, null=True,
+        'maasserver.NodeGroup', default=None, blank=True, null=True,
         editable=False)
 
     objects = NodeManager()

=== modified file 'src/maasserver/models/nodegroup.py'
--- src/maasserver/models/nodegroup.py	2012-07-25 08:20:23 +0000
+++ src/maasserver/models/nodegroup.py	2012-07-26 09:16:25 +0000
@@ -39,6 +39,9 @@
     the model class it manages.
     """
 
+    # Cached master nodegroup.
+    cached_master = None
+
     def new(self, name, worker_ip, subnet_mask=None, broadcast_ip=None,
             router_ip=None, ip_range_low=None, ip_range_high=None):
         """Create a :class:`NodeGroup` with the given parameters.
@@ -71,10 +74,23 @@
 
     def ensure_master(self):
         """Obtain the master node group, creating it first if needed."""
-        try:
-            return self.get(name='master')
-        except NodeGroup.DoesNotExist:
-            return self.new('master', '127.0.0.1')
+        # Avoid circular imports.
+        from maasserver.models import Node
+
+        if self.cached_master is None:
+            try:
+                self.cached_master = self.get(name='master')
+            except NodeGroup.DoesNotExist:
+                self.cached_master = self.new('master', '127.0.0.1')
+                Node.objects.filter(nodegroup=None).update(
+                    nodegroup=self.cached_master)
+
+        return self.cached_master
+
+    def _delete_master(self):
+        """For use by tests: delete the master nodegroup."""
+        self.cached_master = None
+        self.filter(name='master').delete()
 
 
 class NodeGroup(TimestampedModel):

=== modified file 'src/maasserver/tests/test_commands_config_master_dhcp.py'
--- src/maasserver/tests/test_commands_config_master_dhcp.py	2012-07-18 09:45:07 +0000
+++ src/maasserver/tests/test_commands_config_master_dhcp.py	2012-07-26 09:16:25 +0000
@@ -95,12 +95,14 @@
             call_command, 'config_master_dhcp', clear=True, ensure=True)
 
     def test_ensure_creates_master_nodegroup_without_dhcp_settings(self):
+        NodeGroup.objects._delete_master()
         call_command('config_master_dhcp', ensure=True)
         self.assertThat(
             NodeGroup.objects.get(name='master'),
             MatchesStructure.fromExample(make_cleared_dhcp_settings()))
 
     def test_ensure_leaves_cleared_settings_cleared(self):
+        NodeGroup.objects._delete_master()
         call_command('config_master_dhcp', clear=True)
         call_command('config_master_dhcp', ensure=True)
         self.assertThat(
@@ -108,6 +110,7 @@
             MatchesStructure.fromExample(make_cleared_dhcp_settings()))
 
     def test_ensure_leaves_dhcp_settings_intact(self):
+        NodeGroup.objects._delete_master()
         settings = make_dhcp_settings()
         call_command('config_master_dhcp', **settings)
         call_command('config_master_dhcp', ensure=True)

=== modified file 'src/maasserver/tests/test_nodegroup.py'
--- src/maasserver/tests/test_nodegroup.py	2012-07-25 08:20:23 +0000
+++ src/maasserver/tests/test_nodegroup.py	2012-07-26 09:16:25 +0000
@@ -76,6 +76,7 @@
         self.assertEqual(nodegroup.api_key, nodegroup.api_token.key)
 
     def test_ensure_master_creates_minimal_master_nodegroup(self):
+        NodeGroup.objects._delete_master()
         self.assertThat(
             NodeGroup.objects.ensure_master(),
             MatchesStructure.fromExample({
@@ -88,19 +89,40 @@
                 'ip_range_high': None,
             }))
 
-    def test_writes_master_nodegroup_to_database(self):
+    def test_ensure_master_writes_master_nodegroup_to_database(self):
+        NodeGroup.objects._delete_master()
         master = NodeGroup.objects.ensure_master()
         self.assertEqual(
             master.id, NodeGroup.objects.get(name=master.name).id)
 
     def test_ensure_master_returns_same_nodegroup_every_time(self):
+        NodeGroup.objects._delete_master()
         self.assertEqual(
             NodeGroup.objects.ensure_master().id,
             NodeGroup.objects.ensure_master().id)
 
+    def test_ensure_master_does_not_return_other_nodegroup(self):
+        NodeGroup.objects._delete_master()
+        self.assertNotEqual(
+            NodeGroup.objects.new(
+                factory.make_name('nodegroup'), factory.getRandomIPAddress()),
+            NodeGroup.objects.ensure_master())
+
     def test_ensure_master_preserves_existing_attributes(self):
         master = NodeGroup.objects.ensure_master()
         ip = factory.getRandomIPAddress()
         master.worker_ip = ip
         master.save()
         self.assertEqual(ip, NodeGroup.objects.ensure_master().worker_ip)
+
+    def test_ensure_master_updates_groupless_nodes(self):
+        NodeGroup.objects._delete_master()
+        # This test becomes obsolete (and the failure impossible to test
+        # for) once Node.nodegroup is NOT NULL.
+        groupless_node = factory.make_node()
+        groupless_node.nodegroup = None
+        groupless_node.save()
+        groupful_node = factory.make_node(nodegroup=factory.make_node_group())
+        master = NodeGroup.objects.ensure_master()
+        self.assertEqual(master, reload_object(groupless_node).nodegroup)
+        self.assertNotEqual(master, reload_object(groupful_node).nodegroup)