Merge lp:~mvo/ubuntu/vivid/ubuntu-core-upgrader/more-tests-tweaks into lp:ubuntu/vivid/ubuntu-core-upgrader

[Michael Vogt]

Some tweaks to the tests.

[Michael Vogt (mvo)]

Hrm, looks like LP didn't take the Prerequisite branch into account when generating the diff. Thats a bit disappointing.

[William Grant (wgrant)]

The prereq seems to have been rebased (r13 here and r15 in the prereq seem suspiciously identical).

[Michael Vogt (mvo)]

Hey William, thanks for looking at this and I'm sorry for blaming LP. I think you are right and its a rebase in the prereq thats causing the issue. It will also make merging more difficult I guess (without having tried it).

Unmerged revisions

17. By Michael Vogt on 2015-03-03

merged lp:ubuntu/vivid/ubuntu-core-upgrader

16. By Michael Vogt on 2015-03-03

some more simplifications

15. By James Hunt on 2015-03-03

* Fluff removal.

14. By James Hunt on 2015-03-03

* Review changes.

13. By James Hunt on 2015-03-03

* bin/ubuntu-core-upgrade:
  - parse_args(): Return a dict for easier testing.
  - Update for options dict.
* ubuntucoreupgrader/tests/test_upgrader.py:
  - More tar_generator() tests.
  - Added first Upgrader() object test.
* ubuntucoreupgrader/upgrader.py:
  - Comments.
  - tar_generator():
    - Fix to allow '--root-dir' to work again.
    - Fix bug where path not set (only affecting tests).
  - Updates for accessing self.options dict.
  - Assert root_dir option set.

=== modified file 'bin/ubuntu-core-upgrade'
--- bin/ubuntu-core-upgrade	2015-02-20 17:00:34 +0000
+++ bin/ubuntu-core-upgrade	2015-03-03 16:49:20 +0000
@@ -34,20 +34,14 @@
 import atexit
 import shutil
 import subprocess
-import argparse
 
 from ubuntucoreupgrader.unshare import unshare, UnshareFlags
-from ubuntucoreupgrader.upgrader import Upgrader
+from ubuntucoreupgrader.upgrader import Upgrader, parse_args
 
 # Extension added to the command-file to show the new image is in the
 # process of being applied.
 IN_PROGRESS_SUFFIX = 'applying'
 
-# seconds to wait before a reboot (should one be required)
-DEFAULT_REBOOT_DELAY = 2
-
-DEFAULT_ROOT = '/'
-
 log = logging.getLogger()
 
 
@@ -80,91 +74,6 @@
     log.addHandler(handler)
 
 
-def parse_args():
-    '''
-    Handle command-line options.
-    '''
-
-    parser = argparse.ArgumentParser(description='System image Upgrader')
-
-    parser.add_argument(
-        '--check-reboot',
-        action='store_true',
-        help='''
-        Attempt to determine if a reboot may be required.
-
-        This option is similar to --dry-run: no system changes are made.
-
-        Note that the result of this command cannot be definitive since a
-        reboot would be triggered if a service failed to start (but this
-        option does not attempt to restart any services).
-        ''')
-
-    parser.add_argument(
-        '--clean-only',
-        action='store_true',
-        help='Clean up from a previous upgrader run, but do not upgrade)')
-
-    parser.add_argument(
-        '--debug',
-        nargs='?', const=1, default=0, type=int,
-        help='Dump debug info (specify numeric value to increase verbosity)')
-
-    parser.add_argument(
-        '-n', '--dry-run',
-        action='store_true',
-        help='''
-        Simulate an update including showing processes that have locks
-        on files
-        ''')
-
-    parser.add_argument(
-        '--force-inplace-upgrade',
-        action='store_true',
-        help='Apply an upgrade to current rootfs even if running " \
-        "on dual rootfs system')
-
-    parser.add_argument(
-        '--leave-files',
-        action='store_true',
-        help='Do not remove the downloaded system image files after upgrade')
-
-    parser.add_argument(
-        '--no-reboot',
-        action='store_true',
-        help='Do not reboot even if one would normally be required')
-
-    parser.add_argument(
-        '--reboot-delay',
-        type=int, default=DEFAULT_REBOOT_DELAY,
-        help='''
-        Wait for specified number of seconds before rebooting
-        (default={})
-        '''.format(DEFAULT_REBOOT_DELAY))
-
-    parser.add_argument(
-        '--root-dir',
-        default=DEFAULT_ROOT,
-        help='Specify an alternative root directory (for testing ONLY)')
-
-    parser.add_argument(
-        '--show-other-details',
-        action='store_true',
-        help='Dump the details of the system-image vesion on the " \
-        "other root partition')
-
-    parser.add_argument(
-        '-t', '--tmpdir',
-        help='Specify name for pre-existing temporary directory to use')
-
-    parser.add_argument(
-        'cmdfile', action="store",
-        nargs='?',
-        help='Name of file containing commands to execute')
-
-    return parser.parse_args()
-
-
 def upgrade(options, commands, remove_list):
     upgrader = Upgrader(options, commands, remove_list)
     upgrader.run()
