launchpad-reviewers team mailing list archive

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

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

Commit message:
Disallow changes to Node.hostname on nodes that are currently allocated.  The change would confuse Juju, which will still refer to the node by its old hostname.

Requested reviews:
  MAAS Maintainers (maas-maintainers)
Related bugs:
  Bug #1070774 in MAAS: "The hostname of a node can still be changed once the node is in use"
  https://bugs.launchpad.net/maas/+bug/1070774

For more details, see:
https://code.launchpad.net/~jtv/maas/1.2-bug-1070774/+merge/131612

As discussed with Raphaël.  Meant to be forward-ported to trunk as well.

Jeroen
-- 
https://code.launchpad.net/~jtv/maas/1.2-bug-1070774/+merge/131612
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/maas/1.2-bug-1070774 into lp:maas/1.2.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-10-26 10:20:19 +0000
+++ src/maasserver/api.py	2012-10-29 11:09:32 +0000
@@ -503,10 +503,10 @@
             The default is 'false'.
         :type power_parameters_skip_validation: basestring
         """
-
         node = Node.objects.get_node_or_404(
             system_id=system_id, user=request.user, perm=NODE_PERMISSION.EDIT)
         data = get_overrided_query_dict(model_to_dict(node), request.data)
+
         Form = get_node_edit_form(request.user)
         form = Form(data, instance=node)
         if form.is_valid():

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2012-10-04 13:00:54 +0000
+++ src/maasserver/forms.py	2012-10-29 11:09:32 +0000
@@ -61,6 +61,7 @@
     DISTRO_SERIES_CHOICES,
     NODE_AFTER_COMMISSIONING_ACTION,
     NODE_AFTER_COMMISSIONING_ACTION_CHOICES,
+    NODE_STATUS,
     NODEGROUPINTERFACE_MANAGEMENT,
     NODEGROUPINTERFACE_MANAGEMENT_CHOICES,
     )
@@ -119,6 +120,19 @@
             self.fields['nodegroup'] = NodeGroupFormField(
                 required=False, empty_label="Default (master)")
 
+    def clean_hostname(self):
+        # Don't allow the hostname to be changed if the node is
+        # currently allocated.  Juju knows the node by its old name, so
+        # changing the name would confuse things.
+        hostname = self.instance.hostname
+        status = self.instance.status
+        new_hostname = self.cleaned_data.get('hostname', hostname)
+        if new_hostname != hostname and status == NODE_STATUS.ALLOCATED:
+            raise ValidationError(
+                "Can't change hostname to %s: node is in use." % new_hostname)
+
+        return new_hostname
+
     after_commissioning_action = forms.TypedChoiceField(
         label="After commissioning",
         choices=NODE_AFTER_COMMISSIONING_ACTION_CHOICES, required=False,
@@ -284,7 +298,7 @@
         exclude.remove('user')
         try:
             self.instance.validate_unique(exclude=exclude)
-        except ValidationError, e:
+        except ValidationError as e:
             # Publish this error as a 'key' error rather than a 'general'
             # error because only the 'key' errors are displayed on the
             # 'add key' form.

=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2012-10-07 11:00:05 +0000
+++ src/maasserver/tests/test_forms.py	2012-10-29 11:09:32 +0000
@@ -334,6 +334,48 @@
             after_commissioning_action, node.after_commissioning_action)
         self.assertEqual(power_type, node.power_type)
 
+    def test_AdminNodeForm_refuses_to_update_hostname_on_allocated_node(self):
+        old_name = factory.make_name('old-hostname')
+        new_name = factory.make_name('new-hostname')
+        node = factory.make_node(
+            hostname=old_name, status=NODE_STATUS.ALLOCATED)
+        form = AdminNodeForm(
+            data={
+                'hostname': new_name,
+                'architecture': node.architecture,
+                },
+            instance=node)
+        self.assertFalse(form.is_valid())
+        self.assertEqual(
+            ["Can't change hostname to %s: node is in use." % new_name],
+            form._errors['hostname'])
+
+    def test_AdminNodeForm_accepts_unchanged_hostname_on_allocated_node(self):
+        old_name = factory.make_name('old-hostname')
+        node = factory.make_node(
+            hostname=old_name, status=NODE_STATUS.ALLOCATED)
+        form = AdminNodeForm(
+            data={
+                'hostname': old_name,
+                'architecture': node.architecture,
+            },
+            instance=node)
+        self.assertTrue(form.is_valid(), form._errors)
+        form.save()
+        self.assertEqual(old_name, reload_object(node).hostname)
+
+    def test_AdminNodeForm_accepts_omitted_hostname_on_allocated_node(self):
+        node = factory.make_node(status=NODE_STATUS.ALLOCATED)
+        old_name = node.hostname
+        form = AdminNodeForm(
+            data={
+                'architecture': node.architecture,
+                },
+            instance=node)
+        self.assertTrue(form.is_valid())
+        form.save()
+        self.assertEqual(old_name, reload_object(node).hostname)
+
     def test_remove_None_values_removes_None_values_in_dict(self):
         random_input = factory.getRandomString()
         self.assertEqual(