From c3805b2bea404cf6b831a3262e2602db72324616 Mon Sep 17 00:00:00 2001 From: Steve Baker Date: Tue, 11 Jan 2022 11:21:32 +1300 Subject: [PATCH] Use mtools mcopy in create_vfat_image This change replaces the mount/unmount calls with a single mcopy call to populate the vfat image with the contents of the populated temp directory. This means create_vfat_image can now run without any elevated privilages. Change-Id: I1d93eed1240fbb2ae6598699bad9914f44171e8e Story: 2009704 Task: 44034 --- bindep.txt | 3 +- ironic/common/images.py | 22 +++---- ironic/tests/unit/common/test_images.py | 76 +++++-------------------- 3 files changed, 27 insertions(+), 74 deletions(-) diff --git a/bindep.txt b/bindep.txt index 79e4979f7e..8637877638 100644 --- a/bindep.txt +++ b/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] diff --git a/ironic/common/images.py b/ironic/common/images.py index 0f83b8726e..cf51d723c8 100644 --- a/ironic/common/images.py +++ b/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. diff --git a/ironic/tests/unit/common/test_images.py b/ironic/tests/unit/common/test_images.py index 14017682bc..9892d671c8 100644 --- a/ironic/tests/unit/common/test_images.py +++ b/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.patch.object(utils, 'execute', autospec=True) + def test_create_vfat_image_dd_fails(self, execute_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): - - 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')