Merge lp:~ltrager/maas/rootfs_over_http into lp:~maas-committers/maas/trunk

[Lee Trager]

When booting the ephemeral environment, fetch the rootfs over HTTP instead of using iSCSI.

[Mike Pontillo (mpontillo)]

This looks like a nice improvement! A few questions below (mainly around IPv6 support; please add specific tests for that, too).

Somewhat related question: do we ever present a hostname in the fs_host, or is it always an IP address? (I assume it's just an IP address, and I think that's okay.)

We should also make sure we don't present an IPv6 link-local address as the fs_host (if we aren't doing that already).

review: Needs Information

[Lee Trager (ltrager)]

Thanks for the review. fs_host was being used by the ISCSI code so I assume it properly works with IPv6. I agree we need to do some IPv6 testing with this but I don't have access to an IPv6 enabled environment right now.

[Mike Pontillo (mpontillo)]

Thanks for the replies. In my view, only one thing needs to be done for this to land: add a separate IPv6 test case and ensure that IPv6 addresses have brackets surrounding them, while IPv4 addresses do not.

review: Needs Fixing

[Lee Trager (ltrager)]

Thanks for the review. I've updated the branch to ensure that [] are wrapped around IPv6 addresses.

[Mike Pontillo (mpontillo)]

Thanks for the fixes. Ship it!

review: Approve

[Andres Rodriguez (andreserl)]

To follow on the discussion:

This needs to have a feature flag.

review: Needs Fixing

[Lee Trager (ltrager)]

Booting over iSCSI is now the default. To enable booting over HTTP use the MAAS configuration option http_boot.

[Blake Rouse (blake-rouse)]

This is okay as a global setting allow it to be changed over the API, which is nice. But this needs to be undocumented and removed before release. So this option should really be short lived.

review: Approve

=== modified file 'src/maasserver/forms/settings.py'
--- src/maasserver/forms/settings.py	2017-06-15 14:27:01 +0000
+++ src/maasserver/forms/settings.py	2017-06-21 22:04:06 +0000
@@ -1,4 +1,4 @@
-# Copyright 2013-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2013-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Configuration items definition and utilities."""
@@ -538,6 +538,14 @@
             'min_value': 1,
         },
     },
+    'http_boot': {
+        'default': False,
+        'form': forms.BooleanField,
+        'form_kwargs': {
+            'label': "When true all ephemeral environments boot over HTTP.",
+            'required': False
+        }
+    },
 }
 
 

=== modified file 'src/maasserver/models/config.py'
--- src/maasserver/models/config.py	2017-05-29 13:44:04 +0000
+++ src/maasserver/models/config.py	2017-06-21 22:04:06 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Configuration items."""
@@ -101,7 +101,8 @@
         'max_node_testing_results': 10,
         'max_node_installation_results': 1,
         # Notifications.
-        'subnet_ip_exhaustion_threshold_count': 16
+        'subnet_ip_exhaustion_threshold_count': 16,
+        'http_boot': False,
     }
 
 

=== modified file 'src/maasserver/rpc/boot.py'
--- src/maasserver/rpc/boot.py	2017-06-01 06:16:00 +0000
+++ src/maasserver/rpc/boot.py	2017-06-21 22:04:06 +0000
@@ -326,6 +326,7 @@
         "fs_host": local_ip,
         "log_host": server_host,
         "extra_opts": '' if extra_kernel_opts is None else extra_kernel_opts,
+        "http_boot": Config.objects.get_config('http_boot'),
     }
     if machine is not None:
         params["system_id"] = machine.system_id

=== modified file 'src/provisioningserver/kernel_opts.py'
--- src/provisioningserver/kernel_opts.py	2017-04-12 22:00:36 +0000
+++ src/provisioningserver/kernel_opts.py	2017-06-21 22:04:06 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Generate kernel command-line options for inclusion in PXE configs."""
@@ -6,7 +6,6 @@
 __all__ = [
     'compose_kernel_command_line',
     'KernelParameters',
-    'prefix_target_name',
     ]
 
 from collections import namedtuple
