← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~jtv/maas/tftproot into lp:maas

 

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/tftproot into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/tftproot/+merge/112001

We expose our PXE files over TFTP in a TFTP directory hierarchy rooted at /maas.  And we publish these files by placing them into /var/lib/tftpboot/maas/ somewhere.

The whole /var/lib/tftpboot/maas filesystem path was configurable as PXE_TARGET_DIR, but doing it that way complicates the composition of TFTP paths because the “maas” part that's exposed on TFTP was implicitly configurable together with the root TFTP directory, when clients need to know about the one but not the other.

And so, after a quick check with Julian, I'm moving the “/maas” portion out of the configurable path.  This way we can simply say that the filesystem path for a given tftp_path is os.path.join(PXE_TARGET_DIR, tftp_path).  And because the meaning changed, I renamed PXE_TARGET_DIR to TFTPROOT.  It is now unambiguously defined as the root of the TFTP directory hierarchy on the local filesystem.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/tftproot/+merge/112001
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/tftproot into lp:maas.
=== modified file 'etc/celeryconfig.py'
--- etc/celeryconfig.py	2012-06-18 23:15:38 +0000
+++ etc/celeryconfig.py	2012-06-26 04:36:19 +0000
@@ -29,8 +29,8 @@
     os.path.dirname(__file__), os.pardir, "src", "provisioningserver", "pxe",
     "templates")
 
-# Where to write PXE config files.
-PXE_TARGET_DIR = "/var/lib/tftp/maas"
+# TFTP server's root directory.
+TFTPROOT = "/var/lib/tftp"
 
 
 try:

=== modified file 'scripts/maas-import-pxe-files'
--- scripts/maas-import-pxe-files	2012-06-22 05:09:37 +0000
+++ scripts/maas-import-pxe-files	2012-06-26 04:36:19 +0000
@@ -97,7 +97,7 @@
 
     maas install_pxe_image \
         --arch=$arch --release=$release --purpose="install" \
-        --image="install" --pxe-target-dir="$TFTPROOT/maas"
+        --image="install" --tftproot=$TFTPROOT
 }
 
 
@@ -128,7 +128,7 @@
 
         maas generate_enlistment_pxe \
             --arch=$arch --release=$CURRENT_RELEASE \
-            --pxe-target-dir=$TFTPROOT/maas
+            --tftproot=$TFTPROOT
     done
 
     rm -rf -- $DOWNLOAD_DIR

=== modified file 'src/maasserver/management/commands/generate_enlistment_pxe.py'
--- src/maasserver/management/commands/generate_enlistment_pxe.py	2012-06-25 15:01:47 +0000
+++ src/maasserver/management/commands/generate_enlistment_pxe.py	2012-06-26 04:36:19 +0000
@@ -40,12 +40,12 @@
             '--release', dest='release', default=None,
             help="Ubuntu release to run when enlisting nodes."),
         make_option(
-            '--pxe-target-dir', dest='pxe_target_dir', default=None,
-            help="Write PXE config here instead of in its normal location."),
+            '--tftproot', dest='tftproot', default=None,
+            help="Root of TFTP directory hierarchy to place config into."),
         )
 
     def handle(self, arch=None, subarch='generic', release=None,
-               pxe_target_dir=None, **kwargs):
+               tftproot=None, **kwargs):
         image_path = '/maas/%s/%s/%s/install' % (arch, subarch, release)
         # TODO: This needs to go somewhere more appropriate, and
         # probably contain more appropriate options.
@@ -69,5 +69,5 @@
             'kernelimage': '/'.join([image_path, 'linux']),
             'append': kernel_opts,
         }
-        writer = PXEConfig(arch, subarch, pxe_target_dir=pxe_target_dir)
+        writer = PXEConfig(arch, subarch, tftproot=tftproot)
         writer.write_config(**template_args)

=== modified file 'src/maasserver/management/commands/install_pxe_image.py'
--- src/maasserver/management/commands/install_pxe_image.py	2012-06-25 07:24:40 +0000
+++ src/maasserver/management/commands/install_pxe_image.py	2012-06-26 04:36:19 +0000
@@ -22,22 +22,22 @@
     rmtree,
     )
 
-from celeryconfig import PXE_TARGET_DIR
+from celeryconfig import TFTPROOT
 from django.core.management.base import BaseCommand
 
 
-def make_destination(pxe_target_dir, arch, subarch, release):
+def make_destination(tftproot, arch, subarch, release):
     """Locate the destination directory, creating it if necessary.
 
-    :param pxe_target_dir: The TFTP directory containing the MAAS portion
-        of the PXE directory tree, e.g. /var/lib/tftpboot/maas/.
+    :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.
     """
