Merge lp:~jamesodhunt/ubuntu/vivid/ubuntu-core-upgrader/implement-format-command into lp:ubuntu/vivid/ubuntu-core-upgrader

[James Hunt]

* Implement s-i format command to handle full-image (non-delta) updates
  correctly.

Details:

* ubuntucoreupgrader/tests/test_upgrader.py:
  - test_format(): New test to ensure mkfs called appropriately.
* ubuntucoreupgrader/upgrader.py:
  - get_mount_details(): New function, called by _cmd_format(), introduced to make testing easier.

[Michael Vogt (mvo)]

Thanks for working on this! I like the tests!

I put some comments inline.

A note about the tests that is too big to fit into the inline comments. The tests can be simplified by using the automatic mocking features from unittest.mock. It provides automatic construction of mock function and asserts to test if the mocks go called (or got called with the right parameters). So something like:
"""
=== modified file 'ubuntucoreupgrader/tests/test_upgrader.py'
--- ubuntucoreupgrader/tests/test_upgrader.py 2015-04-07 13:30:32 +0000
+++ ubuntucoreupgrader/tests/test_upgrader.py 2015-04-08 14:18:03 +0000
@@ -195,18 +195,6 @@
     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):
@@ -255,20 +243,16 @@

         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
-
+ @patch('ubuntucoreupgrader.upgrader.get_mount_details')
+ @patch('ubuntucoreupgrader.upgrader.mount')
+ @patch('ubuntucoreupgrader.upgrader.unmount')
+ def test_format(self, mock_umount, mock_mount, mock_mount_details):
+ MOCK_FS_TUPLE = ("device", "fstype", "label")
+ mock_mount_details.return_value = MOCK_FS_TUPLE
+
         # If the command file does not contain the format command, mkfs
         # should not be called.
- with patch('ubuntucoreupgrader.upgrader.mkfs', mock_mkfs):
+ with patch('ubuntucoreupgrader.upgrader.mkfs') as mock_mkfs:
             args = ['cmdfile']
             options = parse_args(args=args)
             commands = make_commands([self.TARFILE])
@@ -279,11 +263,11 @@

         # No format command in command file, so should not have been
         # called.
- self.assertFalse(self.mkfs_called)
+ self.assertFalse(mock_mkfs.called)

         # mkfs should be called if the format command is specified in
         # the command file.
- with patch('ubuntucoreupgrader.upgrader.mkfs', mock_mkfs):
+ with patch('ubuntucoreupgrader.upgrader.mkfs') as mock_mkfs:
             args = ['cmdfile']
             options = parse_args(args=args)
             commands = make_commands([self.TARFILE])
@@ -296,8 +280,7 @@
             upgrader.MOUNTPOINT_CMD = "true"
             upgrader.run()

- self.assertTrue(self.mkfs_called)
-
+ mock_mkfs.assert_called_with(*MOCK_FS_TUPLE)

 if __name__ == "__main__":
     unittest.main()

"""

[lp:~jamesodhunt/ubuntu/vivid/ubuntu-core-upgrader/implement-format-command]

27. By James Hunt on 2015-04-08

* Review changes.

[James Hunt (jamesodhunt)]

I've applied most of the suggestions, but see responses below.

[Michael Vogt (mvo)]

Thanks for updating the branch. I still feel uneasy about the sys.exit() but as you pointed out this is already done in other places in the code.

=== modified file 'debian/changelog'
--- debian/changelog	2015-03-13 08:14:06 +0000
+++ debian/changelog	2015-04-08 15:51:51 +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 <[e-mail]>  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-08 15:51:51 +0000
@@ -27,6 +27,7 @@
 import tarfile
 import tempfile
 import unittest
+from unittest.mock import patch
 import os
 import shutil
 import sys
@@ -242,6 +243,50 @@
 
         shutil.rmtree(root_dir)
 
+    @patch('ubuntucoreupgrader.upgrader.get_mount_details')
+    @patch('ubuntucoreupgrader.upgrader.mount')
+    @patch('ubuntucoreupgrader.upgrader.unmount')
+    def test_no_format_in_cmd(self, mock_umount, mock_mount,
+                              mock_mount_details):
+
+        # If the command file does not contain the format command, mkfs
+        # should not be called.
+        with patch('ubuntucoreupgrader.upgrader.mkfs') as 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(mock_mkfs.called)
+
+    @patch('ubuntucoreupgrader.upgrader.get_mount_details')
+    @patch('ubuntucoreupgrader.upgrader.mount')
+    @patch('ubuntucoreupgrader.upgrader.unmount')
+    def test_format(self, mock_umount, mock_mount, mock_mount_details):
+        MOCK_FS_TUPLE = ("device", "fstype", "label")
+        mock_mount_details.return_value = MOCK_FS_TUPLE
+
+        # mkfs should be called if the format command is specified in
+        # the command file.
+        with patch('ubuntucoreupgrader.upgrader.mkfs') as 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()
+
+        mock_mkfs.assert_called_with(*MOCK_FS_TUPLE)
 
 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-08 15:51:51 +0000
@@ -203,6 +203,56 @@
         sys.exit(1)
 
 
+def mkfs(device, fs_type, label):
+    '''
+    Run mkfs(8) on specified device.
+    '''
+    assert (os.path.exists(device))
+
+    args = ['/sbin/mkfs',
+            '-v'
+            '-t', fs_type,
+            '-L', label,
+            device]
+
+    log.debug('running: {}'.format(args))
+
+    proc = subprocess.Popen(args,
+                            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)
+
+
+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 +518,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 +617,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()
+
+        source, fstype, label = get_mount_details(other)
+
+        unmount(other)
+        mkfs(source, fstype, label)
+
+        # leave the mount as it was initially.
+        mount(source, other, "ro")
+
+        self.other_has_been_formatted = True
 
     def _cmd_load_keyring(self, args):
         try:
@@ -609,6 +668,19 @@
                         .format(target_type))
             return
 
+        if self.other_is_empty and not self.other_has_been_formatted:
+            # 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 +811,7 @@
         else:
             to_remove = []
 
+        # by definition, full images don't have 'removed' files.
         if not self.full_image:
 
             if found_removed_file:
@@ -816,7 +889,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 +919,3 @@
 
         unmount(bindmount_rootfs_dir)
         os.rmdir(bindmount_rootfs_dir)
-        self.other_is_empty = False