[Merge] lp:~jamesodhunt/ubuntu/vivid/ubuntu-core-upgrader/add-more-tests into lp:ubuntu/ubuntu-core-upgrader
Michael Vogt
michael.vogt at canonical.com
Tue Mar 3 11:56:50 UTC 2015
Thanks for this branch. Its great to see that the upgrader gets good tests!
I have some inline comments, mostly about making it nicer and easier to read. I hope you find them useful.
Cheers,
Michael
Diff comments:
> === modified file 'bin/ubuntu-core-upgrade'
> --- bin/ubuntu-core-upgrade 2015-02-20 17:00:34 +0000
> +++ bin/ubuntu-core-upgrade 2015-03-03 10:04:20 +0000
> @@ -59,10 +59,10 @@
>
> atexit.register(shutdown_logging)
>
> - if options.debug:
> + if options['debug']:
> log.setLevel(logging.DEBUG)
>
> - if options.dry_run or options.check_reboot:
> + if options['dry_run'] or options['check_reboot']:
> # use default logger which displays output to stderr only.
> return
>
> @@ -83,6 +83,8 @@
> def parse_args():
If parse_args() would get a new "argv" parameter and move into ubuntucoreupgrader/something.py (maybe upgrader.py or a new main.py) it could be reused in the tests.
Then the make_default_options() function in test_upgrader() would be simpler and just become "return parse_args([])" and when a new option is added, make_default_options would automatically have it.The tests would
This also has the benefit that options.check_reboot is available which looks more pythonic than options['check_reboot'] and it would make the diff smaller (no more options.name -> options["name"] needed).
> '''
> Handle command-line options.
> +
> + Returns: dict of command-line options.
> '''
>
> parser = argparse.ArgumentParser(description='System image Upgrader')
> @@ -162,7 +164,7 @@
> nargs='?',
> help='Name of file containing commands to execute')
>
> - return parser.parse_args()
> + return vars(parser.parse_args())
>
>
> def upgrade(options, commands, remove_list):
> @@ -173,19 +175,19 @@
> def prepare_upgrade(options):
>
> try:
> - with open(options.cmdfile, 'r') as fh:
> + with open(options['cmdfile'], 'r') as fh:
> commands = fh.readlines()
> except:
> - sys.exit('Failed to read command file: {}'.format(options.cmdfile))
> + sys.exit('Failed to read command file: {}'.format(options['cmdfile']))
>
> remove_list = []
>
> setup_logger(options)
>
> # Rename the file to show the upgrade is in progress
> - if not options.dry_run:
> - in_progress = '{}.{}'.format(options.cmdfile, IN_PROGRESS_SUFFIX)
> - shutil.move(options.cmdfile, in_progress)
> + if not options['dry_run']:
> + in_progress = '{}.{}'.format(options['cmdfile'], IN_PROGRESS_SUFFIX)
> + shutil.move(options['cmdfile'], in_progress)
>
> # Remember to remove it
> remove_list.append(in_progress)
> @@ -196,32 +198,34 @@
> def main():
> options = parse_args()
>
> - 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):
> + 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:
> + if options['check_reboot']:
> # check options must be inert
> - options.dry_run = True
> + options['dry_run'] = True
>
> - if options.dry_run or options.root_dir != '/':
> + if options['dry_run'] or options['root_dir'] != '/':
> prepare_upgrade(options)
> sys.exit(0)
>
> - if options.show_other_details:
> + if options['show_other_details']:
> upgrader = Upgrader(options, [], None)
> upgrader.show_other_partition_details()
> sys.exit(0)
>
> - if not options.cmdfile:
> + 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-02-25 19:19:16 +0000
> +++ debian/changelog 2015-03-03 10:04:20 +0000
> @@ -1,3 +1,21 @@
> +ubuntu-core-upgrader (0.7.4) UNRELEASED; urgency=medium
> +
> + * 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.
> +
> + -- James Hunt <james.hunt at ubuntu.com> Mon, 02 Mar 2015 14:59:43 +0000
> +
> ubuntu-core-upgrader (0.7.3) vivid; urgency=low
>
> * fix config.machine_readable to be real json
>
> === modified file 'ubuntucoreupgrader/tests/test_upgrader.py'
> --- ubuntucoreupgrader/tests/test_upgrader.py 2015-02-27 09:46:37 +0000
> +++ ubuntucoreupgrader/tests/test_upgrader.py 2015-03-03 10:04:20 +0000
> @@ -1,25 +1,576 @@
> -#!/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
> +
> +import copy
> +import subprocess
> +
> +from unittest.mock import patch
>
> from ubuntucoreupgrader.upgrader import (
> tar_generator,
> + Upgrader,
> )
>
> +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():
See previous comment, there is potential to simplify this code.
> + options = {}
> +
> + # bools
> + options['dry_run'] = None
> + options['check_reboot'] = False
> + options['force_inplace_upgrade'] = False
> + options['clean_only'] = False
> + options['leave_files'] = False
> + options['no_reboot'] = False
> + options['show_other_details'] = False
> +
> + # ints
> + options['debug'] = 0
> + options['reboot_delay'] = 2
> +
> + # strings
> + options['root_dir'] = '/'
> + options['tmpdir'] = None
> + options['cmdfile'] = None
> +
> + return options
> +
> +
> +def call_upgrader(command_file, root_dir):
I can not find a code reference to this function, it appears this function is not used at all? Maybe a missing commit/push?
> + '''
> + Invoke the upgrader script directly.
> +
> + :param command_file: commands file to drive the upgrader.
> + :param root_dir: Test directory to apply the upgrade to.
> + '''
> + root = os.path.join(os.path.dirname(__file__), '..', '..')
> + upgrader = os.path.join(root, 'bin', 'ubuntu-core-upgrade')
> + env = copy.copy(os.environ)
> + env['PYTHONPATH'] = root
> +
> + args = []
> + args.append(upgrader)
> + args += ['--root-dir', root_dir]
> +
> + tmpdir = make_tmp_dir(tag='cache')
> +
> + target = os.path.join(tmpdir, "system/writable/cache")
> +
> + # the upgrader now expects the target directory to exist
> + os.makedirs(target, mode=TEST_DIR_MODE, exist_ok=True)
> +
> + args += ['--tmpdir', tmpdir]
> + args += ['--debug', '1']
> +
> + # don't delete the archive and command files.
> + # The tests clean up after themselves so they will get removed then,
> + # but useful to have them around to diagnose test failures.
> + args.append('--leave-files')
> +
> + args.append(command_file)
> +
> + subprocess.call(args, env=env)
> +
> +
> +def make_tmp_dir(tag=None):
> + '''
> + Create a temporary directory.
> + '''
> +
> + prefix = '{}-{}-'.format(script_name, tag) \
> + if tag else script_name
This is unusual in python, I find "if tag: prefix.. else prefix=script_name" easier to read.
> +
> + return tempfile.mkdtemp(prefix=prefix)
> +
>
> class UpgradeTestCase(unittest.TestCase):
>
> - def test_tar_generator_regression_unpack_assets(self):
> + def test_tar_generator_unpack_assets(self):
> tempf = tempfile.TemporaryFile()
> with tarfile.TarFile(fileobj=tempf, mode="w") as tar:
> +
> + # special top-level file that should not be unpacked
> + tar.add(__file__, "removed")
> +
> tar.add(__file__, "system/bin/true")
> - tar.add(__file__, "assets/kernel")
> + tar.add(__file__, "assets/vmlinuz")
> + tar.add(__file__, "assets/initrd.img")
> tar.add(__file__, "unreleated/something")
>
> result = [m.name for m in tar_generator(tar, "cache_dir", [], "/")]
> - self.assertEqual(result, ["system/bin/true", "assets/kernel"])
> + self.assertEqual(result, ["system/bin/true", "assets/vmlinuz",
> + "assets/initrd.img"])
> +
> + def test_tar_generator_system_files_unpack(self):
> + tempf = tempfile.TemporaryFile()
> + root_dir = make_tmp_dir()
> + cache_dir = make_tmp_dir()
> +
> + sys_dir = os.path.join(cache_dir, 'system')
> +
> + os.makedirs(os.path.join(root_dir, 'dev'))
> + os.makedirs(sys_dir)
> +
> + with tarfile.TarFile(fileobj=tempf, mode="w") as tar:
> + tar.add(__file__, "assets/vmlinuz")
> + tar.add(__file__, "assets/initrd.img")
> +
> + device_a = '/dev/null'
> + path = (os.path.normpath('{}/{}'.format(sys_dir, device_a)))
> + touch_file(path)
> +
> + self.assertTrue(os.path.exists(os.path.join(root_dir, device_a)))
> +
> + # should not be unpacked as already exists
> + tar.add(__file__, "system{}".format(device_a))
> +
> + device_b = '/dev/does-not-exist'
> + self.assertFalse(os.path.exists(os.path.join(root_dir, device_b)))
> +
> + # should be unpacked as does not exist
> + tar.add(__file__, "system{}".format(device_b))
> +
> + expected = ["assets/vmlinuz", "assets/initrd.img",
> + "dev/does-not-exist"]
> +
> + expected_results = [os.path.join(root_dir, file)
> + for file in expected]
> +
> + result = [m.name for m in
> + tar_generator(tar, cache_dir, [], root_dir)]
> +
> + self.assertEqual(result, expected_results)
> +
> + shutil.rmtree(root_dir)
> + shutil.rmtree(cache_dir)
> +
> + 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.
> + """
> +
> + tempf = tempfile.TemporaryFile()
> + root_dir = make_tmp_dir()
> + cache_dir = make_tmp_dir()
> + sys_dir = os.path.join(cache_dir, 'system')
> +
> + os.makedirs(sys_dir)
> +
> + file = 'a/file'
> + dir = 'a/dir'
> +
> + with tarfile.TarFile(fileobj=tempf, mode="w") as tar:
> +
> + file_path = os.path.normpath('{}/{}'.format(root_dir, file))
> + touch_file(file_path)
> + self.assertTrue(os.path.exists(file_path))
> +
> + dir_path = os.path.normpath('{}/{}'.format(root_dir, dir))
> + os.makedirs(dir_path)
> + self.assertTrue(os.path.exists(dir_path))
> +
> + tar.add(__file__, "system/{}".format(file))
> +
> + expected = [file]
> + expected_results = [os.path.join(root_dir, f)
> + for f in expected]
> +
> + result = [m.name for m in
> + tar_generator(tar, cache_dir, [], 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))
> +
> + shutil.rmtree(root_dir)
> + shutil.rmtree(cache_dir)
> +
> +
> +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.
> + '''
> + os.makedirs(os.path.dirname(path), exist_ok=True)
> + open(path, 'w').close()
You could use "create_file(path, "") here.
> +
> +
> +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
> +
> +
> +def make_command_file(path, update_list):
It appears this function is not used?
> + '''
> + Create a command file that the upgrader processes.
> +
> + :param path: full path to file to create,
> + :param update_list: list of update archives to include.
> + '''
> + l = make_commands(update_list)
> +
> + # flatten
> + contents = "\n".join(l) + '\n'
> +
> + append_file(path, contents)
> +
> +
> +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 add_to_removed_file(self, removed_files):
This function is not used AFAICS. Maybe a missing push/commit?
> + '''
> + Add the specified list of files to the removed file.
> +
> + The 'removed' file is simply a file with a well-known name that
> + contains a list of files (one per line) to be removed from a
> + system before the rest of the update archive is unpacked.
> +
> + :param removed_files: list of file names to add to the removed file.
> +
> + '''
> + # all files listed in the removed list must be system files
> + final = list(map(lambda a:
> + '{}{}'.format(self.TEST_SYSTEM_DIR, a), removed_files))
> +
> + contents = "".join(final)
> + append_file(self.removed_file, contents)
> +
> + 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.
> +
> + Most of the tests follow a standard pattern:
> +
> + 1) Create an UpdateTree object:
> +
> + update = UpdateTree()
> +
> + This creates 2 temporary directories:
> +
> + - self.system_dir: Used to generate an update archive from.
> +
> + - self.tmp_dir: Used to write the generated archive file to. The
> + intention is that this directory should also be used to hold
> + the command file.
> +
> + 2) Removal tests call update.add_to_removed_file(file) to add a
> + particular file to the 'removals' file in the update archive.
> +
> + 3) Create/Modify tests create files below update.system_dir.
> +
> + 4) Create an update archive (which includes the removals file
> + and all files below update.system_dir):
> +
> + archive = update.create_archive(self.TARFILE)
> +
> + 5) Create a command file (which tells the upgrader what to do
> + and which archive files to apply):
> +
> + make_command_file(...)
> +
> + 6) Create a victim directory. This is a temporary directory where
> + the upgrade will happen.
> +
> + 7) Start the upgrade:
> +
> + call_upgrader(...)
This is not called, it seems this got replaced with "Upgrader.run()" now.
> +
> + 8) Perform checks on the victim directory to ensure that upgrade
> + did what was expected.
> +
> + '''
> +
> + 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.
> + '''
> +
> + ok = False
> +
> + if self.currentResult:
> + # was the test that has just been run successful?
> + ok = self.currentResult.wasSuccessful()
Would it make sense to write: "if not self.currentResult.wasSuccessful(): return" here?
> +
> + if not ok:
> + # Do not clean up - the only sane option if a test fails.
> + return
> +
> + self.update.destroy()
> + self.update = None
> +
> + for d in (self.victim_dir,):
Why a loop if the tuple contains only a single entry?
> + shutil.rmtree(d)
> + 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 MockSystemd():
Maybe self.systemd could be created in restart_associated_services() if we don't need it for the AB case?
> + def version(self):
> + return "mock systemd version"
> +
> +cache_dir = None
> +
> +
> +def mock_get_cache_dir(self):
> + '''
> + This is rather nasty - returns the value of a global variable to
If its nasty, maybe it can be avoided?
It seems that this:
upgrader = Upgrader(options, commands, [])
Upgrader.get_cache_dir = mock_get_cache_dir
upgrader.run()
should work in test_upgrader.py if the self.mountpoint_stat = os.stat(target) moves out of __init__ into something like prepare()?
> + allow the cache directory value to be known before the Upgrader
> + object is created.
> + '''
> + assert(cache_dir)
> + self.sys_dir = os.path.join(cache_dir, 'system')
> + os.makedirs(self.sys_dir, exist_ok=True)
> + return cache_dir
> +
> +
> +class UbuntuCoreUpgraderObectTestCase(UbuntuCoreUpgraderTestCase):
> +
> + @patch.object(Upgrader, 'get_cache_dir', mock_get_cache_dir)
> + @patch('ubuntucoreupgrader.upgrader.get_root_partitions_by_label',
> + mock_get_root_partitions_by_label)
> + @patch('ubuntucoreupgrader.upgrader.make_mount_private',
> + mock_make_mount_private)
> + @patch('ubuntucoreupgrader.upgrader.Systemd', MockSystemd)
> + 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')
> +
> + global cache_dir
This can potentially be avoided, see comment for mock_get_cache_dir().
> + cache_dir = make_tmp_dir()
> +
> + archive = self.update.create_archive(self.TARFILE)
> + self.assertTrue(os.path.exists(archive))
> +
> + src = archive
> + dest = os.path.join(cache_dir, os.path.basename(archive))
> + shutil.move(src, dest)
Could the extra move and create of a .asc file (that is also created by create_archive() earlier) be avoided if the cache_dir in line 683 would point to self.update.tmp_dir.
> +
> + 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(cache_dir, 'ubuntu_command')
> +
> + options['root_dir'] = root_dir
> + options['debug'] = 2
Why is debug set here to 2 instead of leaving the default?
> +
> + upgrader = Upgrader(options, commands, [])
> + 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-02-27 09:46:37 +0000
> +++ ubuntucoreupgrader/upgrader.py 2015-03-03 10:04:20 +0000
> @@ -87,7 +87,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/'
>
> TAR_FILE_REMOVED_FILE = 'removed'
> @@ -730,7 +729,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:
>
> @@ -746,6 +745,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
> @@ -759,8 +763,14 @@
> # device already exists
> unpack = False
>
> - if not (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
> +
> + if not member_prefix:
> # unrecognised prefix directory
> unpack = False
It seems like "continue" could be used here (with log message probably).
>
> @@ -771,8 +781,13 @@
> # 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_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('/'):
> @@ -786,10 +801,13 @@
> base = os.path.join(root_dir, member.linkname)
>
> member.linkname = '{}{}'.format(root_dir, base)
> +
> + path = member.name
> else:
> path = mount_path
>
> - log.debug('unpacking file {}'.format(member.name))
> + 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
> @@ -838,8 +856,8 @@
> scratch pad, for downloading new images to and bind mounting the
> rootfs.
> '''
> - return self.options.tmpdir \
> - if self.options.tmpdir \
> + return self.options['tmpdir'] \
> + if self.options['tmpdir'] \
> else self.DEFAULT_CACHE_DIR
>
> def get_mount_target(self):
> @@ -849,6 +867,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,
> @@ -878,6 +904,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
> @@ -902,8 +930,8 @@
>
> # Identify the directory the files the command file refers to
> # live in.
> - if self.options.cmdfile:
> - self.file_dir = os.path.dirname(self.options.cmdfile)
> + if self.options['cmdfile']:
> + self.file_dir = os.path.dirname(self.options['cmdfile'])
>
> self.systemd = Systemd()
>
> @@ -961,26 +989,26 @@
> .format(self.systemd.version()))
>
> log.debug('system upgrade type is {}'.format(self.upgrade_type))
> - if self.options.debug > 1:
> + if self.options['debug'] > 1:
> log.debug('current rootfs device is {}'
> .format(self.current_rootfs_device))
> log.debug('rootfs to update is {}'
> .format(self.rootfs_to_modify))
>
> - if self.options.force_inplace_upgrade:
> + if self.options['force_inplace_upgrade']:
> self.upgrade_type = self.upgrade_types.UPGRADE_IN_PLACE
> self.rootfs_to_modify = self.current_rootfs_device
> log.debug('forcing upgrade type to be {}'
> .format(self.upgrade_type))
>
> - if self.options.dry_run:
> + if self.options['dry_run']:
> return
>
> - if self.options.clean_only:
> + if self.options['clean_only']:
> self.cleanup_inodes()
> return
>
> - if self.options.root_dir != '/':
> + if self.options['root_dir'] != '/':
> # Don't modify root when running in test mode
> return
>
> @@ -989,9 +1017,9 @@
>
> # Necessary since systemd makes the rootfs shared, which allows
> # mount operations visible across all mount namespaces.
> - log.debug('making {} private'.format(self.options.root_dir))
> + log.debug('making {} private'.format(self.options['root_dir']))
>
> - make_mount_private(self.options.root_dir)
> + make_mount_private(self.options['root_dir'])
>
> # Unmount all the bind mounts to avoid any possibility
> # of writing to the writable partition.
> @@ -1009,7 +1037,7 @@
> '''
> self.prepare()
>
> - if self.options.clean_only:
> + if self.options['clean_only']:
> return
>
> for cmdline in self.commands:
> @@ -1032,11 +1060,11 @@
> Final tidy-up.
> '''
>
> - if self.options.dry_run:
> + if self.options['dry_run']:
> self.show_outcome()
> return
>
> - if self.options.leave_files:
> + if self.options['leave_files']:
> log.debug('not removing files')
> else:
> for file in self.remove_list:
> @@ -1044,7 +1072,7 @@
> os.remove(file)
>
> # Don't remount or reboot in test mode.
> - if self.options.root_dir != '/':
> + if self.options['root_dir'] != '/':
> return
>
> if self.upgrade_type == self.upgrade_types.UPGRADE_IN_PLACE:
> @@ -1072,14 +1100,15 @@
> self.upgrade_types.UPGRADE_AB_PARTITIONS:
> return
>
> - if self.options.no_reboot:
> + if self.options['no_reboot']:
> 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):
> @@ -1093,7 +1122,7 @@
> self.full_image = True
>
> # Don't modify the system state
> - if self.options.dry_run or self.options.root_dir != '/':
> + if self.options['dry_run'] or self.options['root_dir'] != '/':
> return
>
> log.warning('ingoring format target {} (unsupported operation)'
> @@ -1423,7 +1452,7 @@
> # save original inode number
> self.file_map[file] = st.st_ino
>
> - if self.options.dry_run:
> + if self.options['dry_run']:
> continue
>
> # File is crosses the filesytem boundary,
> @@ -1432,7 +1461,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
> @@ -1451,7 +1480,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,
> @@ -1484,7 +1513,7 @@
> specified by the files in "exclude_list.
> '''
>
> - if self.options.dry_run:
> + if self.options['dry_run']:
> return
>
> prefix = self.get_saved_link_dir_prefix()
> @@ -1598,7 +1627,7 @@
> # a reboot since the image on that partition is newer.
> self.reboot_reason = self.reboot_reasons.REBOOT_NEW_IMAGE
>
> - if not self.full_image and not self.options.check_reboot:
> + if not self.full_image and not self.options['check_reboot']:
>
> if found_removed_file:
> log.debug('processing {} file'
> @@ -1621,14 +1650,13 @@
> if '../' in remove:
> continue
>
> - if self.options.root_dir == '/':
> + if self.options['root_dir'] == '/':
> final = os.path.join(self.get_cache_dir(), remove)
> else:
> # 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
> @@ -1639,7 +1667,7 @@
> .format(final))
> continue
>
> - if self.options.dry_run:
> + if self.options['dry_run']:
> log.info('DRY-RUN: would remove file {}'.format(final))
> continue
>
> @@ -1653,8 +1681,8 @@
> log.warning('failed to remove {}: {}'
> .format(final, e))
>
> - if self.options.dry_run:
> - if not self.options.check_reboot:
> + if self.options['dry_run']:
> + if not self.options['check_reboot']:
> log.info('DRY-RUN: would apply the following files:')
> tar.list(verbose=True)
> else:
> @@ -1668,7 +1696,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:
> @@ -1697,7 +1726,7 @@
> # more to be done.
> return
>
> - if self.options.check_reboot:
> + if self.options['check_reboot']:
> return
>
> if not self.restart_associated_services():
> @@ -1754,7 +1783,7 @@
>
> if pid == 1:
> # As always, this is special :)
> - if self.options.dry_run:
> + if self.options['dry_run']:
> log.info('DRY-RUN: would restart systemd (pid {})'
> .format(pid))
> return True
> @@ -1769,7 +1798,7 @@
> # trigger a reboot
> return False
>
> - if self.options.dry_run:
> + if self.options['dry_run']:
> log.info('DRY-RUN: would restart service {} (pid {})'
> .format(service, pid))
> return True
>
--
https://code.launchpad.net/~jamesodhunt/ubuntu/vivid/ubuntu-core-upgrader/add-more-tests/+merge/251571
Your team Ubuntu branches is subscribed to branch lp:ubuntu/ubuntu-core-upgrader.
More information about the Ubuntu-reviews
mailing list