openstack/ironic
Code Issues Proposed changes

Refactor deploy_utils.validate_image_properties

Browse Source 
This function has a confusing public interface and is always preceeded
by roughly the same logic, copy-pasted across boot interfaces. Move
this logic inside of the function and streamline its interface.

Change-Id: I4fc63be4e3cd4656d0ca7e893d4f3a98c07a8b4c
This commit is contained in:
Dmitry Tantsur 2021-06-24 12:57:56 +02:00
parent f67df12021
commit 1d6441cc34
10 changed files with 86 additions and 272 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
ironic/drivers/modules/deploy_utils.py
View File

@ -537,16 +537,15 @@ def get_image_properties(ctx, image_href):
raise exception.InvalidParameterValue(err=e)
def validate_image_properties(ctx, deploy_info, properties):
def validate_image_properties(task, deploy_info):
"""Validate the image.
For Glance images it checks that the image exists in Glance and its
properties or deployment info contain the properties passed. If it's not a
Glance image, it checks that deployment info contains needed properties.
:param ctx: security context
:param task: TaskManager instance with a valid node
:param deploy_info: the deploy_info to be validated
:param properties: the list of image meta-properties to be validated.
:raises: InvalidParameterValue if:
* connection to glance failed;
* authorization for accessing image failed;
@ -565,7 +564,18 @@ def validate_image_properties(ctx, deploy_info, properties):
if not image_href:
image_href = boot_iso
image_props = get_image_properties(ctx, image_href)
properties = []
if (not boot_iso
and not task.node.driver_internal_info.get('is_whole_disk_image')):
if service_utils.is_glance_image(image_href):
properties = ['kernel_id', 'ramdisk_id']
boot_option = get_boot_option(task.node)
if boot_option == 'kickstart':
properties.append('squashfs_id')
else:
properties = ['kernel', 'ramdisk']
image_props = get_image_properties(task.context, image_href)
missing_props = []
for prop in properties:

18
ironic/drivers/modules/ilo/boot.py
View File

@ -246,14 +246,7 @@ def _validate_instance_image_info(task):
node = task.node
d_info = _parse_deploy_info(node)
if node.driver_internal_info.get('is_whole_disk_image'):
props = []
elif service_utils.is_glance_image(d_info['image_source']):
props = ['kernel_id', 'ramdisk_id']
else:
props = ['kernel', 'ramdisk']
deploy_utils.validate_image_properties(task.context, d_info, props)
deploy_utils.validate_image_properties(task, d_info)
def _disable_secure_boot(task):
@ -920,14 +913,7 @@ class IloUefiHttpsBoot(base.BootInterface):
d_info = deploy_utils.get_image_instance_info(node)
self._validate_hrefs(d_info)
if node.driver_internal_info.get('is_whole_disk_image'):
props = []
elif service_utils.is_glance_image(d_info['image_source']):
props = ['kernel_id', 'ramdisk_id']
else:
props = ['kernel', 'ramdisk']
deploy_utils.validate_image_properties(task.context, d_info, props)
deploy_utils.validate_image_properties(task, d_info)
@METRICS.timer('IloUefiHttpsBoot.validate')
def validate(self, task):

9
ironic/drivers/modules/irmc/boot.py
View File

@ -957,14 +957,7 @@ class IRMCVirtualMediaBoot(base.BootInterface, IRMCVolumeBootMixIn):
return
d_info = _parse_deploy_info(task.node)
if task.node.driver_internal_info.get('is_whole_disk_image'):
props = []
elif service_utils.is_glance_image(d_info['image_source']):
props = ['kernel_id', 'ramdisk_id']
else:
props = ['kernel', 'ramdisk']
deploy_utils.validate_image_properties(task.context, d_info,
props)
deploy_utils.validate_image_properties(task, d_info)
@METRICS.timer('IRMCVirtualMediaBoot.prepare_ramdisk')
def prepare_ramdisk(self, task, ramdisk_params):

13
ironic/drivers/modules/pxe_base.py
View File

@ -21,7 +21,6 @@ from oslo_log import log as logging
from ironic.common import boot_devices
from ironic.common import dhcp_factory
from ironic.common import exception
from ironic.common.glance_service import service_utils
from ironic.common.i18n import _
from ironic.common import pxe_utils
from ironic.common import states
@ -423,17 +422,7 @@ class PXEBaseMixin(object):
return
d_info = deploy_utils.get_image_instance_info(node)
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 boot_option == 'kickstart':
props.append('squashfs_id')
else:
props = ['kernel', 'ramdisk']
deploy_utils.validate_image_properties(task.context, d_info, props)
deploy_utils.validate_image_properties(task, d_info)
@METRICS.timer('PXEBaseMixin.validate_rescue')
def validate_rescue(self, task):

14
ironic/drivers/modules/redfish/boot.py
View File

@ -19,7 +19,6 @@ import tenacity
from ironic.common import boot_devices
from ironic.common import exception
from ironic.common.glance_service import service_utils
from ironic.common.i18n import _
from ironic.common import states
from ironic.conductor import utils as manager_utils
@ -394,18 +393,7 @@ class RedfishVirtualMediaBoot(base.BootInterface):
return
d_info = _parse_deploy_info(node)
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']
else:
props = ['kernel', 'ramdisk']
deploy_utils.validate_image_properties(task.context, d_info, props)
deploy_utils.validate_image_properties(task, d_info)
def _validate_vendor(self, task):
vendor = task.node.properties.get('vendor')

115
ironic/tests/unit/drivers/modules/ilo/test_boot.py
View File

@ -261,38 +261,15 @@ class IloBootPrivateMethodsTestCase(test_common.BaseIloTest):
spec_set=True, autospec=True)
@mock.patch.object(ilo_boot, '_parse_deploy_info', spec_set=True,
autospec=True)
def _test__validate_instance_image_info(self,
deploy_info_mock,
validate_prop_mock,
props_expected):
def test__validate_instance_image_info(self, deploy_info_mock,
validate_prop_mock):
d_info = {'image_source': 'uuid'}
deploy_info_mock.return_value = d_info
with task_manager.acquire(self.context, self.node.uuid,
shared=False) as task:
ilo_boot._validate_instance_image_info(task)
deploy_info_mock.assert_called_once_with(task.node)
validate_prop_mock.assert_called_once_with(
task.context, d_info, props_expected)
@mock.patch.object(service_utils, 'is_glance_image', spec_set=True,
autospec=True)
def test__validate_glance_partition_image(self,
is_glance_image_mock):
is_glance_image_mock.return_value = True
self._test__validate_instance_image_info(props_expected=['kernel_id',
'ramdisk_id'])
def test__validate_whole_disk_image(self):
self.node.driver_internal_info = {'is_whole_disk_image': True}
self.node.save()
self._test__validate_instance_image_info(props_expected=[])
@mock.patch.object(service_utils, 'is_glance_image', spec_set=True,
autospec=True)
def test__validate_non_glance_partition_image(self, is_glance_image_mock):
is_glance_image_mock.return_value = False
self._test__validate_instance_image_info(props_expected=['kernel',
'ramdisk'])
validate_prop_mock.assert_called_once_with(task, d_info)
@mock.patch.object(ilo_common, 'set_secure_boot_mode', spec_set=True,
autospec=True)
@ -1604,100 +1581,20 @@ class IloUefiHttpsBootTestCase(db_base.DbTestCase):
spec_set=True, autospec=True)
@mock.patch.object(deploy_utils, 'get_image_instance_info',
spec_set=True, autospec=True)
@mock.patch.object(service_utils, 'is_glance_image', spec_set=True,
autospec=True)
def test__validate_instance_image_info_not_iwdi(
self, glance_mock, get_image_inst_mock, validate_image_mock,
def test__validate_instance_image_info(
self, get_image_inst_mock, validate_image_mock,
validate_href_mock):
instance_info = {
'boot_iso': 'boot-iso',
'image_source': '6b2f0c0c-79e8-4db6-842e-43c9764204af'
}
driver_internal_info = self.node.driver_internal_info
driver_internal_info.pop('is_whole_disk_image', None)
self.node.driver_internal_info = driver_internal_info
self.node.instance_info = instance_info
self.node.save()
get_image_inst_mock.return_value = instance_info
glance_mock.return_value = True
with task_manager.acquire(self.context, self.node.uuid,
shared=False) as task:
task.driver.boot._validate_instance_image_info(task)
get_image_inst_mock.assert_called_once_with(task.node)
glance_mock.assert_called_once_with(
'6b2f0c0c-79e8-4db6-842e-43c9764204af')
validate_image_mock.assert_called_once_with(task.context,
instance_info,
['kernel_id',
'ramdisk_id'])
validate_href_mock.assert_called_once_with(mock.ANY, instance_info)
@mock.patch.object(ilo_boot.IloUefiHttpsBoot, '_validate_hrefs',
autospec=True)
@mock.patch.object(deploy_utils, 'validate_image_properties',
spec_set=True, autospec=True)
@mock.patch.object(deploy_utils, 'get_image_instance_info',
spec_set=True, autospec=True)
@mock.patch.object(service_utils, 'is_glance_image', spec_set=True,
autospec=True)
def test__validate_instance_image_info_neither_iwdi_nor_glance(
self, glance_mock, get_image_inst_mock, validate_image_mock,
validate_href_mock):
instance_info = {
'boot_iso': 'boot-iso',
'image_source': '6b2f0c0c-79e8-4db6-842e-43c9764204af'
}
driver_internal_info = self.node.driver_internal_info
driver_internal_info.pop('is_whole_disk_image', None)
self.node.driver_internal_info = driver_internal_info
self.node.instance_info = instance_info
self.node.save()
get_image_inst_mock.return_value = instance_info
glance_mock.return_value = False
with task_manager.acquire(self.context, self.node.uuid,
shared=False) as task:
task.driver.boot._validate_instance_image_info(task)
get_image_inst_mock.assert_called_once_with(task.node)
glance_mock.assert_called_once_with(
'6b2f0c0c-79e8-4db6-842e-43c9764204af')
validate_image_mock.assert_called_once_with(task.context,
instance_info,
['kernel',
'ramdisk'])
validate_href_mock.assert_called_once_with(mock.ANY, instance_info)
@mock.patch.object(ilo_boot.IloUefiHttpsBoot, '_validate_hrefs',
autospec=True)
@mock.patch.object(deploy_utils, 'validate_image_properties',
spec_set=True, autospec=True)
@mock.patch.object(deploy_utils, 'get_image_instance_info',
spec_set=True, autospec=True)
@mock.patch.object(service_utils, 'is_glance_image', spec_set=True,
autospec=True)
def test__validate_instance_image_info_iwdi(
self, glance_mock, get_image_inst_mock, validate_image_mock,
validate_href_mock):
instance_info = {
'boot_iso': 'boot-iso',
'image_source': '6b2f0c0c-79e8-4db6-842e-43c9764204af'
}
driver_internal_info = self.node.driver_internal_info or {}
driver_internal_info['is_whole_disk_image'] = True
self.node.driver_internal_info = driver_internal_info
self.node.save()
get_image_inst_mock.return_value = instance_info
glance_mock.return_value = True
with task_manager.acquire(self.context, self.node.uuid,
shared=False) as task:
task.driver.boot._validate_instance_image_info(task)
get_image_inst_mock.assert_called_once_with(task.node)
glance_mock.assert_not_called()
validate_image_mock.assert_called_once_with(task.context,
instance_info, [])
validate_image_mock.assert_called_once_with(task, instance_info)
validate_href_mock.assert_called_once_with(mock.ANY, instance_info)
@mock.patch.object(ilo_common, 'get_current_boot_mode',

59
ironic/tests/unit/drivers/modules/irmc/test_boot.py
View File

@ -1002,69 +1002,18 @@ class IRMCVirtualMediaBootTestCase(test_common.BaseIRMCTest):
autospec=True)
@mock.patch.object(irmc_boot, '_parse_deploy_info', spec_set=True,
autospec=True)
def test_validate_whole_disk_image(self,
deploy_info_mock,
is_glance_image_mock,
validate_prop_mock,
check_share_fs_mounted_mock):
def test_validate(self, deploy_info_mock, is_glance_image_mock,
validate_prop_mock, check_share_fs_mounted_mock):
d_info = {'image_source': '733d1c44-a2ea-414b-aca7-69decf20d810'}
deploy_info_mock.return_value = d_info
with task_manager.acquire(self.context, self.node.uuid,
shared=False) as task:
task.node.driver_internal_info = {'is_whole_disk_image': True}
task.driver.boot.validate(task)
self.assertEqual(check_share_fs_mounted_mock.call_count, 2)
deploy_info_mock.assert_called_once_with(task.node)
self.assertFalse(is_glance_image_mock.called)
validate_prop_mock.assert_called_once_with(task.context,
d_info, [])
@mock.patch.object(deploy_utils, 'validate_image_properties',
spec_set=True, autospec=True)
@mock.patch.object(service_utils, 'is_glance_image', spec_set=True,
autospec=True)
@mock.patch.object(irmc_boot, '_parse_deploy_info', spec_set=True,
autospec=True)
def test_validate_glance_image(self,
deploy_info_mock,
is_glance_image_mock,
validate_prop_mock,
check_share_fs_mounted_mock):
d_info = {'image_source': '733d1c44-a2ea-414b-aca7-69decf20d810'}
deploy_info_mock.return_value = d_info
is_glance_image_mock.return_value = True
with task_manager.acquire(self.context, self.node.uuid,
shared=False) as task:
task.driver.boot.validate(task)
self.assertEqual(check_share_fs_mounted_mock.call_count, 2)
deploy_info_mock.assert_called_once_with(task.node)
validate_prop_mock.assert_called_once_with(
task.context, d_info, ['kernel_id', 'ramdisk_id'])
@mock.patch.object(deploy_utils, 'validate_image_properties',
spec_set=True, autospec=True)
@mock.patch.object(service_utils, 'is_glance_image', spec_set=True,
autospec=True)
@mock.patch.object(irmc_boot, '_parse_deploy_info', spec_set=True,
autospec=True)
def test_validate_non_glance_image(self,
deploy_info_mock,
is_glance_image_mock,
validate_prop_mock,
check_share_fs_mounted_mock):
d_info = {'image_source': '733d1c44-a2ea-414b-aca7-69decf20d810'}
deploy_info_mock.return_value = d_info
is_glance_image_mock.return_value = False
with task_manager.acquire(self.context, self.node.uuid,
shared=False) as task:
task.driver.boot.validate(task)
self.assertEqual(check_share_fs_mounted_mock.call_count, 2)
deploy_info_mock.assert_called_once_with(task.node)
validate_prop_mock.assert_called_once_with(
task.context, d_info, ['kernel', 'ramdisk'])
self.assertFalse(is_glance_image_mock.called)
validate_prop_mock.assert_called_once_with(task, d_info)
@mock.patch.object(irmc_management, 'backup_bios_config', spec_set=True,
autospec=True)

4
ironic/tests/unit/drivers/modules/redfish/test_boot.py
View File

@ -266,7 +266,7 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
task.driver.boot.validate(task)
mock_validate_image_properties.assert_called_once_with(
task.context, mock.ANY, ['kernel', 'ramdisk'])
task, mock.ANY)
@mock.patch.object(deploy_utils, 'get_boot_option', lambda node: 'ramdisk')
@mock.patch.object(redfish_utils, 'parse_driver_info', autospec=True)
@ -289,7 +289,7 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
task.driver.boot.validate(task)
mock_validate_image_properties.assert_called_once_with(
task.context, mock.ANY, ['boot_iso'])
task, mock.ANY)
@mock.patch.object(redfish_utils, 'parse_driver_info', autospec=True)
@mock.patch.object(deploy_utils, 'validate_image_properties',

87
ironic/tests/unit/drivers/modules/test_deploy_utils.py
View File

@ -1302,44 +1302,59 @@ class AgentMethodsTestCase(db_base.DbTestCase):
class ValidateImagePropertiesTestCase(db_base.DbTestCase):
@mock.patch.object(image_service, 'get_image_service', autospec=True)
def test_validate_image_properties_glance_image(self, image_service_mock):
node = obj_utils.create_test_node(
def setUp(self):
super().setUp()
self.node = obj_utils.create_test_node(
self.context, boot_interface='pxe',
instance_info=INST_INFO_DICT,
driver_info=DRV_INFO_DICT,
driver_internal_info=DRV_INTERNAL_INFO_DICT,
)
inst_info = utils.get_image_instance_info(node)
self.task = mock.Mock(context=self.context, node=self.node,
spec=['context', 'node'])
@mock.patch.object(image_service, 'get_image_service', autospec=True)
def test_validate_image_properties_glance_image(self, image_service_mock):
inst_info = utils.get_image_instance_info(self.node)
image_service_mock.return_value.show.return_value = {
'properties': {'kernel_id': '1111', 'ramdisk_id': '2222'},
}
utils.validate_image_properties(self.context, inst_info,
['kernel_id', 'ramdisk_id'])
utils.validate_image_properties(self.task, inst_info)
image_service_mock.assert_called_once_with(
node.instance_info['image_source'], context=self.context
self.node.instance_info['image_source'], context=self.context
)
@mock.patch.object(image_service, 'get_image_service', autospec=True)
def test_validate_image_properties_glance_image_missing_prop(
self, image_service_mock):
node = obj_utils.create_test_node(
self.context, boot_interface='pxe',
instance_info=INST_INFO_DICT,
driver_info=DRV_INFO_DICT,
driver_internal_info=DRV_INTERNAL_INFO_DICT,
)
inst_info = utils.get_image_instance_info(node)
inst_info = utils.get_image_instance_info(self.node)
image_service_mock.return_value.show.return_value = {
'properties': {'kernel_id': '1111'},
}
self.assertRaises(exception.MissingParameterValue,
utils.validate_image_properties,
self.context, inst_info, ['kernel_id', 'ramdisk_id'])
self.task, inst_info)
image_service_mock.assert_called_once_with(
node.instance_info['image_source'], context=self.context
self.node.instance_info['image_source'], context=self.context
)
@mock.patch.object(utils, 'get_boot_option', autospec=True,
return_value='kickstart')
@mock.patch.object(image_service, 'get_image_service', autospec=True)
def test_validate_image_properties_glance_image_missing_squashfs_id(
self, image_service_mock, boot_options_mock):
inst_info = utils.get_image_instance_info(self.node)
image_service_mock.return_value.show.return_value = {
'properties': {'kernel_id': '1111', 'ramdisk_id': '2222'},
}
self.assertRaises(exception.MissingParameterValue,
utils.validate_image_properties,
self.task, inst_info)
image_service_mock.assert_called_once_with(
self.node.instance_info['image_source'], context=self.context
)
@mock.patch.object(image_service, 'get_image_service', autospec=True)
@ -1349,8 +1364,8 @@ class ValidateImagePropertiesTestCase(db_base.DbTestCase):
show_mock = image_service_mock.return_value.show
show_mock.side_effect = exception.ImageNotAuthorized(image_id='uuid')
self.assertRaises(exception.InvalidParameterValue,
utils.validate_image_properties, self.context,
inst_info, [])
utils.validate_image_properties, self.task,
inst_info)
@mock.patch.object(image_service, 'get_image_service', autospec=True)
def test_validate_image_properties_glance_image_not_found(
@ -1359,14 +1374,14 @@ class ValidateImagePropertiesTestCase(db_base.DbTestCase):
show_mock = image_service_mock.return_value.show
show_mock.side_effect = exception.ImageNotFound(image_id='uuid')
self.assertRaises(exception.InvalidParameterValue,
utils.validate_image_properties, self.context,
inst_info, [])
utils.validate_image_properties, self.task,
inst_info)
def test_validate_image_properties_invalid_image_href(self):
inst_info = {'image_source': 'emule://uuid'}
self.assertRaises(exception.InvalidParameterValue,
utils.validate_image_properties, self.context,
inst_info, [])
utils.validate_image_properties, self.task,
inst_info)
@mock.patch.object(image_service.HttpImageService, 'show', autospec=True)
def test_validate_image_properties_nonglance_image(
@ -1378,15 +1393,9 @@ class ValidateImagePropertiesTestCase(db_base.DbTestCase):
'root_gb': 100,
}
image_service_show_mock.return_value = {'size': 1, 'properties': {}}
node = obj_utils.create_test_node(
self.context, boot_interface='pxe',
instance_info=instance_info,
driver_info=DRV_INFO_DICT,
driver_internal_info=DRV_INTERNAL_INFO_DICT,
)
inst_info = utils.get_image_instance_info(node)
utils.validate_image_properties(self.context, inst_info,
['kernel', 'ramdisk'])
self.node.instance_info = instance_info
inst_info = utils.get_image_instance_info(self.node)
utils.validate_image_properties(self.task, inst_info)
image_service_show_mock.assert_called_once_with(
mock.ANY, instance_info['image_source'])
@ -1401,19 +1410,13 @@ class ValidateImagePropertiesTestCase(db_base.DbTestCase):
}
img_service_show_mock.side_effect = exception.ImageRefValidationFailed(
image_href='http://ubuntu', reason='HTTPError')
node = obj_utils.create_test_node(
self.context, boot_interface='pxe',
instance_info=instance_info,
driver_info=DRV_INFO_DICT,
driver_internal_info=DRV_INTERNAL_INFO_DICT,
)
inst_info = utils.get_image_instance_info(node)
self.node.instance_info = instance_info
inst_info = utils.get_image_instance_info(self.node)
expected_error = ('Validation of image href http://ubuntu '
'failed, reason: HTTPError')
error = self.assertRaises(exception.InvalidParameterValue,
utils.validate_image_properties,
self.context,
inst_info, ['kernel', 'ramdisk'])
self.task, inst_info)
self.assertEqual(expected_error, str(error))
def test_validate_image_properties_boot_iso_conflict(self):
@ -1426,8 +1429,8 @@ class ValidateImagePropertiesTestCase(db_base.DbTestCase):
"the same time.")
error = self.assertRaises(exception.InvalidParameterValue,
utils.validate_image_properties,
self.context,
instance_info, [])
self.task,
instance_info)
self.assertEqual(expected_error, str(error))

21
ironic/tests/unit/drivers/modules/test_pxe.py
View File

@ -236,18 +236,17 @@ class PXEBootTestCase(db_base.DbTestCase):
self.assertRaises(exception.UnsupportedDriverExtension,
task.driver.boot.validate_inspection, task)
@mock.patch.object(deploy_utils, 'validate_image_properties',
autospec=True)
def test_validate_kickstart_has_squashfs_id(self, mock_validate_img):
node = self.node
node.deploy_interface = 'anaconda'
node.save()
@mock.patch.object(image_service.GlanceImageService, 'show', autospec=True)
def test_validate_kickstart_has_squashfs_id(self, mock_glance):
mock_glance.return_value = {'properties': {'kernel_id': 'fake-kernel',
'ramdisk_id': 'fake-initr'}}
self.node.deploy_interface = 'anaconda'
self.node.save()
self.config(http_url='http://fake_url', group='deploy')
with task_manager.acquire(self.context, node.uuid) as task:
task.driver.boot.validate(task)
mock_validate_img.assert_called_once_with(
mock.ANY, mock.ANY, ['kernel_id', 'ramdisk_id', 'squashfs_id']
)
with task_manager.acquire(self.context, self.node.uuid) as task:
self.assertRaisesRegex(exception.MissingParameterValue,
'squashfs_id',
task.driver.boot.validate, task)
def test_validate_kickstart_fail_http_url_not_set(self):
node = self.node