@@ -43,6 +42,7 @@
         "fs_host",      # Host/IP on which ephemeral filesystems are hosted.
         "extra_opts",   # String of extra options to supply, will be appended
                         # verbatim to the kernel command line
+        "http_boot",    # Whether or not to boot over HTTP.
         ))
 
 
@@ -52,21 +52,6 @@
     __call__ = KernelParametersBase._replace
 
 
-def compose_preseed_opt(preseed_url):
-    """Compose a kernel option for preseed URL.
-
-    :param preseed_url: The URL from which a preseed can be fetched.
-    """
-    # See https://help.ubuntu.com/12.04/installation-guide
-    #   /i386/preseed-using.html#preseed-auto
-    return "auto url=%s" % preseed_url
-
-
-def compose_locale_opt():
-    locale = 'en_US'
-    return "locale=%s" % locale
-
-
 def compose_logging_opts(log_host):
     return [
         'log_host=%s' % log_host,
@@ -101,19 +86,6 @@
         )
 
 
-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 prefix_target_name(name):
     """Prefix an ISCSI target name with the standard target-name prefix."""
     return "%s:%s" % (ISCSI_TARGET_NAME_PREFIX, name)
@@ -121,8 +93,18 @@
 
 def compose_purpose_opts(params):
     """Return the list of the purpose-specific kernel options."""
