Move fstrim to block device layer

The call to fstrim in disk-image-create is currently useless, because
at the time this is called, the file systems were already umounted by
the block device layer.

The current implementation of the block-device mount plugin does not
call fstrim at all: resulting in larger image sizes.

This patch removes the useless fstrim call from the disk-image-create
script and moves this into the block-device mount.py.

The resulting image might be much smaller.  Example: Ubuntu Xenial
with some elements; once with and once without this patch:

-rw-r--r-- 1 dib dib 475661824 Sep 16 06:43 ubuntu-xenial-without-fstrim.qcow2
-rw-r--r-- 1 dib dib 364249088 Sep 16 09:30 ubuntu-xenial-with-fstrim.qcow2

Change-Id: I4e21ae50c5e6e26dc9f50f004ed6413132c81047
Signed-off-by: Andreas Florath <andreas@florath.net>

diskimage_builder/block_device/level3/mount.py

@@ -95,6 +95,17 @@ class MountPointNode(NodeBase):
 
     def umount(self):
         logger.info("Called for [%s]", self.name)
+        # Before calling umount call 'fstrim' on the mounted file
+        # system.  This discards unused blocks from the mounted
+        # file system and therefore decreases the resulting image
+        # size.
+        # A race condition can occur when trying to fstrim immediately after
+        # deleting a file resulting in that free space not being reclaimed.
+        # Calling sync before fstrim is a workaround for this behaviour.
+        # https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg02978.html
+        exec_sudo(["sync"])
+        exec_sudo(["fstrim", "--verbose",
+                   self.state['mount'][self.mount_point]['path']])
         exec_sudo(["umount", self.state['mount'][self.mount_point]['path']])
 
     def delete(self):

diskimage_builder/block_device/tests/test_mount_order.py

@@ -13,6 +13,7 @@
 import functools
 import logging
 import mock
+import os
 
 import diskimage_builder.block_device.tests.test_config as tc
 
@@ -51,39 +52,67 @@ class TestMountComparator(TestBase):
 
 class TestMountOrder(tc.TestGraphGeneration):
 
-    @mock.patch('diskimage_builder.block_device.level3.mount.exec_sudo')
+    def _exec_sudo_log(*args, **kwargs):
+        # Used as a side-effect from exec_sudo mocking so we can see
+        # the call in-place in logs
+        logger.debug("exec_sudo: %s", " ".join(args[0]))
+
+    @mock.patch('diskimage_builder.block_device.level3.mount.exec_sudo',
+                side_effect=_exec_sudo_log)
     def test_mount_order(self, mock_exec_sudo):
+        # XXX: better mocking for the os.path.exists calls to avoid
+        # failing if this exists.
+        self.assertFalse(os.path.exists('/fake/'))
+
         # This is probably in order after graph creation, so ensure it
-        # remains stable
+        # remains stable.  We test the mount and umount call sequences
         config = self.load_config_file('multiple_partitions_graph.yaml')
-
         state = {}
-
         graph, call_order = create_graph(config, self.fake_default_config,
                                          state)
 
         # build up some fake state so that we don't have to mock out
         # all the parent calls that would really make these values, as
         # we just want to test MountPointNode
-        state['filesys'] = {}
+        state['filesys'] = {
-        state['filesys']['mkfs_root'] = {}
+            'mkfs_root': {'device': 'fake_root_device'},
-        state['filesys']['mkfs_root']['device'] = 'fake'
+            'mkfs_var': {'device': 'fake_var_device'},
-        state['filesys']['mkfs_var'] = {}
+            'mkfs_var_log': {'device': 'fake_var_log_device'}
-        state['filesys']['mkfs_var']['device'] = 'fake'
+        }
-        state['filesys']['mkfs_var_log'] = {}
-        state['filesys']['mkfs_var_log']['device'] = 'fake'
 
         for node in call_order:
             if isinstance(node, MountPointNode):
-                # XXX: do we even need to create?  We could test the
-                # sudo arguments from the mock in the below asserts
-                # too
                 node.create()
+        for node in reversed(call_order):
+            if isinstance(node, MountPointNode):
+                node.umount()
 
         # ensure that partitions are mounted in order root->var->var/log
         self.assertListEqual(state['mount_order'], ['/', '/var', '/var/log'])
 
