Merge lp:~racb/maas/arch-detect into lp:~maas-committers/maas/trunk

[Robie Basak]

Add architecture detection

Rather than always responding to pxelinux.cfg/01-<mac> dynamically, instead cause nodes to fall back to pxelinux.cfg/default if the architecture of the machine with the provided MAC address is unknown. This allows non-Intel machines to fall back to pxelinux.cfg/default.<arch>-<subarch> in the middle, at which point serve a configuration for the correct non-Intel architecture.

This allows multiple architectures to be served the correct pxelinux.cfg configuration for enlistment without any needed intervention.

See bug 1041092 for details on pxelinux.cfg/default.<arch>-<subarch>.

[Gavin Panella (allenap)]

Several minor comments, but they're mostly about style; the approach
is good, afaict.

[1]

+    if macaddress:
+        return macaddress.node
+    else:
+        return None

Don't know how you feel about conditional expressions, but this might
be as clear but more concise as:

    return macaddress.node if macaddress else None

[2]

+    :param arch: Architecture name (in the pxelinux namespace, eg. 'arm' not
+                 'armhf').

Typically we align follow-on lines 4-spaces in from the first
line. Yay, another trivial nitpick :)

[3]

+                # Request was pxelinux.cfg/01-<mac>, so attempt fall back
+                # to pxelinux.cfg/default-<arch> for arch detection.

Is it default-<arch> or default.<arch>?

Update: the regex says either.

[4]

+            if pxelinux_arch == 'arm':
+                arch = 'armhf'
+            else:
+                arch = pxelinux_arch

One step in the direction of a lookup-table, and more concise:

            arch = {"arm": "armhf"}.get(pxelinux_arch, pxelinux_arch)

Er, but it's not very pretty. Never mind. Why is "arm" being sent
instead of "armhf"? Should we normalise on just "arm"?

[5]

+        response = self.client.get(reverse('pxeconfig'),
+                                   self.get_default_params())

We have followed the style of wrapping 4 spaces in most places:

        response = self.client.get(
            reverse('pxeconfig'), self.get_default_params())

Also, when we need to break onto a newline, we break at the opening
brace, so not like this:

        response = self.client.get(reverse('pxeconfig'),
            self.get_default_params())

Too much code on Launchpad ended up in the rightmost 20 columns
(there, like here, we chose to use a right margin) so we changed the
convention.

Hardly needs saying, but Emacs' python-mode does this all perfectly,
right out of the box ;)

[6]

