[Merge] lp:~jamesodhunt/ubuntu/vivid/ubuntu-core-upgrader/handle-invalid-removed-file into lp:ubuntu/ubuntu-core-upgrader
Michael Vogt
michael.vogt at canonical.com
Thu Apr 9 13:57:17 UTC 2015
Review: Needs Information
Thanks, looks good, one question about the strip() below seems to have slipped through, would be nice to get this clarified in some way. Otherwise fine and ready to go.
Diff comments:
> === modified file 'debian/changelog'
> --- debian/changelog 2015-03-13 08:14:06 +0000
> +++ debian/changelog 2015-04-08 14:54:30 +0000
> @@ -1,3 +1,12 @@
> +ubuntu-core-upgrader (0.7.8) UNRELEASED; urgency=medium
> +
> + * ubuntucoreupgrader/upgrader.py: Tolerate an invalid 'removed' file
> + to avoid the upgrade failing attempting to remove '/writable/cache'
> + (see LP: #1437225).
> + * functional/test_upgrader.py: Add tests for an invalid removed file.
> +
> + -- James Hunt <james.hunt at ubuntu.com> Fri, 27 Mar 2015 09:33:15 +0000
> +
> ubuntu-core-upgrader (0.7.7) vivid; urgency=low
>
> * fix entry point for s-i 3.0
>
> === modified file 'functional/test_upgrader.py'
> --- functional/test_upgrader.py 2015-03-04 16:27:30 +0000
> +++ functional/test_upgrader.py 2015-04-08 14:54:30 +0000
> @@ -25,7 +25,6 @@
> import os
> import logging
> import unittest
> -import shutil
>
> from ubuntucoreupgrader.upgrader import (
> Upgrader,
> @@ -37,7 +36,6 @@
> sys.path.append(base_dir)
>
> from ubuntucoreupgrader.tests.utils import (
> - make_tmp_dir,
> append_file,
> TEST_DIR_MODE,
> create_file,
> @@ -68,21 +66,11 @@
> args.append(command_file)
> commands = file_to_list(command_file)
>
> - cache_dir = make_tmp_dir()
> -
> - def mock_get_cache_dir():
> - cache_dir = update.tmp_dir
> - sys_dir = os.path.join(cache_dir, 'system')
> - os.makedirs(sys_dir, exist_ok=True)
> - return cache_dir
> -
> upgrader = Upgrader(parse_args(args), commands, [])
> - upgrader.get_cache_dir = mock_get_cache_dir
> + upgrader.cache_dir = root_dir
> upgrader.MOUNTPOINT_CMD = "true"
> upgrader.run()
>
> - shutil.rmtree(cache_dir)
> -
>
> def create_device_file(path, type='c', major=-1, minor=-1):
> '''
> @@ -216,7 +204,10 @@
> self.assertTrue(os.path.exists(file_path))
> self.assertTrue(os.path.isfile(file_path))
>
> - call_upgrader(cmd_file, self.victim_dir, self.update)
> + # remove 'system' suffix that upgrader will add back on
> + vdir = os.path.split(self.victim_dir)[0]
> +
> + call_upgrader(cmd_file, vdir, self.update)
>
> self.assertFalse(os.path.exists(file_path))
>
> @@ -240,7 +231,8 @@
> self.assertTrue(os.path.exists(dir_path))
> self.assertTrue(os.path.isdir(dir_path))
>
> - call_upgrader(cmd_file, self.victim_dir, self.update)
> + vdir = os.path.split(self.victim_dir)[0]
> + call_upgrader(cmd_file, vdir, self.update)
>
> self.assertFalse(os.path.exists(dir_path))
>
> @@ -272,7 +264,8 @@
> self.assertTrue(os.path.exists(link_file_path))
> self.assertTrue(os.path.islink(link_file_path))
>
> - call_upgrader(cmd_file, self.victim_dir, self.update)
> + vdir = os.path.split(self.victim_dir)[0]
> + call_upgrader(cmd_file, vdir, self.update)
>
> # original file should still be there
> self.assertTrue(os.path.exists(src_file_path))
> @@ -310,7 +303,8 @@
> self.assertTrue(os.path.exists(link_file_path))
> self.assertTrue(os.path.islink(link_file_path))
>
> - call_upgrader(cmd_file, self.victim_dir, self.update)
> + vdir = os.path.split(self.victim_dir)[0]
> + call_upgrader(cmd_file, vdir, self.update)
>
> # original directory should still be there
> self.assertTrue(os.path.exists(src_dir_path))
> @@ -352,7 +346,8 @@
>
> self.assertTrue(src_inode == link_inode)
>
> - call_upgrader(cmd_file, self.victim_dir, self.update)
> + vdir = os.path.split(self.victim_dir)[0]
> + call_upgrader(cmd_file, vdir, self.update)
>
> # original file should still be there
> self.assertTrue(os.path.exists(src_file_path))
> @@ -392,9 +387,11 @@
> # device because it won't be :)
> self.assertTrue(os.path.isfile(file_path))
>
> - call_upgrader(cmd_file, self.victim_dir, self.update)
> + vdir = os.path.split(self.victim_dir)[0]
> + call_upgrader(cmd_file, vdir, self.update)
>
> - self.assertFalse(os.path.exists(file_path))
> + # upgrader doesn't currently remove device files
> + self.assertTrue(os.path.exists(file_path))
>
>
> class UpgraderFileAddTestCase(UbuntuCoreUpgraderTestCase):
> @@ -595,6 +592,67 @@
> self.assertTrue(is_sym_link_broken(victim_link_file_path))
>
>
> +class UpgraderRemoveFileTests(UbuntuCoreUpgraderTestCase):
> +
> + def common_removed_file_test(self, contents):
> + '''
> + Common code to test for an invalid removed file.
> +
> + The contents parameter specifies the contents of the removed
> + file.
> + '''
> + file = 'created-regular-file'
> +
> + create_file(self.update.removed_file, contents)
> +
> + file_path = os.path.join(self.update.system_dir, file)
> + create_file(file_path, 'foo bar')
> +
> + archive = self.update.create_archive(self.TARFILE)
> + self.assertTrue(os.path.exists(archive))
> +
> + cmd_file = os.path.join(self.update.tmp_dir, CMD_FILE)
> + make_command_file(cmd_file, [self.TARFILE])
> +
> + vdir = os.path.split(self.victim_dir)[0]
> +
> + file_path = os.path.join(vdir, file)
> + self.assertFalse(os.path.exists(file_path))
> +
> + # XXX: There is an implicit test here since if the upgrader
> + # fails (as documented on LP: #1437225), this test will also
> + # fail.
> + call_upgrader(cmd_file, vdir, self.update)
> +
> + self.assertTrue(os.path.exists(vdir))
> + self.assertTrue(os.path.exists(self.victim_dir))
> + self.assertTrue(os.path.exists(file_path))
> +
> + # ensure the empty removed file hasn't removed the directory the
> + # unpack applies to.
> + self.assertTrue(self.victim_dir)
> +
> + def test_removed_file_empty(self):
> + '''
> + Ensure the upgrader ignores an empty 'removed' file.
> + '''
> + self.common_removed_file_test('')
> +
> + def test_removed_file_space(self):
> + '''
> + Ensure the upgrader handles a 'removed' file containing just a
> + space.
> + '''
> + self.common_removed_file_test(' ')
> +
> + def test_removed_file_nl(self):
> + '''
> + Ensure the upgrader handles a 'removed' file containing just a
> + newline
> + '''
> + self.common_removed_file_test('\n')
> +
> +
> def main():
> kwargs = {}
> format = \
>
> === modified file 'ubuntucoreupgrader/tests/test_upgrader.py'
> --- ubuntucoreupgrader/tests/test_upgrader.py 2015-03-04 16:50:08 +0000
> +++ ubuntucoreupgrader/tests/test_upgrader.py 2015-04-08 14:54:30 +0000
> @@ -196,15 +196,6 @@
>
> class UbuntuCoreUpgraderObectTestCase(UbuntuCoreUpgraderTestCase):
>
> - def mock_get_cache_dir(self):
> - '''
> - Mock to allow the tests to control the cache directory location.
> - '''
> - assert(self.cache_dir)
> - self.sys_dir = os.path.join(self.cache_dir, 'system')
> - os.makedirs(self.sys_dir, exist_ok=True)
> - return self.cache_dir
> -
> def test_create_object(self):
> root_dir = make_tmp_dir()
>
> @@ -233,7 +224,6 @@
> options.root_dir = root_dir
>
> upgrader = Upgrader(options, commands, [])
> - upgrader.get_cache_dir = self.mock_get_cache_dir
> upgrader.MOUNTPOINT_CMD = "true"
> upgrader.run()
>
> @@ -242,6 +232,52 @@
>
> shutil.rmtree(root_dir)
>
> + def test_empty_removed_file(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
> +
> + removed_file = self.update.removed_file
> + # Create an empty removed file
> + create_file(removed_file, '')
> +
> + 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.cache_dir = self.cache_dir
> + upgrader.MOUNTPOINT_CMD = "true"
> +
> + si_file = self.TARFILE
> + si_signature = "{}.asc".format(self.TARFILE)
> + upgrader._cmd_update([si_file, si_signature])
> +
> + # Ensure that the upgrader has not attempted to remove the
> + # cache_dir when the 'removed' file is empty.
> + #
> + # (Regression test for LP: #1437225).
> + self.assertTrue(os.path.exists(upgrader.cache_dir))
> +
> + shutil.rmtree(self.cache_dir)
>
> if __name__ == "__main__":
> unittest.main()
>
> === modified file 'ubuntucoreupgrader/tests/utils.py'
> --- ubuntucoreupgrader/tests/utils.py 2015-03-04 16:50:08 +0000
> +++ ubuntucoreupgrader/tests/utils.py 2015-04-08 14:54:30 +0000
> @@ -119,7 +119,8 @@
> '''
> # all files listed in the removed list must be system files
> final = list(map(lambda a:
> - '{}{}'.format(self.TEST_SYSTEM_DIR, a), removed_files))
> + os.path.normpath('{}{}'.format(self.TEST_SYSTEM_DIR,
> + a)), removed_files))
>
> contents = "".join(final)
> append_file(self.removed_file, contents)
> @@ -194,7 +195,7 @@
>
> This creates 2 temporary directories:
>
> - - self.system dir: Used as to generate an update archive from.
> + - 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
> @@ -244,7 +245,9 @@
>
> # The directory which will have the update archive applied to
> # it.
> - self.victim_dir = make_tmp_dir(tag='victim')
> + self.victim_dir_base = make_tmp_dir(tag='victim')
> + self.victim_dir = os.path.join(self.victim_dir_base, 'system')
> + os.makedirs(self.victim_dir, mode=0o750)
>
> def tearDown(self):
> '''
>
> === modified file 'ubuntucoreupgrader/upgrader.py'
> --- ubuntucoreupgrader/upgrader.py 2015-03-05 13:24:40 +0000
> +++ ubuntucoreupgrader/upgrader.py 2015-04-08 14:54:30 +0000
> @@ -472,29 +472,26 @@
> # Note: Only used by UPGRADE_IN_PLACE.
> self.other_is_empty = False
>
> + # cache_dir is used as a scratch pad, for downloading new images
> + # to and bind mounting the rootfs.
> + if self.options.tmpdir:
> + self.cache_dir = self.options.tmpdir
> + else:
> + self.cache_dir = self.DEFAULT_CACHE_DIR
> +
> def update_timestamp(self):
> '''
> Update the timestamp file to record the time the last upgrade
> completed successfully.
> '''
> - file = os.path.join(self.get_cache_dir(), self.TIMESTAMP_FILE)
> + file = os.path.join(self.cache_dir, self.TIMESTAMP_FILE)
> open(file, 'w').close()
>
> - def get_cache_dir(self):
> - '''
> - Returns the full path to the cache directory, which is used as a
> - scratch pad, for downloading new images to and bind mounting the
> - rootfs.
> - '''
> - return self.options.tmpdir \
> - if self.options.tmpdir \
> - else self.DEFAULT_CACHE_DIR
> -
> def get_mount_target(self):
> '''
> Get the full path to the mount target directory.
> '''
> - return os.path.join(self.get_cache_dir(), self.MOUNT_TARGET)
> + return os.path.join(self.cache_dir, self.MOUNT_TARGET)
>
> def prepare(self):
> '''
> @@ -740,13 +737,17 @@
> to_remove = []
>
> if not self.full_image:
> -
> if found_removed_file:
> log.debug('processing {} file'
> .format(self.removed_file))
>
> # process backwards to work around bug LP:#1381134.
> for remove in sorted(to_remove, reverse=True):
> +
> + if not remove:
> + # handle invalid removed file entry (see LP:#1437225)
> + continue
> +
> remove = remove.strip()
See my previous review, the check for a empty line before doing the strip() looks odd, usually its the other way around. If this is intentional please add a small comment why its done this way.
>
> # don't remove devices
> @@ -762,13 +763,7 @@
> if '../' in remove:
> continue
>
> - 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 = os.path.join(self.cache_dir, remove)
>
> if not os.path.exists(final):
> # This scenario can only mean there is a bug
> @@ -804,9 +799,9 @@
> from unittest.mock import patch
> with patch("grp.getgrnam") as m:
> m.side_effect = KeyError()
> - tar.extractall(path=self.get_cache_dir(),
> + tar.extractall(path=self.cache_dir,
> members=tar_generator(
> - tar, self.get_cache_dir(),
> + tar, self.cache_dir,
> self.removed_file,
> self.options.root_dir))
> tar.close()
> @@ -821,7 +816,7 @@
> '''
> target = self.get_mount_target()
> bindmount_rootfs_dir = tempfile.mkdtemp(prefix=script_name,
> - dir=self.get_cache_dir())
> + dir=self.cache_dir)
> bind_mount("/", bindmount_rootfs_dir)
>
> cwd = os.getcwd()
>
--
https://code.launchpad.net/~jamesodhunt/ubuntu/vivid/ubuntu-core-upgrader/handle-invalid-removed-file/+merge/254372
Your team Ubuntu branches is subscribed to branch lp:ubuntu/ubuntu-core-upgrader.
More information about the Ubuntu-reviews
mailing list