Using non-persistent boot in PXE interface

Non-persistent boot device change is not being used in places
where it should be during cleaning and deployment phases,
due to the default behavior of PXE interface forcing a
persistent change when using legacy function
deploy_utils.try_set_boot_device.
For some drivers, e.g. OneView, a persistent change is far more
costly than a non-persistent one, so this fix can bring
performance improvements.

Change-Id: I213e9c6173ee9c7c6c31064afcfae07764af0f7b
Closes-Bug: 1701721
Co-Authored-By: Stenio Araujo <steniaraujo@lsd.ufcg.edu.br>

ironic/drivers/modules/agent_base_vendor.py

@@ -129,6 +129,10 @@ def _cleaning_reboot(task):
     :param task: a TaskManager instance
     """
     try:
+        # NOTE(fellypefca): Call prepare_ramdisk on ensure that the
+        # baremetal node boots back into the ramdisk after reboot.
+        deploy_opts = deploy_utils.build_agent_options(task.node)
+        task.driver.boot.prepare_ramdisk(task, deploy_opts)
         manager_utils.node_power_action(task, states.REBOOT)
     except Exception as e:
         msg = (_('Reboot requested by clean step %(step)s failed for '

ironic/drivers/modules/pxe.py

@@ -32,6 +32,7 @@ from ironic.common import images
 from ironic.common import pxe_utils
 from ironic.common import states
 from ironic.common import utils
+from ironic.conductor import utils as manager_utils
 from ironic.conf import CONF
 from ironic.drivers import base
 from ironic.drivers.modules import deploy_utils
@@ -490,7 +491,8 @@ class PXEBoot(base.BootInterface):
 
         pxe_utils.create_pxe_config(task, pxe_options,
                                     pxe_config_template)
-        deploy_utils.try_set_boot_device(task, boot_devices.PXE)
+        manager_utils.node_set_boot_device(task, boot_devices.PXE,
+                                           persistent=False)
 
         if CONF.pxe.ipxe_enabled and CONF.pxe.ipxe_use_swift:
             pxe_info.pop('deploy_kernel', None)
@@ -602,7 +604,8 @@ class PXEBoot(base.BootInterface):
         # NOTE(pas-ha) do not re-set boot device on ACTIVE nodes
         # during takeover
         if boot_device and task.node.provision_state != states.ACTIVE:
-            deploy_utils.try_set_boot_device(task, boot_device)
+            manager_utils.node_set_boot_device(task, boot_device,
+                                               persistent=True)
 
     @METRICS.timer('PXEBoot.clean_up_instance')
     def clean_up_instance(self, task):

ironic/tests/unit/drivers/modules/test_agent_base_vendor.py

@@ -755,17 +755,26 @@ class AgentDeployMixinTest(AgentDeployMixinBaseTest):
             self.deploy.continue_cleaning(task)
             notify_mock.assert_called_once_with(task)
 
+    @mock.patch.object(deploy_utils, 'build_agent_options', autospec=True)
+    @mock.patch.object(pxe.PXEBoot, 'prepare_ramdisk', spec_set=True,
+                       autospec=True)
     @mock.patch.object(manager_utils, 'node_power_action', autospec=True)
-    def test__cleaning_reboot(self, mock_reboot):
+    def test__cleaning_reboot(self, mock_reboot, mock_prepare, mock_build_opt):
         with task_manager.acquire(self.context, self.node['uuid'],
                                   shared=False) as task:
             agent_base_vendor._cleaning_reboot(task)
+            self.assertTrue(mock_build_opt.called)
+            self.assertTrue(mock_prepare.called)
             mock_reboot.assert_called_once_with(task, states.REBOOT)
             self.assertTrue(task.node.driver_internal_info['cleaning_reboot'])
 
+    @mock.patch.object(deploy_utils, 'build_agent_options', autospec=True)
+    @mock.patch.object(pxe.PXEBoot, 'prepare_ramdisk', spec_set=True,
+                       autospec=True)
     @mock.patch.object(manager_utils, 'cleaning_error_handler', autospec=True)
     @mock.patch.object(manager_utils, 'node_power_action', autospec=True)
-    def test__cleaning_reboot_fail(self, mock_reboot, mock_handler):
+    def test__cleaning_reboot_fail(self, mock_reboot, mock_handler,
+                                   mock_prepare, mock_build_opt):
         mock_reboot.side_effect = RuntimeError("broken")
 
         with task_manager.acquire(self.context, self.node['uuid'],
@@ -776,10 +785,14 @@ class AgentDeployMixinTest(AgentDeployMixinBaseTest):
             self.assertNotIn('cleaning_reboot',
                              task.node.driver_internal_info)
 
+    @mock.patch.object(deploy_utils, 'build_agent_options', autospec=True)
+    @mock.patch.object(pxe.PXEBoot, 'prepare_ramdisk', spec_set=True,
+                       autospec=True)
     @mock.patch.object(manager_utils, 'node_power_action', autospec=True)
     @mock.patch.object(agent_client.AgentClient, 'get_commands_status',
                        autospec=True)
-    def test_continue_cleaning_reboot(self, status_mock, reboot_mock):
+    def test_continue_cleaning_reboot(
+            self, status_mock, reboot_mock, mock_prepare, mock_build_opt):
         # Test a successful execute clean step on the agent, with reboot
         self.node.clean_step = {
             'priority': 42,

ironic/tests/unit/drivers/modules/test_pxe.py

@@ -32,6 +32,7 @@ from ironic.common.glance_service import base_image_service
 from ironic.common import pxe_utils
 from ironic.common import states
 from ironic.conductor import task_manager
+from ironic.conductor import utils as manager_utils
 from ironic.drivers.modules import agent_base_vendor
 from ironic.drivers.modules import deploy_utils
 from ironic.drivers.modules import pxe
@@ -800,6 +801,7 @@ class PXEBootTestCase(db_base.DbTestCase):
                 self.assertRaises(exception.InvalidParameterValue,
                                   task.driver.boot.validate, task)
 
+    @mock.patch.object(manager_utils, 'node_set_boot_device', autospec=True)
     @mock.patch.object(dhcp_factory, 'DHCPFactory')
     @mock.patch.object(pxe, '_get_instance_image_info', autospec=True)
     @mock.patch.object(pxe, '_get_deploy_image_info', autospec=True)
@@ -810,7 +812,9 @@ class PXEBootTestCase(db_base.DbTestCase):
                               mock_build_pxe, mock_cache_r_k,
                               mock_deploy_img_info,
                               mock_instance_img_info,
-                              dhcp_factory_mock, uefi=False,
+                              dhcp_factory_mock,
+                              set_boot_device_mock,
+                              uefi=False,
                               cleaning=False,
                               ipxe_use_swift=False,
                               whole_disk_image=False):
@@ -833,6 +837,9 @@ class PXEBootTestCase(db_base.DbTestCase):
             task.driver.boot.prepare_ramdisk(task, {'foo': 'bar'})
             mock_deploy_img_info.assert_called_once_with(task.node)
             provider_mock.update_dhcp.assert_called_once_with(task, dhcp_opts)
+            set_boot_device_mock.assert_called_once_with(task,
+                                                         boot_devices.PXE,
+                                                         persistent=False)
             if ipxe_use_swift:
                 if whole_disk_image:
                     self.assertFalse(mock_cache_r_k.called)
@@ -984,7 +991,7 @@ class PXEBootTestCase(db_base.DbTestCase):
             clean_up_pxe_env_mock.assert_called_once_with(task, image_info)
             get_deploy_image_info_mock.assert_called_once_with(task.node)
 
-    @mock.patch.object(deploy_utils, 'try_set_boot_device', autospec=True)
+    @mock.patch.object(manager_utils, 'node_set_boot_device', autospec=True)
     @mock.patch.object(deploy_utils, 'switch_pxe_config', autospec=True)
     @mock.patch.object(dhcp_factory, 'DHCPFactory', autospec=True)
     @mock.patch.object(pxe, '_cache_ramdisk_kernel', autospec=True)
@@ -1018,11 +1025,12 @@ class PXEBootTestCase(db_base.DbTestCase):
                 pxe_config_path, "30212642-09d3-467f-8e09-21685826ab50",
                 'bios', False, False, False)
             set_boot_device_mock.assert_called_once_with(task,
-                                                         boot_devices.PXE)
+                                                         boot_devices.PXE,
+                                                         persistent=True)
 
     @mock.patch('os.path.isfile', return_value=False)
     @mock.patch.object(pxe_utils, 'create_pxe_config', autospec=True)
-    @mock.patch.object(deploy_utils, 'try_set_boot_device', autospec=True)
+    @mock.patch.object(manager_utils, 'node_set_boot_device', autospec=True)
     @mock.patch.object(deploy_utils, 'switch_pxe_config', autospec=True)
     @mock.patch.object(dhcp_factory, 'DHCPFactory', autospec=True)
     @mock.patch.object(pxe, '_cache_ramdisk_kernel', autospec=True)
@@ -1061,7 +1069,7 @@ class PXEBootTestCase(db_base.DbTestCase):
                 'bios', False, False, False)
             self.assertFalse(set_boot_device_mock.called)
 
-    @mock.patch.object(deploy_utils, 'try_set_boot_device', autospec=True)
+    @mock.patch.object(manager_utils, 'node_set_boot_device', autospec=True)
     @mock.patch.object(deploy_utils, 'switch_pxe_config', autospec=True)
     @mock.patch.object(dhcp_factory, 'DHCPFactory')
     @mock.patch.object(pxe, '_cache_ramdisk_kernel', autospec=True)
@@ -1095,7 +1103,7 @@ class PXEBootTestCase(db_base.DbTestCase):
     @mock.patch.object(deploy_utils, 'is_iscsi_boot', autospec=True)
     @mock.patch.object(noop_storage.NoopStorage, 'should_write_image',
                        autospec=True)
-    @mock.patch.object(deploy_utils, 'try_set_boot_device', autospec=True)
+    @mock.patch.object(manager_utils, 'node_set_boot_device', autospec=True)
     @mock.patch.object(deploy_utils, 'switch_pxe_config', autospec=True)
     @mock.patch.object(dhcp_factory, 'DHCPFactory', autospec=True)
     @mock.patch.object(pxe, '_cache_ramdisk_kernel', autospec=True)
@@ -1136,9 +1144,10 @@ class PXEBootTestCase(db_base.DbTestCase):
             switch_pxe_config_mock.assert_called_once_with(
                 pxe_config_path, None, None, False, iscsi_boot=True)
             set_boot_device_mock.assert_called_once_with(task,
-                                                         boot_devices.PXE)
+                                                         boot_devices.PXE,
+                                                         persistent=True)
 
-    @mock.patch.object(deploy_utils, 'try_set_boot_device', autospec=True)
+    @mock.patch.object(manager_utils, 'node_set_boot_device', autospec=True)
     @mock.patch.object(pxe_utils, 'clean_up_pxe_config', autospec=True)
     def test_prepare_instance_localboot(self, clean_up_pxe_config_mock,
                                         set_boot_device_mock):
@@ -1147,9 +1156,10 @@ class PXEBootTestCase(db_base.DbTestCase):
             task.driver.boot.prepare_instance(task)
             clean_up_pxe_config_mock.assert_called_once_with(task)
             set_boot_device_mock.assert_called_once_with(task,
-                                                         boot_devices.DISK)
+                                                         boot_devices.DISK,
+                                                         persistent=True)
 
-    @mock.patch.object(deploy_utils, 'try_set_boot_device', autospec=True)
+    @mock.patch.object(manager_utils, 'node_set_boot_device', autospec=True)
     @mock.patch.object(pxe_utils, 'clean_up_pxe_config', autospec=True)
     def test_prepare_instance_localboot_active(self, clean_up_pxe_config_mock,
                                                set_boot_device_mock):

releasenotes/notes/non-persistent-boot-5e3a0cd78e9dc91b.yaml

@@ -0,0 +1,7 @@
+fixes:
+  - |
+    Fixes a bug which caused boot device changes to be persistent in
+    places where they did not need to be during cleaning and deployment
+    phases, due to the default behavior of PXE interface forcing a persistent
+    change.
+    For more info, see https://bugs.launchpad.net/ironic/+bug/1701721