+        ( # either a MAC

Make this a non-capturing group, i.e. (?:...)

+            {htype:02x}    # ARP HTYPE.
+            -
+            (?P<mac>{re_mac_address.pattern})    # Capture MAC.
+        | # or "default"
+            default
+              ( # perhaps with specified arch

Here too?

+                [.-](?P<arch>\w+) # arch
+                (-(?P<subarch>\w+))? # optional subarch

And here?

+              )?

[7]

+            params = {k: v
+                      for k, v in config_file_match.groupdict().iteritems()
+                      if v is not None}

We have tended to avoid single-letter variable names (sorry, again,
this is Launchpad engineering heritage showing its face, along with my
pedantry). Also, we decided to use the non-iter methods on dicts, I
think because they're spelled the same as in Python 3. I preferred to
use them, because they behave differently, and 2to3 translates them
anyway, but we, Red Squad, in days of old, agreed this. So, we'd write
this as:

            params = {
                key: value
                for key, value in config_file_match.groupdict().items()
                if value is not None
                }

review: Approve

[Gavin Panella (allenap)]

>                                                    ... I preferred to
> use them, because they behave differently, and 2to3 translates them
> anyway, but we, Red Squad, in days of old, agreed this. ...

There's a "not" missing from the start of the second line there, but
you probably guessed that.

[lp:~racb/maas/arch-detect]

1128. By Robie Basak on 2012-10-03

Use conditional expression for conciseness

1129. By Robie Basak on 2012-10-03

Improve TFTP fallback filename comments

1130. By Robie Basak on 2012-10-03

Use non-capturing regexp groups

1131. By Robie Basak on 2012-10-03

Fix formatting

1132. By Robie Basak on 2012-10-03

Use dict.items() instead of dict.iteritems()

[Robie Basak (racb)]

[1] Done
[2] Done
[3] Clarified separator in corresponding comments

[4]

> Why is "arm" being sent instead of "armhf"? Should we normalise on just "arm"?

Outside userspace, the "hf" part of armhf has no meaning as there is no defined userspace ABI at that point. So before userspace, armel and armhf are identical. But MAAS does care about userspace. albeit only armhf userspace for ARM, because it needs to know which userspace to deploy. So pxelinux emulators should be agnostic of el/hf yet MAAS must be aware of it. So we must map it.

[5] Done
[6] Done
[7] Done

Thanks Gavin!

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-10-03 02:28:04 +0000
+++ src/maasserver/api.py	2012-10-03 08:57:21 +0000
@@ -1496,33 +1496,87 @@
         return "poweroff"
 
 
+def get_node_from_mac_string(mac_string):
+    """Get a Node object from a MAC address string.
+
+    Returns a Node object or None if no node with the given MAC address exists.
+
+    :param mac_string: MAC address string in the form "12-34-56-78-9a-bc"
+    :return: Node object or None
+    """
+    if mac_string is None:
+        return None
+    macaddress = get_one(MACAddress.objects.filter(mac_address=mac_string))
+    return macaddress.node if macaddress else None
+
+
 def pxeconfig(request):
     """Get the PXE configuration given a node's details.
 
     Returns a JSON object corresponding to a
     :class:`provisioningserver.kernel_opts.KernelParameters` instance.
 
+    This is now fairly decoupled from pxelinux's TFTP filename encoding
+    mechanism, with one notable exception. Call this function with (mac, arch,
+    subarch) and it will do the right thing. If details it needs are missing
+    (ie. arch/subarch missing when the MAC is supplied but unknown), then it
+    will as an exception return an HTTP NO_CONTENT (204) in the expectation
+    that this will be translated to a TFTP file not found and pxelinux (or an
+    emulator) will fall back to default-<arch>-<subarch> (in the case of an
+    alternate architecture emulator) or just straight to default (in the case
+    of native pxelinux on i386 or amd64). See bug 1041092 for details and
+    discussion.
+
     :param mac: MAC address to produce a boot configuration for.
+    :param arch: Architecture name (in the pxelinux namespace, eg. 'arm' not
+        'armhf').
+    :param subarch: Subarchitecture name (in the pxelinux namespace).
     """
-    mac = get_mandatory_param(request.GET, 'mac')
+    node = get_node_from_mac_string(request.GET.get('mac', None))
 
-    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.split('/')
-        preseed_url = compose_enlistment_preseed_url()
-        hostname = 'maas-enlist'
-        domain = Config.objects.get_config('enlistment_domain')
-    else:
-        node = macaddress.node
+    if node:
         arch, subarch = node.architecture.split('/')
         preseed_url = compose_preseed_url(node)
         # The node's hostname may include a domain, but we ignore that
         # and use the one from the nodegroup instead.
         hostname = node.hostname.split('.', 1)[0]
         domain = node.nodegroup.name
+    else:
+        try:
+            pxelinux_arch = request.GET['arch']
+        except KeyError:
+            if 'mac' in request.GET:
+                # Request was pxelinux.cfg/01-<mac>, so attempt fall back
+                # to pxelinux.cfg/default-<arch>-<subarch> for arch detection.
+                return HttpResponse(status=httplib.NO_CONTENT)
+            else:
+                # Request has already fallen back, so if arch is still not
+                # provided then use i386.
+                arch = ARCHITECTURE.i386.split('/')[0]
+        else:
+            # Map from pxelinux namespace architecture names to MAAS namespace
+            # architecture names. If this gets bigger, an external lookup table
+            # would make sense. But here is fine for something as trivial as it
+            # is right now.
+            if pxelinux_arch == 'arm':
+                arch = 'armhf'
+            else:
+                arch = pxelinux_arch
+
+        # Use subarch if supplied; otherwise assume 'generic'.
+        try:
+            pxelinux_subarch = request.GET['subarch']
+        except KeyError:
+            subarch = 'generic'
+        else:
+            # Map from pxelinux namespace subarchitecture names to MAAS
+            # namespace subarchitecture names. Right now this happens to be a
+            # 1-1 mapping.
+            subarch = pxelinux_subarch
+
+        preseed_url = compose_enlistment_preseed_url()
+        hostname = 'maas-enlist'
+        domain = Config.objects.get_config('enlistment_domain')
 
     if node is None or node.status == NODE_STATUS.COMMISSIONING:
         series = Config.objects.get_config('commissioning_distro_series')
 
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-10-03 02:28:04 +0000
+++ src/maasserver/tests/test_api.py	2012-10-03 08:57:21 +0000
@@ -2715,21 +2715,22 @@
 
 class TestPXEConfigAPI(AnonAPITestCase):
 
-    def get_params(self):
+    def get_mac_params(self):
         return {'mac': factory.make_mac_address().mac_address}
 
-    def get_optional_params(self):
-        return ['mac']
+    def get_default_params(self):
+        return dict()
 
     def get_pxeconfig(self, params=None):
         """Make a request to `pxeconfig`, and return its response dict."""
         if params is None:
-            params = self.get_params()
+            params = self.get_default_params()
         response = self.client.get(reverse('pxeconfig'), params)
         return json.loads(response.content)
 
     def test_pxeconfig_returns_json(self):
-        response = self.client.get(reverse('pxeconfig'), self.get_params())
+        response = self.client.get(
+            reverse('pxeconfig'), self.get_default_params())
         self.assertThat(
             (
                 response.status_code,
@@ -2751,7 +2752,28 @@
             self.get_pxeconfig(),
             ContainsAll(KernelParameters._fields))
 
-    def test_pxeconfig_defaults_to_i386_when_node_unknown(self):
+    def test_pxeconfig_returns_data_for_known_node(self):
+        params = self.get_mac_params()
+        node = MACAddress.objects.get(mac_address=params['mac']).node
+        response = self.client.get(reverse('pxeconfig'), params)
+        self.assertEqual(httplib.OK, response.status_code)
+
+    def test_pxeconfig_returns_no_content_for_unknown_node(self):
+        params = dict(mac=factory.getRandomMACAddress(delimiter=b'-'))
+        response = self.client.get(reverse('pxeconfig'), params)
+        self.assertEqual(httplib.NO_CONTENT, response.status_code)
+
+    def test_pxeconfig_returns_data_for_detailed_but_unknown_node(self):
+        architecture = factory.getRandomEnum(ARCHITECTURE)
+        arch, subarch = architecture.split('/')
+        params = dict(
+            mac=factory.getRandomMACAddress(delimiter=b'-'),
+            arch=arch,
+            subarch=subarch)
+        response = self.client.get(reverse('pxeconfig'), params)
+        self.assertEqual(httplib.OK, response.status_code)
+
+    def test_pxeconfig_defaults_to_i386_for_default(self):
         # As a lowest-common-denominator, i386 is chosen when the node is not
         # yet known to MAAS.
         expected_arch = tuple(ARCHITECTURE.i386.split('/'))
@@ -2760,16 +2782,12 @@
         self.assertEqual(expected_arch, observed_arch)
 
     def test_pxeconfig_uses_fixed_hostname_for_enlisting_node(self):
-        new_mac = factory.getRandomMACAddress()
-        self.assertEqual(
-            'maas-enlist',
-            self.get_pxeconfig({'mac': new_mac}).get('hostname'))
+        self.assertEqual('maas-enlist', self.get_pxeconfig().get('hostname'))
 
     def test_pxeconfig_uses_enlistment_domain_for_enlisting_node(self):
-        new_mac = factory.getRandomMACAddress()
         self.assertEqual(
             Config.objects.get_config('enlistment_domain'),
-            self.get_pxeconfig({'mac': new_mac}).get('domain'))
+            self.get_pxeconfig().get('domain'))
 
     def test_pxeconfig_splits_domain_from_node_hostname(self):
         host = factory.make_name('host')
@@ -2800,24 +2818,16 @@
             kernel_opts, 'get_ephemeral_name',
             FakeMethod(result=factory.getRandomString()))
 