@@ -194,14 +103,14 @@
 
 
 def main():
-    options = parse_args()
+    options = parse_args(sys.argv[1:])
 
-    if options.reboot_delay < 0:
+    if options.reboot_delay and options.reboot_delay < 0:
         sys.exit('ERROR: must specify a positive number')
 
     if not os.path.exists(options.root_dir):
         sys.exit('ERROR: root directory does not exist: {}'
-                 .format(options.root))
+                 .format(options.root_dir))
 
     if options.check_reboot:
         # check options must be inert
@@ -219,9 +128,11 @@
     if not options.cmdfile:
         sys.exit('ERROR: need command file')
 
-    if not os.path.exists(options.cmdfile):
+    cmdfile = options.cmdfile
+
+    if not os.path.exists(cmdfile):
         sys.exit('ERROR: command file does not exist: {}'
-                 .format(options.cmdfile))
+                 .format(cmdfile))
 
     if os.getuid() != 0:
         sys.exit("ERROR: need to be root")

=== modified file 'debian/changelog'
--- debian/changelog	2015-03-03 15:22:49 +0000
+++ debian/changelog	2015-03-03 16:49:20 +0000
@@ -1,3 +1,18 @@
+ubuntu-core-upgrader (0.7.5) UNRELEASED; urgency=medium
+
+  * ubuntucoreupgrader/tests/test_upgrader.py:
+    - More tar_generator() tests.
+    - Added first Upgrader() object test.
+  * ubuntucoreupgrader/upgrader.py:
+    - Comments.
+    - tar_generator():
+      - Fix to allow '--root-dir' to work again.
+      - Fix bug where path not set (only affecting tests).
+    - Updates for accessing self.options dict.
+    - Assert root_dir option set.
+
+ -- James Hunt <[e-mail]>  Mon, 02 Mar 2015 14:59:43 +0000
+
 ubuntu-core-upgrader (0.7.4) vivid; urgency=low
 
   * ubuntucoreupgrader/tests/test_upgrader.py:

=== modified file 'ubuntucoreupgrader/tests/test_upgrader.py'
--- ubuntucoreupgrader/tests/test_upgrader.py	2015-03-03 14:36:01 +0000
+++ ubuntucoreupgrader/tests/test_upgrader.py	2015-03-03 16:49:20 +0000
@@ -1,27 +1,399 @@
-#!/usr/bin/python
+#!/usr/bin/python3
+# -*- coding: utf-8 -*-
+# --------------------------------------------------------------------
+# Copyright © 2014-2015 Canonical Ltd.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+# --------------------------------------------------------------------
+
+# --------------------------------------------------------------------
+# Terminology:
+#
+# - "update archive": a fake system-image "ubuntu-hash.tar.xz" tar
+#   archive.
+# --------------------------------------------------------------------
 
 import tarfile
 import tempfile
 import unittest
+import os
+import shutil
+
+from unittest.mock import patch
 
 from ubuntucoreupgrader.upgrader import (
     tar_generator,
+    Upgrader,
+    parse_args,
 )
 
