openstack/ironic-python-agent
Code Issues Proposed changes

Stop accepting duplicated configdrive

Browse Source 
We're currently requiring it twice: in image_info and in a separate
configdrive argument. I think we should eventually settle on separate
arguments for separate entities, so this change makes the value in
image_info optional with a goal to stop accepting it.

We could probably just remove the handling in image_info, but a
deprecation is safer.

The (unused in ironic) cache_image call is updated with an optional
configdrive arguments.

Story: #2008904
Task: #42480
Change-Id: I1e2efa28efa3ea7e389774cb7633d916757bc6ed
This commit is contained in:
Dmitry Tantsur 2021-05-10 17:01:45 +02:00
parent 6fb4cec7aa
commit f657526807
3 changed files with 77 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

48
ironic_python_agent/extensions/standby.py
View File

@ -139,7 +139,7 @@ def _fetch_checksum(checksum, image_info):
checksum, "Checksum file does not contain name %s" % expected_fname) checksum, "Checksum file does not contain name %s" % expected_fname)
def _write_partition_image(image, image_info, device): def _write_partition_image(image, image_info, device, configdrive=None):
"""Call disk_util to create partition and write the partition image. """Call disk_util to create partition and write the partition image.
:param image: Local path to image file to be written to the partition. :param image: Local path to image file to be written to the partition.
@ -147,6 +147,9 @@ def _write_partition_image(image, image_info, device):
:param image_info: Image information dictionary. :param image_info: Image information dictionary.
:param device: The device name, as a string, on which to store the image. :param device: The device name, as a string, on which to store the image.
Example: '/dev/sda' Example: '/dev/sda'
:param configdrive: A string containing the location of the config
drive as a URL OR the contents (as gzip/base64)
of the configdrive. Optional, defaults to None.
:raises: InvalidCommandParamsError if the partition is too small for the :raises: InvalidCommandParamsError if the partition is too small for the
provided image. provided image.
@ -159,7 +162,6 @@ def _write_partition_image(image, image_info, device):
node_uuid = image_info.get('node_uuid') node_uuid = image_info.get('node_uuid')
preserve_ep = image_info['preserve_ephemeral'] preserve_ep = image_info['preserve_ephemeral']
configdrive = image_info['configdrive']
boot_option = image_info.get('boot_option', 'local') boot_option = image_info.get('boot_option', 'local')
boot_mode = utils.get_node_boot_mode(cached_node) boot_mode = utils.get_node_boot_mode(cached_node)
disk_label = utils.get_partition_table_type_from_specs(cached_node) disk_label = utils.get_partition_table_type_from_specs(cached_node)
@ -212,12 +214,15 @@ def _write_whole_disk_image(image, image_info, device):
raise errors.ImageWriteError(device, e.exit_code, e.stdout, e.stderr) raise errors.ImageWriteError(device, e.exit_code, e.stdout, e.stderr)
def _write_image(image_info, device): def _write_image(image_info, device, configdrive=None):
"""Writes an image to the specified device. """Writes an image to the specified device.
:param image_info: Image information dictionary. :param image_info: Image information dictionary.
:param device: The disk name, as a string, on which to store the image. :param device: The disk name, as a string, on which to store the image.
Example: '/dev/sda' Example: '/dev/sda'
:param configdrive: A string containing the location of the config
drive as a URL OR the contents (as gzip/base64)
of the configdrive. Optional, defaults to None.
:raises: ImageWriteError if the command to write the image encounters an :raises: ImageWriteError if the command to write the image encounters an
error. error.
""" """
@ -225,7 +230,7 @@ def _write_image(image_info, device):
image = _image_location(image_info) image = _image_location(image_info)
uuids = {} uuids = {}
if image_info.get('image_type') == 'partition': if image_info.get('image_type') == 'partition':
uuids = _write_partition_image(image, image_info, device) uuids = _write_partition_image(image, image_info, device, configdrive)
else: else:
_write_whole_disk_image(image, image_info, device) _write_whole_disk_image(image, image_info, device)
totaltime = time.time() - starttime totaltime = time.time() - starttime
@ -536,12 +541,15 @@ class StandbyExtension(base.BaseAgentExtension):
self.cached_image_id = None self.cached_image_id = None
self.partition_uuids = None self.partition_uuids = None
def _cache_and_write_image(self, image_info, device): def _cache_and_write_image(self, image_info, device, configdrive=None):
"""Cache an image and write it to a local device. """Cache an image and write it to a local device.
:param image_info: Image information dictionary. :param image_info: Image information dictionary.
:param device: The disk name, as a string, on which to store the :param device: The disk name, as a string, on which to store the
image. Example: '/dev/sda' image. Example: '/dev/sda'
:param configdrive: A string containing the location of the config
drive as a URL OR the contents (as gzip/base64)
of the configdrive. Optional, defaults to None.
:raises: ImageDownloadError if the image download fails for any reason. :raises: ImageDownloadError if the image download fails for any reason.
:raises: ImageChecksumError if the downloaded image's checksum does not :raises: ImageChecksumError if the downloaded image's checksum does not
@ -549,7 +557,7 @@ class StandbyExtension(base.BaseAgentExtension):
:raises: ImageWriteError if writing the image fails. :raises: ImageWriteError if writing the image fails.
""" """
_download_image(image_info) _download_image(image_info)
self.partition_uuids = _write_image(image_info, device) self.partition_uuids = _write_image(image_info, device, configdrive)
self.cached_image_id = image_info['id'] self.cached_image_id = image_info['id']
def _stream_raw_image_onto_device(self, image_info, device): def _stream_raw_image_onto_device(self, image_info, device):
@ -623,13 +631,16 @@ class StandbyExtension(base.BaseAgentExtension):
self.partition_uuids['root uuid'] = root_uuid self.partition_uuids['root uuid'] = root_uuid
@base.async_command('cache_image', _validate_image_info) @base.async_command('cache_image', _validate_image_info)
def cache_image(self, image_info=None, force=False): def cache_image(self, image_info, force=False, configdrive=None):
"""Asynchronously caches specified image to the local OS device. """Asynchronously caches specified image to the local OS device.
:param image_info: Image information dictionary. :param image_info: Image information dictionary.
:param force: Optional. If True forces cache_image to download and :param force: Optional. If True forces cache_image to download and
cache image, even if the same image already exists on cache image, even if the same image already exists on
the local OS install device. Defaults to False. the local OS install device. Defaults to False.
:param configdrive: A string containing the location of the config
drive as a URL OR the contents (as gzip/base64)
of the configdrive. Optional, defaults to None.
:raises: ImageDownloadError if the image download fails for any reason. :raises: ImageDownloadError if the image download fails for any reason.
:raises: ImageChecksumError if the downloaded image's checksum does not :raises: ImageChecksumError if the downloaded image's checksum does not
@ -645,7 +656,10 @@ class StandbyExtension(base.BaseAgentExtension):
if self.cached_image_id != image_info['id'] or force: if self.cached_image_id != image_info['id'] or force:
LOG.debug('Already had %s cached, overwriting', LOG.debug('Already had %s cached, overwriting',
self.cached_image_id) self.cached_image_id)
self._cache_and_write_image(image_info, device) # NOTE(dtantsur): backward compatibility
if configdrive is None:
configdrive = image_info.pop('configdrive', None)
self._cache_and_write_image(image_info, device, configdrive)
msg = 'image ({}) cached to device {} ' msg = 'image ({}) cached to device {} '
self._fix_up_partition_uuids(image_info, device) self._fix_up_partition_uuids(image_info, device)
@ -656,9 +670,7 @@ class StandbyExtension(base.BaseAgentExtension):
return result_msg return result_msg
@base.async_command('prepare_image', _validate_image_info) @base.async_command('prepare_image', _validate_image_info)
def prepare_image(self, def prepare_image(self, image_info, configdrive=None):
image_info=None,
configdrive=None):
"""Asynchronously prepares specified image on local OS install device. """Asynchronously prepares specified image on local OS install device.
In this case, 'prepare' means make local machine completely ready to In this case, 'prepare' means make local machine completely ready to
@ -680,6 +692,9 @@ class StandbyExtension(base.BaseAgentExtension):
large to store on the given device. large to store on the given device.
""" """
LOG.debug('Preparing image %s', image_info['id']) LOG.debug('Preparing image %s', image_info['id'])
# NOTE(dtantsur): backward compatibility
if configdrive is None:
configdrive = image_info.pop('configdrive', None)
device = hardware.dispatch_to_managers('get_os_install_device', device = hardware.dispatch_to_managers('get_os_install_device',
permit_refresh=True) permit_refresh=True)
@ -695,7 +710,8 @@ class StandbyExtension(base.BaseAgentExtension):
if image_info.get('image_type') == 'partition': if image_info.get('image_type') == 'partition':
self.partition_uuids = _write_partition_image(None, self.partition_uuids = _write_partition_image(None,
image_info, image_info,
device) device,
configdrive)
stream_to = self.partition_uuids['partitions']['root'] stream_to = self.partition_uuids['partitions']['root']
else: else:
self.partition_uuids = {} self.partition_uuids = {}
@ -703,12 +719,14 @@ class StandbyExtension(base.BaseAgentExtension):
self._stream_raw_image_onto_device(image_info, stream_to) self._stream_raw_image_onto_device(image_info, stream_to)
else: else:
self._cache_and_write_image(image_info, device) self._cache_and_write_image(image_info, device, configdrive)
_validate_partitioning(device) _validate_partitioning(device)
# the configdrive creation is taken care by ironic-lib's # For partition images the configdrive creation is taken care by
# work_on_disk(). # partition_utils.work_on_disk(), invoked from either
# _write_partition_image or _cache_and_write_image above.
# Handle whole disk images explicitly now.
if image_info.get('image_type') != 'partition': if image_info.get('image_type') != 'partition':
if configdrive is not None: if configdrive is not None:
# Will use dummy value of 'local' for 'node_uuid', # Will use dummy value of 'local' for 'node_uuid',

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

