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.