Merge "Use mtools mcopy in create_vfat_image"

bindep.txt

@@ -78,7 +78,7 @@ graphviz [!platform:gentoo test doc]
 librsvg2-tools [doc platform:rpm]
 librsvg2-bin [doc platform:dpkg]
 
-# these are needed to build a deploy ramdisk
+# these are needed to build images
 
 # NOTE apparmor is an undeclared dependency for docker on ubuntu,
 # see https://github.com/docker/docker/issues/9745
@@ -95,3 +95,4 @@ python-pip [imagebuild]
 unzip [imagebuild]
 sudo [imagebuild]
 gawk [imagebuild]
+mtools [imagebuild]

ironic/common/images.py

@@ -24,7 +24,6 @@ import shutil
 import time
 
 from ironic_lib import disk_utils
-from ironic_lib import utils as ironic_utils
 from oslo_concurrency import processutils
 from oslo_log import log as logging
 from oslo_utils import fileutils
@@ -107,8 +106,9 @@ def create_vfat_image(output_file, files_info=None, parameters=None,
              mounting, creating filesystem, copying files, etc.
     """
     try:
-        ironic_utils.dd('/dev/zero', output_file, 'count=1',
-                        "bs=%dKiB" % fs_size_kib)
+        # TODO(sbaker): use ironic_lib.utils.dd when rootwrap has been removed
+        utils.execute('dd', 'if=/dev/zero', 'of=%s' % output_file, 'count=1',
+                      'bs=%dKiB' % fs_size_kib)
     except processutils.ProcessExecutionError as e:
         raise exception.ImageCreationFailed(image_type='vfat', error=e)
 
@@ -118,8 +118,9 @@ def create_vfat_image(output_file, files_info=None, parameters=None,
             # The label helps ramdisks to find the partition containing
             # the parameters (by using /dev/disk/by-label/ir-vfd-dev).
             # NOTE: FAT filesystem label can be up to 11 characters long.
-            ironic_utils.mkfs('vfat', output_file, label="ir-vfd-dev")
-            utils.mount(output_file, tmpdir, '-o', 'umask=0')
+            # TODO(sbaker): use ironic_lib.utils.mkfs when rootwrap has been
+            #              removed
+            utils.execute('mkfs', '-t', 'vfat', '-n', 'ir-vfd-de', output_file)
         except processutils.ProcessExecutionError as e:
             raise exception.ImageCreationFailed(image_type='vfat', error=e)
 
@@ -134,16 +135,15 @@ def create_vfat_image(output_file, files_info=None, parameters=None,
                 file_contents = '\n'.join(params_list)
                 utils.write_to_file(parameters_file, file_contents)
 
+            # use mtools to copy the files into the image in a single
+            # operation
+            utils.execute('mcopy', '-s', '%s/*' % tmpdir,
+                          '-i', output_file, '::')
+
         except Exception as e:
             LOG.exception("vfat image creation failed. Error: %s", e)
             raise exception.ImageCreationFailed(image_type='vfat', error=e)
 
-        finally:
-            try:
-                utils.umount(tmpdir)
-            except processutils.ProcessExecutionError as e:
-                raise exception.ImageCreationFailed(image_type='vfat', error=e)
-
 
 def _generate_cfg(kernel_params, template, options):
     """Generates a isolinux or grub configuration file.

ironic/tests/unit/common/test_images.py

@@ -22,7 +22,6 @@ import shutil
 from unittest import mock
 
 from ironic_lib import disk_utils
-from ironic_lib import utils as ironic_utils
 from oslo_concurrency import processutils
 from oslo_config import cfg
 
@@ -299,12 +298,9 @@ class FsImageTestCase(base.TestCase):
     @mock.patch.object(images, '_create_root_fs', autospec=True)
     @mock.patch.object(utils, 'tempdir', autospec=True)
     @mock.patch.object(utils, 'write_to_file', autospec=True)
-    @mock.patch.object(ironic_utils, 'dd', autospec=True)
-    @mock.patch.object(utils, 'umount', autospec=True)
-    @mock.patch.object(utils, 'mount', autospec=True)
-    @mock.patch.object(ironic_utils, 'mkfs', autospec=True)
+    @mock.patch.object(utils, 'execute', autospec=True)
     def test_create_vfat_image(
-            self, mkfs_mock, mount_mock, umount_mock, dd_mock, write_mock,
+            self, execute_mock, write_mock,
             tempdir_mock, create_root_fs_mock):
 
         mock_file_handle = mock.MagicMock(spec=io.BytesIO)
@@ -317,78 +313,34 @@ class FsImageTestCase(base.TestCase):
                                  files_info=files_info, parameters_file='qwe',
                                  fs_size_kib=1000)
 
