From 0500fe1072be3e5061da59781ff3a19a9caccb36 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Thu, 15 Apr 2021 16:07:20 +0200 Subject: [PATCH] Avoid unnecessary validation in boot interfaces Interfaces should only validate values they're going to use. Boot interfaces do not care about image properties when local boot is used (which is the default), so they shouldn't validate them. The deploy interface has to provide validation for images. This change fixes PXE, iPXE and redfish-virtual-media, although other boot interfaces may need a similar change. We also need to refactor handling instance_info in deploy_utils, but that can wait until the iSCSI deploy removal. Also refactor unit tests for redfish-virtual-media. Story: #2008874 Task: #42418 Change-Id: Ida21f21d6435c0d7fa46cb5b1161f034ad8956ee (cherry picked from commit e6bb99cd8ff94a24c3f2108e649c7ca39189d220) --- ironic/drivers/modules/pxe_base.py | 14 ++- ironic/drivers/modules/redfish/boot.py | 11 ++- .../unit/drivers/modules/redfish/test_boot.py | 98 +++++-------------- .../tests/unit/drivers/modules/test_ipxe.py | 37 +++++-- ironic/tests/unit/drivers/modules/test_pxe.py | 27 +++-- .../notes/boot-validate-6b4b6b40c8e27273.yaml | 7 ++ 6 files changed, 91 insertions(+), 103 deletions(-) create mode 100644 releasenotes/notes/boot-validate-6b4b6b40c8e27273.yaml diff --git a/ironic/drivers/modules/pxe_base.py b/ironic/drivers/modules/pxe_base.py index eb30f4846e..8b8cc4f13c 100644 --- a/ironic/drivers/modules/pxe_base.py +++ b/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') - or deploy_utils.get_boot_option(node) == 'local'): + if node.driver_internal_info.get('is_whole_disk_image'): 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'] diff --git a/ironic/drivers/modules/redfish/boot.py b/ironic/drivers/modules/redfish/boot.py index 3fd3f1cd22..b854fc994d 100644 --- a/ironic/drivers/modules/redfish/boot.py +++ b/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) - - if task.driver.storage.should_write_image(task): - self._validate_instance_info(task) + self._validate_instance_info(task) def validate_inspection(self, task): """Validate that the node has required properties for inspection. diff --git a/ironic/tests/unit/drivers/modules/redfish/test_boot.py b/ironic/tests/unit/drivers/modules/redfish/test_boot.py index fd3304c84e..d4471cda51 100644 --- a/ironic/tests/unit/drivers/modules/redfish/test_boot.py +++ b/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', - autospec=True) - def test_validate_uefi_boot(self, mock_get_boot_mode, - mock_validate_image_properties, - mock_parse_driver_info): + def test_validate_kernel_ramdisk(self, 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', - autospec=True) - 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): + def test_validate_boot_iso(self, 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) - - @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) + task.context, mock.ANY, ['boot_iso']) @mock.patch.object(redfish_utils, 'parse_driver_info', autospec=True) @mock.patch.object(deploy_utils, 'validate_image_properties', diff --git a/ironic/tests/unit/drivers/modules/test_ipxe.py b/ironic/tests/unit/drivers/modules/test_ipxe.py index e576922687..b9f94a92b3 100644 --- a/ironic/tests/unit/drivers/modules/test_ipxe.py +++ b/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): - info = dict(INST_INFO_DICT) - del info['image_source'] - self.node.instance_info = json.dumps(info) + @mock.patch('ironic.drivers.modules.deploy_utils.get_boot_option', + return_value='local', autospec=True) + def test_validate_no_image_source_for_local_boot(self, mock_boot_option): 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: diff --git a/ironic/tests/unit/drivers/modules/test_pxe.py b/ironic/tests/unit/drivers/modules/test_pxe.py index 4b06a495a3..874073b6cd 100644 --- a/ironic/tests/unit/drivers/modules/test_pxe.py +++ b/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 @@ -141,13 +140,18 @@ class PXEBootTestCase(db_base.DbTestCase): self.assertRaises(exception.MissingParameterValue, task.driver.boot.validate, task) - def test_validate_fail_missing_image_source(self): - info = dict(INST_INFO_DICT) - del info['image_source'] - self.node.instance_info = json.dumps(info) + def test_validate_no_image_source_for_local_boot(self): 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) @@ -201,6 +205,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) @@ -213,6 +219,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) @@ -974,10 +982,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 - node.properties['capabilities'] = 'boot_option:netboot' - node.save() - with task_manager.acquire(self.context, node.uuid) as task: + with task_manager.acquire(self.context, self.node.uuid) as task: + task.node.instance_info['capabilities'] = { + 'boot_option': 'netboot'} task.driver.deploy.validate(task) self.assertTrue(mock_validate_img.called) diff --git a/releasenotes/notes/boot-validate-6b4b6b40c8e27273.yaml b/releasenotes/notes/boot-validate-6b4b6b40c8e27273.yaml new file mode 100644 index 0000000000..282ac630d9 --- /dev/null +++ b/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.