+script_name = os.path.basename(__file__)
+
+# file mode to use for creating test directories.
+TEST_DIR_MODE = 0o750
+
+CMD_FILE = 'ubuntu_command'
+
+
+def make_default_options():
+    return parse_args([])
+
+
+def make_tmp_dir(tag=None):
+    '''
+    Create a temporary directory.
+    '''
+
+    if tag:
+        prefix = '{}-{}-'.format(script_name, tag)
+    else:
+        prefix = script_name
+
+    return tempfile.mkdtemp(prefix=prefix)
+
 
 class UpgradeTestCase(unittest.TestCase):
 
-    def test_tar_generator_regression_unpack_assets(self):
+    def setUp(self):
+        self.root_dir = make_tmp_dir()
+        self.cache_dir = make_tmp_dir()
+        self.sys_dir = os.path.join(self.cache_dir, 'system')
+        os.makedirs(self.sys_dir)
         tempf = tempfile.TemporaryFile()
-        with tarfile.TarFile(fileobj=tempf, mode="w") as tar:
-            tar.add(__file__, "system/bin/true")
-            tar.add(__file__, "assets/kernel")
-            tar.add(__file__, "unreleated/something")
-            tar.add(__file__, "hardware.yaml")
-
-            result = [m.name for m in tar_generator(tar, "cache_dir", [], "/")]
-            self.assertEqual(
-                result, ["system/bin/true", "assets/kernel", "hardware.yaml"])
+        self.tar = tarfile.TarFile(fileobj=tempf, mode="w")
+
+    def tearDown(self):
+        shutil.rmtree(self.root_dir)
+        shutil.rmtree(self.cache_dir)
+        self.tar.close()
+    
+    def test_tar_generator_unpack_assets(self):
+        # special top-level file that should not be unpacked
+        self.tar.add(__file__, "removed")
+
+        self.tar.add(__file__, "system/bin/true")
+        self.tar.add(__file__, "assets/vmlinuz")
+        self.tar.add(__file__, "assets/initrd.img")
+        self.tar.add(__file__, "unreleated/something")
+        self.tar.add(__file__, "hardware.yaml")
+
+        result = [m.name for m in tar_generator(self.tar, "cache_dir", [], "/")]
+        self.assertEqual(result, ["system/bin/true", "assets/vmlinuz",
+                                  "assets/initrd.img", "hardware.yaml"])
+
+    def test_tar_generator_system_files_unpack(self):
+        os.makedirs(os.path.join(self.root_dir, 'dev'))
+
+        self.tar.add(__file__, "assets/vmlinuz")
+        self.tar.add(__file__, "assets/initrd.img")
+
+        device_a = '/dev/null'
+        path = (os.path.normpath('{}/{}'.format(self.sys_dir, device_a)))
+        touch_file(path)
+        
+        self.assertTrue(os.path.exists(os.path.join(self.root_dir, device_a)))
+
+        # should not be unpacked as already exists
+        self.tar.add(__file__, "system{}".format(device_a))
+        
+        device_b = '/dev/does-not-exist'
+        self.assertFalse(os.path.exists(os.path.join(self.root_dir, device_b)))
+        
+        # should be unpacked as does not exist
+        self.tar.add(__file__, "system{}".format(device_b))
+        
+        expected = ["assets/vmlinuz", "assets/initrd.img",
+                    "dev/does-not-exist"]
+        
+        expected_results = [os.path.join(self.root_dir, file)
+                            for file in expected]
+        
+        result = [m.name for m in
+                  tar_generator(self.tar, self.cache_dir, [], self.root_dir)]
+        self.assertEqual(result, expected_results)
+
+    def test_tar_generator_system_files_remove_before_unpack(self):
+        """
+        Test that the upgrader removes certain files just prior to
+        overwriting them via the unpack.
+        """
+        file = 'a/file'
+        dir = 'a/dir'
+
+        file_path = os.path.normpath('{}/{}'.format(self.root_dir, file))
+        touch_file(file_path)
+        self.assertTrue(os.path.exists(file_path))
+        
+        dir_path = os.path.normpath('{}/{}'.format(self.root_dir, dir))
+        os.makedirs(dir_path)
+        self.assertTrue(os.path.exists(dir_path))
+        
+        self.tar.add(__file__, "system/{}".format(file))
+        
+        expected = [file]
+        expected_results = [os.path.join(self.root_dir, f)
+                            for f in expected]
+        
+        result = [m.name for m in
+                  tar_generator(self.tar, self.cache_dir, [], self.root_dir)]
+        
+        self.assertEqual(result, expected_results)
+        
+        # file should be removed
+        self.assertFalse(os.path.exists(file_path))
+        
+        # directory should not be removed
+        self.assertTrue(os.path.exists(dir_path))
+
+
+def append_file(path, contents):
+    '''
+    Append to a regular file (create it doesn't exist).
+    '''
+    dirname = os.path.dirname(path)
+    os.makedirs(dirname, mode=TEST_DIR_MODE, exist_ok=True)
+
+    with open(path, 'a') as fh:
+        fh.writelines(contents)
+
+        if not contents.endswith('\n'):
+            fh.write('\n')
+
+
+def create_file(path, contents):
+    '''
+    Create a regular file.
+    '''
+    append_file(path, contents)
+
+
+def touch_file(path):
+    '''
+    Create an empty file (creating any necessary intermediate
+    directories in @path.
+    '''
+    create_file(path, '')
+
+
+def make_commands(update_list):
+    """
+    Convert the specified list of update archives into a list of command
+    file commands.
+    """
+    l = []
+
+    # FIXME: we don't currently add a mount verb (which would be more
+    # realistics) since we can't handle that in the tests.
+    # ##l.append('mount system')
+
+    for file in update_list:
+        l.append('update {} {}.asc'.format(file, file))
+
+    # FIXME: we don't currently add an unmount verb (which would be more
+    # realistics) since we can't handle that in the tests.
+    # ##l.append('unmount system')
+
+    return l
+
+
+class UpdateTree():
+    '''
+    Representation of a directory tree that will be converted into an
+    update archive.
+    '''
+    TEST_REMOVED_FILE = 'removed'
+    TEST_SYSTEM_DIR = 'system/'
+
+    def __init__(self):
+        # Directory tree used to construct the tar file from.
+        # Also used to hold the TEST_REMOVED_FILE file.
+        self.dir = make_tmp_dir(tag='UpdateTree-tar-source')
+
+        self.removed_file = os.path.join(self.dir, self.TEST_REMOVED_FILE)
+
+        # Directory to place create/modify files into.
+        self.system_dir = os.path.join(self.dir, self.TEST_SYSTEM_DIR)
+
+        # Directory used to write the generated tarfile to.
+        # This directory should also be used to write the command file
+        # to.
+        self.tmp_dir = make_tmp_dir(tag='UpdateTree-cache')
+
+    def destroy(self):
+        if os.path.exists(self.dir):
+            shutil.rmtree(self.dir)
+
+        if os.path.exists(self.tmp_dir):
+            shutil.rmtree(self.tmp_dir)
+
+    def tar_filter(self, member):
+        '''
+        Function to filter the tarinfo members before creating the
+        archive.
+        '''
+        # members are created with relative paths (no leading slash)
+        path = os.sep + member.name
+
+        if member.name == '/.':
+            return None
+
+        i = path.find(self.dir)
+        assert(i == 0)
+
+        # remove the temporary directory elements
+        # (+1 for the os.sep we added above)
+        member.name = path[len(self.dir)+1:]
+
+        return member
+
+    def create_archive(self, name):
+        '''
+        Create an archive with the specified name from the UpdateTree
+        object. Also creates a fake signature file alongside the archive
+        file since this is currently required by the upgrader (although
+        it is not validated).
+
+        :param name: name of tarfile.
+        :param name: full path to xz archive to create.
+        :return full path to tar file with name @name.
+        '''
+
+        tar_path = os.path.join(self.tmp_dir, name)
+        tar = tarfile.open(tar_path, 'w:xz')
+
+        # We can't just add recursively since that would attempt to add
+        # the parent directory. However, the real update tars don't
+        # include that, and attempting to ignore the parent directory
+        # results in an empty archive. So, walk the tree and add
+        # file-by-file.
+        for path, names, files in os.walk(self.dir):
+            for file in files:
+                full = os.path.join(path, file)
+                tar.add(full, recursive=False, filter=self.tar_filter)
+            if not files and not names:
+                # add (empty) directories
+                tar.add(path, recursive=False, filter=self.tar_filter)
+
+        tar.close()
+
+        signature = '{}.asc'.format(tar_path)
+
+        with open(signature, 'w') as fh:
+            fh.write('fake signature file')
+
+        return tar_path
+
+
+class UbuntuCoreUpgraderTestCase(unittest.TestCase):
+    '''
+    Base class for Upgrader tests.
+    '''
+
+    TARFILE = 'update.tar.xz'
+
+    # result of last test run. Hack to deal with fact that even if a
+    # test fails, unittest still calls .tearDown() (whomever thought
+    # that was a good idea...?)
+    currentResult = None
+
+    def setUp(self):
+        '''
+        Test setup.
+        '''
+        # Create an object to hold the tree that will be converted into
+        # an upgrade archive.
+        self.update = UpdateTree()
+
+        # The directory which will have the update archive applied to
+        # it.
+        self.victim_dir = make_tmp_dir(tag='victim')
+
+    def tearDown(self):
+        '''
+        Test cleanup.
+        '''
+
+        if not self.currentResult.wasSuccessful():
+            # Do not clean up - the only sane option if a test fails.
+            return
+
+        self.update.destroy()
+        self.update = None
+
+        shutil.rmtree(self.victim_dir)
+        self.victim_dir = None
+
+    def run(self, result=None):
+        self.currentResult = result
+        unittest.TestCase.run(self, result)
+
+
+def mock_get_root_partitions_by_label():
+    matches = []
+
+    matches.append(('system-a', '/dev/sda3', '/'))
+    matches.append(('system-b', '/dev/sda4', '/writable/cache/system'))
+
+    return matches
+
+
+def mock_make_mount_private(target):
+    pass
+
+
+class UbuntuCoreUpgraderObectTestCase(UbuntuCoreUpgraderTestCase):
+
+    @patch('ubuntucoreupgrader.upgrader.get_root_partitions_by_label',
+           mock_get_root_partitions_by_label)
+    @patch('ubuntucoreupgrader.upgrader.make_mount_private',
+           mock_make_mount_private)
+    def test_create_object(self):
+        root_dir = make_tmp_dir()
+
+        file = 'created-regular-file'
+
+        file_path = os.path.join(self.update.system_dir, file)
+        create_file(file_path, 'foo bar')
+
+        self.cache_dir = self.update.tmp_dir
+        os.makedirs(os.path.join(self.cache_dir, 'system'))
+
+        archive = self.update.create_archive(self.TARFILE)
+        self.assertTrue(os.path.exists(archive))
+
+        dest = os.path.join(self.cache_dir, os.path.basename(archive))
+        touch_file('{}.asc'.format(dest))
+
+        commands = make_commands([self.TARFILE])
+
+        options = make_default_options()
+
+        # XXX: doesn't actually exist, but the option must be set since
+        # the upgrader looks for the update archives in the directory
+        # where this file is claimed to be.
+        options.cmdfile = os.path.join(self.cache_dir, 'ubuntu_command')
+
+        options.root_dir = root_dir
+
+        upgrader = Upgrader(options, commands, [])
+        Upgrader.get_cache_dir = lambda s: self.cache_dir
+        upgrader.run()
+
+        path = os.path.join(root_dir, file)
+        self.assertTrue(os.path.exists(path))
+
+        shutil.rmtree(root_dir)
 
 
 if __name__ == "__main__":

