launchpad-reviewers team mailing list archive

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

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

Commit message:
Split domain name from node's hostname when composing its kernel parameters (and leave out domain name if omitted).  Node.hostname includes the domain name.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1059044 in MAAS: "provisioningserver/kernel_opts.py has bad hostname and domain"
  https://bugs.launchpad.net/maas/+bug/1059044

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

There hasn't been any chance to pre-imp this first.  I'm still angling for confirmation from the server team that this is a sane approach.  Putting it up for review so that it can land expeditiously if there are no surprises.


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

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-09-29 21:27:52 +0000
+++ src/maasserver/api.py	2012-10-01 09:21:41 +0000
@@ -1529,7 +1529,14 @@
         series = node.get_distro_series()
 
     purpose = get_boot_purpose(node)
-    domain = 'local.lan'  # TODO: This is probably not enough!
+
+    host_and_domain = hostname.split('.', 1)
+    if len(host_and_domain) == 1:
+        [hostname] = host_and_domain
+        domain = None
+    else:
+        hostname, domain = host_and_domain
+
     server_address = get_maas_facing_server_address()
 
     params = KernelParameters(

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2012-09-28 14:49:41 +0000
+++ src/maasserver/models/node.py	2012-10-01 09:21:41 +0000
@@ -389,6 +389,7 @@
         max_length=41, unique=True, default=generate_node_system_id,
         editable=False)
 
+    # Fully-qualified host name (so including the domain).
     hostname = CharField(max_length=255, default='', blank=True)
 
     status = IntegerField(
@@ -585,6 +586,10 @@
         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.
+        """
         mac_hostname = mac_address.replace(':', '').lower()
         domain = Config.objects.get_config("enlistment_domain")
         domain = domain.strip("." + whitespace)

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-09-29 21:27:52 +0000
+++ src/maasserver/tests/test_api.py	2012-10-01 09:21:41 +0000
@@ -2767,6 +2767,26 @@
         observed_arch = params_out["arch"], params_out["subarch"]
         self.assertEqual(expected_arch, observed_arch)
 
+    def test_pxeconfig_splits_domain_from_hostname(self):
+        host = factory.make_name('host')
+        domain = factory.make_name('domain')
+        full_hostname = '.'.join([host, domain])
+        node = factory.make_node(hostname=full_hostname)
+        mac = factory.make_mac_address(node=node)
+        pxe_config = self.get_pxeconfig(params={'mac': mac.mac_address})
+        self.assertEqual(
+            (host, domain),
+            (pxe_config.get('hostname'), pxe_config.get('domain')))
+
+    def test_pxeconfig_omits_domain_if_not_included_in_hostname(self):
+        hostname = factory.make_name('host')
+        node = factory.make_node(hostname=hostname)
+        mac = factory.make_mac_address(node=node)
+        pxe_config = self.get_pxeconfig(params={'mac': mac.mac_address})
+        self.assertEqual(
+            (hostname, None),
+            (pxe_config.get('hostname'), pxe_config.get('domain')))
+
     def get_without_param(self, param):
         """Request a `pxeconfig()` response, but omit `param` from request."""
         params = self.get_params()

=== modified file 'src/provisioningserver/kernel_opts.py'
--- src/provisioningserver/kernel_opts.py	2012-09-25 16:22:41 +0000
+++ src/provisioningserver/kernel_opts.py	2012-10-01 09:21:41 +0000
@@ -107,6 +107,19 @@
     return name
 
 
+def compose_hostname_opts(params):
+    """Return list of hostname/domain options based on `params`.
+
+    The domain is omitted if `params` does not include it.
+    """
+    options = [
+        'hostname=%s' % params.hostname,
+        ]
+    if params.domain is not None:
+        options.append('domain=%s' % params.domain)
+    return options
+
+
 def compose_purpose_opts(params):
     """Return the list of the purpose-specific kernel options."""
     if params.purpose == "commissioning":
@@ -119,10 +132,6 @@
             "iscsi_target_ip=%s" % params.fs_host,
             "iscsi_target_port=3260",
             "iscsi_initiator=%s" % params.hostname,
-            ## TODO(smoser): remove hostname after an ephemeral image is
-            ## released with cloud-initramfs-dyn-netconf. see LP: #1046405 for
-            ## more info. instead use the updated 'ip=' line below.
-            "hostname=%s" % params.hostname,
             # Read by klibc 'ipconfig' in initramfs.
             "ip=dhcp",  # TODO(smoser) remove this
             # "ip=::::%s:BOOTIF" % params.hostname, # TODO(smoser) use this
@@ -132,18 +141,19 @@
             "overlayroot=tmpfs",
             # Read by cloud-init.
             "cloud-config-url=%s" % params.preseed_url,
-            ]
+            ## TODO(smoser): remove hostname after an ephemeral image is
+            ## released with cloud-initramfs-dyn-netconf. see LP: #1046405 for
+            ## more info. instead use the updated 'ip=' line above.
+            ] + compose_hostname_opts(params)
     else:
         # These are options used by the Debian Installer.
         return [
             "netcfg/choose_interface=auto",
-            "hostname=%s" % params.hostname,
-            "domain=%s" % params.domain,
             # Use the text installer, display only critical messages.
             "text priority=critical",
             compose_preseed_opt(params.preseed_url),
             compose_locale_opt(),
-            ]
+            ] + compose_hostname_opts(params)
 
 
 def compose_arch_opts(params):

=== modified file 'src/provisioningserver/tests/test_kernel_opts.py'
--- src/provisioningserver/tests/test_kernel_opts.py	2012-09-25 14:29:47 +0000
+++ src/provisioningserver/tests/test_kernel_opts.py	2012-10-01 09:21:41 +0000
@@ -80,6 +80,13 @@
                 "domain=%s" % params.domain,
                 ]))
 
+    def test_install_compose_kernel_command_line_omits_domain_if_omitted(self):
+        params = make_kernel_parameters(purpose="install")._replace(
+            domain=None)
+        kernel_command_line = compose_kernel_command_line(params)
+        self.assertIn("hostname=%s" % params.hostname, kernel_command_line)
+        self.assertNotIn('domain=', kernel_command_line)
+
     def test_install_compose_kernel_command_line_includes_locale(self):
         params = make_kernel_parameters(purpose="install")
         locale = "en_US"