-    @mock.patch('diskimage_builder.block_device.level3.mount.exec_sudo')
+        cmd_sequence = [
+            # mount sequence
+            mock.call(['mkdir', '-p', '/fake/']),
+            mock.call(['mount', 'fake_root_device', '/fake/']),
+            mock.call(['mkdir', '-p', '/fake/var']),
+            mock.call(['mount', 'fake_var_device', '/fake/var']),
+            mock.call(['mkdir', '-p', '/fake/var/log']),
+            mock.call(['mount', 'fake_var_log_device', '/fake/var/log']),
+            # umount sequence
+            mock.call(['sync']),
+            mock.call(['fstrim', '--verbose', '/fake/var/log']),
+            mock.call(['umount', '/fake/var/log']),
+            mock.call(['sync']),
+            mock.call(['fstrim', '--verbose', '/fake/var']),
+            mock.call(['umount', '/fake/var']),
+            mock.call(['sync']),
+            mock.call(['fstrim', '--verbose', '/fake/']),
+            mock.call(['umount', '/fake/'])
+        ]
+        self.assertListEqual(mock_exec_sudo.call_args_list, cmd_sequence)
+
+    @mock.patch('diskimage_builder.block_device.level3.mount.exec_sudo',
+                side_effect=_exec_sudo_log)
     def test_mount_order_unsorted(self, mock_exec_sudo):
         # As above, but this is out of order and gets sorted
         # so that root is mounted first.
@@ -94,17 +123,39 @@ class TestMountOrder(tc.TestGraphGeneration):
         graph, call_order = create_graph(parsed_graph,
                                          self.fake_default_config,
                                          state)
-        state['filesys'] = {}
+        state['filesys'] = {
-        state['filesys']['mkfs_root'] = {}
+            'mkfs_root': {'device': 'fake_root_device'},
-        state['filesys']['mkfs_root']['device'] = 'fake'
+            'mkfs_var': {'device': 'fake_var_device'},
-        state['filesys']['mkfs_var'] = {}
+            'mkfs_boot': {'device': 'fake_boot_device'}
-        state['filesys']['mkfs_var']['device'] = 'fake'
+        }
-        state['filesys']['mkfs_boot'] = {}
-        state['filesys']['mkfs_boot']['device'] = 'fake'
 
         for node in call_order:
             if isinstance(node, MountPointNode):
                 node.create()
+        for node in reversed(call_order):
+            if isinstance(node, MountPointNode):
+                node.umount()
 
         # ensure that partitions are mounted in order / -> /boot -> /var
         self.assertListEqual(state['mount_order'], ['/', '/boot', '/var'])
+
+        cmd_sequence = [
+            # mount sequence
+            mock.call(['mkdir', '-p', '/fake/']),
+            mock.call(['mount', 'fake_root_device', '/fake/']),
+            mock.call(['mkdir', '-p', '/fake/boot']),
+            mock.call(['mount', 'fake_boot_device', '/fake/boot']),
+            mock.call(['mkdir', '-p', '/fake/var']),
+            mock.call(['mount', 'fake_var_device', '/fake/var']),
+            # umount sequence
+            mock.call(['sync']),
+            mock.call(['fstrim', '--verbose', '/fake/var']),
+            mock.call(['umount', '/fake/var']),
+            mock.call(['sync']),
+            mock.call(['fstrim', '--verbose', '/fake/boot']),
+            mock.call(['umount', '/fake/boot']),
+            mock.call(['sync']),
+            mock.call(['fstrim', '--verbose', '/fake/']),
+            mock.call(['umount', '/fake/'])
+        ]
+        self.assertListEqual(mock_exec_sudo.call_args_list, cmd_sequence)

diskimage_builder/lib/disk-image-create

@@ -480,11 +480,6 @@ for X in ${!IMAGE_TYPES[@]} ; do
   fi
 done
 
-if [[ ! $IMAGE_ELEMENT =~ no-final-image ]]; then
-  # Prep filesystem by discarding all unused space
-  fstrim_image
-fi
-
 # Unmount and cleanup the /mnt and /build subdirectories, to save
 # space before converting the image to some other format.
 # XXX ? needed?

diskimage_builder/lib/img-functions

@@ -32,17 +32,6 @@ function unmount_image () {
     fi
 }
 
-function fstrim_image () {
-    # A race condition can occur when trying to fstrim immediately after
-    # deleting a file resulting in that free space not being reclaimed.
-    # Calling sync before fstrim is a workaround for this behaviour.
-    # https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg02978.html
-    sync
-
-    # Discard all unused bytes
-    sudo fstrim "${TMP_BUILD_DIR}/mnt"
-}
-
 function trap_cleanup() {
     exitval=$?
     cleanup