=== modified file 'ubuntucoreupgrader/upgrader.py'
--- ubuntucoreupgrader/upgrader.py	2015-03-03 14:36:01 +0000
+++ ubuntucoreupgrader/upgrader.py	2015-03-03 16:49:20 +0000
@@ -48,6 +48,7 @@
 import stat
 import errno
 import dbus
+import argparse
 
 from enum import Enum
 from time import sleep
@@ -56,6 +57,11 @@
 
 log = logging.getLogger()
 
+# seconds to wait before a reboot (should one be required)
+DEFAULT_REBOOT_DELAY = 2
+
+DEFAULT_ROOT = '/'
+
 # Name of the writable user data partition label as created by
 # ubuntu-device-flash(1).
 WRITABLE_DATA_LABEL = 'writable'
@@ -87,7 +93,6 @@
 TAR_FILE_SYSTEM_PREFIX = 'system/'
 
 # boot assets (kernel, initrd, .dtb's)
-# (XXX: note no trailing slash to ensure we unpack the directory itself).
 TAR_FILE_ASSETS_PREFIX = 'assets/'
 
 # the file that describes the boot assets
@@ -98,6 +103,93 @@
 SYSTEM_IMAGE_CHANNEL_CONFIG = '/etc/system-image/channel.ini'
 
 