-    def test_pxeconfig_requires_mac_address(self):
-        # The `mac` parameter is mandatory.
-        self.silence_get_ephemeral_name()
-        self.assertEqual(
-            httplib.BAD_REQUEST,
-            self.get_without_param("mac").status_code)
-
-    def test_pxeconfig_has_enlistment_preseed_url_for_unknown_node(self):
-        self.silence_get_ephemeral_name()
-        params = self.get_params()
-        params['mac'] = factory.getRandomMACAddress()
+    def test_pxeconfig_has_enlistment_preseed_url_for_default(self):
+        self.silence_get_ephemeral_name()
+        params = self.get_default_params()
         response = self.client.get(reverse('pxeconfig'), params)
         self.assertEqual(
             compose_enlistment_preseed_url(),
             json.loads(response.content)["preseed_url"])
 
     def test_pxeconfig_has_preseed_url_for_known_node(self):
-        params = self.get_params()
+        params = self.get_mac_params()
         node = MACAddress.objects.get(mac_address=params['mac']).node
         response = self.client.get(reverse('pxeconfig'), params)
         self.assertEqual(
@@ -2852,7 +2862,8 @@
     def test_pxeconfig_uses_boot_purpose(self):
         fake_boot_purpose = factory.make_name("purpose")
         self.patch(api, "get_boot_purpose", lambda node: fake_boot_purpose)
-        response = self.client.get(reverse('pxeconfig'), self.get_params())
+        response = self.client.get(reverse('pxeconfig'),
+                                   self.get_default_params())
         self.assertEqual(
             fake_boot_purpose,
             json.loads(response.content)["purpose"])
 
=== modified file 'src/provisioningserver/tests/test_tftp.py'
--- src/provisioningserver/tests/test_tftp.py	2012-08-30 10:37:26 +0000
+++ src/provisioningserver/tests/test_tftp.py	2012-10-03 08:57:21 +0000
@@ -70,7 +70,9 @@
         The path is intended to match `re_config_file`, and the components are
         the expected groups from a match.
         """
-        components = {"mac": factory.getRandomMACAddress(b"-")}
+        components = {"mac": factory.getRandomMACAddress(b"-"),
+                      "arch": None,
+                      "subarch": None}
         config_path = compose_config_path(components["mac"])
         return config_path, components
 
@@ -112,13 +114,15 @@
         mac = 'aa-bb-cc-dd-ee-ff'
         match = TFTPBackend.re_config_file.match('pxelinux.cfg/01-%s' % mac)
         self.assertIsNotNone(match)
-        self.assertEqual({'mac': mac}, match.groupdict())
+        self.assertEqual({'mac': mac, 'arch': None, 'subarch': None},
+                         match.groupdict())
 
     def test_re_config_file_matches_pxelinux_cfg_with_leading_slash(self):
         mac = 'aa-bb-cc-dd-ee-ff'
         match = TFTPBackend.re_config_file.match('/pxelinux.cfg/01-%s' % mac)
         self.assertIsNotNone(match)
-        self.assertEqual({'mac': mac}, match.groupdict())
+        self.assertEqual({'mac': mac, 'arch': None, 'subarch': None},
+                         match.groupdict())
 
     def test_re_config_file_does_not_match_non_config_file(self):
         self.assertIsNone(
@@ -132,6 +136,29 @@
         self.assertIsNone(
             TFTPBackend.re_config_file.match('foo/01-aa-bb-cc-dd-ee-ff'))
 
+    def test_re_config_file_with_default(self):
+        match = TFTPBackend.re_config_file.match('pxelinux.cfg/default')
+        self.assertIsNotNone(match)
+        self.assertEqual({'mac': None, 'arch': None, 'subarch': None},
+            match.groupdict())
+
+    def test_re_config_file_with_default_arch(self):
+        arch = factory.make_name('arch', sep='')
+        match = TFTPBackend.re_config_file.match('pxelinux.cfg/default.%s' %
+                                                 arch)
+        self.assertIsNotNone(match)
+        self.assertEqual({'mac': None, 'arch': arch, 'subarch': None},
+            match.groupdict())
+
+    def test_re_config_file_with_default_arch_and_subarch(self):
+        arch = factory.make_name('arch', sep='')
+        subarch = factory.make_name('subarch', sep='')
+        match = TFTPBackend.re_config_file.match(
+            'pxelinux.cfg/default.%s-%s' % (arch, subarch))
+        self.assertIsNotNone(match)
+        self.assertEqual({'mac': None, 'arch': arch, 'subarch': subarch},
+            match.groupdict())
+
 
 class TestTFTPBackend(TestCase):
     """Tests for `provisioningserver.tftp.TFTPBackend`."""
 
=== modified file 'src/provisioningserver/tftp.py'
--- src/provisioningserver/tftp.py	2012-09-12 19:56:23 +0000
+++ src/provisioningserver/tftp.py	2012-10-03 08:57:21 +0000
@@ -14,6 +14,7 @@
     "TFTPBackend",
     ]
 
+import httplib
 from io import BytesIO
 from itertools import repeat
 import json
@@ -32,7 +33,9 @@
     FilesystemSynchronousBackend,
     IReader,
     )
+from tftp.errors import FileNotFound
 from twisted.web.client import getPage
+import twisted.web.error
 from zope.interface import implementer
 
 
@@ -89,9 +92,18 @@
         ^/*
         pxelinux[.]cfg    # PXELINUX expects this.
         /
-        {htype:02x}    # ARP HTYPE.
-        -
-        (?P<mac>{re_mac_address.pattern})    # Capture MAC.
+        (?: # either a MAC
+            {htype:02x}    # ARP HTYPE.
+            -
+            (?P<mac>{re_mac_address.pattern})    # Capture MAC.
+        | # or "default"
+            default
+              (?: # perhaps with specified arch, with a separator of either '-'
+                # or '.', since the spec was changed and both are unambiguous
+                [.-](?P<arch>\w+) # arch
+                (?:-(?P<subarch>\w+))? # optional subarch
+              )?
+        )
         $
         '''.format(
             htype=ARP_HTYPE.ETHERNET,
@@ -162,6 +174,25 @@
         d.addCallback(BytesReader)
         return d
 
+    @staticmethod
+    def get_page_errback(failure, file_name):
+        failure.trap(twisted.web.error.Error)
+        # This twisted.web.error.Error.status object ends up being a
+        # string for some reason, but the constants we can compare against
+        # (both in httplib and twisted.web.http) are ints.
+        try:
+            status_int = int(failure.value.status)
+        except ValueError:
+            # Assume that it's some other error and propagate it
+            return failure
+
+        if status_int == httplib.NO_CONTENT:
+            # Convert HTTP No Content to a TFTP file not found
+            raise FileNotFound(file_name)
+        else:
+            # Otherwise propogate the unknown error
+            return failure
+
     @deferred
     def get_reader(self, file_name):
         """See `IBackend.get_reader()`.
@@ -174,5 +205,12 @@
         if config_file_match is None:
             return super(TFTPBackend, self).get_reader(file_name)
         else:
-            params = config_file_match.groupdict()
-            return self.get_config_reader(params)
+            # Do not include any element that has not matched (ie. is None)
+            params = {
+                key: value
+                for key, value in config_file_match.groupdict().items()
+                if value is not None
+                }
+            d = self.get_config_reader(params)
+            d.addErrback(self.get_page_errback, file_name)
+            return d