[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
Wed Apr 8 13:57:36 UTC 2015
Thanks, I put some suggestions inline.
Diff comments:
> === modified file 'debian/changelog'
> --- debian/changelog 2015-03-13 08:14:06 +0000
> +++ debian/changelog 2015-04-07 11:06:19 +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-07 11:06:19 +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 UpgraderMiscTests(UbuntuCoreUpgraderTestCase):
Maybe call it "UpgraderRemoveFileTests" ? It seems its pretty specific to this.
> +
> + 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-07 11:06:19 +0000
> @@ -242,6 +242,48 @@
>
> 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])
> +
> + self.assertTrue(os.path.exists(upgrader.cache_dir))
A comment why this is tested would be nice, maybe something like "ensure that the upgrader has not removed the cache_dir when the remove file is empty, regression test for #1437225"
> +
> + 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-07 11:06:19 +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-07 11:06:19 +0000
> @@ -472,12 +472,14 @@
> # Note: Only used by UPGRADE_IN_PLACE.
> self.other_is_empty = False
>
> + self.cache_dir = self.get_cache_dir()
It looks like "get_cache_dir()" is no longer used (exepct here) so maybe remove the function and just put it here? If thats the new way to access the cache_dir? It seems simpler this way.
> +
> 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):
> @@ -494,7 +496,7 @@
> '''
> 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 +742,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()
This "strip()" should probably go before the "if not remove: continue".
>
> # don't remove devices
> @@ -762,13 +768,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 +804,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 +821,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