-    if params.purpose in ["commissioning", "xinstall", "enlist"]:
-        # These are kernel parameters read by the ephemeral environment.
+    if params.http_boot:
+        kernel_params = [
+            "root=squash:http://%s:5248/images/%s/%s/%s/%s/%s/squashfs" % (
+                (
+                    '[%s]' % params.fs_host
+                    if IPAddress(params.fs_host).version == 6
+                    else params.fs_host
+                ),
+                params.osystem, params.arch, params.subarch, params.release,
+                params.label),
+        ]
+    else:
         tname = prefix_target_name(
             get_ephemeral_name(
                 params.osystem, params.arch, params.subarch,
@@ -133,43 +115,38 @@
             "iscsi_target_ip=%s" % params.fs_host,
             "iscsi_target_port=3260",
             "iscsi_initiator=%s" % params.hostname,
-            # Read by cloud-initramfs-dyn-netconf initramfs-tools networking
-            # configuration in the initramfs.  Choose IPv4 or IPv6 based on the
-            # family of fs_host.  If BOOTIF is set, IPv6 config uses that
-            # exclusively.
-            (
-                "ip=::::%s:BOOTIF" % params.hostname
-                if IPAddress(params.fs_host).version == 4 else "ip=off"
-            ),
-            (
-                "ip6=dhcp"
-                if IPAddress(params.fs_host).version == 6 else "ip6=off"
-            ),
             # kernel / udev name iscsi devices with this path
-            "ro root=/dev/disk/by-path/ip-%s:%s-iscsi-%s-lun-1" % (
+            "root=/dev/disk/by-path/ip-%s:%s-iscsi-%s-lun-1" % (
                 params.fs_host, "3260", tname),
-            # Read by overlayroot package.
-            "overlayroot=tmpfs",
-            # LP:1533822 - Disable reading overlay data from disk.
-            "overlayroot_cfgdisk=disabled",
-            # Select the MAAS datasource by default.
-            "cc:{'datasource_list': ['MAAS']}end_cc",
-            # Read by cloud-init.
-            "cloud-config-url=%s" % params.preseed_url,
-            # Disable apparmor in the ephemeral environment. This addresses
-            # MAAS bug LP: #1677336 due to LP: #1408106
-            "apparmor=0",
-            ]
-        return kernel_params
-    else:
-        # These are options used by the Debian Installer.
-        return [
-            "netcfg/choose_interface=auto",
-            # Use the text installer, display only critical messages.
-            "text priority=critical",
-            compose_preseed_opt(params.preseed_url),
-            compose_locale_opt(),
-            ] + compose_hostname_opts(params)
+        ]
+
+    kernel_params += [
+        "ro",
+        # Read by cloud-initramfs-dyn-netconf initramfs-tools networking
+        # configuration in the initramfs.  Choose IPv4 or IPv6 based on the
+        # family of fs_host.  If BOOTIF is set, IPv6 config uses that
+        # exclusively.
+        (
+            "ip=::::%s:BOOTIF" % params.hostname
+            if IPAddress(params.fs_host).version == 4 else "ip=off"
+        ),
+        (
+            "ip6=dhcp"
+            if IPAddress(params.fs_host).version == 6 else "ip6=off"
+        ),
+        # Read by overlayroot package.
+        "overlayroot=tmpfs",
+        # LP:1533822 - Disable reading overlay data from disk.
+        "overlayroot_cfgdisk=disabled",
+        # Select the MAAS datasource by default.
+        "cc:{'datasource_list': ['MAAS']}end_cc",
+        # Read by cloud-init.
+        "cloud-config-url=%s" % params.preseed_url,
+        # Disable apparmor in the ephemeral environment. This addresses
+        # MAAS bug LP: #1677336 due to LP: #1408106
+        "apparmor=0",
+        ]
+    return kernel_params
 
 
 def compose_arch_opts(params):

=== modified file 'src/provisioningserver/rpc/region.py'
--- src/provisioningserver/rpc/region.py	2017-01-28 00:51:47 +0000
+++ src/provisioningserver/rpc/region.py	2017-06-21 22:04:06 +0000
@@ -138,6 +138,7 @@
         (b"log_host", amp.Unicode()),
         (b"extra_opts", amp.Unicode()),
         (b"system_id", amp.Unicode(optional=True)),
+        (b"http_boot", amp.Boolean(optional=True)),
     ]
     errors = {
         BootConfigNoResponse: b"BootConfigNoResponse",

=== modified file 'src/provisioningserver/tests/test_kernel_opts.py'
--- src/provisioningserver/tests/test_kernel_opts.py	2017-04-12 22:00:36 +0000
+++ src/provisioningserver/tests/test_kernel_opts.py	2017-06-21 22:04:06 +0000
@@ -21,14 +21,10 @@
 from provisioningserver.kernel_opts import (
     compose_arch_opts,
     compose_kernel_command_line,
-    compose_preseed_opt,
     CURTIN_KERNEL_CMDLINE_NAME,
     get_curtin_kernel_cmdline_sep,
-    get_ephemeral_name,
     get_last_directory,
-    ISCSI_TARGET_NAME_PREFIX,
     KernelParameters,
-    prefix_target_name,
 )
 from testtools.matchers import (
     Contains,
@@ -51,6 +47,8 @@
         {field: factory.make_name(field)
          for field in KernelParameters._fields
          if field not in parms})
+    if not isinstance(parms['http_boot'], bool):
+        parms['http_boot'] = True
     params = KernelParameters(**parms)
 
     if testcase is not None:
@@ -86,23 +84,6 @@
         self.assertTrue(callable(params))
         self.assertIs(params._replace.__func__, params.__call__.__func__)
 
-    def test_prefix_target_name_adds_prefix(self):
-        prefix = factory.make_name('prefix')
-        target = factory.make_name('tgt')
-        self.patch(kernel_opts, 'ISCSI_TARGET_NAME_PREFIX', prefix)
-
-        self.assertEqual(
-            '%s:%s' % (prefix, target),
-            prefix_target_name(target))
-
-    def test_prefix_target_name_produces_exactly_one_separating_colon(self):
-        target = factory.make_name('tgt')
-
-        full_name = prefix_target_name(target)
-
-        self.assertIn(':' + target, full_name)
-        self.assertNotIn('::' + target, full_name)
-
 
 class TestGetCurtinKernelCmdlineSepTest(MAASTestCase):
 
@@ -138,59 +119,6 @@
         cmdline = compose_kernel_command_line(params)
         self.assertThat(cmdline, Contains("overlayroot_cfgdisk=disabled"))
 
-    def test_compose_kernel_command_line_includes_preseed_url(self):
-        params = self.make_kernel_parameters()
-        self.assertIn(
-            "auto url=%s" % params.preseed_url,
-            compose_kernel_command_line(params))
-
-    def test_install_compose_kernel_command_line_includes_name_domain(self):
-        params = self.make_kernel_parameters(purpose="install")
-        self.assertThat(
-            compose_kernel_command_line(params),
-            ContainsAll([
-                "hostname=%s" % params.hostname,
-                "domain=%s" % params.domain,
-                ]))
-
-    def test_install_compose_kernel_command_line_omits_domain_if_omitted(self):
-        params = self.make_kernel_parameters(purpose="install", 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 = self.make_kernel_parameters(purpose="install")
-        locale = "en_US"
-        self.assertIn(
-            "locale=%s" % locale,
-            compose_kernel_command_line(params))
-
-    def test_install_compose_kernel_command_line_includes_log_settings(self):
-        params = self.make_kernel_parameters(purpose="install")
-        # Port 514 (UDP) is syslog.
-        log_port = "514"
-        self.assertThat(
-            compose_kernel_command_line(params),
-            ContainsAll([
-                "log_host=%s" % params.log_host,
-                "log_port=%s" % log_port,
-                ]))
-
-    def test_install_compose_kernel_command_line_includes_di_settings(self):
-        params = self.make_kernel_parameters(purpose="install")
-        self.assertThat(
-            compose_kernel_command_line(params),
-            Contains("text priority=critical"))
-
-    def test_install_compose_kernel_command_line_inc_purpose_opts(self):
-        # The result of compose_kernel_command_line includes the purpose
-        # options for a non "commissioning" node.
-        params = self.make_kernel_parameters(purpose="install")
-        self.assertIn(
-            "netcfg/choose_interface=auto",
-            compose_kernel_command_line(params))
-
     def test_xinstall_compose_kernel_command_line_inc_purpose_opts4(self):
         # The result of compose_kernel_command_line includes the purpose
         # options for a non "xinstall" node.
@@ -200,8 +128,7 @@
         self.assertThat(
             cmdline,
             ContainsAll([
-                "root=/dev/disk/by-path/ip-",
-                "iscsi_initiator=",
+                "root=squash:http://",
                 "overlayroot=tmpfs",
                 "ip6=off",
                 "ip=::::%s:BOOTIF" % params.hostname]))
@@ -215,8 +142,7 @@
         self.assertThat(
             cmdline,
             ContainsAll([
-                "root=/dev/disk/by-path/ip-",
-                "iscsi_initiator=",
+                "root=squash:http://",
                 "overlayroot=tmpfs",
                 "ip=off",
                 "ip6=dhcp"]))
@@ -242,8 +168,7 @@
         self.assertThat(
             cmdline,
             ContainsAll([
-                "root=/dev/disk/by-path/ip-",
-                "iscsi_initiator=",
+                "root=squash:http://",
                 "overlayroot=tmpfs",
                 "ip6=off",
                 "ip=::::%s:BOOTIF" % params.hostname]))
@@ -257,8 +182,7 @@
         self.assertThat(
             cmdline,
             ContainsAll([
-                "root=/dev/disk/by-path/ip-",
-                "iscsi_initiator=",
+                "root=squash:http://",
                 "overlayroot=tmpfs",
                 "ip=off",
                 "ip6=dhcp"]))
@@ -284,8 +208,7 @@
         self.assertThat(
             cmdline,
             ContainsAll([
-                "root=/dev/disk/by-path/ip-",
-                "iscsi_initiator=",
+                "root=squash:http://",
                 "overlayroot=tmpfs",
                 "ip6=off",
                 "ip=::::%s:BOOTIF" % params.hostname]))
@@ -299,8 +222,7 @@
         self.assertThat(
             cmdline,
             ContainsAll([
-                "root=/dev/disk/by-path/ip-",
-                "iscsi_initiator=",
+                "root=squash:http://",
                 "overlayroot=tmpfs",
                 "ip=off",
                 "ip6=dhcp"]))
@@ -378,39 +300,17 @@
         # The result of compose_kernel_command_line includes the purpose
         # options for a "xinstall" node.
         params = self.make_kernel_parameters(purpose="xinstall")
-        ephemeral_name = get_ephemeral_name(
-            params.osystem, params.arch, params.subarch,
-            params.release, params.label)
         self.assertThat(
             compose_kernel_command_line(params),
-            ContainsAll([
-                "iscsi_target_name=%s:%s" % (
-                    ISCSI_TARGET_NAME_PREFIX, ephemeral_name),
-                "iscsi_target_port=3260",
-                "iscsi_target_ip=%s" % params.fs_host,
-                ]))
+            ContainsAll(["root=squash:http://"]))
 
     def test_compose_kernel_command_line_inc_purpose_opts_comm_node(self):
         # The result of compose_kernel_command_line includes the purpose
         # options for a "commissioning" node.
         params = self.make_kernel_parameters(purpose="commissioning")
-        ephemeral_name = get_ephemeral_name(
-            params.osystem, params.arch, params.subarch,
-            params.release, params.label)
         self.assertThat(
             compose_kernel_command_line(params),
-            ContainsAll([
-                "iscsi_target_name=%s:%s" % (
-                    ISCSI_TARGET_NAME_PREFIX, ephemeral_name),
-                "iscsi_target_port=3260",
-                "iscsi_target_ip=%s" % params.fs_host,
-                ]))
-
-    def test_compose_preseed_kernel_opt_returns_kernel_option(self):
-        dummy_preseed_url = factory.make_name("url")
-        self.assertEqual(
-            "auto url=%s" % dummy_preseed_url,
-            compose_preseed_opt(dummy_preseed_url))
+            ContainsAll(["root=squash:http://"]))
 
     def test_compose_kernel_command_line_inc_arm_specific_option(self):
         params = self.make_kernel_parameters(arch="armhf", subarch="highbank")
