From c3805b2bea404cf6b831a3262e2602db72324616 Mon Sep 17 00:00:00 2001
From: Steve Baker <sbaker@redhat.com>
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')