+def parse_args(args):
+    '''
+    Handle command-line options.
+
+    Returns: dict of command-line options.
+    '''
+
+    parser = argparse.ArgumentParser(description='System image Upgrader')
+
+    parser.add_argument(
+        '--check-reboot',
+        action='store_true',
+        help='''
+        Attempt to determine if a reboot may be required.
+
+        This option is similar to --dry-run: no system changes are made.
+
+        Note that the result of this command cannot be definitive since a
+        reboot would be triggered if a service failed to start (but this
+        option does not attempt to restart any services).
+        ''')
+
+    parser.add_argument(
+        '--clean-only',
+        action='store_true',
+        help='Clean up from a previous upgrader run, but do not upgrade)')
+
+    parser.add_argument(
+        '--debug',
+        nargs='?', const=1, default=0, type=int,
+        help='Dump debug info (specify numeric value to increase verbosity)')
+
+    parser.add_argument(
+        '-n', '--dry-run',
+        action='store_true',
+        help='''
+        Simulate an update including showing processes that have locks
+        on files
+        ''')
+
+    parser.add_argument(
+        '--force-inplace-upgrade',
+        action='store_true',
+        help='Apply an upgrade to current rootfs even if running " \
+        "on dual rootfs system')
+
+    parser.add_argument(
+        '--leave-files',
+        action='store_true',
+        help='Do not remove the downloaded system image files after upgrade')
+
+    parser.add_argument(
+        '--no-reboot',
+        action='store_true',
+        help='Do not reboot even if one would normally be required')
+
+    parser.add_argument(
+        '--reboot-delay',
+        type=int, default=DEFAULT_REBOOT_DELAY,
+        help='''
+        Wait for specified number of seconds before rebooting
+        (default={})
+        '''.format(DEFAULT_REBOOT_DELAY))
+
+    parser.add_argument(
+        '--root-dir',
+        default=DEFAULT_ROOT,
+        help='Specify an alternative root directory (for testing ONLY)')
+
+    parser.add_argument(
+        '--show-other-details',
+        action='store_true',
+        help='Dump the details of the system-image vesion on the " \
+        "other root partition')
+
+    parser.add_argument(
+        '-t', '--tmpdir',
+        help='Specify name for pre-existing temporary directory to use')
+
+    parser.add_argument(
+        'cmdfile', action="store",
+        nargs='?',
+        help='Name of file containing commands to execute')
+
+    return parser.parse_args(args=args)
+
+
 def remove_prefix(path, prefix=TAR_FILE_SYSTEM_PREFIX):
     '''
     Remove specified prefix from path and return the result.
@@ -733,7 +825,7 @@
 def tar_generator(tar, cache_dir, removed_files, root_dir):
     '''
     Generator function to handle extracting members from the system