@@ -432,3 +332,33 @@
             arch=factory.make_name("arch"),
             subarch=factory.make_name("subarch"))
         self.assertEqual([], compose_arch_opts(params))
+
+    def test_compose_rootfs_over_http_ipv4(self):
+        params = make_kernel_parameters(fs_host=factory.make_ipv4_address())
+        self.assertThat(
+            compose_kernel_command_line(params),
+            ContainsAll([
+                "ro",
+                "root=squash:http://%s:5248/images/%s/%s/%s/%s/%s/squashfs" % (
+                    params.fs_host, params.osystem, params.arch,
+                    params.subarch, params.release, params.label)]))
+
+    def test_compose_rootfs_over_http_ipv6(self):
+        params = make_kernel_parameters(fs_host=factory.make_ipv6_address())
+        self.assertThat(
+            compose_kernel_command_line(params),
+            ContainsAll([
+                "ro",
+                "root=squash:http://[%s]:5248/images/%s/%s/%s/%s/%s/squashfs" %
+                (
+                    params.fs_host, params.osystem, params.arch,
+                    params.subarch, params.release, params.label)]))
+
+    def test_compose_rootfs_over_iscsi(self):
+        params = make_kernel_parameters(
+            fs_host=factory.make_ipv6_address(), http_boot=False)
+        self.assertThat(
+            compose_kernel_command_line(params),
+            ContainsAll([
+                "root=/dev/disk/by-path/ip-",
+                "iscsi_initiator="]))