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

5 files changed, 106 insertions, 92 deletions

diff --git a/ironic/drivers/modules/ilo/boot.py b/ironic/drivers/modules/ilo/boot.py
index 32b0f9a1c..bcfb7bb96 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:<object-name>'. It is
-          assumed that the image object is present in
-          CONF.ilo.swift_ilo_container;
-        * A Glance image - It should be format 'glance://<glance-image-uuid>'
-          or just <glance-image-uuid>;
-        * 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)
-
-    image_href = deploy_info['image_source']
-    image_properties = (
-        images.get_image_properties(
-            task.context, image_href, ['boot_iso']))
+    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']))
 
-    boot_iso_uuid = image_properties.get('boot_iso')
+        boot_iso = 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 a0092ce22..5f877fb44 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 74263cae1..da03cf304 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 f06b4eb7c..f45822771 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 000000000..ba674c917
--- /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``.