[Merge] lp:~jamesodhunt/ubuntu/vivid/ubuntu-core-upgrader/implement-format-command into lp:ubuntu/ubuntu-core-upgrader
James Hunt
james.hunt at canonical.com
Wed Apr 8 15:52:06 UTC 2015
I've applied most of the suggestions, but see responses below.
Diff comments:
> === modified file 'debian/changelog'
> --- debian/changelog 2015-03-13 08:14:06 +0000
> +++ debian/changelog 2015-04-07 13:31:57 +0000
> @@ -1,3 +1,10 @@
> +ubuntu-core-upgrader (0.7.8) UNRELEASED; urgency=medium
> +
> + * Implement s-i format command to handle full-image (non-delta) updates
> + correctly.
> +
> + -- James Hunt <james.hunt at ubuntu.com> Tue, 07 Apr 2015 14:29:29 +0100
> +
> ubuntu-core-upgrader (0.7.7) vivid; urgency=low
>
> * fix entry point for s-i 3.0
>
> === 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 13:31:57 +0000
> @@ -27,6 +27,7 @@
> import tarfile
> import tempfile
> import unittest
> +from unittest.mock import patch
> import os
> import shutil
> import sys
> @@ -194,6 +195,18 @@
> return l
>
>
> +def mock_get_mount_details(target):
> + return [True, True, True]
> +
> +
> +def mock_mount(source, target, options=None):
> + pass
> +
> +
> +def mock_unmount(target, options=None):
> + pass
> +
> +
> class UbuntuCoreUpgraderObectTestCase(UbuntuCoreUpgraderTestCase):
>
> def mock_get_cache_dir(self):
> @@ -242,6 +255,49 @@
>
> shutil.rmtree(root_dir)
>
> + @patch('ubuntucoreupgrader.upgrader.get_mount_details',
> + mock_get_mount_details)
> + @patch('ubuntucoreupgrader.upgrader.mount', mock_mount)
> + @patch('ubuntucoreupgrader.upgrader.unmount', mock_unmount)
> + def test_format(self):
> +
> + self.mkfs_called = False
> +
> + def mock_mkfs(device, fs_type, label):
> + self.mkfs_called = True
> +
> + # If the command file does not contain the format command, mkfs
> + # should not be called.
> + with patch('ubuntucoreupgrader.upgrader.mkfs', mock_mkfs):
> + args = ['cmdfile']
> + options = parse_args(args=args)
> + commands = make_commands([self.TARFILE])
> + upgrader = Upgrader(options, commands, [])
> + upgrader.TIMESTAMP_FILE = '/dev/null'
> + upgrader.MOUNTPOINT_CMD = "true"
> + upgrader.run()
> +
> + # No format command in command file, so should not have been
> + # called.
> + self.assertFalse(self.mkfs_called)
> +
> + # mkfs should be called if the format command is specified in
> + # the command file.
> + with patch('ubuntucoreupgrader.upgrader.mkfs', mock_mkfs):
> + args = ['cmdfile']
> + options = parse_args(args=args)
> + commands = make_commands([self.TARFILE])
> +
> + # add format command to command file
> + commands.insert(0, 'format system')
> +
> + upgrader = Upgrader(options, commands, [])
> + upgrader.TIMESTAMP_FILE = '/dev/null'
> + upgrader.MOUNTPOINT_CMD = "true"
> + upgrader.run()
> +
> + self.assertTrue(self.mkfs_called)
> +
>
> if __name__ == "__main__":
> unittest.main()
>
> === modified file 'ubuntucoreupgrader/upgrader.py'
> --- ubuntucoreupgrader/upgrader.py 2015-03-05 13:24:40 +0000
> +++ ubuntucoreupgrader/upgrader.py 2015-04-07 13:31:57 +0000
> @@ -203,6 +203,59 @@
> sys.exit(1)
>
>
> +def mkfs(device, fs_type, label):
> + '''
> + Run mkfs(8) on specified device.
> + '''
> + assert (os.path.exists(device))
> +
> + cmd = '/sbin/mkfs'
> + args = []
> +
> + args.append(cmd)
> + args.append('-v')
> + args += ['-t', fs_type]
> + args += ['-L', label]
> + args.append(device)
> +
> + log.debug('running: {}'.format(args))
> +
> + proc = subprocess.Popen(args,
I'd really like this branch to be a minimal change, in line with the existing code. You've raised a good point, but I don't think the code as-it suffers, and this change is consistent with the other 5 Popen calls.
> + stdout=subprocess.PIPE,
> + stderr=subprocess.PIPE,
> + universal_newlines=True)
> +
> + ret = proc.wait()
> +
> + if ret == 0:
> + return
> +
> + stdout, stderr = proc.communicate()
> +
> + log.error('{} returned {}: {}, {}'
> + .format(args,
> + ret,
> + stdout,
> + stderr))
> + sys.exit(1)
Agreed, but see above - I'd like this to be a minimal change which is in line with the existing code (particularly since this is a critical piece of code and we're very close to a release). Also, we are already calling exit in various other places ("fail fast").
> +
> +
> +def get_mount_details(target):
> + '''
> + Returns a list comprising device, filesystem type and filesystem
> + label for the mount target specified.
> + '''
> + args = ['findmnt', '-o', 'SOURCE,FSTYPE,LABEL', '-n', target]
> + output = subprocess.check_output(args, universal_newlines=True)
> + output = output.strip().split()
> +
> + if len(output) != 3:
> + sys.exit('Failed to determine mount details for {}'
> + .format(target))
> +
> + return output
> +
> +
> def remount(mountpoint, options):
> """
> Remount mountpoint using the specified options string (which is
> @@ -468,10 +521,11 @@
> # If True, the other partition is considered empty. In this
> # scenario, prior to unpacking the latest image, the _current_
> # rootfs image is copied to the other partition.
> - #
> - # Note: Only used by UPGRADE_IN_PLACE.
> self.other_is_empty = False
>
> + # True if the upgrader has reformatted the "other" partition.
> + self.other_has_been_formatted = False
> +
> def update_timestamp(self):
> '''
> Update the timestamp file to record the time the last upgrade
> @@ -566,9 +620,17 @@
> if self.options.dry_run or self.options.root_dir != '/':
> return
>
> - log.warning('ingoring format target {} (unsupported operation)'
> - .format(target))
> - return
> + other = self.get_mount_target()
> +
> + output = get_mount_details(other)
Done.
> +
> + unmount(other)
> + mkfs(output[0], output[1], output[2])
> +
> + # leave the mount as it was initially.
> + mount(output[0], other, "ro")
> +
> + self.other_has_been_formatted = True
>
> def _cmd_load_keyring(self, args):
> try:
> @@ -609,6 +671,19 @@
> .format(target_type))
> return
>
> + if self.other_is_empty and not self.other_has_been_formatted:
Formatting could be extremely slow on some systems, so imho we want to avoid any extra format calls.
> + # We believe "other" is empty. However, it's possible that a
> + # previous attempt at upgrading failed due to a power
> + # outage. In that scenario, "other" may contain most of a
> + # rootfs image, but just be missing the files used to
> + # determine that the partition is empty. As such, if we
> + # believe the partition is empty, it should forcibly be made
> + # empty since it may contain detritus from a
> + # previously-failed unpack (possibly caused by a power
> + # outage).
> + log.warning('reformatting other (no system image)')
> + self._cmd_format("system")
> +
> self.remount_rootfs(writable=True)
>
> if self.other_is_empty:
> @@ -739,6 +814,7 @@
> else:
> to_remove = []
>
> + # by definition, full images don't have 'removed' files.
> if not self.full_image:
>
> if found_removed_file:
> @@ -816,7 +892,7 @@
> Copy all rootfs data from the current partition to the other
> partition.
>
> - XXX: Assumes that the other rootfs is already mounted to
> + XXX: Assumes that the other rootfs is already mounted r/w to
> mountpoint get_mount_target().
> '''
> target = self.get_mount_target()
> @@ -846,4 +922,3 @@
>
> unmount(bindmount_rootfs_dir)
> os.rmdir(bindmount_rootfs_dir)
> - self.other_is_empty = False
>
--
https://code.launchpad.net/~jamesodhunt/ubuntu/vivid/ubuntu-core-upgrader/implement-format-command/+merge/255359
Your team Ubuntu branches is requested to review the proposed merge of lp:~jamesodhunt/ubuntu/vivid/ubuntu-core-upgrader/implement-format-command into lp:ubuntu/ubuntu-core-upgrader.
More information about the Ubuntu-reviews
mailing list