Merge lp:~jtv/maas/import-to-tftp into lp:maas/trunk

[Jeroen T. Vermeulen]

Script to import kernels/initrds for install netboots.

See the “PXE boot requirements” document for the reasoning and conventions behind this branch. The branch adds a script that downloads the files that MAAS needs to provide over TFTP for a node to boot into the installer.

(Only the kernel and initrd are needed for this use-case; the minimal netbooted system will download the installer from an Ubuntu archive and start executing it.)

In a Cobblerless world this script replaces most, but not quite all, of what the existing maas-import-isos script does. There's still the preseeds and kernel parameters to be dealt with, but I think those will need to be done with knowledge from the database. My guess right now is that it can be done separately from the downloads.

There is still one problem with this script: a race condition. Updates are skipped when there are no changes, and the files are replaced as near-atomically as possible, but that doesn't help downloads that are ongoing at the time. Even if tftpd keeps file handles open for ongoing downloads (not too likely for a stateless UDP-based protocol!) so that each file will produce a consistent byte stream, the download is still 2 files, so a node could still see a mismatch between the kernel and the initrd. We agreed to tolerate this for now, but I'll file a bug.

For testing purposes I had to introduce a test dependency on curl. Amazingly, wget does not support the “file://” URLs I needed for my test. So for testing (and testing only) I substitute curl. The script is written such that it should work with either.

Jeroen

[Robie Basak (racb)]

This is really readable and shouldn't be too difficult to add ARM support to cleanly - thanks!

Can we please use "generic" instead of "base"? "base" sounds like it is used as the foundation for something else which I think is a bit misleading. The armhf subarchitectures correspond to kernel flavours and "generic" is the standard i386/amd64 flavour that we are moving to so I think this makes sense.

