launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/tftp-path into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/tftp-path/+merge/112019

I apologize: this branch is tedious.

We had far too many code figuring out for itself, based on the PXE requirements document, where various files should be in TFTP (and how they should get there through the local filesystem).

Here's a module that figures out the paths.  Most of the code is about converting existing usage, which sometimes involves moving the name of the lowest-level directory (currently always “install” or “commissioning”) away from special treatment and into the same code paths.  The code tends to grow a bit, alas, and we may want a convenience wrapper at some point.  But the main thing is that we get more control and less manual fidgeting.

A follow-up branch will get rid of some shell-side path computation that's in the maas-import-pxe-files script.  That will require yet another custom maas command, probably for installing a new pre-boot-loader.

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

=== modified file 'src/maasserver/management/commands/generate_enlistment_pxe.py'
--- src/maasserver/management/commands/generate_enlistment_pxe.py	2012-06-26 04:28:13 +0000
+++ src/maasserver/management/commands/generate_enlistment_pxe.py	2012-06-26 07:46:21 +0000
@@ -24,6 +24,7 @@
 
 from django.core.management.base import BaseCommand
 from provisioningserver.pxe.pxeconfig import PXEConfig
+from provisioningserver.pxe.tftppath import compose_image_path
 
 
 class Command(BaseCommand):
@@ -46,7 +47,7 @@
 
     def handle(self, arch=None, subarch='generic', release=None,
                tftproot=None, **kwargs):
