launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/install-pxe-bootloader into lp:maas with lp:~jtv/maas/tftp-bootloader-path as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/install-pxe-bootloader/+merge/112711

This lets script code install a new PXE bootloader for a given architecture into the TFTP directory tree, without knowing the details of exactly where that file goes — because that's all neatly centralized in the tftppath module now.  Discussion was pretty much with the entire team, but with Gavin in particular.  We needed to coördinate because he also hopes to turn the scripts into celery jobs at some point, and creating this custom command wouldn't make much sense if that was going to happen in the immediate term.

A predecessor branch extended the tftppath module to meet this new need.  A subsequent branch will update the maas-import-pxe-files script to make use of the new custom command.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/install-pxe-bootloader/+merge/112711
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/install-pxe-bootloader into lp:maas.

=== added file 'src/maasserver/management/commands/install_pxe_bootloader.py'
--- src/maasserver/management/commands/install_pxe_bootloader.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/management/commands/install_pxe_bootloader.py	2012-06-29 06:47:30 +0000
@@ -0,0 +1,121 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Install a PXE pre-boot loader for TFTP download."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'Command',
+    ]
+
+import filecmp
+from optparse import make_option
+import os.path
+from shutil import copyfile
+
+from celeryconfig import TFTPROOT
+from django.core.management.base import BaseCommand
+from provisioningserver.pxe.tftppath import (
+    compose_bootloader_path,
+    locate_tftp_path,
+    )
+
+
+def make_destination(tftproot, arch, subarch):
+    """Locate a loader'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.
+    :return: Full path describing the filename that the installed loader
+        should end up having.  For example, the loader for i386 (with
+        sub-architecture "generic") should install at
+        /maas/i386/generic/pxelinux.0.
+    """
+    path = locate_tftp_path(
+        compose_bootloader_path(arch, subarch),
+        tftproot=tftproot)
+    directory = os.path.dirname(path)
+    if not os.path.isdir(directory):
+        os.makedirs(directory)
+    return path
+
+
+def are_identical_files(old, new):
+    """Are `old` and `new` identical?
+
+    If `old` does not exist, the two are considered different (`new` is
+    assumed to exist).
+    """
+    if os.path.isfile(old):
+        return filecmp.cmp(old, new, shallow=False)
+    else:
+        return False
+
+
+def install_bootloader(loader, destination):
+    """Install bootloader file at path `loader` as `destination`.
+
+    Installation will be atomic.  If an identical loader is already
+    installed, it will be left untouched.
+
+    However it is still conceivable, depending on the TFTP implementation,
+    that a download that is already in progress may suddenly start receiving
+    data from the new file instead of the one it originally started
+    downloading.
+
+    :param loader: Name of loader to install.
+    :param destination: Loader's intended filename, including full path,
+        where it will become available over TFTP.
+    """
+    if are_identical_files(destination, loader):
+        return
+
+    # Copy new loader next to the old one, to ensure that it is on the
+    # same filesystem.  Once it is, we can replace the old one with an
+    # atomic rename operation.
+    temp_file = '%s.new' % destination
+    if os.path.exists(temp_file):
+        os.remove(temp_file)
+    copyfile(loader, temp_file)
+    os.rename(temp_file, destination)
+
+
+class Command(BaseCommand):
+    """Install a PXE pre-boot loader into the TFTP directory structure.
+
+    This won't overwrite an existing loader if its contents are unchanged.
+    However the new loader you give it will be deleted regardless.
+    """
+
+    option_list = BaseCommand.option_list + (
+        make_option(
+            '--arch', dest='arch', default=None,
+            help="Main system architecture that the bootloader is for."),
+        make_option(
+            '--subarch', dest='subarch', default='generic',
+            help="Sub-architecture of the main architecture."),
+        make_option(
+            '--loader', dest='loader', default=None,
+            help="PXE pre-boot loader to install."),
+        make_option(
+            '--tftproot', dest='tftproot', default=TFTPROOT,
+            help="Store to this TFTP directory tree instead of the default."),
+        )
+
+    def handle(self, arch=None, subarch='generic', loader=None, tftproot=None,
+               **kwargs):
+        if tftproot is None:
+            tftproot = TFTPROOT
+
+        install_bootloader(loader, make_destination(tftproot, arch, subarch))
+
+        if os.path.exists(loader):
+            os.remove(loader)

