Merge lp:~allenap/maas/pxe-intel-arch-config into lp:maas/trunk

[Gavin Panella]

The pxeconfig API view now returns a representation of a KernelParameters instance.

Create a KernelParameters object, and serialize it, in pxeconfig, eliminating the need for the transitional "kernel" module.

[lp:~allenap/maas/pxe-intel-arch-config]

896. By Gavin Panella on 2012-08-23

Merged trunk into pxe-intel-arch-config.

[Jeroen T. Vermeulen (jtv)]

Why is it OK to force enlistment to use an i386 initrd? At the very least this is worth a comment.

Also, the way Django's Manager.get() pushes you towards using exceptions as non-exceptional control flow is really getting to me. In this case it's clearly meant to be a normal “if,” but the layer below checks for empty result and raises an exception just so you can then catch it again. Worth a wrapper similar to Storm's result.one() maybe? I think I'll just hack one up so that we can solve this once and for all.

review: Needs Information

[Jeroen T. Vermeulen (jtv)]

Quick convenience ref: to use that “get zero or one item” helper:

from maasserver.utils.orm import get_one

model = get_one(ModelClass.objects.filter(field=value))

Jeroen

[lp:~allenap/maas/pxe-intel-arch-config]

897. By Gavin Panella on 2012-08-24

Comment on default to i386.

898. By Gavin Panella on 2012-08-24

Merged trunk into pxe-intel-arch-config.

899. By Gavin Panella on 2012-08-24

Use get_one().

[Gavin Panella (allenap)]

> Why is it OK to force enlistment to use an i386 initrd? At the very least
> this is worth a comment.

It's a lowest common denominator (for now, until ARM comes). I've
added some comments to explain.

> Also, the way Django's Manager.get() pushes you towards using exceptions as
> non-exceptional control flow is really getting to me. In this case it's
> clearly meant to be a normal “if,” but the layer below checks for empty result
> and raises an exception just so you can then catch it again. Worth a wrapper
> similar to Storm's result.one() maybe? I think I'll just hack one up so that
> we can solve this once and for all.

Tip-top, I've used get_one().

[lp:~allenap/maas/pxe-intel-arch-config]

900. By Gavin Panella on 2012-08-24

Merged trunk into pxe-intel-arch-config, resolving conflicts.

[Jeroen T. Vermeulen (jtv)]

Thanks for the changes. And, er, sorry for those conflicts... <cough>

review: Approve

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-08-24 10:28:29 +0000
+++ src/maasserver/api.py	2012-08-24 13:00:26 +0000
@@ -100,6 +100,7 @@
 from formencode import validators
 from formencode.validators import Invalid
 from maasserver.enum import (
+    ARCHITECTURE,
     NODE_PERMISSION,
     NODE_STATUS,
     )
@@ -115,7 +116,6 @@
     get_node_create_form,
     get_node_edit_form,
     )
-from maasserver.kernel import compose_kernel_command_line
 from maasserver.models import (
     Config,
     DHCPLease,
@@ -124,6 +124,11 @@
     Node,
     NodeGroup,
     )
