launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-1070775 into lp:maas.

Commit message:
Forward-port r1277 from 1.2: Disallow nodegroup (DNS zone) name changes while allocated nodes are still using that zone name.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1070775 in MAAS: "The zone name (attached to a cluster controller) can still be changed when it contains in-use nodes and DNS is managed."
  https://bugs.launchpad.net/maas/+bug/1070775

For more details, see:
https://code.launchpad.net/~jtv/maas/bug-1070775/+merge/132068

No changes needed in porting this over from the 1.2 branch.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/bug-1070775/+merge/132068
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/bug-1070775 into lp:maas.

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2012-10-30 10:30:34 +0000
+++ src/maasserver/forms.py	2012-10-30 11:28:36 +0000
@@ -63,6 +63,7 @@
     NODE_AFTER_COMMISSIONING_ACTION,
     NODE_AFTER_COMMISSIONING_ACTION_CHOICES,
     NODE_STATUS,
+    NODEGROUP_STATUS,
     NODEGROUPINTERFACE_MANAGEMENT,
     NODEGROUPINTERFACE_MANAGEMENT_CHOICES,
     )
@@ -820,6 +821,31 @@
             'name',
             )
 
+    def clean_name(self):
+        old_name = self.instance.name
+        new_name = self.cleaned_data['name']
+        if new_name == old_name or not new_name:
+            # No change to the name.  Return old name.
+            return old_name
+
+        if self.instance.status != NODEGROUP_STATUS.ACCEPTED:
+            # This nodegroup is not in use.  Change it at will.
+            return new_name
+
+        interface = self.instance.get_managed_interface()
+        if interface.management != NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS:
+            # MAAS is not managing DNS on this network, so the user can
+            # rename the zone at will.
+            return new_name
+
+        nodes_in_use = Node.objects.filter(
+            nodegroup=self.instance, status=NODE_STATUS.ALLOCATED)
+        if nodes_in_use.exists():
+            raise ValidationError(
+                "Can't rename DNS zone to %s; nodes are in use." % new_name)
+
+        return new_name
+
 
 class TagForm(ModelForm):
 

=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2012-10-30 10:30:34 +0000
+++ src/maasserver/tests/test_forms.py	2012-10-30 11:28:36 +0000
@@ -44,6 +44,7 @@
     NewUserCreationForm,
     NodeActionForm,
     NodeForm,
+    NodeGroupEdit,
     NodeGroupInterfaceForm,
     NodeGroupWithInterfacesForm,
     NodeWithMACAddressesForm,
@@ -894,3 +895,100 @@
                 nodegroup.management for nodegroup in
                 nodegroup.nodegroupinterface_set.all()
             ])
+
+
+def make_unrenamable_nodegroup_with_node():
+    """Create a `NodeGroup` that can't be renamed, and `Node`.
+
+    Node groups can't be renamed while they are in an accepted state, have
+    DHCP and DNS management enabled, and have a node that is in allocated
+    state.
+
+    :return: tuple: (`NodeGroup`, `Node`).
+    """
+    name = factory.make_name('original-name')
+    nodegroup = factory.make_node_group(
+        name=name, status=NODEGROUP_STATUS.ACCEPTED)
+    interface = nodegroup.get_managed_interface()
+    interface.management = NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS
+    interface.save()
+    node = factory.make_node(nodegroup=nodegroup, status=NODE_STATUS.ALLOCATED)
+    return nodegroup, node
+
+
+class TestNodeGroupEdit(TestCase):
+
+    def make_form_data(self, nodegroup):
+        """Create `NodeGroupEdit` form data based on `nodegroup`."""
+        return {
+            'name': nodegroup.name,
+            'cluster_name': nodegroup.cluster_name,
+            'status': nodegroup.status,
+        }
+
+    def test_changes_name(self):
+        nodegroup = factory.make_node_group(name=factory.make_name('old-name'))
+        new_name = factory.make_name('new-name')
+        data = self.make_form_data(nodegroup)
+        data['name'] = new_name
+        form = NodeGroupEdit(instance=nodegroup, data=data)
+        self.assertTrue(form.is_valid())
+        form.save()
+        self.assertEqual(new_name, reload_object(nodegroup).name)
+
+    def test_refuses_name_change_if_dns_managed_and_nodes_in_use(self):
+        nodegroup, node = make_unrenamable_nodegroup_with_node()
+        data = self.make_form_data(nodegroup)
+        data['name'] = factory.make_name('new-name')
+        form = NodeGroupEdit(instance=nodegroup, data=data)
+        self.assertFalse(form.is_valid())
+
+    def test_accepts_unchanged_name(self):
+        nodegroup, node = make_unrenamable_nodegroup_with_node()
+        original_name = nodegroup.name
+        form = NodeGroupEdit(
+            instance=nodegroup, data=self.make_form_data(nodegroup))
+        self.assertTrue(form.is_valid())
+        form.save()
+        self.assertEqual(original_name, reload_object(nodegroup).name)
+
+    def test_accepts_omitted_name(self):
+        nodegroup, node = make_unrenamable_nodegroup_with_node()
+        original_name = nodegroup.name
+        data = self.make_form_data(nodegroup)
+        del data['name']
+        form = NodeGroupEdit(instance=nodegroup, data=data)
+        self.assertTrue(form.is_valid())
+        form.save()
+        self.assertEqual(original_name, reload_object(nodegroup).name)
+
+    def test_accepts_name_change_if_nodegroup_not_accepted(self):
+        nodegroup, node = make_unrenamable_nodegroup_with_node()
+        nodegroup.status = NODEGROUP_STATUS.PENDING
+        data = self.make_form_data(nodegroup)
+        data['name'] = factory.make_name('new-name')
+        form = NodeGroupEdit(instance=nodegroup, data=data)
+        self.assertTrue(form.is_valid())
+
+    def test_accepts_name_change_if_dns_managed_but_no_nodes_in_use(self):
+        nodegroup, node = make_unrenamable_nodegroup_with_node()
+        node.status = NODE_STATUS.READY
+        node.save()
+        data = self.make_form_data(nodegroup)
+        data['name'] = factory.make_name('new-name')
+        form = NodeGroupEdit(instance=nodegroup, data=data)
+        self.assertTrue(form.is_valid())
+        form.save()
+        self.assertEqual(data['name'], reload_object(nodegroup).name)
+
+    def test_accepts_name_change_if_nodes_in_use_but_dns_not_managed(self):
+        nodegroup, node = make_unrenamable_nodegroup_with_node()
+        interface = nodegroup.get_managed_interface()
+        interface.management = NODEGROUPINTERFACE_MANAGEMENT.DHCP
+        interface.save()
+        data = self.make_form_data(nodegroup)
+        data['name'] = factory.make_name('new-name')
+        form = NodeGroupEdit(instance=nodegroup, data=data)
+        self.assertTrue(form.is_valid())
+        form.save()
+        self.assertEqual(data['name'], reload_object(nodegroup).name)