Merge "Avoid unnecessary validation in boot interfaces"

ironic/drivers/modules/pxe_base.py

@@ -400,22 +400,26 @@ class PXEBaseMixin(object):
             missing.
         """
         self._validate_common(task)
+        node = task.node
 
         # NOTE(TheJulia): If we're not writing an image, we can skip
         # the remainder of this method.
-        if (not task.driver.storage.should_write_image(task)):
+        # NOTE(dtantsur): if we're are writing an image with local boot
+        # the boot interface does not care about image parameters and
+        # must not validate them.
+        boot_option = deploy_utils.get_boot_option(node)
+        if (not task.driver.storage.should_write_image(task)
+                or boot_option == 'local'):
             return
 
-        node = task.node
         d_info = deploy_utils.get_image_instance_info(node)
-        if (node.driver_internal_info.get('is_whole_disk_image')
+        if node.driver_internal_info.get('is_whole_disk_image'):
-                or deploy_utils.get_boot_option(node) == 'local'):
             props = []
         elif d_info.get('boot_iso'):
             props = ['boot_iso']
         elif service_utils.is_glance_image(d_info['image_source']):
             props = ['kernel_id', 'ramdisk_id']
-            if deploy_utils.get_boot_option(node) == 'kickstart':
+            if boot_option == 'kickstart':
                 props.append('squashfs_id')
         else:
             props = ['kernel', 'ramdisk']

ironic/drivers/modules/redfish/boot.py

@@ -392,6 +392,13 @@ class RedfishVirtualMediaBoot(base.BootInterface):
         """
         node = task.node
 
+        # NOTE(dtantsur): if we're are writing an image with local boot
+        # the boot interface does not care about image parameters and
+        # must not validate them.
+        if (not task.driver.storage.should_write_image(task)
+                or deploy_utils.get_boot_option(node) == 'local'):
+            return
+
         d_info = _parse_deploy_info(node)
 
         if node.driver_internal_info.get('is_whole_disk_image'):
@@ -432,9 +439,7 @@ class RedfishVirtualMediaBoot(base.BootInterface):
         """
         self._validate_vendor(task)
         self._validate_driver_info(task)
-
+        self._validate_instance_info(task)
-        if task.driver.storage.should_write_image(task):
-            self._validate_instance_info(task)
 
     def validate_inspection(self, task):
         """Validate that the node has required properties for inspection.

ironic/tests/unit/drivers/modules/redfish/test_boot.py

@@ -217,14 +217,26 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
                               redfish_boot._parse_deploy_info,
                               task.node)
 
+    @mock.patch.object(redfish_utils, 'parse_driver_info', autospec=True)
+    def test_validate_local(self, mock_parse_driver_info):
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=True) as task:
+            task.node.instance_info = {}
+
+            task.node.driver_info.update(
+                {'deploy_kernel': 'kernel',
+                 'deploy_ramdisk': 'ramdisk',
+                 'bootloader': 'bootloader'}
+            )
+
+            task.driver.boot.validate(task)
+
+    @mock.patch.object(deploy_utils, 'get_boot_option', lambda node: 'ramdisk')
     @mock.patch.object(redfish_utils, 'parse_driver_info', autospec=True)
     @mock.patch.object(deploy_utils, 'validate_image_properties',
                        autospec=True)
-    @mock.patch.object(boot_mode_utils, 'get_boot_mode_for_deploy',
+    def test_validate_kernel_ramdisk(self, mock_validate_image_properties,
-                       autospec=True)
+                                     mock_parse_driver_info):
-    def test_validate_uefi_boot(self, mock_get_boot_mode,
-                                mock_validate_image_properties,
-                                mock_parse_driver_info):
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=True) as task:
             task.node.instance_info.update(
@@ -239,50 +251,17 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
                  'bootloader': 'bootloader'}
             )
 
-            mock_get_boot_mode.return_value = 'uefi'
-
             task.driver.boot.validate(task)
 
             mock_validate_image_properties.assert_called_once_with(
-                mock.ANY, mock.ANY, mock.ANY)
+                task.context, mock.ANY, ['kernel', 'ramdisk'])
 
+    @mock.patch.object(deploy_utils, 'get_boot_option', lambda node: 'ramdisk')
     @mock.patch.object(redfish_utils, 'parse_driver_info', autospec=True)
     @mock.patch.object(deploy_utils, 'validate_image_properties',
                        autospec=True)