@ -52,7 +52,6 @@ def _build_fake_partition_image_info():
'ephemeral_mb': '10', 'ephemeral_mb': '10',
'ephemeral_format': 'abc', 'ephemeral_format': 'abc',
'preserve_ephemeral': 'False', 'preserve_ephemeral': 'False',
'configdrive': 'configdrive',
'image_type': 'partition', 'image_type': 'partition',
'boot_option': 'netboot', 'boot_option': 'netboot',
'disk_label': 'msdos', 'disk_label': 'msdos',
@ -214,7 +213,6 @@ class TestStandbyExtension(base.IronicAgentTest):
ephemeral_format = image_info['ephemeral_format'] ephemeral_format = image_info['ephemeral_format']
node_uuid = image_info['node_uuid'] node_uuid = image_info['node_uuid']
pr_ep = image_info['preserve_ephemeral'] pr_ep = image_info['preserve_ephemeral']
configdrive = image_info['configdrive']
boot_mode = image_info['deploy_boot_mode'] boot_mode = image_info['deploy_boot_mode']
boot_option = image_info['boot_option'] boot_option = image_info['boot_option']
disk_label = image_info['disk_label'] disk_label = image_info['disk_label']
@ -229,14 +227,14 @@ class TestStandbyExtension(base.IronicAgentTest):
work_on_disk_mock.side_effect = Exception_returned work_on_disk_mock.side_effect = Exception_returned
self.assertRaises(exc, standby._write_image, image_info, self.assertRaises(exc, standby._write_image, image_info,
device) device, 'configdrive')
image_mb_mock.assert_called_once_with(image_path) image_mb_mock.assert_called_once_with(image_path)
work_on_disk_mock.assert_called_once_with(device, root_mb, swap_mb, work_on_disk_mock.assert_called_once_with(device, root_mb, swap_mb,
ephemeral_mb, ephemeral_mb,
ephemeral_format, ephemeral_format,
image_path, image_path,
node_uuid, node_uuid,
configdrive=configdrive, configdrive='configdrive',
preserve_ephemeral=pr_ep, preserve_ephemeral=pr_ep,
boot_mode=boot_mode, boot_mode=boot_mode,
boot_option=boot_option, boot_option=boot_option,
@ -262,7 +260,6 @@ class TestStandbyExtension(base.IronicAgentTest):
ephemeral_format = image_info['ephemeral_format'] ephemeral_format = image_info['ephemeral_format']
node_uuid = image_info['node_uuid'] node_uuid = image_info['node_uuid']
pr_ep = image_info['preserve_ephemeral'] pr_ep = image_info['preserve_ephemeral']
configdrive = image_info['configdrive']
boot_mode = image_info['deploy_boot_mode'] boot_mode = image_info['deploy_boot_mode']
boot_option = image_info['boot_option'] boot_option = image_info['boot_option']
disk_label = image_info['disk_label'] disk_label = image_info['disk_label']
@ -277,14 +274,14 @@ class TestStandbyExtension(base.IronicAgentTest):
image_mb_mock.return_value = 1 image_mb_mock.return_value = 1
work_on_disk_mock.return_value = uuids work_on_disk_mock.return_value = uuids
standby._write_image(image_info, device) standby._write_image(image_info, device, 'configdrive')
image_mb_mock.assert_called_once_with(image_path) image_mb_mock.assert_called_once_with(image_path)
work_on_disk_mock.assert_called_once_with(device, root_mb, swap_mb, work_on_disk_mock.assert_called_once_with(device, root_mb, swap_mb,
ephemeral_mb, ephemeral_mb,
ephemeral_format, ephemeral_format,
image_path, image_path,
node_uuid, node_uuid,
configdrive=configdrive, configdrive='configdrive',
preserve_ephemeral=pr_ep, preserve_ephemeral=pr_ep,
boot_mode=boot_mode, boot_mode=boot_mode,
boot_option=boot_option, boot_option=boot_option,
@ -335,7 +332,6 @@ class TestStandbyExtension(base.IronicAgentTest):
ephemeral_format = image_info['ephemeral_format'] ephemeral_format = image_info['ephemeral_format']
node_uuid = image_info['node_uuid'] node_uuid = image_info['node_uuid']
pr_ep = image_info['preserve_ephemeral'] pr_ep = image_info['preserve_ephemeral']
configdrive = image_info['configdrive']
boot_mode = image_info['deploy_boot_mode'] boot_mode = image_info['deploy_boot_mode']
boot_option = image_info['boot_option'] boot_option = image_info['boot_option']
disk_label = image_info['disk_label'] disk_label = image_info['disk_label']
@ -348,14 +344,14 @@ class TestStandbyExtension(base.IronicAgentTest):
dispatch_mock.return_value = self.fake_cpu dispatch_mock.return_value = self.fake_cpu
work_on_disk_mock.return_value = uuids work_on_disk_mock.return_value = uuids
standby._write_image(image_info, device) standby._write_image(image_info, device, 'configdrive')
image_mb_mock.assert_called_once_with(image_path) image_mb_mock.assert_called_once_with(image_path)
work_on_disk_mock.assert_called_once_with(device, root_mb, swap_mb, work_on_disk_mock.assert_called_once_with(device, root_mb, swap_mb,
ephemeral_mb, ephemeral_mb,
ephemeral_format, ephemeral_format,
image_path, image_path,
node_uuid, node_uuid,
configdrive=configdrive, configdrive='configdrive',
preserve_ephemeral=pr_ep, preserve_ephemeral=pr_ep,
boot_mode=boot_mode, boot_mode=boot_mode,
boot_option=boot_option, boot_option=boot_option,
@ -578,7 +574,7 @@ class TestStandbyExtension(base.IronicAgentTest):
async_result = self.agent_extension.cache_image(image_info=image_info) async_result = self.agent_extension.cache_image(image_info=image_info)
async_result.join() async_result.join()
download_mock.assert_called_once_with(image_info) download_mock.assert_called_once_with(image_info)
write_mock.assert_called_once_with(image_info, 'manager') write_mock.assert_called_once_with(image_info, 'manager', None)
dispatch_mock.assert_called_once_with('get_os_install_device', dispatch_mock.assert_called_once_with('get_os_install_device',
permit_refresh=True) permit_refresh=True)
self.assertEqual(image_info['id'], self.assertEqual(image_info['id'],
@ -602,10 +598,12 @@ class TestStandbyExtension(base.IronicAgentTest):
download_mock.return_value = None download_mock.return_value = None
write_mock.return_value = {'root uuid': 'root_uuid'} write_mock.return_value = {'root uuid': 'root_uuid'}
dispatch_mock.return_value = 'manager' dispatch_mock.return_value = 'manager'
async_result = self.agent_extension.cache_image(image_info=image_info) async_result = self.agent_extension.cache_image(
image_info=image_info, configdrive='configdrive_data')
async_result.join() async_result.join()
download_mock.assert_called_once_with(image_info) download_mock.assert_called_once_with(image_info)
write_mock.assert_called_once_with(image_info, 'manager') write_mock.assert_called_once_with(image_info, 'manager',
'configdrive_data')
dispatch_mock.assert_called_once_with('get_os_install_device', dispatch_mock.assert_called_once_with('get_os_install_device',
permit_refresh=True) permit_refresh=True)
self.assertEqual(image_info['id'], self.assertEqual(image_info['id'],
@ -637,7 +635,7 @@ class TestStandbyExtension(base.IronicAgentTest):
) )
async_result.join() async_result.join()
download_mock.assert_called_once_with(image_info) download_mock.assert_called_once_with(image_info)
write_mock.assert_called_once_with(image_info, 'manager') write_mock.assert_called_once_with(image_info, 'manager', None)
dispatch_mock.assert_called_once_with('get_os_install_device', dispatch_mock.assert_called_once_with('get_os_install_device',
permit_refresh=True) permit_refresh=True)
self.assertEqual(image_info['id'], self.assertEqual(image_info['id'],
@ -712,7 +710,8 @@ class TestStandbyExtension(base.IronicAgentTest):
async_result.join() async_result.join()
download_mock.assert_called_once_with(image_info) download_mock.assert_called_once_with(image_info)
write_mock.assert_called_once_with(image_info, 'manager') write_mock.assert_called_once_with(image_info, 'manager',
'configdrive_data')
dispatch_mock.assert_called_once_with('get_os_install_device', dispatch_mock.assert_called_once_with('get_os_install_device',
permit_refresh=True) permit_refresh=True)
configdrive_copy_mock.assert_called_once_with(image_info['node_uuid'], configdrive_copy_mock.assert_called_once_with(image_info['node_uuid'],
@ -763,7 +762,8 @@ class TestStandbyExtension(base.IronicAgentTest):
async_result.join() async_result.join()
download_mock.assert_called_once_with(image_info) download_mock.assert_called_once_with(image_info)
write_mock.assert_called_once_with(image_info, 'manager') write_mock.assert_called_once_with(image_info, 'manager',
'configdrive_data')
dispatch_mock.assert_called_once_with('get_os_install_device', dispatch_mock.assert_called_once_with('get_os_install_device',
permit_refresh=True) permit_refresh=True)
self.assertFalse(configdrive_copy_mock.called) self.assertFalse(configdrive_copy_mock.called)
@ -836,7 +836,7 @@ class TestStandbyExtension(base.IronicAgentTest):
async_result.join() async_result.join()
download_mock.assert_called_once_with(image_info) download_mock.assert_called_once_with(image_info)
write_mock.assert_called_once_with(image_info, 'manager') write_mock.assert_called_once_with(image_info, 'manager', None)
dispatch_mock.assert_called_once_with('get_os_install_device', dispatch_mock.assert_called_once_with('get_os_install_device',
permit_refresh=True) permit_refresh=True)
@ -887,7 +887,7 @@ class TestStandbyExtension(base.IronicAgentTest):
async_result.join() async_result.join()
download_mock.assert_called_once_with(image_info) download_mock.assert_called_once_with(image_info)
write_mock.assert_called_once_with(image_info, 'manager') write_mock.assert_called_once_with(image_info, 'manager', None)
dispatch_mock.assert_called_once_with('get_os_install_device', dispatch_mock.assert_called_once_with('get_os_install_device',
permit_refresh=True) permit_refresh=True)
@ -930,7 +930,8 @@ class TestStandbyExtension(base.IronicAgentTest):
async_result.join() async_result.join()
download_mock.assert_called_once_with(image_info) download_mock.assert_called_once_with(image_info)
write_mock.assert_called_once_with(image_info, 'manager') write_mock.assert_called_once_with(image_info, 'manager',
'configdrive_data')
dispatch_mock.assert_called_once_with('get_os_install_device', dispatch_mock.assert_called_once_with('get_os_install_device',
permit_refresh=True) permit_refresh=True)
configdrive_copy_mock.assert_called_once_with(image_info['node_uuid'], configdrive_copy_mock.assert_called_once_with(image_info['node_uuid'],
@ -998,7 +999,7 @@ class TestStandbyExtension(base.IronicAgentTest):
self.assertIs(partition, work_on_disk_mock.called) self.assertIs(partition, work_on_disk_mock.called)
else: else:
cache_write_mock.assert_called_once_with(mock.ANY, image_info, cache_write_mock.assert_called_once_with(mock.ANY, image_info,
'/dev/foo') '/dev/foo', None)
self.assertFalse(stream_mock.called) self.assertFalse(stream_mock.called)
def test_prepare_image_raw_stream_true(self): def test_prepare_image_raw_stream_true(self):
@ -1169,7 +1170,21 @@ class TestStandbyExtension(base.IronicAgentTest):
device = '/dev/foo' device = '/dev/foo'
self.agent_extension._cache_and_write_image(image_info, device) self.agent_extension._cache_and_write_image(image_info, device)
download_mock.assert_called_once_with(image_info) download_mock.assert_called_once_with(image_info)
write_mock.assert_called_once_with(image_info, device) write_mock.assert_called_once_with(image_info, device, None)
@mock.patch('ironic_python_agent.extensions.standby._write_image',
autospec=True)
@mock.patch('ironic_python_agent.extensions.standby._download_image',
autospec=True)
def test_cache_and_write_image_configdirve(self, download_mock,
write_mock):
image_info = _build_fake_image_info()
device = '/dev/foo'
self.agent_extension._cache_and_write_image(image_info, device,
'configdrive_data')
download_mock.assert_called_once_with(image_info)
write_mock.assert_called_once_with(image_info, device,
'configdrive_data')
@mock.patch('ironic_lib.disk_utils.block_uuid', autospec=True) @mock.patch('ironic_lib.disk_utils.block_uuid', autospec=True)
@mock.patch('ironic_lib.disk_utils.fix_gpt_partition', autospec=True) @mock.patch('ironic_lib.disk_utils.fix_gpt_partition', autospec=True)
@ -1395,7 +1410,6 @@ class TestStandbyExtension(base.IronicAgentTest):
ephemeral_format = image_info['ephemeral_format'] ephemeral_format = image_info['ephemeral_format']
node_uuid = image_info['node_uuid'] node_uuid = image_info['node_uuid']
pr_ep = image_info['preserve_ephemeral'] pr_ep = image_info['preserve_ephemeral']
configdrive = image_info['configdrive']
boot_option = image_info['boot_option'] boot_option = image_info['boot_option']
cpu_arch = self.fake_cpu.architecture cpu_arch = self.fake_cpu.architecture
@ -1408,14 +1422,14 @@ class TestStandbyExtension(base.IronicAgentTest):
image_mb_mock.return_value = 1 image_mb_mock.return_value = 1
work_on_disk_mock.return_value = uuids work_on_disk_mock.return_value = uuids
standby._write_image(image_info, device) standby._write_image(image_info, device, 'configdrive')
image_mb_mock.assert_called_once_with(image_path) image_mb_mock.assert_called_once_with(image_path)
work_on_disk_mock.assert_called_once_with(device, root_mb, swap_mb, work_on_disk_mock.assert_called_once_with(device, root_mb, swap_mb,
ephemeral_mb, ephemeral_mb,
ephemeral_format, ephemeral_format,
image_path, image_path,
node_uuid, node_uuid,
configdrive=configdrive, configdrive='configdrive',
preserve_ephemeral=pr_ep, preserve_ephemeral=pr_ep,
boot_mode='uefi', boot_mode='uefi',
boot_option=boot_option, boot_option=boot_option,

6
releasenotes/notes/configdrive-dup-3fc46a878fe82485.yaml Normal file
View File

@ -0,0 +1,6 @@
---
other:
- |
The API call ``prepare_image`` and the deploy step ``write_image``
will now expect configdrive to be passed as an explicit argument
rather than through ``image_info``.