-    dest = os.path.join(pxe_target_dir, arch, subarch, release)
+    dest = os.path.join(tftproot, 'maas', arch, subarch, release)
     if not os.path.isdir(dest):
         os.makedirs(dest)
     return dest
@@ -132,16 +132,16 @@
             '--image', dest='image', default=None,
             help="Netboot image directory, containing kernel & initrd."),
         make_option(
-            '--pxe-target-dir', dest='pxe_target_dir', default=PXE_TARGET_DIR,
+            '--tftproot', dest='tftproot', default=TFTPROOT,
             help="Store to this TFTP directory tree instead of the default."),
         )
 
     def handle(self, arch=None, subarch='generic', release=None, purpose=None,
-               image=None, pxe_target_dir=None, **kwargs):
-        if pxe_target_dir is None:
-            pxe_target_dir = PXE_TARGET_DIR
+               image=None, tftproot=None, **kwargs):
+        if tftproot is None:
+            tftproot = TFTPROOT
 
-        dest = make_destination(pxe_target_dir, arch, subarch, release)
+        dest = make_destination(tftproot, arch, subarch, release)
         if not are_identical_dirs(os.path.join(dest, purpose), image):
             # Image has changed.  Move the new version into place.
             install_dir(image, os.path.join(dest, purpose))

=== modified file 'src/maasserver/tests/test_commands_generate_enlistment_pxe.py'
--- src/maasserver/tests/test_commands_generate_enlistment_pxe.py	2012-06-25 10:32:05 +0000
+++ src/maasserver/tests/test_commands_generate_enlistment_pxe.py	2012-06-26 04:36:19 +0000
@@ -26,16 +26,16 @@
     def test_generates_default_pxe_config(self):
         arch = factory.getRandomChoice(ARCHITECTURE_CHOICES)
         release = 'precise'
-        tftpdir = self.make_dir()
-        self.patch(PXEConfig, 'target_basedir', tftpdir)
+        tftproot = self.make_dir()
+        self.patch(PXEConfig, 'target_basedir', tftproot)
         call_command(
             'generate_enlistment_pxe', arch=arch, release=release,
-            pxe_target_dir=tftpdir)
+            tftproot=tftproot)
         # 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(
-            tftpdir, arch, 'generic', 'pxelinux.cfg', 'default')
+            tftproot, 'maas', arch, 'generic', 'pxelinux.cfg', 'default')
         with open(result_path) as result_file:
             contents = result_file.read()
         self.assertIn(

=== modified file 'src/maasserver/tests/test_commands_install_pxe_image.py'
--- src/maasserver/tests/test_commands_install_pxe_image.py	2012-06-25 09:03:14 +0000
+++ src/maasserver/tests/test_commands_install_pxe_image.py	2012-06-26 04:36:19 +0000
@@ -46,16 +46,16 @@
         image_dir = os.path.join(download_dir, 'image')
         os.makedirs(image_dir)
         factory.make_file(image_dir, 'kernel')
-        pxe_target_dir = self.make_dir()
+        tftproot = self.make_dir()
 
         call_command(
             'install_pxe_image', arch='arch', subarch='subarch',
             release='release', purpose='purpose', image=image_dir,
-            pxe_target_dir=pxe_target_dir)
+            tftproot=tftproot)
 
         self.assertThat(
             os.path.join(
-                pxe_target_dir, 'arch', 'subarch', 'release', 'purpose',
+                tftproot, 'maas', 'arch', 'subarch', 'release', 'purpose',
                 'kernel'),
             FileExists())
 
@@ -63,41 +63,31 @@
         # The directory that make_destination returns follows the PXE
         # directory hierarchy specified for MAAS:
         # /var/lib/tftproot/maas/<arch>/<subarch>/<release>
-        # (Where the /var/lib/tftproot/maas/ part is configurable, so we
+        # (Where the /var/lib/tftproot/ part is configurable, so we
         # can test this without overwriting system files).
-        pxe_target_dir = self.make_dir()
+        tftproot = self.make_dir()
         arch, subarch, release = make_arch_subarch_release()
         self.assertEqual(
-            os.path.join(pxe_target_dir, arch, subarch, release),
-            make_destination(pxe_target_dir, arch, subarch, release))
-
-    def test_make_destination_assumes_maas_dir_included_in_target_dir(self):
-        # make_destination does not add a "maas" part to the path, as in
-        # the default /var/lib/tftpboot/maas/; that is assumed to be
-        # included already in the pxe-target-dir setting.
-        pxe_target_dir = self.make_dir()
-        arch, subarch, release = make_arch_subarch_release()
-        self.assertNotIn(
-            '/maas/',
-            make_destination(pxe_target_dir, arch, subarch, release))
+            os.path.join(tftproot, 'maas', arch, subarch, release),
+            make_destination(tftproot, arch, subarch, release))
 
     def test_make_destination_creates_directory_if_not_present(self):