-        dd_mock.assert_called_once_with('/dev/zero',
-                                        'tgt_file',
-                                        'count=1',
-                                        'bs=1000KiB')
-
-        mkfs_mock.assert_called_once_with('vfat', 'tgt_file',
-                                          label="ir-vfd-dev")
-        mount_mock.assert_called_once_with('tgt_file', 'tempdir',
-                                           '-o', 'umask=0')
+        execute_mock.assert_has_calls([
+            mock.call('dd', 'if=/dev/zero', 'of=tgt_file', 'count=1',
+                      'bs=1000KiB'),
+            mock.call('mkfs', '-t', 'vfat', '-n', 'ir-vfd-de', 'tgt_file'),
+            mock.call('mcopy', '-s', 'tempdir/*', '-i', 'tgt_file', '::')
+        ])
 
         parameters_file_path = os.path.join('tempdir', 'qwe')
         write_mock.assert_called_once_with(parameters_file_path, 'p1=v1')
         create_root_fs_mock.assert_called_once_with('tempdir', files_info)
-        umount_mock.assert_called_once_with('tempdir')
-
-    @mock.patch.object(images, '_create_root_fs', autospec=True)
-    @mock.patch.object(utils, 'tempdir', autospec=True)
-    @mock.patch.object(ironic_utils, 'dd', autospec=True)
-    @mock.patch.object(utils, 'umount', autospec=True)
-    @mock.patch.object(utils, 'mount', autospec=True)
-    @mock.patch.object(ironic_utils, 'mkfs', autospec=True)
-    def test_create_vfat_image_always_umount(
-            self, mkfs_mock, mount_mock, umount_mock, dd_mock,
-            tempdir_mock, create_root_fs_mock):
-
-        mock_file_handle = mock.MagicMock(spec=io.BytesIO)
-        mock_file_handle.__enter__.return_value = 'tempdir'
-        tempdir_mock.return_value = mock_file_handle
-        files_info = {'a': 'b'}
-        create_root_fs_mock.side_effect = OSError()
-        self.assertRaises(exception.ImageCreationFailed,
-                          images.create_vfat_image, 'tgt_file',
-                          files_info=files_info)
-
-        umount_mock.assert_called_once_with('tempdir')
 
-    @mock.patch.object(ironic_utils, 'dd', autospec=True)
-    def test_create_vfat_image_dd_fails(self, dd_mock):
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test_create_vfat_image_dd_fails(self, execute_mock):
 
-        dd_mock.side_effect = processutils.ProcessExecutionError
+        execute_mock.side_effect = processutils.ProcessExecutionError
         self.assertRaises(exception.ImageCreationFailed,
                           images.create_vfat_image, 'tgt_file')
 
     @mock.patch.object(utils, 'tempdir', autospec=True)
-    @mock.patch.object(ironic_utils, 'dd', autospec=True)
-    @mock.patch.object(ironic_utils, 'mkfs', autospec=True)
-    def test_create_vfat_image_mkfs_fails(self, mkfs_mock, dd_mock,
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test_create_vfat_image_mkfs_fails(self, execute_mock,
                                           tempdir_mock):
 
         mock_file_handle = mock.MagicMock(spec=io.BytesIO)
         mock_file_handle.__enter__.return_value = 'tempdir'
         tempdir_mock.return_value = mock_file_handle
 
-        mkfs_mock.side_effect = processutils.ProcessExecutionError
-        self.assertRaises(exception.ImageCreationFailed,
-                          images.create_vfat_image, 'tgt_file')
-
-    @mock.patch.object(images, '_create_root_fs', autospec=True)
-    @mock.patch.object(utils, 'tempdir', autospec=True)
-    @mock.patch.object(ironic_utils, 'dd', autospec=True)
-    @mock.patch.object(utils, 'umount', autospec=True)
-    @mock.patch.object(utils, 'mount', autospec=True)
-    @mock.patch.object(ironic_utils, 'mkfs', autospec=True)
-    def test_create_vfat_image_umount_fails(
-            self, mkfs_mock, mount_mock, umount_mock, dd_mock,
-            tempdir_mock, create_root_fs_mock):
-
-        mock_file_handle = mock.MagicMock(spec=io.BytesIO)
-        mock_file_handle.__enter__.return_value = 'tempdir'
-        tempdir_mock.return_value = mock_file_handle
-        umount_mock.side_effect = processutils.ProcessExecutionError
-
+        execute_mock.side_effect = [None, processutils.ProcessExecutionError]
         self.assertRaises(exception.ImageCreationFailed,
                           images.create_vfat_image, 'tgt_file')