Merge ~ack/maas:virsh-pod-same-hostname into maas:master

[Alberto Donato]

LP: #1716328 - check if pod machine hostname is already in use

[MAAS Lander (maas-lander)]

UNIT TESTS
-b virsh-pod-same-hostname lp:~ack/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 98f36263d0458d5f9b6eaa0a1d96f47f81a03db2

review: Approve

[Blake Rouse (blake-rouse)]

Code looks good, just a comment that should be changed before landing.

review: Approve

[Andres Rodriguez (andreserl)]

Isn’t “format” the python3 way of dealing with this though?

On Mon, Oct 30, 2017 at 12:54 PM Alberto Donato <
<email address hidden>> wrote:

>
>
> Diff comments:
>
> > diff --git a/src/maasserver/forms/pods.py b/src/maasserver/forms/pods.py
> > index 03b8f06..bedf9a9 100644
> > --- a/src/maasserver/forms/pods.py
> > +++ b/src/maasserver/forms/pods.py
> > @@ -306,7 +307,15 @@ class ComposeMachineForm(forms.Form):
> > else:
> > self.fields['cpu_speed'] = IntegerField(
> > min_value=300, required=False)
> > - self.fields['hostname'] = CharField(required=False)
> > +
> > + def duplicated_hostname(hostname):
> > + if Node.objects.filter(hostname=hostname).exists():
> > + raise ValidationError(
> > + 'Machine with hostname "{}" already exists'.format(
>
> Done. I switched to %s, but I think we should consider using format() for
> new messages (and progressively update code when we touch it. format() is
> nicer to read and more flexible.
>
> > + hostname))
> > +
> > + self.fields['hostname'] = CharField(
> > + required=False, validators=[duplicated_hostname])
> > self.initial['hostname'] = make_unique_hostname()
> > self.fields['domain'] = ModelChoiceField(
> > required=False, queryset=Domain.objects.all())
>
>
> --
> https://code.launchpad.net/~ack/maas/+git/maas/+merge/332981
> You are reviewing the proposed merge of ~ack/maas:virsh-pod-same-hostname
> into maas:master.
>
--
Andres Rodriguez
Engineering Manager, MAAS
Canonical USA, Inc.

[MAAS Lander (maas-lander)]

LANDING
-b virsh-pod-same-hostname lp:~ack/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/587/consoleText

[MAAS Lander (maas-lander)]

UNIT TESTS
-b virsh-pod-same-hostname lp:~ack/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/589/console
COMMIT: 3a3db0764dca64282c9ca58898af31677e345e00

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b virsh-pod-same-hostname lp:~ack/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: ff5f2fe70fc6b82997baeba300449766f4e068ec

review: Approve

[Alberto Donato (ack)]

@andres yes, format() is the preferred way in python3

diff --git a/src/maasserver/forms/pods.py b/src/maasserver/forms/pods.py
index 03b8f06..334bb88 100644
--- a/src/maasserver/forms/pods.py
+++ b/src/maasserver/forms/pods.py
@@ -38,6 +38,7 @@ from maasserver.models import (
     BMCRoutableRackControllerRelationship,
     Domain,
     Machine,
+    Node,
     Pod,
     RackController,
     Zone,
@@ -306,7 +307,14 @@ class ComposeMachineForm(forms.Form):
         else:
             self.fields['cpu_speed'] = IntegerField(
                 min_value=300, required=False)
-        self.fields['hostname'] = CharField(required=False)
+
+        def duplicated_hostname(hostname):
+            if Node.objects.filter(hostname=hostname).exists():
+                raise ValidationError(
+                    'Node with hostname "%s" already exists' % hostname)
+
+        self.fields['hostname'] = CharField(
+            required=False, validators=[duplicated_hostname])
         self.initial['hostname'] = make_unique_hostname()
         self.fields['domain'] = ModelChoiceField(
             required=False, queryset=Domain.objects.all())
diff --git a/src/maasserver/forms/tests/test_pods.py b/src/maasserver/forms/tests/test_pods.py
index 9731d18..9b530bc 100644
--- a/src/maasserver/forms/tests/test_pods.py
+++ b/src/maasserver/forms/tests/test_pods.py
@@ -751,6 +751,19 @@ class TestComposeMachineForm(MAASTransactionServerTestCase):
                 cpu_speed=Equals(300))))
         self.assertThat(mock_commissioning, MockCalledOnce())
 