-        image_path = '/maas/%s/%s/%s/install' % (arch, subarch, release)
+        image_path = compose_image_path(arch, subarch, release, 'install')
         # TODO: This needs to go somewhere more appropriate, and
         # probably contain more appropriate options.
         kernel_opts = ' '.join([

=== modified file 'src/maasserver/management/commands/install_pxe_image.py'
--- src/maasserver/management/commands/install_pxe_image.py	2012-06-26 04:22:16 +0000
+++ src/maasserver/management/commands/install_pxe_image.py	2012-06-26 07:46:21 +0000
@@ -24,22 +24,31 @@
 
 from celeryconfig import TFTPROOT
 from django.core.management.base import BaseCommand
-
-
-def make_destination(tftproot, arch, subarch, release):
-    """Locate the destination directory, creating it if necessary.
+from provisioningserver.pxe.tftppath import (
+    compose_image_path,
+    locate_tftp_path,
+    )
+
+
+def make_destination(tftproot, arch, subarch, release, purpose):
+    """Locate an image's destination.  Create containing directory if needed.
 
     :param tftproot: The root directory served up by the TFTP server,
         e.g. /var/lib/tftpboot/.
     :param arch: Main architecture to locate the destination for.
     :param subarch: Sub-architecture of the main architecture.
     :param release: OS release name, e.g. "precise".
-    :return: Path of the destination directory that the image directory
-        should be stored in.
+    :param purpose: Purpose of this image, e.g. "install".
+    :return: Full path describing the image directory's new location and
+        name.  In other words, a file "linux" in the image directory should
+        become os.path.join(make_destination(...), 'linux').
     """
-    dest = os.path.join(tftproot, 'maas', arch, subarch, release)
-    if not os.path.isdir(dest):
-        os.makedirs(dest)
+    dest = locate_tftp_path(
+        compose_image_path(arch, subarch, release, purpose),
+        tftproot=tftproot)
+    parent = os.path.dirname(dest)
+    if not os.path.isdir(parent):
+        os.makedirs(parent)
     return dest
 
 
@@ -141,8 +150,8 @@
         if tftproot is None:
             tftproot = TFTPROOT
 
-        dest = make_destination(tftproot, arch, subarch, release)
-        if not are_identical_dirs(os.path.join(dest, purpose), image):
+        dest = make_destination(tftproot, arch, subarch, release, purpose)
+        if not are_identical_dirs(dest, image):
             # Image has changed.  Move the new version into place.
-            install_dir(image, os.path.join(dest, purpose))
+            install_dir(image, dest)
         rmtree(image, ignore_errors=True)

=== modified file 'src/maasserver/tests/test_commands_generate_enlistment_pxe.py'
--- src/maasserver/tests/test_commands_generate_enlistment_pxe.py	2012-06-26 04:22:16 +0000
+++ src/maasserver/tests/test_commands_generate_enlistment_pxe.py	2012-06-26 07:46:21 +0000
@@ -12,19 +12,27 @@
 __metaclass__ = type
 __all__ = []
 
-import os.path
-
 from django.core.management import call_command
 from maasserver.enum import ARCHITECTURE_CHOICES
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase
 from provisioningserver.pxe.pxeconfig import PXEConfig
+from provisioningserver.pxe.tftppath import (
+    compose_config_path,
+    compose_image_path,
+    locate_tftp_path,
+    )
+from testtools.matchers import (
+    Contains,
+    FileContains,
+    )
 
 
 class TestGenerateEnlistmentPXE(TestCase):
 
     def test_generates_default_pxe_config(self):
         arch = factory.getRandomChoice(ARCHITECTURE_CHOICES)
+        subarch = 'generic'
         release = 'precise'
         tftproot = self.make_dir()
         self.patch(PXEConfig, 'target_basedir', tftproot)
@@ -34,10 +42,11 @@
         # This produces a "default" PXE config file in the right place.
         # It refers to the kernel and initrd for the requested
         # architecture and release.
-        result_path = os.path.join(
-            tftproot, 'maas', arch, 'generic', 'pxelinux.cfg', 'default')
-        with open(result_path) as result_file:
-            contents = result_file.read()
-        self.assertIn(
-            '/'.join(['/maas', arch, 'generic', release, 'install', 'linux']),
-            contents)
+        result_path = locate_tftp_path(
+            compose_config_path(arch, subarch, 'default'),
+            tftproot=tftproot)
+        self.assertThat(
+            result_path,
+            FileContains(matcher=Contains(
+                compose_image_path(arch, subarch, release, 'install') +
+                    '/linux')))

=== modified file 'src/maasserver/tests/test_commands_install_pxe_image.py'
--- src/maasserver/tests/test_commands_install_pxe_image.py	2012-06-26 04:22:16 +0000
+++ src/maasserver/tests/test_commands_install_pxe_image.py	2012-06-26 07:46:21 +0000
@@ -22,6 +22,10 @@
     )
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase
+from provisioningserver.pxe.tftppath import (
+    compose_image_path,
+    locate_tftp_path,
+    )
 from testtools.matchers import (
     DirExists,
     FileContains,
@@ -30,13 +34,14 @@
     )
 
 
-def make_arch_subarch_release():
+def make_arch_subarch_release_purpose():
     """Create arbitrary architecture/subarchitecture/release names.
 
     :return: A triplet of three identifiers for these respective items.
     """
     return tuple(
-        factory.make_name(item) for item in ('arch', 'subarch', 'release'))
+        factory.make_name(item)
+        for item in ('arch', 'subarch', 'release', 'purpose'))
 
 
 class TestInstallPXEImage(TestCase):
@@ -47,47 +52,53 @@
         os.makedirs(image_dir)
         factory.make_file(image_dir, 'kernel')
         tftproot = self.make_dir()
+        arch, subarch, release, purpose = make_arch_subarch_release_purpose()
 
         call_command(
-            'install_pxe_image', arch='arch', subarch='subarch',
-            release='release', purpose='purpose', image=image_dir,
+            'install_pxe_image', arch=arch, subarch=subarch, release=release,
+            purpose=purpose, image=image_dir,
             tftproot=tftproot)
 
         self.assertThat(
             os.path.join(
-                tftproot, 'maas', 'arch', 'subarch', 'release', 'purpose',
+                locate_tftp_path(
+                    compose_image_path(arch, subarch, release, purpose),
+                    tftproot=tftproot),
                 'kernel'),
             FileExists())
 
     def test_make_destination_follows_pxe_path_conventions(self):
         # The directory that make_destination returns follows the PXE
         # directory hierarchy specified for MAAS:
-        # /var/lib/tftproot/maas/<arch>/<subarch>/<release>
+        # /var/lib/tftproot/maas/<arch>/<subarch>/<release>/<purpose>
         # (Where the /var/lib/tftproot/ part is configurable, so we
         # can test this without overwriting system files).
         tftproot = self.make_dir()
-        arch, subarch, release = make_arch_subarch_release()
+        arch, subarch, release, purpose = make_arch_subarch_release_purpose()
         self.assertEqual(
-            os.path.join(tftproot, 'maas', arch, subarch, release),
-            make_destination(tftproot, arch, subarch, release))
+            os.path.join(tftproot, 'maas', arch, subarch, release, purpose),
+            make_destination(tftproot, arch, subarch, release, purpose))
 
     def test_make_destination_creates_directory_if_not_present(self):
         tftproot = self.make_dir()
-        arch, subarch, release = make_arch_subarch_release()
-        expected_destination = os.path.join(
-            tftproot, 'maas', arch, subarch, release)
-        make_destination(tftproot, arch, subarch, release)
+        arch, subarch, release, purpose = make_arch_subarch_release_purpose()
+        expected_destination = os.path.dirname(locate_tftp_path(
+            compose_image_path(arch, subarch, release, purpose),
+            tftproot=tftproot))
+        make_destination(tftproot, arch, subarch, release, purpose)
         self.assertThat(expected_destination, DirExists())
 
     def test_make_destination_returns_existing_directory(self):
         tftproot = self.make_dir()
-        arch, subarch, release = make_arch_subarch_release()
-        expected_dest = os.path.join(tftproot, 'maas', arch, subarch, release)
+        arch, subarch, release, purpose = make_arch_subarch_release_purpose()
+        expected_dest = locate_tftp_path(
+            compose_image_path(arch, subarch, release, purpose),
+            tftproot=tftproot)
         os.makedirs(expected_dest)
         contents = factory.getRandomString()
-        testfile = factory.getRandomString()
+        testfile = factory.make_name('testfile')
         factory.make_file(expected_dest, contents=contents, name=testfile)
-        dest = make_destination(tftproot, arch, subarch, release)
+        dest = make_destination(tftproot, arch, subarch, release, purpose)
         self.assertThat(os.path.join(dest, testfile), FileContains(contents))
 
     def test_are_identical_dirs_sees_missing_old_dir_as_different(self):

=== modified file 'src/provisioningserver/pxe/pxeconfig.py'
--- src/provisioningserver/pxe/pxeconfig.py	2012-06-26 04:22:16 +0000
+++ src/provisioningserver/pxe/pxeconfig.py	2012-06-26 07:46:21 +0000
@@ -18,9 +18,10 @@
 
 import os
 
-from celeryconfig import (
-    PXE_TEMPLATES_DIR,
-    TFTPROOT,
+from celeryconfig import PXE_TEMPLATES_DIR
+from provisioningserver.pxe.tftppath import (
+    compose_config_path,
+    locate_tftp_path,
     )
 import tempita
 
@@ -50,7 +51,7 @@
     :param tftproot: Base directory to write PXE configurations to,
         e.g.  /var/lib/tftpboot/ (which is also the default).  The config
         file will go into a directory determined by the architecture that
-        it's for: `/maas/<target_dir>/<arch>/<subarch>/pxelinux.cfg/`
+        it's for: `/maas/<arch>/<subarch>/pxelinux.cfg/`
     :type tftproot: string
 
     :raises PXEConfigFail: if there's a problem with template parameters
@@ -70,21 +71,15 @@
     def __init__(self, arch, subarch=None, mac=None, tftproot=None):
         if subarch is None:
             subarch = "generic"
-        if tftproot is None:
-            tftproot = TFTPROOT
-        self.target_basedir = os.path.join(tftproot, 'maas')
         self._validate_mac(mac)
         self.template = os.path.join(self.template_basedir, "maas.template")
-        self.target_dir = os.path.join(
-            self.target_basedir,
-            arch,
-            subarch,
-            "pxelinux.cfg")
         if mac is not None:
             filename = mac.replace(':', '-')
         else:
             filename = "default"
-        self.target_file = os.path.join(self.target_dir, filename)
+        self.target_file = locate_tftp_path(
+            compose_config_path(arch, subarch, filename),
+            tftproot=tftproot)
 
     @property
     def template_basedir(self):
@@ -124,7 +119,8 @@
         """
         template = self.get_template()
         rendered = self.render_template(template, **kwargs)
-        if not os.path.isdir(self.target_dir):
-            os.makedirs(self.target_dir)
+        target_dir = os.path.dirname(self.target_file)
+        if not os.path.isdir(target_dir):
+            os.makedirs(target_dir)
         with open(self.target_file, "w") as f:
             f.write(rendered)

=== modified file 'src/provisioningserver/pxe/tests/test_pxeconfig.py'
--- src/provisioningserver/pxe/tests/test_pxeconfig.py	2012-06-26 04:22:16 +0000
+++ src/provisioningserver/pxe/tests/test_pxeconfig.py	2012-06-26 07:46:21 +0000
@@ -15,16 +15,17 @@
 import os
 import re
 
-from celeryconfig import (
-    PXE_TEMPLATES_DIR,
-    TFTPROOT,
-    )
+from celeryconfig import PXE_TEMPLATES_DIR
 from maastesting.factory import factory
 from maastesting.testcase import TestCase
 from provisioningserver.pxe.pxeconfig import (
     PXEConfig,
     PXEConfigFail,
     )
+from provisioningserver.pxe.tftppath import (
+    compose_config_path,
+    locate_tftp_path,
+    )
 import tempita
 from testtools.matchers import (
     Contains,
@@ -40,21 +41,23 @@
         pxeconfig = PXEConfig("armhf", "armadaxp")
 
         expected_template = os.path.join(PXE_TEMPLATES_DIR, "maas.template")
-        expected_target = os.path.join(
-            TFTPROOT, "maas", "armhf", "armadaxp", "pxelinux.cfg")
+        expected_target = os.path.dirname(locate_tftp_path(
+            compose_config_path('armhf', 'armadaxp', 'default')))
         self.assertEqual(expected_template, pxeconfig.template)
-        self.assertEqual(expected_target, pxeconfig.target_dir)
+        self.assertEqual(
+            expected_target, os.path.dirname(pxeconfig.target_file))
 
     def test_init_with_no_subarch_makes_path_with_generic(self):
         pxeconfig = PXEConfig("i386")
-        expected_target = os.path.join(
-            TFTPROOT, "maas", "i386", "generic", "pxelinux.cfg")
-        self.assertEqual(expected_target, pxeconfig.target_dir)
+        expected_target = os.path.dirname(locate_tftp_path(
+                compose_config_path('i386', 'generic', 'default')))
+        self.assertEqual(
+            expected_target, os.path.dirname(pxeconfig.target_file))
 
     def test_init_with_no_mac_sets_default_filename(self):
         pxeconfig = PXEConfig("armhf", "armadaxp")
-        expected_filename = os.path.join(
-            TFTPROOT, "maas", "armhf", "armadaxp", "pxelinux.cfg", "default")
+        expected_filename = locate_tftp_path(
+            compose_config_path('armhf', 'armadaxp', 'default'))
         self.assertEqual(expected_filename, pxeconfig.target_file)
 
     def test_init_with_dodgy_mac(self):
@@ -67,9 +70,8 @@
 
     def test_init_with_mac_sets_filename(self):
         pxeconfig = PXEConfig("armhf", "armadaxp", mac="00:a1:b2:c3:e4:d5")
-        expected_filename = os.path.join(
-            TFTPROOT, "maas", "armhf", "armadaxp", "pxelinux.cfg",
-            "00-a1-b2-c3-e4-d5")
+        expected_filename = locate_tftp_path(
+            compose_config_path('armhf', 'armadaxp', '00-a1-b2-c3-e4-d5'))
         self.assertEqual(expected_filename, pxeconfig.target_file)
 
     def test_get_template(self):

=== added file 'src/provisioningserver/pxe/tests/test_tftppath.py'
--- src/provisioningserver/pxe/tests/test_tftppath.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/pxe/tests/test_tftppath.py	2012-06-26 07:46:21 +0000
@@ -0,0 +1,78 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the tftppath module."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+import os.path
+
+from celeryconfig import TFTPROOT
+from maastesting.factory import factory
+from maastesting.testcase import TestCase
+from provisioningserver.pxe.tftppath import (
+    compose_config_path,
+    compose_image_path,
+    locate_tftp_path,
+    )
+from testtools.matchers import (
+    Not,
+    StartsWith,
+    )
+
+
+class TestTFTPPath(TestCase):
+
+    def test_compose_config_path_follows_maas_pxe_directory_layout(self):
+        arch = factory.make_name('arch')
+        subarch = factory.make_name('subarch')
+        name = factory.make_name('config')
+        self.assertEqual(
+            '/maas/%s/%s/pxelinux.cfg/%s' % (arch, subarch, name),
+            compose_config_path(arch, subarch, name))
+
+    def test_compose_config_path_does_not_include_tftp_root(self):
+        arch = factory.make_name('arch')
+        subarch = factory.make_name('subarch')
+        name = factory.make_name('config')
+        self.assertThat(
+            compose_config_path(arch, subarch, name),
+            Not(StartsWith('/var/lib')))
+
+    def test_compose_image_path_follows_maas_pxe_directory_layout(self):
+        arch = factory.make_name('arch')
+        subarch = factory.make_name('subarch')
+        release = factory.make_name('release')
+        purpose = factory.make_name('purpose')
+        self.assertEqual(
+            '/maas/%s/%s/%s/%s' % (arch, subarch, release, purpose),
+            compose_image_path(arch, subarch, release, purpose))
+
+    def test_compose_image_path_does_not_include_tftp_root(self):
+        arch = factory.make_name('arch')
+        subarch = factory.make_name('subarch')
+        release = factory.make_name('release')
+        purpose = factory.make_name('purpose')
+        self.assertThat(
+            compose_image_path(arch, subarch, release, purpose),
+            Not(StartsWith('/var/lib')))
+
+    def test_locate_tftp_path_prefixes_tftp_root_by_default(self):
+        pxefile = factory.make_name('pxefile')
+        self.assertEqual(
+            os.path.join(TFTPROOT, pxefile),
+            locate_tftp_path(pxefile))
+
+    def test_locate_tftp_path_overrides_default_tftproot(self):
+        tftproot = '/%s' % factory.make_name('tftproot')
+        pxefile = factory.make_name('pxefile')
+        self.assertEqual(
+            os.path.join(tftproot, pxefile),
+            locate_tftp_path(pxefile, tftproot=tftproot))

=== added file 'src/provisioningserver/pxe/tftppath.py'
--- src/provisioningserver/pxe/tftppath.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/pxe/tftppath.py	2012-06-26 07:46:21 +0000
@@ -0,0 +1,71 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Construct TFTP paths for PXE files."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'compose_config_path',
+    'compose_image_path',
+    'locate_tftp_path',
+    ]
+
+import os.path
+
+from celeryconfig import TFTPROOT
+
+
+def compose_config_path(arch, subarch, name):
+    """Compose the TFTP path for a PXE configuration file.
+
+    The path returned is relative to the TFTP root, as it would be
+    identified by clients on the network.
+
+    :param arch: Main machine architecture.
+    :param subarch: Sub-architecture, or "generic" if there is none.
+    :param name: Configuration file's name.
+    :return: Path for the corresponding PXE config file as exposed over
+        TFTP.
+    """
+    # Not using os.path.join: this is a TFTP path, not a native path.
+    # Yes, in practice for us they're the same.
+    return '/'.join(['/maas', arch, subarch, 'pxelinux.cfg', name])
+
+
+def compose_image_path(arch, subarch, release, purpose):
+    """Compose the TFTP path for a PXE kernel/initrd directory.
+
+    The path returned is relative to the TFTP root, as it would be
+    identified by clients on the network.
+
+    :param arch: Main machine architecture.
+    :param subarch: Sub-architecture, or "generic" if there is none.
+    :param release: Operating system release, e.g. "precise".
+    :param purpose: Purpose of the image, e.g. "install" or
+        "commissioning".
+    :return: Path for the corresponding image directory (containing a
+        kernel and initrd) as exposed over TFTP.
+    """
+    return '/'.join(['/maas', arch, subarch, release, purpose])
+
+
+def locate_tftp_path(tftp_path, tftproot=None):
+    """Return the local filesystem path corresponding to `tftp_path`.
+
+    The return value gives the filesystem path where you'd have to put
+    a file if you wanted it made available over TFTP as `tftp_path`.
+
+    :param tftp_path: Path as used in the TFTP protocol which you want
+        the local filesystem equivalent for.
+    :param tftproot: Optional TFTP root directory to override the
+        configured default.
+    """
+    if tftproot is None:
+        tftproot = TFTPROOT
+    return os.path.join(tftproot, tftp_path.lstrip('/'))