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
This commit is contained in:
parent
83b412cbdb
commit
e6bb99cd8f
@ -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']
|
||||
|
@ -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.
|
||||
|
@ -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',
|
||||
|
@ -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:
|
||||
|
@ -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)
|
||||
|
||||
|
7
releasenotes/notes/boot-validate-6b4b6b40c8e27273.yaml
Normal file
7
releasenotes/notes/boot-validate-6b4b6b40c8e27273.yaml
Normal file
@ -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.
|
Loading…
Reference in New Issue
Block a user