-    image tar file.
+    image tar files.
     '''
     for member in tar:
 
@@ -749,6 +841,11 @@
         #   extracted!
         device_prefix = '{}dev/'.format(TAR_FILE_SYSTEM_PREFIX)
 
+        # The partition to update is mounted at "cache_dir/system".
+        # However, the unpack occurs in directory cache_dir (since the
+        # tar files prefixes all system files with
+        # TAR_FILE_SYSTEM_PREFIX). For example, member.name may be
+        # something like "system/dev/null".
         mount_path = '{}/{}'.format(cache_dir, member.name)
 
         unpack = True
@@ -762,56 +859,72 @@
             # device already exists
             unpack = False
 
-        if not (member.name == TAR_FILE_HARDWARE_YAML or
-                member.name.startswith(TAR_FILE_SYSTEM_PREFIX) or
-                member.name.startswith(TAR_FILE_ASSETS_PREFIX)):
+        member_prefix = None
+
+        if member.name.startswith(TAR_FILE_SYSTEM_PREFIX):
+            member_prefix = TAR_FILE_SYSTEM_PREFIX
+        elif member.name.startswith(TAR_FILE_ASSETS_PREFIX):
+            member_prefix = TAR_FILE_ASSETS_PREFIX
+        elif member.name == TAR_FILE_HARDWARE_YAML:
+            member_prefix = "/"
+
+        if not member_prefix:
             # unrecognised prefix directory
             unpack = False
 
         if not unpack:
             log.debug('not unpacking file {}'.format(member.name))
+            continue
+
+        # A modified root directory requires convering
+        # absolute paths to be located below the modified root
+        # directory.
+        if root_dir != '/':
+            if member_prefix == TAR_FILE_SYSTEM_PREFIX:
+                base = remove_prefix(member.name, prefix=member_prefix)
+                member.name = os.path.normpath('{}/{}'.format(root_dir,
+                                                              base))
+            else:
+                member.name = os.path.normpath('{}/{}'.format(root_dir,
+                                                              member.name))
+
+            if member.type in (tarfile.LNKTYPE, tarfile.SYMTYPE) \
+                    and member.linkname.startswith('/'):
+                # Hard and symbolic links also need their
+                # 'source' updated to take account of the root
+                # directory.
+                #
+                # But rather than remove the prefix, we add the
+                # root directory as a prefix to contain the link
+                # within that root.
+                base = os.path.join(root_dir, member.linkname)
+
+                member.linkname = '{}{}'.format(root_dir, base)
+
+            path = member.name
         else:
-            # A modified root directory requires convering
-            # absolute paths to be located below the modified root
-            # directory.
-            if root_dir != '/':
-                base = remove_prefix(member.name)
-                member.name = '{}{}'.format(root_dir, base)
-
-                if member.type in (tarfile.LNKTYPE, tarfile.SYMTYPE) \
-                        and member.linkname.startswith('/'):
-                    # Hard and symbolic links also need their
-                    # 'source' updated to take account of the root
-                    # directory.
-                    #
-                    # But rather than remove the prefix, we add the
-                    # root directory as a prefix to contain the link
-                    # within that root.
-                    base = os.path.join(root_dir, member.linkname)
-
-                    member.linkname = '{}{}'.format(root_dir, base)
-            else:
-                path = mount_path
-
-            log.debug('unpacking file {}'.format(member.name))
-
-            # If the file is a binary and that binary is currently
-            # being executed by a process, attempting to unpack it
-            # will result in ETXTBSY (OSError: [Errno 26] Text file
-            # busy). The simplest way around this issue is to unlink
-            # the file just before unpacking it (ideally, we'd catch
-            # the exception and handle it separately). This allows
-            # the unpack to continue, and the running process to
-            # continue to use it's (old) version of the binary until
-            # it's corresponding service is restarted.
-            #
-            # Note that at this point, we already have another copy
-            # of the inode below /lost+found/.
-            if not member.isdir() and os.path.lexists(path):
-                log.debug('removing file {}'.format(path))
-                os.unlink(path)
-
-            yield member
+            path = mount_path
+
+        log.debug('unpacking file {} with path {}'
+                  .format(member.name, path))
+
+        # If the file is a binary and that binary is currently
+        # being executed by a process, attempting to unpack it
+        # will result in ETXTBSY (OSError: [Errno 26] Text file
+        # busy). The simplest way around this issue is to unlink
+        # the file just before unpacking it (ideally, we'd catch
+        # the exception and handle it separately). This allows
+        # the unpack to continue, and the running process to
+        # continue to use it's (old) version of the binary until
+        # it's corresponding service is restarted.
+        #
+        # Note that at this point, we already have another copy
+        # of the inode below /lost+found/.
+        if not member.isdir() and os.path.lexists(path):
+            log.debug('removing file {}'.format(path))
+            os.unlink(path)
+
+        yield member
 
 
 class Upgrader():
@@ -853,6 +966,14 @@
         return os.path.join(self.get_cache_dir(), self.MOUNT_TARGET)
 
     def __init__(self, options, commands, remove_list):
+        """
+        @options list of command-line options.
+        @commands: array of declarative commands to run to apply the new
+         s-i rootfs update.
+        @remove_list: list of files to remove before applying the new
+         image.
+
+        """
         self.dispatcher = {
             'format': self._cmd_format,
             'load_keyring': self._cmd_load_keyring,
@@ -882,6 +1003,8 @@
 
         self.options = options
 
+        assert('root_dir' in self.options)
+
         # array of imperative commands to run, as generated by
         # system-image-cli(1).
         self.commands = commands
@@ -909,8 +1032,6 @@
         if self.options.cmdfile:
             self.file_dir = os.path.dirname(self.options.cmdfile)
 
-        self.systemd = Systemd()
-
         # list of systemd unit D-Bus paths (such as
         # '/org/freedesktop/systemd1/unit/cron_2eservice') that need to
         # be restarted since they have running processes that are
@@ -924,12 +1045,6 @@
         # Note: Only used by UPGRADE_IN_PLACE.
         self.other_is_empty = False
 
-        target = self.get_mount_target()
-
-        # Note that we need the stat of the mountpoint,
-        # *NOT* the device itself.
-        self.mountpoint_stat = os.stat(target)
-
     def set_reboot_reason(self, reason):
         '''
         Set the reboot reason, if not already set. This ensures the
