openstack/ironic-python-agent
Code Issues Proposed changes

Migrate more trivial code from ironic-lib

Browse Source 
* The dd and is_http_url code is trivial, inline it.
* Migrate mounted (cannot be used in Ironic since it requires root).
* Remove the leftovers of run_as_root.

Change-Id: Ic6b117e34ccc7f55ebac5f808d2765305c4b317f
This commit is contained in:
Dmitry Tantsur 2024-09-10 11:54:28 +02:00 committed by Jay Faulkner
parent 70aa08dad8
commit 8e0fa1be20
11 changed files with 166 additions and 64 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
ironic_python_agent/disk_utils.py
View File

@ -369,13 +369,12 @@ def is_block_device(dev):
def dd(src, dst, conv_flags=None):
"""Execute dd from src to dst."""
args = ['bs=%s' % CONF.disk_utils.dd_block_size, 'oflag=direct']
if conv_flags:
extra_args = ['conv=%s' % conv_flags]
else:
extra_args = []
args.append('conv=%s' % conv_flags)
utils.dd(src, dst, 'bs=%s' % CONF.disk_utils.dd_block_size, 'oflag=direct',
*extra_args)
utils.execute('dd', 'if=%s' % src, 'of=%s' % dst, *args,
use_standard_locale=True)
def _image_inspection(filename):
@ -481,7 +480,6 @@ def populate_image(src, dst, conv_flags=None, source_format=None, is_raw=False,
else:
qemu_img.convert_image(src, dst,
out_format=out_format,
run_as_root=True,
sparse_size=sparse_size,
source_format=source_format,
**convert_args)

6
ironic_python_agent/inject_files.py
View File

@ -144,7 +144,7 @@ def _find_and_mount_path(path, partition, root_dev):
try:
part_num = int(partition)
except ValueError:
with ironic_utils.mounted(partition) as part_path:
with utils.mounted(partition) as part_path:
yield os.path.join(part_path, path)
else:
# TODO(dtantsur): switch to ironic-lib instead:
@ -154,7 +154,7 @@ def _find_and_mount_path(path, partition, root_dev):
part_template = '%sp%s'
part_dev = part_template % (root_dev, part_num)
with ironic_utils.mounted(part_dev) as part_path:
with utils.mounted(part_dev) as part_path:
yield os.path.join(part_path, path)
else:
try:
@ -199,7 +199,7 @@ def find_partition_with_path(path, device=None):
LOG.debug('Inspecting partition %s for path %s', part, path)
try:
with ironic_utils.mounted(part_path) as local_path:
with utils.mounted(part_path) as local_path:
found_path = os.path.join(local_path, lookup_path)
if not os.path.isdir(found_path):
continue

7
ironic_python_agent/partition_utils.py
View File

@ -51,6 +51,11 @@ MAX_CONFIG_DRIVE_SIZE_MB = 64
MAX_DISK_SIZE_MB_SUPPORTED_BY_MBR = 2097152
def _is_http_url(url):
url = url.lower()
return url.startswith('http://') or url.startswith('https://')
def get_configdrive(configdrive, node_uuid, tempdir=None):
"""Get the information about size and location of the configdrive.
@ -65,7 +70,7 @@ def get_configdrive(configdrive, node_uuid, tempdir=None):
"""
# Check if the configdrive option is a HTTP URL or the content directly
is_url = utils.is_http_url(configdrive)
is_url = _is_http_url(configdrive)
if is_url:
verify, cert = ipa_utils.get_ssl_client_options(CONF)
timeout = CONF.image_download_connection_timeout

7
ironic_python_agent/qemu_img.py
View File

@ -97,8 +97,8 @@ def image_info(path, source_format=None):
retry=tenacity.retry_if_exception(_retry_on_res_temp_unavailable),
stop=tenacity.stop_after_attempt(CONF.disk_utils.image_convert_attempts),
reraise=True)
def convert_image(source, dest, out_format, run_as_root=False, cache=None,
out_of_order=False, sparse_size=None, source_format=None):
def convert_image(source, dest, out_format, cache=None, out_of_order=False,
sparse_size=None, source_format=None):
"""Convert image to other format.
This method is only to be run against images who have passed
@ -140,8 +140,7 @@ def convert_image(source, dest, out_format, run_as_root=False, cache=None,
# be passed through qemu-img.
env_vars = {'MALLOC_ARENA_MAX': '3'}
try:
utils.execute(*cmd, run_as_root=run_as_root,
prlimit=_qemu_img_limits(),
utils.execute(*cmd, prlimit=_qemu_img_limits(),
use_standard_locale=True,
env_variables=env_vars)
except processutils.ProcessExecutionError as e:

1
ironic_python_agent/tests/unit/extensions/test_standby.py
View File

@ -311,7 +311,6 @@ class TestStandbyExtension(base.IronicAgentTest):
convert_mock.assert_called_once_with(location, device,
out_format='host_device',
run_as_root=True,
sparse_size='0',
source_format=source_format,
cache='directsync',

1
ironic_python_agent/tests/unit/test_disk_utils.py
View File

@ -538,7 +538,6 @@ class PopulateImageTestCase(base.IronicAgentTest):
source_format=source_format, is_raw=False)
mock_cg.assert_called_once_with('src', 'dst',
out_format='raw',
run_as_root=True,
sparse_size='0',
source_format=source_format)
self.assertFalse(mock_dd.called)

8
ironic_python_agent/tests/unit/test_inject_files.py
View File

@ -21,7 +21,7 @@ from ironic_python_agent import inject_files
from ironic_python_agent.tests.unit import base
@mock.patch('ironic_lib.utils.mounted', autospec=True)
@mock.patch('ironic_python_agent.utils.mounted', autospec=True)
@mock.patch('ironic_python_agent.disk_utils.list_partitions', autospec=True)
@mock.patch('ironic_python_agent.hardware.dispatch_to_managers',
lambda _call: '/dev/fake')
@ -96,7 +96,7 @@ class TestFindAndMountPath(base.IronicAgentTest):
inject_files._find_and_mount_path('/etc', None,
'/dev/fake').__enter__)
@mock.patch('ironic_lib.utils.mounted', autospec=True)
@mock.patch('ironic_python_agent.utils.mounted', autospec=True)
def test_with_on_as_path(self, mock_mount):
mock_mount.return_value.__enter__.return_value = '/mount/path'
with inject_files._find_and_mount_path('/etc/sysctl.d/my.conf',
@ -105,7 +105,7 @@ class TestFindAndMountPath(base.IronicAgentTest):
self.assertEqual('/mount/path/etc/sysctl.d/my.conf', result)
mock_mount.assert_called_once_with('/dev/on')
@mock.patch('ironic_lib.utils.mounted', autospec=True)
@mock.patch('ironic_python_agent.utils.mounted', autospec=True)
def test_with_on_as_number(self, mock_mount):
mock_mount.return_value.__enter__.return_value = '/mount/path'
with inject_files._find_and_mount_path('/etc/sysctl.d/my.conf',
@ -113,7 +113,7 @@ class TestFindAndMountPath(base.IronicAgentTest):
self.assertEqual('/mount/path/etc/sysctl.d/my.conf', result)
mock_mount.assert_called_once_with('/dev/fake2')
@mock.patch('ironic_lib.utils.mounted', autospec=True)
@mock.patch('ironic_python_agent.utils.mounted', autospec=True)
def test_with_on_as_number_nvme(self, mock_mount):
mock_mount.return_value.__enter__.return_value = '/mount/path'
with inject_files._find_and_mount_path('/etc/sysctl.d/my.conf',

27
ironic_python_agent/tests/unit/test_partition_utils.py
View File

@ -1273,18 +1273,6 @@ class RealFilePartitioningTestCase(base.IronicAgentTest):
utils.execute('dd', 'if=/dev/zero', 'of=%s' % self.file.name,
'bs=1', 'count=0', 'seek=20MiB')
@staticmethod
def _run_without_root(func, *args, **kwargs):
"""Make sure root is not required when using utils.execute."""
real_execute = utils.execute
def fake_execute(*cmd, **kwargs):
kwargs.pop('run_as_root', None)
return real_execute(*cmd, **kwargs)
with mock.patch.object(utils, 'execute', fake_execute):
return func(*args, **kwargs)
def test_different_sizes(self, mock_destroy, mock_populate, mock_mkfs,
mock_convert, mock_dd, mock_block_uuid,
mock_is_block, mock_wait, mock_trigger_rescan):
@ -1293,11 +1281,10 @@ class RealFilePartitioningTestCase(base.IronicAgentTest):
variants = ((0, 0, 12), (4, 2, 8), (0, 4, 10), (5, 0, 10))
for variant in variants:
kwargs = dict(zip(fields, variant))
self._run_without_root(partition_utils.work_on_disk,
partition_utils.work_on_disk(
self.file.name, ephemeral_format='ext4',
node_uuid='', image_path='path', **kwargs)
part_table = self._run_without_root(
disk_utils.list_partitions, self.file.name)
part_table = disk_utils.list_partitions(self.file.name)
for part, expected_size in zip(part_table, filter(None, variant)):
self.assertEqual(expected_size, part['size'],
"comparison failed for %s" % list(variant))
@ -1309,12 +1296,10 @@ class RealFilePartitioningTestCase(base.IronicAgentTest):
# + 1 MiB MAGIC == 20 MiB whole disk
# TODO(dtantsur): figure out why we need 'magic' 1 more MiB
# and why the is different on Ubuntu and Fedora (see below)
self._run_without_root(partition_utils.work_on_disk, self.file.name,
root_mb=9, ephemeral_mb=6, swap_mb=3,
ephemeral_format='ext4', node_uuid='',
image_path='path')
part_table = self._run_without_root(
disk_utils.list_partitions, self.file.name)
partition_utils.work_on_disk(
self.file.name, root_mb=9, ephemeral_mb=6, swap_mb=3,
ephemeral_format='ext4', node_uuid='', image_path='path')
part_table = disk_utils.list_partitions(self.file.name)
sizes = [part['size'] for part in part_table]
# NOTE(dtantsur): parted in Ubuntu 12.04 will occupy the last MiB,
# parted in Fedora 20 won't - thus two possible variants for last part

12
ironic_python_agent/tests/unit/test_qemu_img.py
View File

@ -88,7 +88,6 @@ class ConvertImageTestCase(base.IronicLibTestCase):
execute_mock.assert_called_once_with(
'qemu-img', 'convert', '-O',
'out_format', 'source', 'dest',
run_as_root=False,
prlimit=mock.ANY,
use_standard_locale=True,
env_variables={'MALLOC_ARENA_MAX': '3'})
@ -103,7 +102,6 @@ class ConvertImageTestCase(base.IronicLibTestCase):
'qemu-img', 'convert', '-O',
'out_format', '-t', 'directsync',
'-S', '0', '-W', 'source', 'dest',
run_as_root=False,
prlimit=mock.ANY,
use_standard_locale=True,
env_variables={'MALLOC_ARENA_MAX': '3'})
@ -121,7 +119,6 @@ class ConvertImageTestCase(base.IronicLibTestCase):
qemu_img.convert_image('source', 'dest', 'out_format')
convert_call = mock.call('qemu-img', 'convert', '-O',
'out_format', 'source', 'dest',
run_as_root=False,
prlimit=mock.ANY,
use_standard_locale=True,
env_variables={'MALLOC_ARENA_MAX': '3'})
@ -146,7 +143,6 @@ class ConvertImageTestCase(base.IronicLibTestCase):
qemu_img.convert_image('source', 'dest', 'out_format')
convert_call = mock.call('qemu-img', 'convert', '-O',
'out_format', 'source', 'dest',
run_as_root=False,
prlimit=mock.ANY,
use_standard_locale=True,
env_variables={'MALLOC_ARENA_MAX': '3'})
@ -174,7 +170,6 @@ class ConvertImageTestCase(base.IronicLibTestCase):
'source', 'dest', 'out_format')
convert_call = mock.call('qemu-img', 'convert', '-O',
'out_format', 'source', 'dest',
run_as_root=False,
prlimit=mock.ANY,
use_standard_locale=True,
env_variables={'MALLOC_ARENA_MAX': '3'})
@ -199,7 +194,6 @@ class ConvertImageTestCase(base.IronicLibTestCase):
'source', 'dest', 'out_format')
convert_call = mock.call('qemu-img', 'convert', '-O',
'out_format', 'source', 'dest',
run_as_root=False,
prlimit=mock.ANY,
use_standard_locale=True,
env_variables={'MALLOC_ARENA_MAX': '3'})
@ -215,7 +209,6 @@ class ConvertImageTestCase(base.IronicLibTestCase):
'qemu-img', 'convert', '-O',
'out_format', '-f', 'fmt',
'source', 'dest',
run_as_root=False,
prlimit=mock.ANY,
use_standard_locale=True,
env_variables={'MALLOC_ARENA_MAX': '3'})
@ -229,7 +222,6 @@ class ConvertImageTestCase(base.IronicLibTestCase):
'qemu-img', 'convert', '-O',
'out_format', '-t', 'directsync',
'-S', '0', '-f', 'fmt', '-W', 'source', 'dest',
run_as_root=False,
prlimit=mock.ANY,
use_standard_locale=True,
env_variables={'MALLOC_ARENA_MAX': '3'})
@ -247,7 +239,6 @@ class ConvertImageTestCase(base.IronicLibTestCase):
source_format='fmt')
convert_call = mock.call('qemu-img', 'convert', '-O',
'out_format', '-f', 'fmt', 'source', 'dest',
run_as_root=False,
prlimit=mock.ANY,
use_standard_locale=True,
env_variables={'MALLOC_ARENA_MAX': '3'})
@ -272,7 +263,6 @@ class ConvertImageTestCase(base.IronicLibTestCase):
source_format='fmt')
convert_call = mock.call('qemu-img', 'convert', '-O',
'out_format', '-f', 'fmt', 'source', 'dest',
run_as_root=False,
prlimit=mock.ANY,
use_standard_locale=True,
env_variables={'MALLOC_ARENA_MAX': '3'})
@ -299,7 +289,6 @@ class ConvertImageTestCase(base.IronicLibTestCase):
'source', 'dest', 'out_format', source_format='fmt')
convert_call = mock.call('qemu-img', 'convert', '-O',
'out_format', '-f', 'fmt', 'source', 'dest',
run_as_root=False,
prlimit=mock.ANY,
use_standard_locale=True,
env_variables={'MALLOC_ARENA_MAX': '3'})
@ -323,7 +312,6 @@ class ConvertImageTestCase(base.IronicLibTestCase):
'source', 'dest', 'out_format', source_format='fmt')
convert_call = mock.call('qemu-img', 'convert', '-O',
'out_format', '-f', 'fmt', 'source', 'dest',
run_as_root=False,
prlimit=mock.ANY,
use_standard_locale=True,
env_variables={'MALLOC_ARENA_MAX': '3'})

81
ironic_python_agent/tests/unit/test_utils.py
View File

@ -50,6 +50,79 @@ class ExecuteTestCase(ironic_agent_base.IronicAgentTest):
check_exit_code=False)
@mock.patch('shutil.rmtree', autospec=True)
@mock.patch.object(ironic_utils, 'execute', autospec=True)
@mock.patch('tempfile.mkdtemp', autospec=True)
class MountedTestCase(ironic_agent_base.IronicAgentTest):
def test_temporary(self, mock_temp, mock_execute, mock_rmtree):
with utils.mounted('/dev/fake') as path:
self.assertIs(path, mock_temp.return_value)
mock_execute.assert_has_calls([
mock.call("mount", '/dev/fake', mock_temp.return_value,
attempts=1, delay_on_retry=True),
mock.call("umount", mock_temp.return_value,
attempts=3, delay_on_retry=True),
])
mock_rmtree.assert_called_once_with(mock_temp.return_value)
def test_with_dest(self, mock_temp, mock_execute, mock_rmtree):
with utils.mounted('/dev/fake', '/mnt/fake') as path:
self.assertEqual('/mnt/fake', path)
mock_execute.assert_has_calls([
mock.call("mount", '/dev/fake', '/mnt/fake',
attempts=1, delay_on_retry=True),
mock.call("umount", '/mnt/fake',
attempts=3, delay_on_retry=True),
])
self.assertFalse(mock_temp.called)
self.assertFalse(mock_rmtree.called)
def test_with_opts(self, mock_temp, mock_execute, mock_rmtree):
with utils.mounted('/dev/fake', '/mnt/fake',
opts=['ro', 'foo=bar']) as path:
self.assertEqual('/mnt/fake', path)
mock_execute.assert_has_calls([
mock.call("mount", '/dev/fake', '/mnt/fake', '-o', 'ro,foo=bar',
attempts=1, delay_on_retry=True),
mock.call("umount", '/mnt/fake',
attempts=3, delay_on_retry=True),
])
def test_with_type(self, mock_temp, mock_execute, mock_rmtree):
with utils.mounted('/dev/fake', '/mnt/fake',
fs_type='iso9660') as path:
self.assertEqual('/mnt/fake', path)
mock_execute.assert_has_calls([
mock.call("mount", '/dev/fake', '/mnt/fake', '-t', 'iso9660',
attempts=1, delay_on_retry=True),
mock.call("umount", '/mnt/fake',
attempts=3, delay_on_retry=True),
])
def test_failed_to_mount(self, mock_temp, mock_execute, mock_rmtree):
mock_execute.side_effect = OSError
self.assertRaises(OSError, utils.mounted('/dev/fake').__enter__)
mock_execute.assert_called_once_with("mount", '/dev/fake',
mock_temp.return_value,
attempts=1,
delay_on_retry=True)
mock_rmtree.assert_called_once_with(mock_temp.return_value)
def test_failed_to_unmount(self, mock_temp, mock_execute, mock_rmtree):
mock_execute.side_effect = [('', ''),
processutils.ProcessExecutionError]
with utils.mounted('/dev/fake', '/mnt/fake') as path:
self.assertEqual('/mnt/fake', path)
mock_execute.assert_has_calls([
mock.call("mount", '/dev/fake', '/mnt/fake',
attempts=1, delay_on_retry=True),
mock.call("umount", '/mnt/fake',
attempts=3, delay_on_retry=True),
])
self.assertFalse(mock_rmtree.called)
class GetAgentParamsTestCase(ironic_agent_base.IronicAgentTest):
@mock.patch('oslo_log.log.getLogger', autospec=True)
@ -187,7 +260,7 @@ class GetAgentParamsTestCase(ironic_agent_base.IronicAgentTest):
@mock.patch.object(utils, '_check_vmedia_device', autospec=True)
@mock.patch.object(utils, '_find_vmedia_device_by_labels', autospec=True)
@mock.patch.object(utils, '_read_params_from_file', autospec=True)
@mock.patch.object(ironic_utils, 'mounted', autospec=True)
@mock.patch.object(utils, 'mounted', autospec=True)
def test__get_vmedia_params(self, mount_mock, read_params_mock, find_mock,
check_vmedia_mock):
check_vmedia_mock.return_value = True
@ -206,7 +279,7 @@ class GetAgentParamsTestCase(ironic_agent_base.IronicAgentTest):
@mock.patch.object(utils, '_find_vmedia_device_by_labels', autospec=True)
@mock.patch.object(utils, '_get_vmedia_device', autospec=True)
@mock.patch.object(utils, '_read_params_from_file', autospec=True)
@mock.patch.object(ironic_utils, 'mounted', autospec=True)
@mock.patch.object(utils, 'mounted', autospec=True)
def test__get_vmedia_params_by_device(self, mount_mock, read_params_mock,
get_device_mock, find_mock,
check_vmedia_mock):
@ -228,7 +301,7 @@ class GetAgentParamsTestCase(ironic_agent_base.IronicAgentTest):
@mock.patch.object(utils, '_find_vmedia_device_by_labels', autospec=True)
@mock.patch.object(utils, '_get_vmedia_device', autospec=True)
@mock.patch.object(utils, '_read_params_from_file', autospec=True)
@mock.patch.object(ironic_utils, 'mounted', autospec=True)
@mock.patch.object(utils, 'mounted', autospec=True)
def test__get_vmedia_params_by_device_device_invalid(
self, mount_mock, read_params_mock,
get_device_mock, find_mock,
@ -883,7 +956,7 @@ class TestClockSyncUtils(ironic_agent_base.IronicAgentTest):
@mock.patch.object(utils, '_check_vmedia_device', autospec=True)
@mock.patch.object(utils, '_find_vmedia_device_by_labels', autospec=True)
@mock.patch.object(shutil, 'copy', autospec=True)
@mock.patch.object(ironic_utils, 'mounted', autospec=True)
@mock.patch.object(utils, 'mounted', autospec=True)
@mock.patch.object(utils, 'execute', autospec=True)
class TestCopyConfigFromVmedia(testtools.TestCase):

66
ironic_python_agent/utils.py
View File

@ -27,6 +27,7 @@ import stat
import subprocess
import sys
import tarfile
import tempfile
import time
from ironic_lib import utils as ironic_utils
@ -78,6 +79,61 @@ def execute(*cmd, **kwargs):
return ironic_utils.execute(*cmd, **kwargs)
@contextlib.contextmanager
def mounted(source, dest=None, opts=None, fs_type=None,
mount_attempts=1, umount_attempts=3):
"""A context manager for a temporary mount.
:param source: A device to mount.
:param dest: Mount destination. If not specified, a temporary directory
will be created and removed afterwards. An existing destination is
not removed.
:param opts: Mount options (``-o`` argument).
:param fs_type: File system type (``-t`` argument).
:param mount_attempts: A number of attempts to mount the device.
:param umount_attempts: A number of attempts to unmount the device.
:returns: A generator yielding the destination.
"""
params = []
if opts:
params.extend(['-o', ','.join(opts)])
if fs_type:
params.extend(['-t', fs_type])
if dest is None:
dest = tempfile.mkdtemp()
clean_up = True
else:
clean_up = False
mounted = False
try:
ironic_utils.execute("mount", source, dest, *params,
attempts=mount_attempts, delay_on_retry=True)
mounted = True
yield dest
finally:
if mounted:
try:
ironic_utils.execute("umount", dest, attempts=umount_attempts,
delay_on_retry=True)
except (EnvironmentError,
processutils.ProcessExecutionError) as exc:
LOG.warning(
'Unable to unmount temporary location %(dest)s: %(err)s',
{'dest': dest, 'err': exc})
# NOTE(dtantsur): don't try to remove a still mounted location
clean_up = False
if clean_up:
try:
shutil.rmtree(dest)
except EnvironmentError as exc:
LOG.warning(
'Unable to remove temporary location %(dest)s: %(err)s',
{'dest': dest, 'err': exc})
def _read_params_from_file(filepath):
"""Extract key=value pairs from a file.
@ -168,7 +224,7 @@ def _get_vmedia_params():
# If the device is not valid, return an empty dictionary.
return {}
with ironic_utils.mounted(vmedia_device_file) as vmedia_mount_point:
with mounted(vmedia_device_file) as vmedia_mount_point:
parameters_file_path = os.path.join(vmedia_mount_point,
parameters_file)
params = _read_params_from_file(parameters_file_path)
@ -321,14 +377,14 @@ def copy_config_from_vmedia():
_unmount_any_config_drives()
return
# Determine the device
mounted = _find_mount_point(vmedia_device_file)
if mounted:
mount_point = _find_mount_point(vmedia_device_file)
if mount_point:
# In this case a utility like a configuration drive tool
# has *already* mounted the device we believe to be the
# configuration drive.
_copy_config_from(mounted)
_copy_config_from(mount_point)
else:
with ironic_utils.mounted(vmedia_device_file) as vmedia_mount_point:
with mounted(vmedia_device_file) as vmedia_mount_point:
# In this case, we use a temporary folder and extract the contents
# for our configuration.
_copy_config_from(vmedia_mount_point)