=== added file 'src/maasserver/tests/test_commands_install_pxe_bootloader.py'
--- src/maasserver/tests/test_commands_install_pxe_bootloader.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/test_commands_install_pxe_bootloader.py	2012-06-29 06:47:30 +0000
@@ -0,0 +1,106 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the install_pxe_bootloader command."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+import os.path
+
+from django.core.management import call_command
+from maasserver.management.commands.install_pxe_bootloader import (
+    install_bootloader,
+    make_destination,
+    )
+from maastesting.factory import factory
+from maastesting.testcase import TestCase
+from maastesting.utils import (
+    age_file,
+    get_write_time,
+    )
+from provisioningserver.pxe.tftppath import (
+    compose_bootloader_path,
+    locate_tftp_path,
+    )
+from testtools.matchers import (
+    DirExists,
+    FileContains,
+    FileExists,
+    Not,
+    )
+
+
+class TestInstallPXEBootloader(TestCase):
+
+    def test_integration(self):
+        loader = self.make_file()
+        tftproot = self.make_dir()
+        arch = factory.make_name('arch')
+        subarch = factory.make_name('subarch')
+
+        call_command(
+            'install_pxe_bootloader', arch=arch, subarch=subarch,
+            loader=loader, tftproot=tftproot)
+
+        self.assertThat(
+            locate_tftp_path(
+                compose_bootloader_path(arch, subarch), tftproot=tftproot),
+            FileExists())
+        self.assertThat(loader, Not(FileExists()))
+
+    def test_make_destination_creates_directory_if_not_present(self):
+        tftproot = self.make_dir()
+        arch = factory.make_name('arch')
+        subarch = factory.make_name('subarch')
+        dest = make_destination(tftproot, arch, subarch)
+        self.assertThat(os.path.dirname(dest), DirExists())
+
+    def test_make_destination_returns_existing_directory(self):
+        tftproot = self.make_dir()
+        arch = factory.make_name('arch')
+        subarch = factory.make_name('subarch')
+        make_destination(tftproot, arch, subarch)
+        dest = make_destination(tftproot, arch, subarch)
+        self.assertThat(os.path.dirname(dest), DirExists())
+
+    def test_install_bootloader_installs_new_bootloader(self):
+        contents = factory.getRandomString()
+        loader = self.make_file(contents=contents)
+        install_dir = self.make_dir()
+        dest = os.path.join(install_dir, factory.make_name('loader'))
+        install_bootloader(loader, dest)
+        self.assertThat(dest, FileContains(contents))
+
+    def test_install_bootloader_replaces_bootloader_if_changed(self):
+        contents = factory.getRandomString()
+        loader = self.make_file(contents=contents)
+        dest = self.make_file(contents="Old contents")
+        install_bootloader(loader, dest)
+        self.assertThat(dest, FileContains(contents))
+
+    def test_install_bootloader_skips_if_unchanged(self):
+        contents = factory.getRandomString()
+        dest = self.make_file(contents=contents)
+        age_file(dest, 100)
+        original_write_time = get_write_time(dest)
+        loader = self.make_file(contents=contents)
+        install_bootloader(loader, dest)
+        self.assertThat(dest, FileContains(contents))
+        self.assertEqual(original_write_time, get_write_time(dest))
+
+    def test_install_bootloader_sweeps_aside_dot_new_if_any(self):
+        contents = factory.getRandomString()
+        loader = self.make_file(contents=contents)
+        dest = self.make_file(contents="Old contents")
+        temp_file = '%s.new' % dest
+        factory.make_file(
+            os.path.dirname(temp_file), name=os.path.basename(temp_file))
+        install_bootloader(loader, dest)
+        self.assertThat(dest, FileContains(contents))