ARCHIVE=${ARCHIVE:-http://archive.ubuntu.com/ubuntu/dists/} should skip the dists at the end - the standard way of specifying an archive is one level higher than this, without the dists. Then compose_installer_download_url would insert "dists" into the URL instead.

ARM support:

I'm happy to work on these as a separate merge request. Here are the changes I think we'll need:

The list of supported arches will need to grow from amd64 and i386 to armhf/highbank, armhf/armadaxp etc. At this point, it might be cleanest to specify ARCHES as "amd64/generic i386/generic armhf/highbank" etc.

The archive needs to be selected based on the architecture in use, in order to switch from archive.u.c/ubuntu to ports.u.c/ubuntu-ports. Can this be made into a function, rather than directly a variable?

compose_installer_download_url will need to use a lookup table as the exact URL differs based on arch and release. The needed URLs don't necessary end in pxelinux.0, linux and initrd.gz though, so I think this needs to be a bit more specialised and return the actual URL of the kernel image, initrd image and pxelinux.0 instead. For example: the armadaxp kernel image is called uImage because it is in a different format.

[Raphaël Badin (rvb)]

Looks good. A few remarks:

[0]

99 + if [ -f $dest/pxelinux.0 ] && cmp pxelinux.0 $dest/pxelinux.0

I think you want to add "--quiet" to "cmp pxelinux.0 $dest/pxelinux.0" since you only care about the return code.

[1]

124 + rm -rf -- $dest/$name.old $dest/$name.new

How about using '-f' when moving files instead of cleaning up the files?

[2]

125 + # Put new downloads next to the old. If any file copying is needed

You've written the doc for this method without mentioning 'downloads'. I realize in practice this is used to move downloaded files but still, for consistency, I think you should s/downloads/directory/ here.

[3]

192 + cd -- $DOWNLOAD_DIR

Please consider using pushd/popd to restore go back to the original directory just before the script exits.

[4]

200 + # Replace the "base" with sub-architecture once we start supporting
201 + # those.
202 + arch_dir="$TFTPROOT/maas/$arch/base"
203 +
204 + mkdir -p -- $arch_dir
[…]
209 + rel_dir="$arch_dir/$release"
210 + mkdir -p -- $rel_dir
211 + update_install_files $rel_dir $arch $release

There is an additional white space on lines 202, 209 and 210.

[5]

259 + with open(os.path.join(path, name)) as infile:

Please consider using "open(os.path.join(path, name), 'rb')" for maximum compatibility.

[6]

196 + umask a+r

That looks like an important feature of the script. Don't you think it's worth a test?

[7]

66 +TFTPROOT=${TFTPROOT:-/var/lib/tftpboot/}
and
202 + arch_dir="$TFTPROOT/maas/$arch/base"

arch_dir will be /var/lib/tftpboot//maas/$arch/base. I know it's not a problem but maybe you'll want to remove that '//'.

[8]

Same "problem" with the download path: it downloads http://archive.ubuntu.com/ubuntu/dists//lucid/main/installer-amd64/current/images/netboot/ubuntu-installer/amd64//linux

[9]

54 +# Ubuntu releases that are to be downloaded.
55 +SUPPORTED_RELEASES=$(distro-info --supported)

Just a question here, is MAAS really supporting pre-precise releases?

[10]

70 +DOWNLOAD=${DOWNLOAD:-wget --no-verbose}

You've used --no-verbose and not --quiet here. This means that the output of this script looks like this: http://paste.ubuntu.com/1042263/. I think it's fine but I just wanted to make sure that this was your intention. The only weird thing is that it says "Updating files for oneiric-i386." after the log of the download is displayed. No biggie though.

[11]

I've noticed you've left the existing 'maas-import-isos' untouched. I understand we probably want to keep it around until the Cobbler replacement is done. But don't you think we should file bugs to make sure we don't forget to cleanup things when the replacement will be completed? I'd like your opinion on this because I've got the exact same dilemma about preseed templates/snippets.

review: Approve

[Jeroen T. Vermeulen (jtv)]

Thanks for the unexpected additional review. I've made the changes you asked for:

> Can we please use "generic" instead of "base"? "base" sounds like it is used
> as the foundation for something else which I think is a bit misleading. The
> armhf subarchitectures correspond to kernel flavours and "generic" is the
> standard i386/amd64 flavour that we are moving to so I think this makes sense.

Just goes to show that no matter how many people you sanity-check with, there's always something you don't know about. I updated the code, and the PXE requirements document as well.

> ARCHIVE=${ARCHIVE:-http://archive.ubuntu.com/ubuntu/dists/} should skip the
> dists at the end - the standard way of specifying an archive is one level
> higher than this, without the dists. Then compose_installer_download_url would
> insert "dists" into the URL instead.

Done. Gratifying to see how few changes that took.

You also mentioned some changes you might want to undertake yourself:

> The list of supported arches will need to grow from amd64 and i386 to
> armhf/highbank, armhf/armadaxp etc. At this point, it might be cleanest to
> specify ARCHES as "amd64/generic i386/generic armhf/highbank" etc.

Yes, something like that. Right now we can't quite do that because these settings are shared with the old, Cobbler-based scripts.

> The archive needs to be selected based on the architecture in use, in order to
> switch from archive.u.c/ubuntu to ports.u.c/ubuntu-ports. Can this be made
> into a function, rather than directly a variable?

I suppose. But it's a worry for later... Other solutions may become apparent in the meantime.

> compose_installer_download_url will need to use a lookup table as the exact
> URL differs based on arch and release. The needed URLs don't necessary end in
> pxelinux.0, linux and initrd.gz though, so I think this needs to be a bit more
> specialised and return the actual URL of the kernel image, initrd image and
> pxelinux.0 instead. For example: the armadaxp kernel image is called uImage
> because it is in a different format.

A bit disheartening, but I guess we'll just have to deal with it when the time comes.

Jeroen

[Jeroen T. Vermeulen (jtv)]

And on to Raphaël's review!

> 99 + if [ -f $dest/pxelinux.0 ] && cmp pxelinux.0 $dest/pxelinux.0
>
> I think you want to add "--quiet" to "cmp pxelinux.0 $dest/pxelinux.0" since
> you only care about the return code.

Oh yes, absolutely. That also means I no longer need to check for the file's existence. I only did that to suppress the output — the documentation for --quiet suggested that the error message for a missing file would still be there, as you'd normally expect anyway, so I didn't think to use it.

> [1]
>
> 124 + rm -rf -- $dest/$name.old $dest/$name.new
>
> How about using '-f' when moving files instead of cleaning up the files?

Won't work for (non-empty) directories. There's even an option to make “mv” act as if the destination were a file, but I couldn't find a way to make that do what I wanted either.

> [2]
>
> 125 + # Put new downloads next to the old. If any file copying is
> needed
>
> You've written the doc for this method without mentioning 'downloads'. I
> realize in practice this is used to move downloaded files but still, for
> consistency, I think you should s/downloads/directory/ here.

Oops. Older incarnation of the code, when this wasn't extracted yet. Fixed.

> [3]
>
> 192 + cd -- $DOWNLOAD_DIR
>
> Please consider using pushd/popd to restore go back to the original directory
> just before the script exits.

I did consider it at the time, but bear in mind that “cd” will only affect the process of the shell that was forked for the purpose of running the script. The caller isn't affected.

> [4]
>
> 200 + # Replace the "base" with sub-architecture once we start
> supporting
> 201 + # those.
> 202 + arch_dir="$TFTPROOT/maas/$arch/base"
> 203 +
> 204 + mkdir -p -- $arch_dir
> […]
> 209 + rel_dir="$arch_dir/$release"
> 210 + mkdir -p -- $rel_dir
> 211 + update_install_files $rel_dir $arch $release
>
> There is an additional white space on lines 202, 209 and 210.

Took me a while to figure it out, since I didn't find any extra space in my editor: I got tabs I never asked for! All fixed now.

> [5]
>
> 259 + with open(os.path.join(path, name)) as infile:
>
> Please consider using "open(os.path.join(path, name), 'rb')" for maximum
> compatibility.

Compatibility with what? Windows? Python 3? Unusual encodings? Given that this is for ASCII text files written from the same tests, what's the benefit of treating them as binary files?

> [6]
>
> 196 + umask a+r
>
> That looks like an important feature of the script. Don't you think it's
> worth a test?

Not particularly, no. It just means that the script will write world-readable files even if your umask doesn't normally do that. It doesn't affect its environment in any other way.

> [7]
>
> 66 +TFTPROOT=${TFTPROOT:-/var/lib/tftpboot/}
> and
> 202 + arch_dir="$TFTPROOT/maas/$arch/base"
>
> arch_dir will be /var/lib/tftpboot//maas/$arch/base. I know it's not a
> problem but maybe you'll want to remove that '//'.

I felt the trailing slash on $TFTPROOT was clearer, but if you have a problem...

[MAAS Lander (maas-lander)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[Raphaël Badin (rvb)]

> Compatibility with what? Windows? Python 3? Unusual encodings? Given that this is for ASCII text files written from
> the same tests, what's the benefit of treating them as binary files?

Yes, Windows. I guess it's a little bit stupid in this instance as you mention but nonetheless, it's probably a good habit.

> Remember: cleanup is a noun, clean up is a verb!

Rarg!

> I don't like to worry too much about these small things in advance: if cleanup fits in naturally with the other work,
> we'll just do it and not worry about it. If we discover that it doesn't, then that is a good time to file a bug
> or a card so we don't remember. Until then, piling up detailed bugs or cards will just cloud our view of the work
> ahead. And as always: no matter how clear-cut a remote problem seems now, expect our view of it to change by the
> time it becomes relevant.

Fair enough.

=== modified file 'HACKING.txt'
--- HACKING.txt	2012-06-08 12:27:18 +0000
+++ HACKING.txt	2012-06-15 13:33:19 +0000
@@ -44,16 +44,16 @@
         python-formencode python-oauth python-oops python-oops-datedir-repo \
         python-twisted python-oops-wsgi python-oops-twisted \
         python-psycopg2 python-yaml python-convoy python-django-south \
-        python-avahi python-dbus python-celery python-tempita
+        python-avahi python-dbus python-celery python-tempita distro-info
 
 Additionally, you need to install the following python libraries
 for development convenience::
 
     $ sudo apt-get install python-sphinx python-lxml python-pocket-lint
 
-If you intend to run the test suite, you also need xvfb and firefox::
+If you intend to run the test suite, you also need a few other things:
 
-    $ sudo apt-get install xvfb firefox avahi-utils
+    $ sudo apt-get install xvfb firefox avahi-utils curl
 
 All other development dependencies are pulled automatically from `PyPI`_
 when buildout runs.

=== added file 'scripts/maas-import-pxe-files'
--- scripts/maas-import-pxe-files	1970-01-01 00:00:00 +0000
+++ scripts/maas-import-pxe-files	2012-06-15 13:33:19 +0000
@@ -0,0 +1,191 @@
+#!/usr/bin/env bash
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+#
+# Download static files needed for net-booting nodes through TFTP:
+# pre-boot loader, kernels, and initrd images.
+#
+# This script downloads the required files into the TFTP home directory
+# (by default, /var/lib/tftpboot).  Run it with the necessarily privileges
+# to write them there.
+
+# Exit immediately if a command exits with a non-zero status.
+set -o errexit
+# Treat unset variables as an error when substituting.
+set -o nounset
+
+# Load settings if available.
+settings="/etc/maas/import_isos"
+[ -r $settings ] && . $settings
+local_settings="$(pwd)/$settings"
+[ -r $local_settings ] && . $local_settings
+
+# Download location for Ubuntu releases.
+ARCHIVE=${ARCHIVE:-http://archive.ubuntu.com/ubuntu/}
+
+# Ubuntu releases that are to be downloaded.
+SUPPORTED_RELEASES=$(distro-info --supported)
+RELEASES=${RELEASES:-$SUPPORTED_RELEASES}
+
+# The current Ubuntu release.
+STABLE_RELEASE=$(distro-info --stable)
+CURRENT_RELEASE=${CURRENT_RELEASE:-$STABLE_RELEASE}
+
+# Supported architectures.
+ARCHES=${ARCHES:-amd64 i386}
+
+# TFTP root directory.  (Don't let the "root" vs. "boot" confuse you.)
+TFTPROOT=${TFTPROOT:-/var/lib/tftpboot}
+
+# Command line to download a resource at a given URL into the current
+# directory.  A wget command line will work here, but curl will do as well.
+DOWNLOAD=${DOWNLOAD:-wget --no-verbose}
+
+
+# Put together a full URL for where the installer files for architecture $1
+# and release $2 can be downloaded.
+compose_installer_download_url() {
+    local arch=$1 release=$2
+    local installer_url="$ARCHIVE/dists/$release/main/installer-$arch"
+    echo "$installer_url/current/images/netboot/ubuntu-installer/$arch/"
+}
+
+
+# Download the pre-boot loader, pxelinux.0, for architecture $2 if it exists,
+# and if so, install it into directory $1.  (Not all architectures need this
+# file, and there's rarely an urgent need for the very latest file, so if
+# the download fails this function just skips it.)
+update_pre_boot_loader() {
+    local dest=$1 arch=$2
+    local url=$(compose_installer_download_url $arch $CURRENT_RELEASE)
+    if ! $DOWNLOAD $url/pxelinux.0
+    then
+        echo "No pre-boot loader for $arch; skipping."
+        return
+    fi
+
+    # If the file has changed, move it into place (replacing any previous
+    # version).  Otherwise, leave the filesystem alone.
+    if [ -f pxelinux.0 ]
+    then
+        if cmp --quiet pxelinux.0 $dest/pxelinux.0
+        then
+            rm -f -- pxelinux.0
+        else
+            echo "Updating pre-boot loader for $arch."
+            mv -- pxelinux.0 $dest/
+        fi
+    fi
+}
+
+
+# Move local directory $1 into directory $2, so that it becomes $2/$1.
+# If a directory of the same name already existed in $2, remove it along
+# with all its contents.
+# This will use "$2/$1.new" and "$2/$1.old" as temporary locations, which
+# will also be deleted.
+install_dir() {
+    local name=$1 dest=$2
+    # Use the "old"/"new" directories to maximize speed and minimize
+    # inconvenience: ensure that the new directory contents are on the
+    # partition they will ultimately need to be on, then move the old
+    # ones out of the way and immediately replace them with the new, and
+    # finally remove the old ones outside of the critical path.
+    # This will not resolve races with ongoing downloads by booting nodes.
+    # It merely minimizes this script's side of the race window.
+    rm -rf -- $dest/$name.old $dest/$name.new
+    # Put new downloads next to the old.  If any file copying is needed
+    # because directories are on different partitions, it happens here.
+    mv -- $name $dest/$name.new
+
+    # Start of the critical window.
+
+    # Move existing directory (if any) out of the way.
+    [ -d $dest/$name ] && mv -- $dest/$name $dest/$name.old
+
+    # Move new directory into place.
+    mv -- $dest/$name.new $dest/$name
+
+    # End of the critical window.
+
+    # Clean up the old directory (if any).
+    rm -rf -- $dest/$name.old
+}
+
+
+# Compare given file names between directories $1 and $2.  Print "yes" if
+# they are all identical, or "no" if any of them are different or missing.
+identical_files() {
+    local lhs=$1 rhs=$2
+    local file
+    shift 2
+    for file in $*
+    do
+        if ! cmp --quiet $lhs/$file $rhs/$file
+        then
+            echo "no"
+            return
+        fi
+    done
+    echo "yes"
+}
+
+
+# Download kernel/initrd for installing Ubuntu release $3 for
+# architecture $2 into directory $1/install.
+update_install_files() {
+    local dest=$1 arch=$2 release=$3
+    local files="initrd.gz linux"
+    local url=$(compose_installer_download_url $arch $release)
+    local file
+
+    mkdir "install"
+    pushd "install" >/dev/null
+    for file in $files
+    do
+        $DOWNLOAD $url/$file
+    done
+    popd >/dev/null
+
+    # TODO: Prevent this change from upsetting any currently booting nodes.
+    if [ $(identical_files "install" $dest/"install" $files) != 'yes' ]
+    then
+        echo "Updating files for $release-$arch."
+        install_dir "install" $dest
+    fi
+}
+
+
+main() {
+    local arch release arch_dir rel_dir
+
+    DOWNLOAD_DIR=$(mktemp -d)
+    echo "Downloading to temporary location $DOWNLOAD_DIR."
+    cd -- $DOWNLOAD_DIR
+
+    # All files we create here are public.  The TFTP user will need to be
+    # able to read them.
+    umask a+r
+
+    for arch in $ARCHES
+    do
+        # Replace the "generic" with sub-architecture once we start
+        # supporting those.
+        arch_dir="$TFTPROOT/maas/$arch/generic"
+
+        mkdir -p -- $arch_dir
+        update_pre_boot_loader $arch_dir $arch
+
+        for release in $RELEASES
+        do
+            rel_dir="$arch_dir/$release"
+            mkdir -p -- $rel_dir
+            update_install_files $rel_dir $arch $release
+        done
+    done
+
+    rm -rf -- $DOWNLOAD_DIR
+}
+
+
+main

=== added file 'src/maas/tests/test_maas_import_pxe_files.py'
--- src/maas/tests/test_maas_import_pxe_files.py	1970-01-01 00:00:00 +0000
+++ src/maas/tests/test_maas_import_pxe_files.py	2012-06-15 13:33:19 +0000
@@ -0,0 +1,202 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the maas-import-pxe-files script."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+import os
+from stat import ST_MTIME
+from subprocess import check_call
+
+from maastesting.factory import factory
+from maastesting.testcase import TestCase
+from testtools.matchers import (
+    FileContains,
+    FileExists,
+    Not,
+    )
+
+
+def make_name(prefix, separator='-'):
+    """Return an arbitrary name, with the given prefix."""
+    return separator.join([prefix, factory.getRandomString(4)])
+
+
+def read_file(path, name):
+    """Return the contents of the file at `path/name`."""
+    with open(os.path.join(path, name)) as infile:
+        return infile.read()
+
+
+def get_write_time(path):
+    """Return last modification time of file at `path`."""
+    return os.stat(path)[ST_MTIME]
+
+
+def backdate(path):
+    """Set the last modification time for the file at `path` to the past."""
+    os.utime(path, (99999, 99999))
+
+
+def compose_download_dir(archive, arch, release):
+    """Locate a bootloader, initrd, and kernel in an archive.
+
+    :param archive: Archive directory (corresponding to the script's ARCHIVE
+        setting, except here it's a filesystem path not a URL).
+    :param arch: Architecture.
+    :param release: Ubuntu release name.
+    :return: Full absolute path to the directory holding the requisite files
+        for this archive, arch, and release.
+    """
+    return os.path.join(
+        archive, 'dists', release, 'main', 'installer-%s' % arch, 'current',
+        'images', 'netboot', 'ubuntu-installer', arch)
+
+
+def compose_tftp_path(tftproot, arch, *path):
+    """Compose path for MAAS TFTP files for given architecture.
+
+    After the TFTP root directory and the architecture, just append any path
+    components you want to drill deeper, e.g. the release name to get to the
+    files for a specific release.
+    """
+    return os.path.join(tftproot, 'maas', arch, 'generic', *path)
+
+
+class TestImportPXEFiles(TestCase):
+
+    def make_downloads(self, release=None, arch=None):
+        """Set up a directory with an image for "download" by the script.
+
+        Returns an "ARCHIVE" URL for the download.
+        """
+        if release is None:
+            release = make_name('release')
+        if arch is None:
+            arch = make_name('arch')
+        archive = self.make_dir()
+        download = compose_download_dir(archive, arch, release)
+        os.makedirs(download)
+        for filename in ['initrd.gz', 'linux', 'pxelinux.0']:
+            factory.make_file(download, filename)
+        return archive
+
+    def call_script(self, archive_dir, tftproot, arch=None, release=None):
+        """Call the maas-download-pxe-files script with given settings.
+
+        The ARCHIVE URL and TFTPROOT path must be set, or the script will try
+        to download from the Ubuntu server and store into the system's real
+        TFTP root directory, respectively.  Both bad ideas.
+        """
+        script = './scripts/maas-import-pxe-files'
+        env = {
+            'ARCHIVE': 'file://%s' % archive_dir,
+            # Substitute curl for wget; it accepts file:// URLs.
+            'DOWNLOAD': 'curl -O --silent',
+            'TFTPROOT': tftproot,
+        }
+        if arch is not None:
+            env['ARCHES'] = arch
+        if release is not None:
+            env['RELEASES'] = release
+            env['CURRENT_RELEASE'] = release
+        with open('/dev/null', 'w') as dev_null:
+            check_call(script, env=env, stdout=dev_null)
+
+    def test_downloads_pre_boot_loader(self):
+        arch = make_name('arch')
+        release = 'precise'
+        archive = self.make_downloads(arch=arch, release=release)
+        tftproot = self.make_dir()
+        self.call_script(archive, tftproot, arch=arch, release=release)
+        tftp_path = compose_tftp_path(tftproot, arch, 'pxelinux.0')
+        download_path = compose_download_dir(archive, arch, release)
+        expected_contents = read_file(download_path, 'pxelinux.0')
+        self.assertThat(tftp_path, FileContains(expected_contents))
+
+    def test_ignores_missing_pre_boot_loader(self):
+        arch = make_name('arch')
+        release = 'precise'
+        archive = self.make_downloads(arch=arch, release=release)
+        download_path = compose_download_dir(archive, arch, release)
+        os.remove(os.path.join(download_path, 'pxelinux.0'))
+        tftproot = self.make_dir()
+        self.call_script(archive, tftproot, arch=arch, release=release)
+        tftp_path = compose_tftp_path(tftproot, arch, 'pxelinux.0')
+        self.assertThat(tftp_path, Not(FileExists()))
+
+    def test_updates_pre_boot_loader(self):
+        arch = make_name('arch')
+        release = 'precise'
+        tftproot = self.make_dir()
+        tftp_path = compose_tftp_path(tftproot, arch, 'pxelinux.0')
+        os.makedirs(os.path.dirname(tftp_path))
+        with open(tftp_path, 'w') as existing_file:
+            existing_file.write(factory.getRandomString())
+        archive = self.make_downloads(arch=arch, release=release)
+        self.call_script(archive, tftproot, arch=arch, release=release)
+        download_path = compose_download_dir(archive, arch, release)
+        expected_contents = read_file(download_path, 'pxelinux.0')
+        self.assertThat(tftp_path, FileContains(expected_contents))
+
+    def test_leaves_pre_boot_loader_untouched_if_unchanged(self):
+        # If pxelinux.0 has not changed between script runs, the old
+        # copy stays in place.
+        arch = make_name('arch')
+        release = 'precise'
+        archive = self.make_downloads(arch=arch, release=release)
+        tftproot = self.make_dir()
+        self.call_script(archive, tftproot, arch=arch, release=release)
+        tftp_path = compose_tftp_path(tftproot, arch, 'pxelinux.0')
+        backdate(tftp_path)
+        original_timestamp = get_write_time(tftp_path)
+        self.call_script(archive, tftproot, arch=arch, release=release)
+        self.assertEqual(original_timestamp, get_write_time(tftp_path))
+
+    def test_downloads_install_image(self):
+        arch = make_name('arch')
+        release = 'precise'
+        archive = self.make_downloads(arch=arch, release=release)
+        tftproot = self.make_dir()
+        self.call_script(archive, tftproot, arch=arch, release=release)
+        tftp_path = compose_tftp_path(
+            tftproot, arch, release, 'install', 'linux')
+        download_path = compose_download_dir(archive, arch, release)
+        expected_contents = read_file(download_path, 'linux')
+        self.assertThat(tftp_path, FileContains(expected_contents))
+
+    def test_updates_install_image(self):
+        arch = make_name('arch')
+        release = 'precise'
+        tftproot = self.make_dir()
+        tftp_path = compose_tftp_path(
+            tftproot, arch, release, 'install', 'linux')
+        os.makedirs(os.path.dirname(tftp_path))
+        with open(tftp_path, 'w') as existing_file:
+            existing_file.write(factory.getRandomString())
+        archive = self.make_downloads(arch=arch, release=release)
+        self.call_script(archive, tftproot, arch=arch, release=release)
+        download_path = compose_download_dir(archive, arch, release)
+        expected_contents = read_file(download_path, 'linux')
+        self.assertThat(tftp_path, FileContains(expected_contents))
+
+    def test_leaves_install_image_untouched_if_unchanged(self):
+        arch = make_name('arch')
+        release = 'precise'
+        archive = self.make_downloads(arch=arch, release=release)
+        tftproot = self.make_dir()
+        self.call_script(archive, tftproot, arch=arch, release=release)
+        tftp_path = compose_tftp_path(
+            tftproot, arch, release, 'install', 'linux')
+        backdate(tftp_path)
+        original_timestamp = get_write_time(tftp_path)
+        self.call_script(archive, tftproot, arch=arch, release=release)
+        self.assertEqual(original_timestamp, get_write_time(tftp_path))