+from maasserver.preseed import (
+    compose_enlistment_preseed_url,
+    compose_preseed_url,
+    )
+from maasserver.server_address import get_maas_facing_server_address
 from maasserver.utils.orm import get_one
 from piston.doc import generate_doc
 from piston.handler import (
@@ -134,6 +139,7 @@
 from piston.models import Token
 from piston.resource import Resource
 from piston.utils import rc
+from provisioningserver.kernel_opts import KernelParameters
 
 
 dispatch_methods = {
@@ -1115,31 +1121,38 @@
 def pxeconfig(request):
     """Get the PXE configuration given a node's details.
 
-    :param mac: MAC address to produce a boot configuration for.  This
-        parameter is optional.  If it is not given, the configuration
-        will be the "default" one which boots into an enlistment image.
-    :param arch: Main machine architecture.
-    :param subarch: Sub-architecture, or "generic" if there is none.
+    Returns a JSON object corresponding to a
+    :class:`provisioningserver.kernel_opts.KernelParameters` instance.
+
+    :param mac: MAC address to produce a boot configuration for.
     """
-    mac = request.GET.get('mac', None)
-    arch = get_mandatory_param(request.GET, 'arch')
-    subarch = request.GET.get('subarch', 'generic')
+    mac = get_mandatory_param(request.GET, 'mac')
 
     macaddress = get_one(MACAddress.objects.filter(mac_address=mac))
     if macaddress is None:
+        # Default to i386 as a works-for-all solution. This will not support
+        # non-x86 architectures, but for now this assumption holds.
         node = None
+        arch, subarch = ARCHITECTURE.i386, "generic"
+        preseed_url = compose_enlistment_preseed_url()
+        hostname = 'maas-enlist'
     else:
         node = macaddress.node
+        arch, subarch = node.architecture, "generic"
+        preseed_url = compose_preseed_url(node)
+        hostname = node.hostname
 
+    # XXX JeroenVermeulen 2012-08-06 bug=1013146: Stop hard-coding this.
+    release = 'precise'
     purpose = get_boot_purpose(node)
-    append = compose_kernel_command_line(node, arch, subarch, purpose=purpose)
-
-    # XXX: allenap 2012-07-31 bug=1013146: 'precise' is hardcoded here.
-    release = "precise"
-
-    params = dict(
+    domain = 'local.lan'  # TODO: This is probably not enough!
+    server_address = get_maas_facing_server_address()
+
+    params = KernelParameters(
         arch=arch, subarch=subarch, release=release, purpose=purpose,
-        append=append)
+        hostname=hostname, domain=domain, preseed_url=preseed_url,
+        log_host=server_address, fs_host=server_address)
 
     return HttpResponse(
-        json.dumps(params), content_type="application/json")
+        json.dumps(params._asdict()),
+        content_type="application/json")

=== removed file 'src/maasserver/kernel.py'
--- src/maasserver/kernel.py	2012-08-22 20:29:50 +0000
+++ src/maasserver/kernel.py	1970-01-01 00:00:00 +0000
@@ -1,63 +0,0 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Generate kernel command-line for inclusion in PXE configs.
-
-This is a TRANSITIONAL module. This functionality will move to the pxeconfig()
-view in the most likely scenario.
-"""
-
-from __future__ import (
-    absolute_import,
-    print_function,
-    unicode_literals,
-    )
-
-__metaclass__ = type
-__all__ = [
-    'compose_kernel_command_line',
-    ]
-
-from maasserver.preseed import (
-    compose_enlistment_preseed_url,
-    compose_preseed_url,
-    )
-from maasserver.server_address import get_maas_facing_server_address
-from provisioningserver.kernel_opts import (
-    compose_kernel_command_line_new,
-    KernelParameters,
-    )
-
-
-def compose_kernel_command_line(node, arch, subarch, purpose):
-    """Generate a line of kernel options for booting `node`.
-
-    Include these options in the PXE config file's APPEND argument.
-
-    The node may be None, in which case it will boot into enlistment.
-    """
-    # XXX JeroenVermeulen 2012-08-06 bug=1013146: Stop hard-coding this.
-    release = 'precise'
-
-    # TODO: This is probably not enough!
-    domain = 'local.lan'
-
-    if node is None:
-        preseed_url = compose_enlistment_preseed_url()
-    else:
-        preseed_url = compose_preseed_url(node)
-
-    if node is None:
-        # Not a known host; still needs enlisting.  Make up a name.
-        hostname = 'maas-enlist'
-    else:
-        hostname = node.hostname
-
-    server_address = get_maas_facing_server_address()
-
-    params = KernelParameters(
-        arch=arch, subarch=subarch, release=release, purpose=purpose,
-        hostname=hostname, domain=domain, preseed_url=preseed_url,
-        log_host=server_address, fs_host=server_address)
-
-    return compose_kernel_command_line_new(params)

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-08-24 10:28:29 +0000
+++ src/maasserver/tests/test_api.py	2012-08-24 13:00:26 +0000
@@ -42,6 +42,7 @@
     get_overrided_query_dict,
     )
 from maasserver.enum import (
+    ARCHITECTURE,
     ARCHITECTURE_CHOICES,
     NODE_AFTER_COMMISSIONING_ACTION,
     NODE_STATUS,
@@ -94,6 +95,7 @@
     POWER_TYPE,
     POWER_TYPE_CHOICES,
     )
+from provisioningserver.kernel_opts import KernelParameters
 from provisioningserver.omshell import Omshell
 from testresources import FixtureResource
 from testtools.matchers import (
@@ -2245,14 +2247,10 @@
 class TestPXEConfigAPI(AnonAPITestCase):
 
     def get_params(self):
-        return {
-                'arch': "armhf",
-                'subarch': "armadaxp",
-                'mac': factory.make_mac_address().mac_address,
-            }
+        return {'mac': factory.make_mac_address().mac_address}
 
     def get_optional_params(self):
-        return ['subarch', 'mac']
+        return ['mac']
 
     def get_pxeconfig(self, params=None):
         """Make a request to `pxeconfig`, and return its response dict."""
@@ -2279,37 +2277,17 @@
                 )),
             response)
 
-    def test_pxeconfig_returns_complete_config(self):
+    def test_pxeconfig_returns_all_kernel_parameters(self):
         self.assertThat(
             self.get_pxeconfig(),
-            ContainsAll([
-                'arch',
-                'subarch',
-                'append',
-                'release',
-                'purpose',
-                ]))
-
-    def test_pxeconfig_copies_some_parameters_from_request(self):
-        params_in = self.get_params()
-        params_out = self.get_pxeconfig(params_in)
-        copied_params = ['arch', 'subarch']
-        self.assertEqual(
-            {name: params_in[name] for name in copied_params},
-            {name: params_out[name] for name in copied_params})
-
-    def test_pxeconfig_adds_some_parameters(self):
-        params_in = self.get_params()
-        added_params = {'append', 'release', 'purpose'}
-        for param in added_params:
-            if param in params_in:
-                del params_in[param]
-        params_out = self.get_pxeconfig(params_in)
-        self.assertEqual(
-            set(params_out).difference(params_in), added_params)
-        # The release is always "precise".
-        self.assertEqual('precise', params_out['release'])
-        self.assertThat(params_out['append'], Contains("auto url=http://"))
+            ContainsAll(KernelParameters._fields))
+
+    def test_pxeconfig_defaults_to_i386_when_node_unknown(self):
+        # As a lowest-common-denominator, i386 is chosen when the node is not
+        # yet known to MAAS.
+        params_out = self.get_pxeconfig()
+        self.assertEqual(ARCHITECTURE.i386, params_out["arch"])
+        self.assertEqual("generic", params_out["subarch"])
 
     def get_without_param(self, param):
         """Request a `pxeconfig()` response, but omit `param` from request."""
@@ -2324,39 +2302,29 @@
             kernel_opts, 'get_ephemeral_name',
             FakeMethod(result=factory.getRandomString()))
 
-    def test_pxeconfig_handles_missing_parameters_appropriately(self):
-        # Some parameters are optional, others are mandatory. The
-        # absence of a mandatory parameter always results in a BAD
-        # REQUEST response.
+    def test_pxeconfig_requires_mac_address(self):
+        # The `mac` parameter is mandatory.
         self.silence_get_ephemeral_name()
-        expected_response_to_missing_parameter = {
-            'arch': httplib.BAD_REQUEST,
-            'subarch': httplib.OK,
-            'mac': httplib.OK,
-            }
-        observed_response = {
-            param: self.get_without_param(param).status_code
-            for param in self.get_params()
-            }
         self.assertEqual(
-            expected_response_to_missing_parameter, observed_response)
+            httplib.BAD_REQUEST,
+            self.get_without_param("mac").status_code)
 
-    def test_pxeconfig_appends_enlistment_preseed_url_for_unknown_node(self):
+    def test_pxeconfig_has_enlistment_preseed_url_for_unknown_node(self):
         self.silence_get_ephemeral_name()
         params = self.get_params()
         params['mac'] = factory.getRandomMACAddress()
         response = self.client.get(reverse('pxeconfig'), params)
-        self.assertIn(
+        self.assertEqual(
             compose_enlistment_preseed_url(),
-            json.loads(response.content)["append"])
+            json.loads(response.content)["preseed_url"])
 
-    def test_pxeconfig_appends_preseed_url_for_known_node(self):
+    def test_pxeconfig_has_preseed_url_for_known_node(self):
         params = self.get_params()
         node = MACAddress.objects.get(mac_address=params['mac']).node
         response = self.client.get(reverse('pxeconfig'), params)
-        self.assertIn(
+        self.assertEqual(
             compose_preseed_url(node),
-            json.loads(response.content)["append"])
+            json.loads(response.content)["preseed_url"])
 
     def test_get_boot_purpose_unknown_node(self):
         # A node that's not yet known to MAAS is assumed to be enlisting,

=== removed file 'src/maasserver/tests/test_kernel.py'
--- src/maasserver/tests/test_kernel.py	2012-08-22 20:53:31 +0000
+++ src/maasserver/tests/test_kernel.py	1970-01-01 00:00:00 +0000
@@ -1,176 +0,0 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Test composition of kernel command lines."""
-
-from __future__ import (
-    absolute_import,
-    print_function,
-    unicode_literals,
-    )
-
-__metaclass__ = type
-__all__ = []
-
-import os
-
-from django.conf import settings
-from maasserver.api import get_boot_purpose
-from maasserver.kernel import compose_kernel_command_line
-from maasserver.preseed import (
-    compose_enlistment_preseed_url,
-    compose_preseed_url,
-    )
-from maasserver.server_address import get_maas_facing_server_address
-from maasserver.testing.factory import factory
-from maasserver.testing.testcase import TestCase
-from maastesting.matchers import ContainsAll
-from provisioningserver.kernel_opts import (
-    EphemeralImagesDirectoryNotFound,
-    ISCSI_TARGET_NAME_PREFIX,
-    )
-from provisioningserver.pxe.tftppath import compose_image_path
-from provisioningserver.testing.config import ConfigFixture
-
-
-class TestComposeKernelCommandLine(TestCase):
-
-    def test_compose_kernel_command_line_accepts_None_for_unknown_node(self):
-        self.assertIn(
-            'suite=precise',
-            compose_kernel_command_line(
-                None, factory.make_name('arch'), factory.make_name('subarch'),
-                purpose=factory.make_name('purpose')))
-
-    def test_compose_kernel_command_line_includes_preseed_url(self):
-        node = factory.make_node()
-        self.assertIn(
-            "auto url=%s" % compose_preseed_url(node),
-            compose_kernel_command_line(
-                node, node.architecture, 'generic',
-                purpose=factory.make_name('purpose')))
-
-    def test_compose_kernel_command_line_includes_enlistment_preseed_url(self):
-        self.assertIn(
-            "auto url=%s" % compose_enlistment_preseed_url(),
-            compose_kernel_command_line(
-                None, factory.make_name("arch"), 'generic',
-                purpose=factory.make_name('purpose')))
-
-    def test_compose_kernel_command_line_includes_initrd(self):
-        node = factory.make_node()
-        initrd_path = compose_image_path(
-            node.architecture, 'generic', 'precise',
-            purpose=get_boot_purpose(node))
-        self.assertIn(
-            "initrd=%s" % initrd_path,
-            compose_kernel_command_line(
-                node, node.architecture, 'generic',
-                purpose=get_boot_purpose(node)))
-
-    def test_compose_kernel_command_line_includes_suite(self):
-        # At the moment, the OS release we use is hard-coded to "precise."
-        node = factory.make_node()
-        suite = "precise"
-        self.assertIn(
-            "suite=%s" % suite,
-            compose_kernel_command_line(
-                node, node.architecture, 'generic',
-                purpose=factory.make_name('purpose')))
-
-    def test_compose_kernel_command_line_includes_hostname_and_domain(self):
-        node = factory.make_node()
-        # Cobbler seems to hard-code domain to "local.lan"; we may want
-        # to change it, and update this test.
-        domain = "local.lan"
-        self.assertThat(
-            compose_kernel_command_line(
-                node, node.architecture, 'generic',
-                purpose=factory.make_name('purpose')),
-            ContainsAll([
-                "hostname=%s" % node.hostname,
-                "domain=%s" % domain,
-                ]))
-
-    def test_compose_kernel_command_line_makes_up_hostname_for_new_node(self):
-        dummy_hostname = 'maas-enlist'
-        self.assertIn(
-            "hostname=%s" % dummy_hostname,
-            compose_kernel_command_line(
-                None, factory.make_name('arch'),
-                factory.make_name('subarch'),
-                purpose=factory.make_name('purpose')))
-
-    def test_compose_kernel_command_line_includes_locale(self):
-        node = factory.make_node()
-        locale = "en_US"
-        self.assertIn(
-            "locale=%s" % locale,
-            compose_kernel_command_line(
-                node, node.architecture, 'generic',
-                purpose=factory.make_name('purpose')))
-
-    def test_compose_kernel_command_line_includes_log_settings(self):
-        node = factory.make_node()
-        log_host = factory.getRandomIPAddress()
-        self.patch(settings, 'DEFAULT_MAAS_URL', 'http://%s/' % log_host)
-        # Port 514 (UDP) is syslog.
-        log_port = "514"
-        text_priority = "critical"
-        self.assertThat(
-            compose_kernel_command_line(
-                node, node.architecture, 'generic',
-                purpose=factory.make_name('purpose')),
-            ContainsAll([
-                "log_host=%s" % log_host,
-                "log_port=%s" % log_port,
-                "text priority=%s" % text_priority,
-                ]))
-
-    def create_ephemeral_info(self, name, arch, release):
-        """Create a pseudo-real ephemeral info file."""
-        epheneral_info = """
-            release=%s
-            stream=ephemeral
-            label=release
-            serial=20120424
-            arch=%s
-            name=%s
-            """ % (release, arch, name)
-        ephemeral_root = self.make_dir()
-        config = {"boot": {"ephemeral": {"directory": ephemeral_root}}}
-        self.useFixture(ConfigFixture(config))
-        ephemeral_dir = os.path.join(
-            ephemeral_root, release, 'ephemeral', arch, release)
-        os.makedirs(ephemeral_dir)
-        factory.make_file(
-            ephemeral_dir, name='info', contents=epheneral_info)
-
-    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.
-        ephemeral_name = factory.make_name("ephemeral")
-        arch = factory.make_name('arch')
-        self.create_ephemeral_info(ephemeral_name, arch, "precise")
-        node = factory.make_node()
-        self.assertThat(
-            compose_kernel_command_line(
-                node, arch,
-                factory.make_name('subarch'),
-                purpose="commissioning"),
-            ContainsAll([
-                "iscsi_target_name=%s:%s" % (
-                    ISCSI_TARGET_NAME_PREFIX, ephemeral_name),
-                "iscsi_target_port=3260",
-                "iscsi_target_ip=%s" % get_maas_facing_server_address(),
-                ]))
-
-    def test_compose_kernel_command_line_reports_error_about_missing_dir(self):
-        missing_dir = factory.make_name('missing-dir')
-        config = {"boot": {"ephemeral": {"directory": missing_dir}}}
-        self.useFixture(ConfigFixture(config))
-        node = factory.make_node()
-        self.assertRaises(
-            EphemeralImagesDirectoryNotFound,
-            compose_kernel_command_line, node, factory.make_name('arch'),
-            factory.make_name('subarch'), purpose="commissioning")