launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/1.2-bug-1058998-hostnames into lp:maas/1.2.

Commit message:
Backport of maas r 1330

Requested reviews:
  MAAS Maintainers (maas-maintainers)
Related bugs:
  Bug #1058998 in MAAS: "MAAS hostnames should be 5 easily disambiguated characters"
  https://bugs.launchpad.net/maas/+bug/1058998

For more details, see:
https://code.launchpad.net/~rvb/maas/1.2-bug-1058998-hostnames/+merge/134620

Backport of r 1330 (https://code.launchpad.net/~rvb/maas/bug-1070765-hostname2/+merge/132569).
-- 
https://code.launchpad.net/~rvb/maas/1.2-bug-1058998-hostnames/+merge/134620
Your team MAAS Maintainers is requested to review the proposed merge of lp:~rvb/maas/1.2-bug-1058998-hostnames into lp:maas/1.2.

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2012-11-09 18:12:36 +0000
+++ src/maasserver/forms.py	2012-11-16 08:17:26 +0000
@@ -403,7 +403,7 @@
             hostname == "" or
             IP_BASED_HOSTNAME_REGEXP.match(stripped_hostname) != None)
         if generate_hostname:
-            node.set_mac_based_hostname(self.cleaned_data['mac_addresses'][0])
+            node.set_random_hostname()
         return node
 
 

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2012-11-08 09:14:58 +0000
+++ src/maasserver/models/node.py	2012-11-16 08:17:26 +0000
@@ -18,8 +18,14 @@
     "update_hardware_details",
     ]
 
+from itertools import (
+    imap,
+    islice,
+    repeat,
+    )
 import math
 import os
+import random
 from string import whitespace
 from uuid import uuid1
 
@@ -397,6 +403,16 @@
     node.save()
 
 
+# Non-ambiguous characters (i.e. without 'ilousvz1250').
+non_ambiguous_characters = imap(
+    random.choice, repeat('abcdefghjkmnpqrtwxy346789'))
+
+
+def generate_hostname(size):
+    """Generate a hostname using only non-ambiguous characters."""
+    return "".join(islice(non_ambiguous_characters, size))
+
+
 class Node(CleanSave, TimestampedModel):
     """A `Node` represents a physical machine used by the MAAS Server.
 
@@ -655,19 +671,30 @@
 
         super(Node, self).delete()
 
-    def set_mac_based_hostname(self, mac_address):
-        """Set default `hostname` based on `mac_address`
-
-        The hostname will include the `enlistment_domain` if set.
+    def set_random_hostname(self):
+        """Set 5 character `hostname` using non-ambiguous characters.
+
+        Using 5 letters from the set 'abcdefghjkmnpqrtwxy346789' we get
+        9,765,625 combinations (pow(25, 5)).
+
+        Note that having a hostname starting with a number is perfectly
+        valid, see
+        http://en.wikipedia.org/wiki/Hostname#Restrictions_on_valid_host_names
         """
-        mac_hostname = mac_address.replace(':', '').lower()
         domain = Config.objects.get_config("enlistment_domain")
         domain = domain.strip("." + whitespace)