@@ -959,11 +1074,14 @@
         Required setup.
         '''
 
+        target = self.get_mount_target()
+
+        # Note that we need the stat of the mountpoint,
+        # *NOT* the device itself.
+        self.mountpoint_stat = os.stat(target)
+
         self.determine_upgrade_type()
 
-        log.debug('System is using systemd version {}'
-                  .format(self.systemd.version()))
-
         log.debug('system upgrade type is {}'.format(self.upgrade_type))
         if self.options.debug > 1:
             log.debug('current rootfs device is {}'
@@ -1080,10 +1198,11 @@
             log.warning('Not rebooting at user request')
             return
 
+        reboot_delay = self.options.reboot_delay
         # give the admin a chance to see the message
         log.debug('Waiting for {} seconds before rebooting'
-                  .format(self.options.reboot_delay))
-        sleep(self.options.reboot_delay)
+                  .format(reboot_delay))
+        sleep(reboot_delay)
         os.system('/sbin/reboot')
 
     def _cmd_format(self, args):
@@ -1436,7 +1555,7 @@
             # It will either be a sym-link or a bind-mount
             # unrelated to those specified by writable-paths(5).
             if st.st_dev != self.mountpoint_stat.st_dev:
-                if self.options.debug > 1:
+                if self.options.debug and self.options.debug > 1:
                     log.debug('Ignoring cross-FS file: {}'
                               .format(file))
                 continue
@@ -1455,7 +1574,7 @@
             #  - sym links to directories.
             #
             try:
-                if self.options.debug > 1:
+                if self.options.debug and self.options.debug > 1:
                     log.debug('creating directory {}'
                               .format(saved_link_dirname))
                 os.makedirs(saved_link_dirname,
@@ -1631,8 +1750,7 @@
                         # os.path.join() refuses to work if the file
                         # begins with a slash.
                         base = remove_prefix(remove)
-                        final = '{}{}'.format(self.options.root_dir,
-                                              base)
+                        final = '{}{}'.format(self.options.root_dir, base)
 
                     if not os.path.exists(final):
                         # This scenario can only mean there is a bug
@@ -1672,7 +1790,8 @@
                 tar.extractall(path=self.get_cache_dir(),
                                members=tar_generator(
                                    tar, self.get_cache_dir(),
-                                   self.removed_file, self.options.root_dir))
+                                   self.removed_file,
+                                   self.options.root_dir))
         tar.close()
 
         if self.upgrade_type == self.upgrade_types.UPGRADE_AB_PARTITIONS:
@@ -1793,6 +1912,11 @@
         '''
         failed = False
 
+        self.systemd = Systemd()
+
+        log.debug('System is using systemd version {}'
+                  .format(self.systemd.version()))
+
         for file in self.pid_file_map:
             for pid in self.pid_file_map[file]:
                 ret = self.restart_associated_service(pid, file)