launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #09106
[Merge] lp:~jtv/maas/extract-publish-image into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/extract-publish-image into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/extract-publish-image/+merge/111602
I'm sorry about the size of this branch. I moved stuff out into prep branches, really I did. But with the recursion of for-A-I-need-B so deep, I just stopped doing it somewhere. The branch came together in a very top-down way: write an empty stub for the highest-level code, write tests for it, then write stubs for a lower-level function, write tests for that, then implement the function, and repeat for the next lower-level function.
Actually it was about a hundred lines worse. But at the very end I deleted some tests for the “handle” method. These tests were completely duplicative of a series of tests for the maas-import-pxe-files script, except they were massively longer and harder to understand, and required a helper method. I replaced them with a single integration test.
Anyway. What you see here is a custom command for installing a freshly-downloaded netboot image into the TFTP hierarchy, based on its architecture, release, and purpose. (The purpose might be “install” or “commissioning,“ and in principle we might add more later). This isn't entirely as trivial as it sounds, and it will be used by both the maas-import-pxe-files script (which already uses it and will replace maas-import-isos) and the maas-import-ephemerals script.
You'll note that the code tries to avoid unnecessary changes to the filesystem. One big reason for this is that there is actually a race condition that could disrupt node boots that proceed while their images are being updated. This is a known problem: bug 1013590. Things may need to become more involved later.
Jeroen
--
https://code.launchpad.net/~jtv/maas/extract-publish-image/+merge/111602
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/extract-publish-image into lp:maas.
=== modified file 'scripts/maas-import-pxe-files'
--- scripts/maas-import-pxe-files 2012-06-20 16:56:42 +0000
+++ scripts/maas-import-pxe-files 2012-06-22 14:22:44 +0000
@@ -79,62 +79,10 @@
}
-# 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.
+# Download kernel/initrd for installing Ubuntu release $2 for
+# architecture $1 and install it into the TFTP directory hierarchy.
update_install_files() {
- local dest=$1 arch=$2 release=$3
+ local arch=$1 release=$2
local files="initrd.gz linux"
local url=$(compose_installer_download_url $arch $release)
local file
@@ -147,12 +95,9 @@
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
+ maas install_pxe_image \
+ --arch=$arch --release=$release --purpose="install" \
+ --image="install" --pxe-target-dir="$TFTPROOT/maas"
}
@@ -178,9 +123,7 @@
for release in $RELEASES
do
- rel_dir="$arch_dir/$release"
- mkdir -p -- $rel_dir
- update_install_files $rel_dir $arch $release
+ update_install_files $arch $release
done
maas generate_enlistment_pxe \
=== added file 'src/maasserver/management/commands/install_pxe_image.py'
--- src/maasserver/management/commands/install_pxe_image.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/management/commands/install_pxe_image.py 2012-06-22 14:22:44 +0000
@@ -0,0 +1,144 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Install a netboot image directory for TFTP download."""
+
+from __future__ import (
+ absolute_import,
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = [
+ 'Command',
+ ]
+
+from filecmp import cmpfiles
+from optparse import make_option
+import os.path
+from shutil import rmtree
+
+from celeryconfig import PXE_TARGET_DIR
+from django.core.management.base import BaseCommand
+
+
+def make_destination(pxe_target_dir, 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 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)
+ if not os.path.isdir(dest):
+ os.makedirs(dest)
+ return dest
+
+
+def are_identical_dirs(old, new):
+ """Do directories `old` and `new` contain identical files?
+
+ It's OK for `old` not to exist; that is considered a difference rather
+ than an error. But `new` is assumed to exist - if it doesn't, you
+ shouldn't have come far enough to call this function.
+ """
+ assert os.path.isdir(new)
+ if os.path.isdir(old):
+ files = set(os.listdir(old) + os.listdir(new))
+ # The shallow=False is needed to make cmpfiles() compare file
+ # contents. Otherwise it only compares os.stat() results,
+ match, mismatch, errors = cmpfiles(old, new, files, shallow=False)
+ return len(match) == len(files)
+ else:
+ return False
+
+
+def remove_if_exists(directory):
+ """Recursively remove `directory` if it exists."""
+ if os.path.isdir(directory):
+ rmtree(directory)
+
+
+def install_dir(new, old):
+ """Install directory `new`, replacing directory `old` if it exists.
+
+ This works as atomically as possible, but isn't entirely. Moreover,
+ any TFTP downloads that are reading from the old directory during
+ the move may receive inconsistent data, with some of the files (or
+ parts of files!) coming from the old directory and some from the
+ new.
+
+ Some temporary paths will be used that are identical to `old`, but with
+ suffixes ".old" or ".new". If either of these directories already
+ exists, it will be mercilessly deleted.
+ """
+ # Get rid of any leftover temporary directories from potential
+ # interrupted previous runs.
+ remove_if_exists('%s.old' % old)
+ remove_if_exists('%s.new' % old)
+
+ # We have to move the existing directory out of the way and the new
+ # one into place. Between those steps, there is a window where
+ # neither is in place. To minimize that window, move the new one
+ # into the same location (ensuring that it no longer needs copying
+ # from one partition to another) and then swizzle the two as quickly
+ # as possible.
+ os.rename(new, '%s.new' % old)
+
+ # Start of critical window.
+ if os.path.isdir(old):
+ os.rename(old, '%s.old' % old)
+ os.rename('%s.new' % old, old)
+ # End of critical window.
+
+ # Now delete the old image directory at leisure.
+ remove_if_exists('%s.old' % old)
+
+
+class Command(BaseCommand):
+ """Move a netboot image into the TFTP directory structure.
+
+ The image is a directory containing a kernel and an initrd. If the
+ destination location already has an image of the same name and
+ containing identical files, the new image is deleted and the old one
+ is left untouched.
+ """
+
+ option_list = BaseCommand.option_list + (
+ make_option(
+ '--arch', dest='arch', default=None,
+ help="Main system architecture that the image is for."),
+ make_option(
+ '--subarch', dest='subarch', default='generic',
+ help="Sub-architecture of the main architecture."),
+ make_option(
+ '--release', dest='release', default=None,
+ help="Ubuntu release that the image is for."),
+ make_option(
+ '--purpose', dest='purpose', default=None,
+ help="Purpose of the image (e.g. 'install' or 'commissioning')."),
+ make_option(
+ '--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,
+ 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
+
+ dest = make_destination(pxe_target_dir, arch, subarch, release)
+ if are_identical_dirs(os.path.join(dest, purpose), image):
+ # Nothing new in this image. Delete it.
+ rmtree(image)
+ else:
+ # Image has changed. Move the new version into place.
+ install_dir(image, os.path.join(dest, purpose))
=== added file 'src/maasserver/tests/test_commands_install_pxe_image.py'
--- src/maasserver/tests/test_commands_install_pxe_image.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/test_commands_install_pxe_image.py 2012-06-22 14:22:44 +0000
@@ -0,0 +1,174 @@
+# 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_image command."""
+
+from __future__ import (
+ absolute_import,
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = []
+
+import os
+
+from django.core.management import call_command
+from maasserver.management.commands.install_pxe_image import (
+ are_identical_dirs,
+ install_dir,
+ make_destination,
+ )
+from maasserver.testing.factory import factory
+from maasserver.testing.testcase import TestCase
+from testtools.matchers import (
+ DirExists,
+ FileContains,
+ FileExists,
+ Not,
+ )
+
+
+def make_random_string(prefix):
+ """Return an arbitrary string starting with the given prefix."""
+ return '-'.join([prefix, factory.getRandomString(5)])
+
+
+def make_arch_subarch_release():
+ """Create arbitrary architecture/subarchitecture/release names.
+
+ :return: A triplet of three identifiers for these respective items.
+ """
+ return (
+ make_random_string('arch'),
+ make_random_string('subarch'),
+ make_random_string('release'),
+ )
+
+
+class TestInstallPXEImage(TestCase):
+
+ def test_integration(self):
+ download_dir = self.make_dir()
+ image_dir = os.path.join(download_dir, 'image')
+ os.makedirs(image_dir)
+ factory.make_file(image_dir, 'kernel')
+ pxe_target_dir = 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)
+
+ self.assertThat(
+ os.path.join(
+ pxe_target_dir, 'arch', 'subarch', 'release', 'purpose',
+ '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>
+ # (Where the /var/lib/tftproot/maas/ part is configurable, so we
+ # can test this without overwriting system files).
+ pxe_target_dir = 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))
+
+ def test_make_destination_creates_directory_if_not_present(self):
+ pxe_target_dir = 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)
+ self.assertThat(expected_destination, DirExists())
+
+ def test_make_destination_returns_existing_directory(self):
+ pxe_target_dir = self.make_dir()
+ arch, subarch, release = make_arch_subarch_release()
+ expected_dest = os.path.join(pxe_target_dir, 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)
+ self.assertThat(os.path.join(dest, testfile), FileContains(contents))
+
+ def test_are_identical_dirs_sees_missing_old_dir_as_different(self):
+ self.assertFalse(
+ are_identical_dirs(
+ os.path.join(self.make_dir(), factory.getRandomString()),
+ os.path.dirname(self.make_file())))
+
+ def test_are_identical_dirs_returns_true_if_identical(self):
+ name = factory.getRandomString()
+ contents = factory.getRandomString()
+ self.assertTrue(are_identical_dirs(
+ os.path.dirname(self.make_file(name=name, contents=contents)),
+ os.path.dirname(self.make_file(name=name, contents=contents))))
+
+ def test_are_identical_dirs_returns_false_if_file_has_changed(self):
+ name = factory.getRandomString()
+ old = os.path.dirname(self.make_file(name=name))
+ new = os.path.dirname(self.make_file(name=name))
+ self.assertFalse(are_identical_dirs(old, new))
+
+ def test_are_identical_dirs_returns_false_if_file_was_added(self):
+ shared_file = factory.getRandomString()
+ contents = factory.getRandomString()
+ old = os.path.dirname(
+ self.make_file(name=shared_file, contents=contents))
+ new = os.path.dirname(
+ self.make_file(name=shared_file, contents=contents))
+ factory.make_file(new)
+ self.assertFalse(are_identical_dirs(old, new))
+
+ def test_are_identical_dirs_returns_false_if_file_was_removed(self):
+ shared_file = factory.getRandomString()
+ contents = factory.getRandomString()
+ old = os.path.dirname(
+ self.make_file(name=shared_file, contents=contents))
+ new = os.path.dirname(
+ self.make_file(name=shared_file, contents=contents))
+ factory.make_file(old)
+ self.assertFalse(are_identical_dirs(old, new))
+
+ def test_install_dir_moves_dir_into_place(self):
+ download_image = os.path.join(self.make_dir(), 'download-image')
+ published_image = os.path.join(self.make_dir(), 'published-image')
+ contents = factory.getRandomString()
+ os.makedirs(download_image)
+ sample_file = factory.make_file(download_image, contents=contents)
+ install_dir(download_image, published_image)
+ self.assertThat(
+ os.path.join(published_image, os.path.basename(sample_file)),
+ FileContains(contents))
+ self.assertThat(download_image, Not(DirExists()))
+
+ def test_install_dir_replaces_existing_dir(self):
+ download_image = os.path.join(self.make_dir(), 'download-image')
+ published_image = os.path.join(self.make_dir(), 'published-image')
+ os.makedirs(download_image)
+ sample_file = factory.make_file(download_image)
+ os.makedirs(published_image)
+ obsolete_file = factory.make_file(published_image)
+ install_dir(download_image, published_image)
+ self.assertThat(
+ os.path.join(published_image, os.path.basename(sample_file)),
+ FileExists())
+ self.assertThat(obsolete_file, Not(FileExists()))
+ self.assertThat(download_image, Not(DirExists()))