-    @mock.patch.object(boot_mode_utils, 'get_boot_mode_for_deploy',
+    def test_validate_boot_iso(self, mock_validate_image_properties,
-                       autospec=True)
+                               mock_parse_driver_info):
-    def test_validate_bios_boot(self, mock_get_boot_mode,
-                                mock_validate_image_properties,
-                                mock_parse_driver_info):
-        with task_manager.acquire(self.context, self.node.uuid,
-                                  shared=True) as task:
-            task.node.instance_info.update(
-                {'kernel': 'kernel',
-                 'ramdisk': 'ramdisk',
-                 'image_source': 'http://image/source'}
-            )
-
-            task.node.driver_info.update(
-                {'deploy_kernel': 'kernel',
-                 'deploy_ramdisk': 'ramdisk',
-                 'bootloader': 'bootloader'}
-            )
-
-            mock_get_boot_mode.return_value = 'bios'
-
-            task.driver.boot.validate(task)
-
-            mock_validate_image_properties.assert_called_once_with(
-                mock.ANY, mock.ANY, mock.ANY)
-
-    @mock.patch.object(redfish_utils, 'parse_driver_info', autospec=True)
-    @mock.patch.object(deploy_utils, 'validate_image_properties',
-                       autospec=True)
-    @mock.patch.object(boot_mode_utils, 'get_boot_mode_for_deploy',
-                       autospec=True)
-    def test_validate_bios_boot_iso(self, mock_get_boot_mode,
-                                    mock_validate_image_properties,
-                                    mock_parse_driver_info):
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=True) as task:
             task.node.instance_info.update(
@@ -294,44 +273,11 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
                  'deploy_ramdisk': 'ramdisk',
                  'bootloader': 'bootloader'}
             )
-            # NOTE(TheJulia): Boot mode doesn't matter for this
-            # test scenario.
-            mock_get_boot_mode.return_value = 'bios'
 
             task.driver.boot.validate(task)
 
             mock_validate_image_properties.assert_called_once_with(
-                mock.ANY, mock.ANY, mock.ANY)
+                task.context, mock.ANY, ['boot_iso'])
-
-    @mock.patch.object(redfish_utils, 'parse_driver_info', autospec=True)
-    @mock.patch.object(deploy_utils, 'validate_image_properties',
-                       autospec=True)
-    @mock.patch.object(boot_mode_utils, 'get_boot_mode_for_deploy',
-                       autospec=True)
-    def test_validate_bios_boot_iso_conflicting_image_source(
-            self, mock_get_boot_mode,
-            mock_validate_image_properties,
-            mock_parse_driver_info):
-        with task_manager.acquire(self.context, self.node.uuid,
-                                  shared=True) as task:
-            task.node.instance_info.update(
-                {'boot_iso': 'http://localhost/file.iso',
-                 'image_source': 'http://localhost/file.img'}
-            )
-
-            task.node.driver_info.update(
-                {'deploy_kernel': 'kernel',
-                 'deploy_ramdisk': 'ramdisk',
-                 'bootloader': 'bootloader'}
-            )
-            # NOTE(TheJulia): Boot mode doesn't matter for this
-            # test scenario.
-            mock_get_boot_mode.return_value = 'bios'
-
-            task.driver.boot.validate(task)
-
-            mock_validate_image_properties.assert_called_once_with(
-                mock.ANY, mock.ANY, mock.ANY)
 
     @mock.patch.object(redfish_utils, 'parse_driver_info', autospec=True)
     @mock.patch.object(deploy_utils, 'validate_image_properties',

ironic/tests/unit/drivers/modules/test_ipxe.py

@@ -19,7 +19,6 @@ import os
 from unittest import mock
 
 from oslo_config import cfg
-from oslo_serialization import jsonutils as json
 from oslo_utils import uuidutils
 
 from ironic.common import boot_devices
@@ -144,7 +143,9 @@ class iPXEBootTestCase(db_base.DbTestCase):
             self.assertTrue(mock_boot_option.called)
             self.assertTrue(mock_glance.called)
 
-    def test_validate_with_boot_iso_and_image_source(self):
+    @mock.patch('ironic.drivers.modules.deploy_utils.get_boot_option',
+                return_value='ramdisk', autospec=True)
+    def test_validate_with_boot_iso_and_image_source(self, mock_boot_option):
         i_info = self.node.instance_info
         i_info['image_source'] = "http://localhost:1234/image"
         i_info['boot_iso'] = "http://localhost:1234/boot.iso"
@@ -155,16 +156,26 @@ class iPXEBootTestCase(db_base.DbTestCase):
             self.assertRaises(exception.InvalidParameterValue,
                               task.driver.boot.validate,
                               task)
+            mock_boot_option.assert_called_once_with(task.node)
 
-    def test_validate_fail_missing_image_source(self):
+    @mock.patch('ironic.drivers.modules.deploy_utils.get_boot_option',
-        info = dict(INST_INFO_DICT)
+                return_value='local', autospec=True)
-        del info['image_source']
+    def test_validate_no_image_source_for_local_boot(self, mock_boot_option):
-        self.node.instance_info = json.dumps(info)
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=True) as task:
-            task.node['instance_info'] = json.dumps(info)
+            del task.node['instance_info']['image_source']
+            task.driver.boot.validate(task)
+            mock_boot_option.assert_called_with(task.node)
+
+    @mock.patch('ironic.drivers.modules.deploy_utils.get_boot_option',
+                return_value='netboot', autospec=True)
+    def test_validate_fail_missing_image_source(self, mock_boot_option):
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=True) as task:
+            del task.node['instance_info']['image_source']
             self.assertRaises(exception.MissingParameterValue,
                               task.driver.boot.validate, task)