-        pxe_target_dir = self.make_dir()
+        tftproot = self.make_dir()
         arch, subarch, release = make_arch_subarch_release()
         expected_destination = os.path.join(
-            pxe_target_dir, arch, subarch, release)
-        make_destination(pxe_target_dir, arch, subarch, release)
+            tftproot, 'maas', arch, subarch, release)
+        make_destination(tftproot, arch, subarch, release)
         self.assertThat(expected_destination, DirExists())
 
     def test_make_destination_returns_existing_directory(self):
-        pxe_target_dir = self.make_dir()
+        tftproot = self.make_dir()
         arch, subarch, release = make_arch_subarch_release()
-        expected_dest = os.path.join(pxe_target_dir, arch, subarch, release)
+        expected_dest = os.path.join(tftproot, 'maas', arch, subarch, release)
         os.makedirs(expected_dest)
         contents = factory.getRandomString()
         testfile = factory.getRandomString()
         factory.make_file(expected_dest, contents=contents, name=testfile)
-        dest = make_destination(pxe_target_dir, arch, subarch, release)
+        dest = make_destination(tftproot, arch, subarch, release)
         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-20 16:56:42 +0000
+++ src/provisioningserver/pxe/pxeconfig.py	2012-06-26 04:36:19 +0000
@@ -19,8 +19,8 @@
 import os
 
 from celeryconfig import (
-    PXE_TARGET_DIR,
     PXE_TEMPLATES_DIR,
+    TFTPROOT,
     )
 import tempita
 
@@ -47,11 +47,11 @@
         aa:bb:cc:dd:ee:ff.  This is the default for MAC addresses coming
         from the database fields in MAAS, so it's not heavily checked here.
     :type mac: string
