openstack/ironic
Code Issues Proposed changes

Use mtools mcopy in create_vfat_image

Browse Source 
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
This commit is contained in:
Steve Baker 2022-01-11 11:21:32 +13:00
parent a346ee4d7f
commit c3805b2bea
3 changed files with 27 additions and 74 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
bindep.txt
View File

@ -78,7 +78,7 @@ graphviz [!platform:gentoo test doc]
librsvg2-tools [doc platform:rpm] librsvg2-tools [doc platform:rpm]
librsvg2-bin [doc platform:dpkg] 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, # NOTE apparmor is an undeclared dependency for docker on ubuntu,
# see https://github.com/docker/docker/issues/9745 # see https://github.com/docker/docker/issues/9745
@ -95,3 +95,4 @@ python-pip [imagebuild]
unzip [imagebuild] unzip [imagebuild]
sudo [imagebuild] sudo [imagebuild]
gawk [imagebuild] gawk [imagebuild]
mtools [imagebuild]

22
ironic/common/images.py
View File

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

76
ironic/tests/unit/common/test_images.py
View File

@ -22,7 +22,6 @@ import shutil
from unittest import mock from unittest import mock
from ironic_lib import disk_utils from ironic_lib import disk_utils
from ironic_lib import utils as ironic_utils
from oslo_concurrency import processutils from oslo_concurrency import processutils
from oslo_config import cfg 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(images, '_create_root_fs', autospec=True)
@mock.patch.object(utils, 'tempdir', autospec=True) @mock.patch.object(utils, 'tempdir', autospec=True)
@mock.patch.object(utils, 'write_to_file', autospec=True) @mock.patch.object(utils, 'write_to_file', autospec=True)
@mock.patch.object(ironic_utils, 'dd', autospec=True) @mock.patch.object(utils, 'execute', 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( 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): tempdir_mock, create_root_fs_mock):
mock_file_handle = mock.MagicMock(spec=io.BytesIO) mock_file_handle = mock.MagicMock(spec=io.BytesIO)
@ -317,78 +313,34 @@ class FsImageTestCase(base.TestCase):
files_info=files_info, parameters_file='qwe', files_info=files_info, parameters_file='qwe',
fs_size_kib=1000) fs_size_kib=1000)
dd_mock.assert_called_once_with('/dev/zero', execute_mock.assert_has_calls([
'tgt_file', mock.call('dd', 'if=/dev/zero', 'of=tgt_file', 'count=1',
'count=1', 'bs=1000KiB'),
'bs=1000KiB') mock.call('mkfs', '-t', 'vfat', '-n', 'ir-vfd-de', 'tgt_file'),
mock.call('mcopy', '-s', 'tempdir/*', '-i', 'tgt_file', '::')
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')
parameters_file_path = os.path.join('tempdir', 'qwe') parameters_file_path = os.path.join('tempdir', 'qwe')
write_mock.assert_called_once_with(parameters_file_path, 'p1=v1') write_mock.assert_called_once_with(parameters_file_path, 'p1=v1')
create_root_fs_mock.assert_called_once_with('tempdir', files_info) 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, 'execute', autospec=True)
@mock.patch.object(utils, 'tempdir', autospec=True) def test_create_vfat_image_dd_fails(self, execute_mock):
@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) execute_mock.side_effect = processutils.ProcessExecutionError
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
self.assertRaises(exception.ImageCreationFailed, self.assertRaises(exception.ImageCreationFailed,
images.create_vfat_image, 'tgt_file') images.create_vfat_image, 'tgt_file')
@mock.patch.object(utils, 'tempdir', autospec=True) @mock.patch.object(utils, 'tempdir', autospec=True)
@mock.patch.object(ironic_utils, 'dd', autospec=True) @mock.patch.object(utils, 'execute', autospec=True)
@mock.patch.object(ironic_utils, 'mkfs', autospec=True) def test_create_vfat_image_mkfs_fails(self, execute_mock,
def test_create_vfat_image_mkfs_fails(self, mkfs_mock, dd_mock,
tempdir_mock): tempdir_mock):
mock_file_handle = mock.MagicMock(spec=io.BytesIO) mock_file_handle = mock.MagicMock(spec=io.BytesIO)
mock_file_handle.__enter__.return_value = 'tempdir' mock_file_handle.__enter__.return_value = 'tempdir'
tempdir_mock.return_value = mock_file_handle tempdir_mock.return_value = mock_file_handle
mkfs_mock.side_effect = processutils.ProcessExecutionError execute_mock.side_effect = [None, 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
self.assertRaises(exception.ImageCreationFailed, self.assertRaises(exception.ImageCreationFailed,
images.create_vfat_image, 'tgt_file') images.create_vfat_image, 'tgt_file')