From bf304a62e315abaf2dc12414e2ad38ebfdf840b0 Mon Sep 17 00:00:00 2001 From: paresh-sao Date: Mon, 29 Jan 2018 10:05:22 +0000 Subject: [PATCH] Follow-up for Implementation for UEFI iSCSI boot for ILO This patch addresses outstanding comments for commit 87636372559fa6d83bb51eaaae075ac5e5575c17 Change-Id: I106f36cc3e9be2dee862e13d8e6621328e4f7959 --- ironic/drivers/modules/ilo/boot.py | 9 +- ironic/drivers/modules/ilo/management.py | 29 ++++--- .../unit/drivers/modules/ilo/test_boot.py | 87 +++++++++++-------- .../drivers/modules/ilo/test_management.py | 24 ++++- ...ot-from-iscsi-volume-41e8d510979c5037.yaml | 2 +- 5 files changed, 95 insertions(+), 56 deletions(-) diff --git a/ironic/drivers/modules/ilo/boot.py b/ironic/drivers/modules/ilo/boot.py index b879b4a0bd..9c571f2400 100644 --- a/ironic/drivers/modules/ilo/boot.py +++ b/ironic/drivers/modules/ilo/boot.py @@ -539,7 +539,6 @@ class IloVirtualMediaBoot(base.BootInterface): :returns: None :raises: IloOperationError, if some operation on iLO failed. """ - LOG.debug("Cleaning up the instance.") manager_utils.node_power_action(task, states.POWER_OFF) disable_secure_boot_if_supported(task) @@ -550,16 +549,13 @@ class IloVirtualMediaBoot(base.BootInterface): # It will clear iSCSI info from iLO task.driver.management.clear_iscsi_boot_target(task) driver_internal_info.pop('ilo_uefi_iscsi_boot', None) - task.node.driver_internal_info = driver_internal_info - task.node.save() else: _clean_up_boot_iso_for_instance(task.node) - driver_internal_info = task.node.driver_internal_info driver_internal_info.pop('boot_iso_created_in_web_server', None) driver_internal_info.pop('root_uuid_or_disk_id', None) - task.node.driver_internal_info = driver_internal_info - task.node.save() ilo_common.cleanup_vmedia_boot(task) + task.node.driver_internal_info = driver_internal_info + task.node.save() @METRICS.timer('IloVirtualMediaBoot.clean_up_ramdisk') def clean_up_ramdisk(self, task): @@ -683,7 +679,6 @@ class IloPXEBoot(pxe.PXEBoot): :returns: None :raises: IloOperationError, if some operation on iLO failed. """ - manager_utils.node_power_action(task, states.POWER_OFF) disable_secure_boot_if_supported(task) driver_internal_info = task.node.driver_internal_info diff --git a/ironic/drivers/modules/ilo/management.py b/ironic/drivers/modules/ilo/management.py index 5597df0773..d949333637 100644 --- a/ironic/drivers/modules/ilo/management.py +++ b/ironic/drivers/modules/ilo/management.py @@ -523,6 +523,7 @@ class IloManagement(base.ManagementInterface): The initiator is set with the target details like IQN, LUN, IP, Port etc. :param task: a task from TaskManager. + :raises: MissingParameterValue if a required parameter is missing. :raises: IloCommandNotSupportedInBiosError if system in BIOS boot mode. :raises: IloError on an error from iLO. """ @@ -532,20 +533,24 @@ class IloManagement(base.ManagementInterface): volume = volume_target.VolumeTarget.get_by_uuid(task.context, boot_volume) properties = volume.properties - username = properties.get('auth_username', None) - password = properties.get('auth_password', None) - portal = properties['target_portal'] - iqn = properties['target_iqn'] - lun = properties['target_lun'] - host, port = portal.split(':') - + username = properties.get('auth_username') + password = properties.get('auth_password') + try: + portal = properties['target_portal'] + iqn = properties['target_iqn'] + lun = properties['target_lun'] + host, port = portal.split(':') + except KeyError as e: + raise exception.MissingParameterValue( + _('Failed to get iSCSI target info for node ' + '%(node)s. Error: %(error)s') % {'node': task.node.uuid, + 'error': e}) ilo_object = ilo_common.get_ilo_object(task.node) try: - if username is None: - ilo_object.set_iscsi_info(iqn, lun, host, port) - else: - ilo_object.set_iscsi_info(iqn, lun, host, port, 'CHAP', - username, password) + auth_method = 'CHAP' if username else None + ilo_object.set_iscsi_info( + iqn, lun, host, port, auth_method=auth_method, + username=username, password=password) except ilo_error.IloCommandNotSupportedInBiosError as ilo_exception: operation = (_("Setting of target IQN %(target_iqn)s for node " "%(node)s") diff --git a/ironic/tests/unit/drivers/modules/ilo/test_boot.py b/ironic/tests/unit/drivers/modules/ilo/test_boot.py index 252016dc92..d04a4f06ec 100644 --- a/ironic/tests/unit/drivers/modules/ilo/test_boot.py +++ b/ironic/tests/unit/drivers/modules/ilo/test_boot.py @@ -702,14 +702,17 @@ class IloVirtualMediaBootTestCase(db_base.DbTestCase): autospec=True) @mock.patch.object(ilo_boot, '_validate_driver_info', spec_set=True, autospec=True) - def test_validate_boot_from_volume(self, mock_val_driver_info, - storage_mock): + @mock.patch.object(ilo_boot, '_validate_instance_image_info', + spec_set=True, autospec=True) + def test_validate_boot_from_volume(self, mock_val_instance_image_info, + mock_val_driver_info, storage_mock): with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: task.node.driver_info['ilo_deploy_iso'] = 'deploy-iso' storage_mock.return_value = False task.driver.boot.validate(task) mock_val_driver_info.assert_called_once_with(task) + self.assertFalse(mock_val_instance_image_info.called) @mock.patch.object(ilo_boot, 'prepare_node_for_deploy', spec_set=True, autospec=True) @@ -911,8 +914,8 @@ class IloVirtualMediaBootTestCase(db_base.DbTestCase): clear_iscsi_boot_target_mock.assert_called_once_with(mock.ANY, task) update_secure_boot_mode_mock.assert_called_once_with(task, False) - self.assertIsNone(self.node.driver_internal_info.get( - 'ilo_uefi_iscsi_boot')) + self.assertIsNone(task.node.driver_internal_info.get( + 'ilo_uefi_iscsi_boot')) @mock.patch.object(ilo_common, 'cleanup_vmedia_boot', spec_set=True, autospec=True) @@ -929,7 +932,7 @@ class IloVirtualMediaBootTestCase(db_base.DbTestCase): is_iscsi_boot_mock, cleanup_iso_mock, cleanup_vmedia_mock): with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: - is_iscsi_boot_mock.return_value = True + is_iscsi_boot_mock.return_value = False task.driver.boot.clean_up_instance(task) cleanup_iso_mock.assert_called_once_with(task.node) cleanup_vmedia_mock.assert_called_once_with(task) @@ -976,6 +979,8 @@ class IloVirtualMediaBootTestCase(db_base.DbTestCase): persistent=True) update_boot_mode_mock.assert_called_once_with(task) update_secure_boot_mode_mock.assert_called_once_with(task, True) + self.assertIsNone(task.node.driver_internal_info.get( + 'ilo_uefi_iscsi_boot')) def test_prepare_instance_whole_disk_image_local(self): self.node.instance_info = {'capabilities': '{"boot_option": "local"}'} @@ -1012,6 +1017,8 @@ class IloVirtualMediaBootTestCase(db_base.DbTestCase): mock.ANY, task, "12312642-09d3-467f-8e09-12385826a123") update_boot_mode_mock.assert_called_once_with(task) update_secure_boot_mode_mock.assert_called_once_with(task, True) + self.assertIsNone(task.node.driver_internal_info.get( + 'ilo_uefi_iscsi_boot')) @mock.patch.object(ilo_common, 'cleanup_vmedia_boot', spec_set=True, autospec=True) @@ -1034,6 +1041,10 @@ class IloVirtualMediaBootTestCase(db_base.DbTestCase): is_iscsi_boot_mock, cleanup_vmedia_boot_mock): with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: + driver_internal_info = task.node.driver_internal_info + driver_internal_info['ilo_uefi_iscsi_boot'] = True + task.node.driver_internal_info = driver_internal_info + task.node.save() is_iscsi_boot_mock.return_value = True get_boot_mode_mock.return_value = 'uefi' task.driver.boot.prepare_instance(task) @@ -1043,6 +1054,8 @@ class IloVirtualMediaBootTestCase(db_base.DbTestCase): task, boot_devices.ISCSIBOOT, persistent=True) update_boot_mode_mock.assert_called_once_with(task) update_secure_boot_mode_mock.assert_called_once_with(task, True) + self.assertTrue(task.node.driver_internal_info.get( + 'ilo_uefi_iscsi_boot')) @mock.patch.object(ilo_common, 'cleanup_vmedia_boot', spec_set=True, autospec=True) @@ -1062,6 +1075,8 @@ class IloVirtualMediaBootTestCase(db_base.DbTestCase): "in BIOS boot mode.", task.driver.boot.prepare_instance, task) cleanup_vmedia_boot_mock.assert_called_once_with(task) + self.assertIsNone(task.node.driver_internal_info.get( + 'ilo_uefi_iscsi_boot')) class IloPXEBootTestCase(db_base.DbTestCase): @@ -1144,6 +1159,33 @@ class IloPXEBootTestCase(db_base.DbTestCase): update_secure_boot_mode_mock.assert_called_once_with(task, False) pxe_cleanup_mock.assert_called_once_with(mock.ANY, task) + @mock.patch.object(deploy_utils, 'is_iscsi_boot', + spec_set=True, autospec=True) + @mock.patch.object(ilo_management.IloManagement, 'clear_iscsi_boot_target', + spec_set=True, autospec=True) + @mock.patch.object(ilo_common, 'update_secure_boot_mode', spec_set=True, + autospec=True) + @mock.patch.object(manager_utils, 'node_power_action', spec_set=True, + autospec=True) + def test_clean_up_instance_boot_from_volume(self, node_power_mock, + update_secure_boot_mode_mock, + clear_iscsi_boot_target_mock, + is_iscsi_boot_mock): + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + driver_internal_info = task.node.driver_internal_info + driver_internal_info['ilo_uefi_iscsi_boot'] = True + task.node.driver_internal_info = driver_internal_info + task.node.save() + is_iscsi_boot_mock.return_value = True + task.driver.boot.clean_up_instance(task) + clear_iscsi_boot_target_mock.assert_called_once_with(mock.ANY, + task) + node_power_mock.assert_called_once_with(task, states.POWER_OFF) + update_secure_boot_mode_mock.assert_called_once_with(task, False) + self.assertIsNone(task.node.driver_internal_info.get( + 'ilo_uefi_iscsi_boot')) + @mock.patch.object(deploy_utils, 'is_iscsi_boot', spec_set=True, autospec=True) @mock.patch.object(deploy_utils, 'get_boot_mode_for_deploy', @@ -1167,6 +1209,8 @@ class IloPXEBootTestCase(db_base.DbTestCase): update_boot_mode_mock.assert_called_once_with(task) update_secure_boot_mode_mock.assert_called_once_with(task, True) pxe_prepare_instance_mock.assert_called_once_with(mock.ANY, task) + self.assertIsNone(task.node.driver_internal_info.get( + 'ilo_uefi_iscsi_boot')) @mock.patch.object(deploy_utils, 'is_iscsi_boot', spec_set=True, autospec=True) @@ -1191,6 +1235,8 @@ class IloPXEBootTestCase(db_base.DbTestCase): update_boot_mode_mock.assert_called_once_with(task) update_secure_boot_mode_mock.assert_called_once_with(task, True) pxe_prepare_instance_mock.assert_called_once_with(mock.ANY, task) + self.assertIsNone(task.node.driver_internal_info.get( + 'ilo_uefi_iscsi_boot')) @mock.patch.object(deploy_utils, 'is_iscsi_boot', spec_set=True, autospec=True) @@ -1219,32 +1265,5 @@ class IloPXEBootTestCase(db_base.DbTestCase): task, boot_devices.ISCSIBOOT, persistent=True) update_boot_mode_mock.assert_called_once_with(task) update_secure_boot_mode_mock.assert_called_once_with(task, True) - self.assertIsNone(self.node.driver_internal_info.get( - 'ilo_uefi_iscsi_boot')) - - @mock.patch.object(deploy_utils, 'is_iscsi_boot', - spec_set=True, autospec=True) - @mock.patch.object(ilo_management.IloManagement, 'clear_iscsi_boot_target', - spec_set=True, autospec=True) - @mock.patch.object(ilo_common, 'update_secure_boot_mode', spec_set=True, - autospec=True) - @mock.patch.object(manager_utils, 'node_power_action', spec_set=True, - autospec=True) - def test_clean_up_instance_boot_from_volume(self, node_power_mock, - update_secure_boot_mode_mock, - clear_iscsi_boot_target_mock, - is_iscsi_boot_mock): - with task_manager.acquire(self.context, self.node.uuid, - shared=False) as task: - driver_internal_info = task.node.driver_internal_info - driver_internal_info['ilo_uefi_iscsi_boot'] = True - task.node.driver_internal_info = driver_internal_info - task.node.save() - is_iscsi_boot_mock.return_value = True - task.driver.boot.clean_up_instance(task) - clear_iscsi_boot_target_mock.assert_called_once_with(mock.ANY, - task) - node_power_mock.assert_called_once_with(task, states.POWER_OFF) - update_secure_boot_mode_mock.assert_called_once_with(task, False) - self.assertIsNone(self.node.driver_internal_info.get( - 'ilo_uefi_iscsi_boot')) + self.assertTrue(task.node.driver_internal_info.get( + 'ilo_uefi_iscsi_boot')) diff --git a/ironic/tests/unit/drivers/modules/ilo/test_management.py b/ironic/tests/unit/drivers/modules/ilo/test_management.py index 3b60a9e993..2ea24bd1ad 100644 --- a/ironic/tests/unit/drivers/modules/ilo/test_management.py +++ b/ironic/tests/unit/drivers/modules/ilo/test_management.py @@ -733,7 +733,8 @@ class IloManagementTestCase(db_base.DbTestCase): task.driver.management.set_iscsi_boot_target(task) ilo_object_mock.set_iscsi_info.assert_called_once_with( 'fake_iqn', 0, 'fake_host', '3260', - 'CHAP', 'fake_username', 'fake_password') + auth_method='CHAP', username='fake_username', + password='fake_password') @mock.patch.object(ilo_common, 'get_ilo_object', spec_set=True, autospec=True) @@ -754,7 +755,8 @@ class IloManagementTestCase(db_base.DbTestCase): ilo_object_mock = get_ilo_object_mock.return_value task.driver.management.set_iscsi_boot_target(task) ilo_object_mock.set_iscsi_info.assert_called_once_with( - 'fake_iqn', 0, 'fake_host', '3260') + 'fake_iqn', 0, 'fake_host', '3260', auth_method=None, + password=None, username=None) @mock.patch.object(ilo_common, 'get_ilo_object', spec_set=True, autospec=True) @@ -781,6 +783,24 @@ class IloManagementTestCase(db_base.DbTestCase): task.driver.management.set_iscsi_boot_target, task) + def test_set_iscsi_boot_target_missed_properties(self): + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + vol_id = uuidutils.generate_uuid() + obj_utils.create_test_volume_target( + self.context, node_id=self.node.id, volume_type='iscsi', + boot_index=0, volume_id='1234', uuid=vol_id, + properties={'target_iqn': 'fake_iqn', + 'auth_username': 'fake_username', + 'auth_password': 'fake_password'}) + driver_internal_info = task.node.driver_internal_info + driver_internal_info['boot_from_volume'] = vol_id + task.node.driver_internal_info = driver_internal_info + task.node.save() + self.assertRaises(exception.MissingParameterValue, + task.driver.management.set_iscsi_boot_target, + task) + @mock.patch.object(ilo_common, 'get_ilo_object', spec_set=True, autospec=True) def test_set_iscsi_boot_target_in_bios(self, get_ilo_object_mock): diff --git a/releasenotes/notes/ilo-boot-from-iscsi-volume-41e8d510979c5037.yaml b/releasenotes/notes/ilo-boot-from-iscsi-volume-41e8d510979c5037.yaml index b10647d13e..6489ee4085 100644 --- a/releasenotes/notes/ilo-boot-from-iscsi-volume-41e8d510979c5037.yaml +++ b/releasenotes/notes/ilo-boot-from-iscsi-volume-41e8d510979c5037.yaml @@ -1,7 +1,7 @@ --- features: - Enhanced boot interface 'ilo-pxe' and 'ilo-virtual-media' to support - firmware based booting from iSCSI volume. + firmware based booting from iSCSI volume in UEFI boot mode. upgrade: - The ``update_persistent_boot`` and ``[un]set_iscsi_info`` interfaces of 'proliantutils' library has been enhanced to support booting from