-    :param pxe_target_dir: Base directory to write PXE configurations to,
-        e.g.  /var/lib/tftpboot/maas (which is also the default).  The
-        config file will go into a directory determined by the architecture
-        that it's for: `<target_dir>/<arch>/<subarch>/pxelinux.cfg/`
-    :type pxe_target_dir: string
+    :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/`
+    :type tftproot: string
 
     :raises PXEConfigFail: if there's a problem with template parameters
         or the MAC address looks incorrectly formatted.
@@ -67,12 +67,12 @@
             append="initrd=blah url=blah")
     """
 
-    def __init__(self, arch, subarch=None, mac=None, pxe_target_dir=None):
+    def __init__(self, arch, subarch=None, mac=None, tftproot=None):
         if subarch is None:
             subarch = "generic"
-        if pxe_target_dir is None:
-            pxe_target_dir = PXE_TARGET_DIR
-        self.target_basedir = pxe_target_dir
+        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(

=== modified file 'src/provisioningserver/pxe/tests/test_pxeconfig.py'
--- src/provisioningserver/pxe/tests/test_pxeconfig.py	2012-06-20 16:56:42 +0000
+++ src/provisioningserver/pxe/tests/test_pxeconfig.py	2012-06-26 04:36:19 +0000
@@ -16,8 +16,8 @@
 import re
 
 from celeryconfig import (
-    PXE_TARGET_DIR,
     PXE_TEMPLATES_DIR,
+    TFTPROOT,
     )
 from maastesting.factory import factory
 from maastesting.testcase import TestCase
@@ -41,20 +41,20 @@
 
         expected_template = os.path.join(PXE_TEMPLATES_DIR, "maas.template")
         expected_target = os.path.join(
-            PXE_TARGET_DIR, "armhf", "armadaxp", "pxelinux.cfg")
+            TFTPROOT, "maas", "armhf", "armadaxp", "pxelinux.cfg")
         self.assertEqual(expected_template, pxeconfig.template)
         self.assertEqual(expected_target, pxeconfig.target_dir)
 
     def test_init_with_no_subarch_makes_path_with_generic(self):
         pxeconfig = PXEConfig("i386")
         expected_target = os.path.join(
-            PXE_TARGET_DIR, "i386", "generic", "pxelinux.cfg")
+            TFTPROOT, "maas", "i386", "generic", "pxelinux.cfg")
         self.assertEqual(expected_target, pxeconfig.target_dir)
 
     def test_init_with_no_mac_sets_default_filename(self):
         pxeconfig = PXEConfig("armhf", "armadaxp")
         expected_filename = os.path.join(
-            PXE_TARGET_DIR, "armhf", "armadaxp", "pxelinux.cfg", "default")
+            TFTPROOT, "maas", "armhf", "armadaxp", "pxelinux.cfg", "default")
         self.assertEqual(expected_filename, pxeconfig.target_file)
 
     def test_init_with_dodgy_mac(self):
@@ -68,7 +68,7 @@
     def test_init_with_mac_sets_filename(self):
         pxeconfig = PXEConfig("armhf", "armadaxp", mac="00:a1:b2:c3:e4:d5")
         expected_filename = os.path.join(
-            PXE_TARGET_DIR, "armhf", "armadaxp", "pxelinux.cfg",
+            TFTPROOT, "maas", "armhf", "armadaxp", "pxelinux.cfg",
             "00-a1-b2-c3-e4-d5")
         self.assertEqual(expected_filename, pxeconfig.target_file)
 
@@ -100,8 +100,8 @@
 
     def test_write_config_writes_config(self):
         # Ensure that a rendered template is written to the right place.
-        out_dir = self.make_dir()
-        pxeconfig = PXEConfig("armhf", "armadaxp", pxe_target_dir=out_dir)
+        tftproot = self.make_dir()
+        pxeconfig = PXEConfig("armhf", "armadaxp", tftproot=tftproot)
         pxeconfig.write_config(
             menutitle="menutitle", kernelimage="/my/kernel", append="append")
 
@@ -113,11 +113,11 @@
         self.assertThat(pxeconfig.target_file, FileContains(expected))
 
     def test_write_config_overwrites_config(self):
-        out_dir = self.make_dir()
-        pxeconfig = PXEConfig("amd64", "generic", pxe_target_dir=out_dir)
+        tftproot = self.make_dir()
+        pxeconfig = PXEConfig("amd64", "generic", tftproot=tftproot)
         pxeconfig.write_config(
             menutitle="oldtitle", kernelimage="/old/kernel", append="append")
-        pxeconfig = PXEConfig("amd64", "generic", pxe_target_dir=out_dir)
+        pxeconfig = PXEConfig("amd64", "generic", tftproot=tftproot)
         pxeconfig.write_config(
             menutitle="newtitle", kernelimage="/new/kernel", append="append")
 

=== modified file 'src/provisioningserver/tasks.py'
--- src/provisioningserver/tasks.py	2012-06-21 06:47:34 +0000
+++ src/provisioningserver/tasks.py	2012-06-26 04:36:19 +0000
@@ -62,7 +62,7 @@
 
 @task
 def write_tftp_config_for_node(arch, macs, subarch="generic",
-                               pxe_target_dir=None, **kwargs):
+                               tftproot=None, **kwargs):
     """Write out the TFTP MAC-based config for a node.
 
     A config file is written for each MAC associated with the node.
@@ -73,9 +73,10 @@
     :type macs: Iterable of strings
     :param subarch: The subarchitecture of the node, defaults to "generic" for
         architectures without sub-architectures.
+    :param tftproot: Root TFTP directory.
     :param **kwargs: Keyword args passed to PXEConfig.write_config()
     """
     # TODO: fix subarch when node.py starts modelling sub-architecture for ARM
     for mac in macs:
-        pxeconfig = PXEConfig(arch, subarch, mac, pxe_target_dir)
+        pxeconfig = PXEConfig(arch, subarch, mac, tftproot)
         pxeconfig.write_config(**kwargs)

=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py	2012-06-25 13:59:23 +0000
+++ src/provisioningserver/tests/test_tasks.py	2012-06-26 04:36:19 +0000
@@ -67,20 +67,21 @@
         arch = ARCHITECTURE.i386
         mac = factory.getRandomMACAddress()
         mac2 = factory.getRandomMACAddress()
-        target_dir = self.make_dir()
+        tftproot = self.make_dir()
         kernel = factory.getRandomString()
         menutitle = factory.getRandomString()
         append = factory.getRandomString()
 
         result = write_tftp_config_for_node.delay(
-            arch, (mac, mac2), pxe_target_dir=target_dir, menutitle=menutitle,
+            arch, (mac, mac2), tftproot=tftproot, menutitle=menutitle,
             kernelimage=kernel, append=append)
 
         self.assertTrue(result.successful(), result)
         expected_file1 = os.path.join(
-            target_dir, arch, "generic", "pxelinux.cfg", mac.replace(":", "-"))
+            tftproot, 'maas', arch, "generic", "pxelinux.cfg",
+            mac.replace(":", "-"))
         expected_file2 = os.path.join(
-            target_dir, arch, "generic", "pxelinux.cfg",
+            tftproot, 'maas', arch, "generic", "pxelinux.cfg",
             mac2.replace(":", "-"))
         self.assertThat(
             expected_file1,