+            mock_boot_option.assert_called_with(task.node)
 
     def test_validate_fail_no_port(self):
         new_node = obj_utils.create_test_node(
@@ -212,18 +223,25 @@ class iPXEBootTestCase(db_base.DbTestCase):
                               task.driver.boot.validate,
                               task)
 
+    @mock.patch('ironic.drivers.modules.deploy_utils.get_boot_option',
+                return_value='netboot', autospec=True)
     @mock.patch.object(image_service.GlanceImageService, 'show',
                        autospec=True)
-    def test_validate_fail_glance_image_doesnt_exists(self, mock_glance):
+    def test_validate_fail_glance_image_doesnt_exists(self, mock_glance,
+                                                      mock_boot_option):
         mock_glance.side_effect = exception.ImageNotFound('not found')
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=True) as task:
             self.assertRaises(exception.InvalidParameterValue,
                               task.driver.boot.validate, task)
+            mock_boot_option.assert_called_with(task.node)
 
+    @mock.patch('ironic.drivers.modules.deploy_utils.get_boot_option',
+                return_value='netboot', autospec=True)
     @mock.patch.object(image_service.GlanceImageService, 'show',
                        autospec=True)
-    def test_validate_fail_glance_conn_problem(self, mock_glance):
+    def test_validate_fail_glance_conn_problem(self, mock_glance,
+                                               mock_boot_option):
         exceptions = (exception.GlanceConnectionFailed('connection fail'),
                       exception.ImageNotAuthorized('not authorized'),
                       exception.Invalid('invalid'))
@@ -233,6 +251,7 @@ class iPXEBootTestCase(db_base.DbTestCase):
                                       shared=True) as task:
                 self.assertRaises(exception.InvalidParameterValue,
                                   task.driver.boot.validate, task)
+                mock_boot_option.assert_called_with(task.node)
 
     def test_validate_inspection(self):
         with task_manager.acquire(self.context, self.node.uuid) as task:

ironic/tests/unit/drivers/modules/test_pxe.py

@@ -20,7 +20,6 @@ import tempfile
 from unittest import mock
 
 from oslo_config import cfg
-from oslo_serialization import jsonutils as json
 from oslo_utils import timeutils
 from oslo_utils import uuidutils
 
@@ -140,13 +139,18 @@ class PXEBootTestCase(db_base.DbTestCase):
             self.assertRaises(exception.MissingParameterValue,
                               task.driver.boot.validate, task)
 
-    def test_validate_fail_missing_image_source(self):
+    def test_validate_no_image_source_for_local_boot(self):
-        info = dict(INST_INFO_DICT)
-        del info['image_source']
-        self.node.instance_info = json.dumps(info)
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=True) as task:
-            task.node['instance_info'] = json.dumps(info)
+            del task.node['instance_info']['image_source']
+            task.driver.boot.validate(task)
+
+    def test_validate_fail_missing_image_source(self):
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=True) as task:
+            task.node['instance_info']['capabilities'] = {
+                'boot_option': 'netboot'}
+            del task.node['instance_info']['image_source']
             self.assertRaises(exception.MissingParameterValue,
                               task.driver.boot.validate, task)
 
@@ -200,6 +204,8 @@ class PXEBootTestCase(db_base.DbTestCase):
         mock_glance.side_effect = exception.ImageNotFound('not found')
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=True) as task:
+            task.node.instance_info['capabilities'] = {
+                'boot_option': 'netboot'}
             self.assertRaises(exception.InvalidParameterValue,
                               task.driver.boot.validate, task)
 
@@ -212,6 +218,8 @@ class PXEBootTestCase(db_base.DbTestCase):
         for exc in exceptions:
             with task_manager.acquire(self.context, self.node.uuid,
                                       shared=True) as task:
+                task.node.instance_info['capabilities'] = {
+                    'boot_option': 'netboot'}
                 self.assertRaises(exception.InvalidParameterValue,
                                   task.driver.boot.validate, task)
 
@@ -973,10 +981,9 @@ class PXERamdiskDeployTestCase(db_base.DbTestCase):
     @mock.patch.object(deploy_utils, 'validate_image_properties',
                        autospec=True)
     def test_validate(self, mock_validate_img):
-        node = self.node
+        with task_manager.acquire(self.context, self.node.uuid) as task:
-        node.properties['capabilities'] = 'boot_option:netboot'
+            task.node.instance_info['capabilities'] = {
-        node.save()
+                'boot_option': 'netboot'}
-        with task_manager.acquire(self.context, node.uuid) as task:
             task.driver.deploy.validate(task)
         self.assertTrue(mock_validate_img.called)
 

releasenotes/notes/boot-validate-6b4b6b40c8e27273.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    When local boot is used (e.g. by default), the instance image validation
+    now happens only in the deploy interface, not in the boot interface (as
+    before). This means that the boot interface validation will now pass
+    in many cases where it would previously fail.