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

@@ -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

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)
+        username = properties.get('auth_username')
-        password = properties.get('auth_password', None)
+        password = properties.get('auth_password')
-        portal = properties['target_portal']
+        try:
-        iqn = properties['target_iqn']
+            portal = properties['target_portal']
-        lun = properties['target_lun']
+            iqn = properties['target_iqn']
-        host, port = portal.split(':')
+            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:
+            auth_method = 'CHAP' if username else None
-                ilo_object.set_iscsi_info(iqn, lun, host, port)
+            ilo_object.set_iscsi_info(
-            else:
+                iqn, lun, host, port, auth_method=auth_method,
-                ilo_object.set_iscsi_info(iqn, lun, host, port, 'CHAP',
+                username=username, password=password)
-                                          username, password)
         except ilo_error.IloCommandNotSupportedInBiosError as ilo_exception:
             operation = (_("Setting of target IQN %(target_iqn)s for node "
                            "%(node)s")

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,
+    @mock.patch.object(ilo_boot, '_validate_instance_image_info',
-                                       storage_mock):
+                       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(
+            self.assertIsNone(task.node.driver_internal_info.get(
-                          'ilo_uefi_iscsi_boot'))
+                              '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(
+            self.assertTrue(task.node.driver_internal_info.get(
-                          'ilo_uefi_iscsi_boot'))
+                            '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'))

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):

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