Merge lp:~julian-edwards/maas/cluster-name-override-bug-1380805 into lp:~maas-committers/maas/trunk

[Julian Edwards]

Don't default cluster_name in NodeGroupDefineForm if it's already set. This is possibly the cause of clusters getting unilaterally renamed as part of the new cluster registration protocol for RPC when upgrading to 1.7.

The fix depends on the "instance" being present in the form; I'm testing this now in the real world to see if that's the case, but it will take an hour or more to install a fresh 1.5 and then upgrade to a test 1.7 package.

[Julian Edwards (julian-edwards)]

Oh well I just tested this, and after upgrade the cluster name was empty! So this is clearly the wrong approach, but I am a little stumped. I'll wait for rvb to get here as I am still missing info on how rpc-based cluster registration works nowadays.

[Gavin Panella (allenap)]

See in maasserver.rpc.clusters.register_cluster:

def register_cluster(uuid, name=None, domain=None, networks=None, url=None):
    ...
    # Massage the data so that we can pass it into NodeGroupDefineForm.
    data = {
        "cluster_name": name,
        "name": domain,
        "uuid": uuid,
    }

I suggest only including `cluster_name` (and `name` too, thought that's
not directly relevant to this bug) when it's not None, i.e.:

    data = {"uuid": uuid}
    if name is not None:
        data["cluster_name"] = name
    if domain is not None:
        data["name"] = domain

I'm not 100% certain that'll work, but it seems worth trying.

review: Needs Fixing

[Raphaël Badin (rvb)]

review: Approve

[Raphaël Badin (rvb)]

review: Abstain

[Gavin Panella (allenap)]

I think lp:~allenap/maas/cluster-name-override--bug-1380805 has superseded this, but maybe there's a need for this too?

review: Abstain

[Julian Edwards (julian-edwards)]

On Tuesday 14 Oct 2014 16:00:32 you wrote:
> Review: Abstain
>
> I think lp:~allenap/maas/cluster-name-override--bug-1380805 has superseded
> this, but maybe there's a need for this too?

It's possible there's a need but does anyone remember why it was done like
that in the first place? There's an intriguing comment in the docstring:

    This form can create a new `NodeGroup`, or in the case where a cluster
    automatically becomes the master, updating an existing one.

Unmerged revisions

3248. By Julian Edwards on 2014-10-14

add back defaulting behaviour if the name doesn't exist yet

3247. By Julian Edwards on 2014-10-14

Don't default cluster_name as it can override the existing name

3246. By Julian Edwards on 2014-10-14

Failing test

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2014-10-10 04:55:03 +0000
+++ src/maasserver/forms.py	2014-10-14 01:36:02 +0000
@@ -1650,6 +1650,10 @@
 
     def clean(self):
         cleaned_data = super(NodeGroupDefineForm, self).clean()
+        if self.instance.cluster_name:
+            # Deliberately vague check, could be None or empty.
+            # Don't override any existing cluster_name.
+            return cleaned_data
         cluster_name = cleaned_data.get("cluster_name")
         uuid = cleaned_data.get("uuid")
         if uuid and not cluster_name:

=== modified file 'src/maasserver/tests/test_forms_nodegroup.py'
--- src/maasserver/tests/test_forms_nodegroup.py	2014-09-19 04:49:07 +0000
+++ src/maasserver/tests/test_forms_nodegroup.py	2014-10-14 01:36:02 +0000
@@ -227,6 +227,17 @@
         nodegroup = form.save()
         self.assertIn(uuid, nodegroup.cluster_name)
 
+    def test_does_not_override_existing_cluster_name(self):
+        cluster_name = factory.make_name('cluster_name')
+        cluster = factory.make_NodeGroup(cluster_name=cluster_name)
+        form = NodeGroupDefineForm(
+            instance=cluster,
+            status=NODEGROUP_STATUS.ACCEPTED,
+            data={'name': cluster.name, 'uuid': cluster.uuid})
+        self.assertTrue(form.is_valid(), form._errors)
+        new_cluster = form.save()
+        self.assertEqual(cluster_name, new_cluster.cluster_name)
+
     def test_populates_cluster_name(self):
         cluster_name = factory.make_name('cluster_name')
         uuid = factory.make_UUID()