-        if len(domain) > 0:
-            self.hostname = "node-%s.%s" % (mac_hostname, domain)
-        else:
-            self.hostname = "node-%s" % mac_hostname
-        self.save()
+        while True:
+            new_hostname = generate_hostname(5)
+            if len(domain) > 0:
+                self.hostname = "%s.%s" % (new_hostname, domain)
+            else:
+                self.hostname = "%s" % new_hostname
+            try:
+                self.save()
+            except ValidationError:
+                pass
+            else:
+                break
 
     def get_effective_power_type(self):
         """Get power-type to use for this node.

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-11-08 09:54:37 +0000
+++ src/maasserver/tests/test_api.py	2012-11-16 08:17:26 +0000
@@ -106,7 +106,10 @@
     TestCase,
     )
 from maasserver.tests.test_forms import make_interface_settings
-from maasserver.utils import map_enum
+from maasserver.utils import (
+    map_enum,
+    strip_domain,
+    )
 from maasserver.utils.orm import get_one
 from maasserver.worker_user import get_worker_user
 from maastesting.celery import CeleryFixture
@@ -555,11 +558,11 @@
             {
                 'op': 'new',
                 'architecture': architecture,
-                'mac_addresses': ['aa:BB:cc:dd:ee:ff', '22:bb:cc:dd:ee:ff'],
+                'mac_addresses': [factory.getRandomMACAddress()],
             })
         node = Node.objects.get(
             system_id=json.loads(response.content)['system_id'])
-        self.assertEqual('node-aabbccddeeff.local', node.hostname)
+        self.assertEqual(5, len(strip_domain(node.hostname)))
 
     def test_POST_fails_without_operation(self):
         # If there is no operation ('op=operation_name') specified in the

=== modified file 'src/maasserver/tests/test_node.py'
--- src/maasserver/tests/test_node.py	2012-11-08 09:14:58 +0000
+++ src/maasserver/tests/test_node.py	2012-11-16 08:17:26 +0000
@@ -13,6 +13,7 @@
 __all__ = []
 
 from datetime import timedelta
+import random
 
 from django.conf import settings
 from django.core.exceptions import (
@@ -36,7 +37,10 @@
     Node,
     node as node_module,
     )
-from maasserver.models.node import NODE_TRANSITIONS
+from maasserver.models.node import (
+    generate_hostname,
+    NODE_TRANSITIONS,
+    )
 from maasserver.models.user import create_auth_token
 from maasserver.testing import reload_object
 from maasserver.testing.factory import factory
@@ -45,6 +49,7 @@
     ignore_unused,
     map_enum,
     )
+from maastesting.testcase import TestCase as DjangoLessTestCase
 from metadataserver.models import (
     NodeCommissionResult,
     NodeUserData,
@@ -52,12 +57,34 @@
 from provisioningserver.enum import POWER_TYPE
 from provisioningserver.power.poweraction import PowerAction
 from testtools.matchers import (
+    AllMatch,
+    Contains,
     Equals,
     FileContains,
+    MatchesAll,
     MatchesListwise,
+    Not,
     )
 
 
+class UtilitiesTest(DjangoLessTestCase):
+
+    def test_generate_hostname_does_not_contain_ambiguous_chars(self):
+        ambiguous_chars = 'ilousvz1250'
+        hostnames = [generate_hostname(5) for i in range(200)]
+        does_not_contain_chars_matcher = (
+            MatchesAll(*[Not(Contains(char)) for char in ambiguous_chars]))
+        self.assertThat(
+            hostnames, AllMatch(does_not_contain_chars_matcher))
+
+    def test_generate_hostname_uses_size(self):
+        sizes = [
+            random.randint(1, 10), random.randint(1, 10),
+            random.randint(1, 10)]
+        hostnames = [generate_hostname(size) for size in sizes]
+        self.assertEqual(sizes, [len(hostname) for hostname in hostnames])
+
+
 class NodeTest(TestCase):
 
     def test_system_id(self):
@@ -182,38 +209,25 @@
         node.delete()
         self.assertEqual(2, mocked_apply_async.call_count)
 
-    def test_set_mac_based_hostname_default_enlistment_domain(self):
-        # The enlistment domain defaults to `local`.
-        node = factory.make_node()
-        node.set_mac_based_hostname('AA:BB:CC:DD:EE:FF')
-        hostname = 'node-aabbccddeeff.local'
-        self.assertEqual(hostname, node.hostname)
-
-    def test_set_mac_based_hostname_alt_enlistment_domain(self):
-        # A non-default enlistment domain can be specified.
-        Config.objects.set_config("enlistment_domain", "example.com")
-        node = factory.make_node()
-        node.set_mac_based_hostname('AA:BB:CC:DD:EE:FF')
-        hostname = 'node-aabbccddeeff.example.com'
-        self.assertEqual(hostname, node.hostname)
-
-    def test_set_mac_based_hostname_cleaning_enlistment_domain(self):
-        # Leading and trailing dots and whitespace are cleaned from the
-        # configured enlistment domain before it's joined to the hostname.
-        Config.objects.set_config("enlistment_domain", " .example.com. ")
-        node = factory.make_node()
-        node.set_mac_based_hostname('AA:BB:CC:DD:EE:FF')
-        hostname = 'node-aabbccddeeff.example.com'
-        self.assertEqual(hostname, node.hostname)
-
-    def test_set_mac_based_hostname_no_enlistment_domain(self):
-        # The enlistment domain can be set to the empty string and
-        # set_mac_based_hostname sets a hostname with no domain.
-        Config.objects.set_config("enlistment_domain", "")
-        node = factory.make_node()
-        node.set_mac_based_hostname('AA:BB:CC:DD:EE:FF')
-        hostname = 'node-aabbccddeeff'
-        self.assertEqual(hostname, node.hostname)
+    def test_set_random_hostname_set_hostname(self):
+        # Blank out enlistment_domain.
+        Config.objects.set_config("enlistment_domain", '')
+        node = factory.make_node('test' * 10)
+        node.set_random_hostname()
+        self.assertEqual(5, len(node.hostname))
+
+    def test_set_random_hostname_checks_hostname_existence(self):
+        Config.objects.set_config("enlistment_domain", '')
+        existing_node = factory.make_node(hostname='hostname')
+
+        hostnames = [existing_node.hostname, "new_hostname"]
+        self.patch(
+            node_module, "generate_hostname",
+            lambda size: hostnames.pop(0))
+
+        node = factory.make_node()
+        node.set_random_hostname()
+        self.assertEqual('new_hostname', node.hostname)
 
     def test_get_effective_power_type_defaults_to_config(self):
         power_types = list(map_enum(POWER_TYPE).values())