From d3737b23bdb5bc79b27009aa06f0ffcfe54d6310 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Tue, 6 Jul 2021 17:24:49 +0200 Subject: [PATCH] Bring boot_iso/deploy_iso handling in iLO closer to Redfish The iLO version is, as far as I remember, the oldest implementation of virtual media in Ironic. Since then a few cool feature have been developed in this area, but they are limited to the code using image_utils (e.g. Redfish). The iLO boot interface only uses image_utils when an ISO needs to be built, but not when one is provided. This patch changes that. There are a few visible side-effects to that: 1) file:/// images are now supported 2) ramdisk_image_download_source is now supported 3) the default caching behavior changes to caching http:// links (previously they were passed to the BMC directly). 4) glance images are supported with backends other than swift Story: #2008987 Task: #42638 Change-Id: I23c21188776c511eddcdaf66a222ce64876678e2 --- ironic/drivers/modules/ilo/boot.py | 63 ++++--------- ironic/drivers/modules/image_utils.py | 12 ++- .../unit/drivers/modules/ilo/test_boot.py | 92 +++++++++---------- .../unit/drivers/modules/test_image_utils.py | 9 ++ .../ilo-deploy-iso-0c88edb5daff8a4e.yaml | 22 +++++ 5 files changed, 106 insertions(+), 92 deletions(-) create mode 100644 releasenotes/notes/ilo-deploy-iso-0c88edb5daff8a4e.yaml diff --git a/ironic/drivers/modules/ilo/boot.py b/ironic/drivers/modules/ilo/boot.py index 32b0f9a1c5..bcfb7bb967 100644 --- a/ironic/drivers/modules/ilo/boot.py +++ b/ironic/drivers/modules/ilo/boot.py @@ -132,14 +132,7 @@ def _get_boot_iso(task, root_uuid): :param task: a TaskManager instance containing the node to act on. :param root_uuid: the uuid of the root partition. - :returns: boot ISO URL. Should be either of below: - * A Swift object - It should be of format 'swift:'. It is - assumed that the image object is present in - CONF.ilo.swift_ilo_container; - * A Glance image - It should be format 'glance://' - or just ; - * An HTTP URL. - On error finding the boot iso, it returns None. + :returns: boot ISO HTTP or swift:// URL. :raises: MissingParameterValue, if any of the required parameters are missing in the node's driver_info or instance_info. :raises: InvalidParameterValue, if any of the parameters have invalid @@ -156,35 +149,24 @@ def _get_boot_iso(task, root_uuid): boot_iso = driver_utils.get_field(task.node, 'boot_iso', deprecated_prefix='ilo', collection='instance_info') - if boot_iso: - LOG.debug("Using boot_iso provided in node's instance_info") - if not service_utils.is_glance_image(boot_iso): - try: - image_service.HttpImageService().validate_href(boot_iso) - except exception.ImageRefValidationFailed: - with excutils.save_and_reraise_exception(): - LOG.error("Virtual media deploy accepts only Glance " - "images or HTTP(S) URLs as " - "instance_info['boot_iso']. Either %s " - "is not a valid HTTP(S) URL or is " - "not reachable.", boot_iso) - - return boot_iso + deploy_info = _parse_deploy_info(task.node) # Option 2 - Check if user has provided a boot_iso in Glance. If boot_iso # is a supported non-glance href execution will proceed to option 3. - deploy_info = _parse_deploy_info(task.node) + if not boot_iso: + # TODO(dtantsur): this approach should be generic across drivers. + image_href = deploy_info['image_source'] + image_properties = ( + images.get_image_properties( + task.context, image_href, ['boot_iso'])) - image_href = deploy_info['image_source'] - image_properties = ( - images.get_image_properties( - task.context, image_href, ['boot_iso'])) + boot_iso = image_properties.get('boot_iso') - boot_iso_uuid = image_properties.get('boot_iso') - - if boot_iso_uuid: - LOG.debug("Found boot_iso %s in Glance", boot_iso_uuid) - return boot_iso_uuid + if boot_iso: + i_info = task.node.instance_info + i_info['boot_iso'] = boot_iso + task.node.instance_info = i_info + task.node.save() # NOTE(rameshg87): Functionality to share the boot ISOs created for # similar instances (instances with same deployed image) is @@ -436,14 +418,12 @@ class IloVirtualMediaBoot(base.BootInterface): mode = deploy_utils.rescue_or_deploy_mode(node) d_info = parse_driver_info(node, mode) - if 'rescue_iso' in d_info: - ilo_common.setup_vmedia(task, d_info['rescue_iso'], ramdisk_params) - elif 'deploy_iso' in d_info: - ilo_common.setup_vmedia(task, d_info['deploy_iso'], ramdisk_params) - else: - iso = image_utils.prepare_deploy_iso(task, ramdisk_params, - mode, d_info) - ilo_common.setup_vmedia(task, iso) + iso = image_utils.prepare_deploy_iso(task, ramdisk_params, + mode, d_info) + if not d_info.get(f'{mode}_iso'): + # No need to attach a floopy when an ISO is built by us. + ramdisk_params = None + ilo_common.setup_vmedia(task, iso, ramdisk_params) @METRICS.timer('IloVirtualMediaBoot.prepare_instance') def prepare_instance(self, task): @@ -569,9 +549,6 @@ class IloVirtualMediaBoot(base.BootInterface): node = task.node boot_iso = _get_boot_iso(task, root_uuid) - if not boot_iso: - LOG.error("Cannot get boot ISO for node %s", node.uuid) - return # Upon deploy complete, some distros cloud images reboot the system as # part of its configuration. Hence boot device should be persistent and diff --git a/ironic/drivers/modules/image_utils.py b/ironic/drivers/modules/image_utils.py index a0092ce22f..5f877fb44d 100644 --- a/ironic/drivers/modules/image_utils.py +++ b/ironic/drivers/modules/image_utils.py @@ -437,13 +437,23 @@ def _prepare_iso_image(task, kernel_href, ramdisk_href, # NOTE(rpittau): if base_iso is defined as http address, we just access # it directly. if base_iso: + scheme = urlparse.urlparse(base_iso).scheme.lower() + if scheme == 'swift': + # FIXME(dtantsur): iLO supports swift: scheme. In the long run we + # should support it for all boot interfaces by using temporary + # URLs. Until it's done, return base_iso as it is. + return base_iso + if (download_source == 'swift' and service_utils.is_glance_image(base_iso)): base_iso = ( images.get_temp_url_for_glance_image(task.context, base_iso)) + # get_temp_url_for_glance_image return an HTTP (or HTTPS - doesn't + # matter here) image. + scheme = 'http' if download_source != 'local': - if base_iso.startswith(('http://', 'https://')): + if scheme in ('http', 'https'): return base_iso LOG.debug("ramdisk_image_download_source set to http but " "boot_iso is not an HTTP URL: %(boot_iso)s", diff --git a/ironic/tests/unit/drivers/modules/ilo/test_boot.py b/ironic/tests/unit/drivers/modules/ilo/test_boot.py index 74263cae1d..da03cf3048 100644 --- a/ironic/tests/unit/drivers/modules/ilo/test_boot.py +++ b/ironic/tests/unit/drivers/modules/ilo/test_boot.py @@ -156,9 +156,12 @@ class IloBootPrivateMethodsTestCase(test_common.BaseIloTest): boot_interface = 'ilo-virtual-media' - @mock.patch.object(image_service.HttpImageService, 'validate_href', - spec_set=True, autospec=True) - def test__get_boot_iso_http_url(self, service_mock): + @mock.patch.object(image_utils, 'prepare_boot_iso', spec_set=True, + autospec=True) + @mock.patch.object(ilo_boot, '_parse_deploy_info', spec_set=True, + autospec=True) + def test__get_boot_iso_http_url(self, deploy_info_mock, + prepare_iso_mock): url = 'http://abc.org/image/qcow2' i_info = self.node.instance_info i_info['boot_iso'] = url @@ -168,31 +171,20 @@ class IloBootPrivateMethodsTestCase(test_common.BaseIloTest): with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: boot_iso_actual = ilo_boot._get_boot_iso(task, 'root-uuid') - service_mock.assert_called_once_with(mock.ANY, url) - self.assertEqual(url, boot_iso_actual) - - @mock.patch.object(image_service.HttpImageService, 'validate_href', - spec_set=True, autospec=True) - def test__get_boot_iso_unsupported_url(self, validate_href_mock): - validate_href_mock.side_effect = exception.ImageRefValidationFailed( - image_href='file://img.qcow2', reason='fail') - url = 'file://img.qcow2' - i_info = self.node.instance_info - i_info['boot_iso'] = url - self.node.instance_info = i_info - self.node.save() - - with task_manager.acquire(self.context, self.node.uuid, - shared=False) as task: - self.assertRaises(exception.ImageRefValidationFailed, - ilo_boot._get_boot_iso, task, 'root-uuid') + deploy_info_mock.assert_called_once_with(task.node) + prepare_iso_mock.assert_called_once_with( + task, deploy_info_mock.return_value, 'root-uuid') + self.assertEqual(prepare_iso_mock.return_value, boot_iso_actual) + @mock.patch.object(image_utils, 'prepare_boot_iso', spec_set=True, + autospec=True) @mock.patch.object(images, 'get_image_properties', spec_set=True, autospec=True) @mock.patch.object(ilo_boot, '_parse_deploy_info', spec_set=True, autospec=True) def test__get_boot_iso_glance_image(self, deploy_info_mock, - image_props_mock): + image_props_mock, + prepare_iso_mock): deploy_info_mock.return_value = {'image_source': 'image-uuid', 'ilo_deploy_iso': 'deploy_iso_uuid'} image_props_mock.return_value = {'boot_iso': u'glance://uui\u0111'} @@ -204,7 +196,26 @@ class IloBootPrivateMethodsTestCase(test_common.BaseIloTest): image_props_mock.assert_called_once_with( task.context, 'image-uuid', ['boot_iso']) boot_iso_expected = u'glance://uui\u0111' - self.assertEqual(boot_iso_expected, boot_iso_actual) + prepare_iso_mock.assert_called_once_with( + task, deploy_info_mock.return_value, 'root-uuid') + self.assertEqual(prepare_iso_mock.return_value, boot_iso_actual) + self.assertEqual(boot_iso_expected, + task.node.instance_info['boot_iso']) + + @mock.patch.object(ilo_boot, '_parse_deploy_info', spec_set=True, + autospec=True) + def test__get_boot_iso_swift_image(self, deploy_info_mock): + url = 'swift://abc.org/image/qcow2' + i_info = self.node.instance_info + i_info['boot_iso'] = url + self.node.instance_info = i_info + self.node.save() + + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + boot_iso_actual = ilo_boot._get_boot_iso(task, 'root-uuid') + self.assertEqual(url, boot_iso_actual) + deploy_info_mock.assert_called_once_with(task.node) @mock.patch.object(image_utils, 'prepare_boot_iso', spec_set=True, autospec=True) @@ -549,6 +560,7 @@ class IloVirtualMediaBootTestCase(test_common.BaseIloTest): self.assertRaises(exception.UnsupportedDriverExtension, task.driver.boot.validate_inspection, task) + @mock.patch.object(image_utils, 'prepare_deploy_iso', autospec=True) @mock.patch.object(ilo_boot, 'prepare_node_for_deploy', spec_set=True, autospec=True) @mock.patch.object(ilo_common, 'eject_vmedia_devices', @@ -560,6 +572,7 @@ class IloVirtualMediaBootTestCase(test_common.BaseIloTest): def _test_prepare_ramdisk(self, get_nic_mock, setup_vmedia_mock, eject_mock, prepare_node_for_deploy_mock, + prepare_boot_iso_mock, ilo_boot_iso, image_source, ramdisk_params={'a': 'b'}, mode='deploy'): @@ -586,8 +599,12 @@ class IloVirtualMediaBootTestCase(test_common.BaseIloTest): 'ipa-agent-token': mock.ANY, 'boot_method': 'vmedia'} get_nic_mock.assert_called_once_with(task) - setup_vmedia_mock.assert_called_once_with(task, iso, - expected_ramdisk_opts) + prepare_boot_iso_mock.assert_called_once_with( + task, expected_ramdisk_opts, mode, + {f'{mode}_iso': iso, 'kernel_append_params': None}) + setup_vmedia_mock.assert_called_once_with( + task, prepare_boot_iso_mock.return_value, + expected_ramdisk_opts) @mock.patch.object(service_utils, 'is_glance_image', spec_set=True, autospec=True) @@ -703,7 +720,8 @@ class IloVirtualMediaBootTestCase(test_common.BaseIloTest): driver_info_mock.assert_called_once_with(task.node, mode) prepare_deploy_iso_mock.assert_called_once_with( task, ramdisk_params, mode, d_info) - setup_vmedia_mock.assert_called_once_with(task, 'recreated-iso') + setup_vmedia_mock.assert_called_once_with( + task, 'recreated-iso', None) @mock.patch.object(manager_utils, 'node_set_boot_device', spec_set=True, autospec=True) @@ -731,28 +749,6 @@ class IloVirtualMediaBootTestCase(test_common.BaseIloTest): self.assertEqual('boot.iso', task.node.instance_info['boot_iso']) - @mock.patch.object(manager_utils, 'node_set_boot_device', spec_set=True, - autospec=True) - @mock.patch.object(ilo_common, 'setup_vmedia_for_boot', spec_set=True, - autospec=True) - @mock.patch.object(ilo_boot, '_get_boot_iso', spec_set=True, - autospec=True) - def test__configure_vmedia_boot_without_boot_iso( - self, get_boot_iso_mock, setup_vmedia_mock, set_boot_device_mock): - root_uuid = {'root uuid': 'root_uuid'} - - with task_manager.acquire(self.context, self.node.uuid, - shared=False) as task: - get_boot_iso_mock.return_value = None - - task.driver.boot._configure_vmedia_boot( - task, root_uuid) - - get_boot_iso_mock.assert_called_once_with( - task, root_uuid) - self.assertFalse(setup_vmedia_mock.called) - self.assertFalse(set_boot_device_mock.called) - @mock.patch.object(deploy_utils, 'is_iscsi_boot', spec_set=True, autospec=True) @mock.patch.object(boot_mode_utils, 'deconfigure_secure_boot_if_needed', diff --git a/ironic/tests/unit/drivers/modules/test_image_utils.py b/ironic/tests/unit/drivers/modules/test_image_utils.py index f06b4eb7c0..f458227714 100644 --- a/ironic/tests/unit/drivers/modules/test_image_utils.py +++ b/ironic/tests/unit/drivers/modules/test_image_utils.py @@ -666,6 +666,15 @@ class RedfishImageUtilsTestCase(db_base.DbTestCase): base_iso=base_image_url) self.assertEqual(url, base_image_url) + def test__prepare_iso_image_bootable_iso_swift_schema(self): + with task_manager.acquire(self.context, self.node.uuid, + shared=True) as task: + base_image_url = 'swift://bearmetal.net/boot.iso' + url = image_utils._prepare_iso_image( + task, None, None, bootloader_href=None, root_uuid=None, + base_iso=base_image_url) + self.assertEqual(url, base_image_url) + def test__find_param(self): param_dict = { 'deploy_kernel': 'kernel', diff --git a/releasenotes/notes/ilo-deploy-iso-0c88edb5daff8a4e.yaml b/releasenotes/notes/ilo-deploy-iso-0c88edb5daff8a4e.yaml new file mode 100644 index 0000000000..ba674c917c --- /dev/null +++ b/releasenotes/notes/ilo-deploy-iso-0c88edb5daff8a4e.yaml @@ -0,0 +1,22 @@ +--- +features: + - | + The ``ilo-virtual-media`` deploy interface now supports ``file:///`` URLs + for ``boot_iso`` and ``deploy_iso``. + - | + The ``ilo-virtual-media`` deploy interface now supports the + ``[deploy]ramdisk_image_download_source`` configuration option. +fixes: + - | + The ``ilo-virtual-media`` deploy interface no longer requires the Image + service backend to be Swift for Glance images in ``boot_iso`` and + ``deploy_iso``. +upgrade: + - | + Since ``ilo-virtual-media`` deploy interface now respects the + ``[deploy]ramdisk_image_download_source`` configuration options, its + default caching behavior has changed. Now HTTP ``boot_iso``/``deploy_iso`` + are cached locally and served from the conductor's HTTP server instead of + passing them directly to the BMC. Glance images are also cached locally. + To revert to the previous behavior, set the + ``[deploy]ramdisk_image_download_source`` option to ``swift``.