+    def test__compose_duplicated_hostname(self):
+        factory.make_Node(hostname='test')
+
+        request = MagicMock()
+        pod = make_pod_with_hints()
+
+        form = ComposeMachineForm(
+            data={'hostname': 'test'}, request=request, pod=pod)
+        self.assertFalse(form.is_valid())
+        self.assertEqual(
+            {'hostname': ['Node with hostname "test" already exists']},
+            form.errors)
+
     def test__compose_without_commissioning(self):
         request = MagicMock()
         pod = make_pod_with_hints()
diff --git a/src/maasserver/models/bmc.py b/src/maasserver/models/bmc.py
index 3fcc8b9..5cc286c 100644
--- a/src/maasserver/models/bmc.py
+++ b/src/maasserver/models/bmc.py
@@ -9,7 +9,6 @@ __all__ = [
 
 from functools import partial
 import re
-import string
 
 from django.contrib.postgres.fields import ArrayField
 from django.core.exceptions import ValidationError
@@ -433,6 +432,8 @@ class Pod(BMC):
 
     objects = PodManager()
 
+    _machine_name_re = re.compile(r'[a-z][a-z0-9-]+$', flags=re.I)
+
     def __init__(self, *args, **kwargs):
         super(Pod, self).__init__(
             bmc_type=BMC_TYPE.POD, *args, **kwargs)
@@ -562,14 +563,12 @@ class Pod(BMC):
 
         # Check to see if discovered machine's hostname is legal and unique.
         if discovered_machine.hostname:
-            for char in discovered_machine.hostname:
-                if char not in (string.ascii_letters + string.digits + '-'):
-                    discovered_machine.hostname = None
-                    break
-        if discovered_machine.hostname:
             if Node.objects.filter(
                     hostname=discovered_machine.hostname).exists():
                 discovered_machine.hostname = None
+            elif not self._machine_name_re.match(
+                    discovered_machine.hostname):
+                discovered_machine.hostname = None
 
         # Create the machine.
         machine = Machine(
diff --git a/src/maasserver/models/tests/test_bmc.py b/src/maasserver/models/tests/test_bmc.py
index 46c913b..2e2477f 100644
--- a/src/maasserver/models/tests/test_bmc.py
+++ b/src/maasserver/models/tests/test_bmc.py
@@ -806,6 +806,22 @@ class TestPod(MAASServerTestCase):
         # that will cause a database exception.
         pod.create_machine(discovered_machine, factory.make_User())
 
+    def test_create_machine_invalid_hostname(self):
+        discovered_machine = self.make_discovered_machine()
+        discovered_machine.hostname = 'invalid_name'
+        self.patch(Machine, "set_default_storage_layout")
+        self.patch(Machine, "set_initial_networking_configuration")
+        self.patch(Machine, "start_commissioning")
+        fabric = factory.make_Fabric()
+        factory.make_VLAN(
+            fabric=fabric, dhcp_on=True,
+            primary_rack=factory.make_RackController())
+        pod = factory.make_Pod()
+        # Doesn't raise an exception ensures that the hostname is unique as
+        # that will cause a database exception.
+        machine = pod.create_machine(discovered_machine, factory.make_User())
+        self.assertNotEqual(machine.hostname, 'invalid_name')
+
     def test_sync_pod_creates_new_machines_connected_to_dhcp_vlan(self):
         discovered = self.make_discovered_pod()
         mock_set_default_storage_layout = self.patch(