From c7091fb8e2898e526f8ce242f50096a2cabeb1fa Mon Sep 17 00:00:00 2001 From: Fellype Cavalcante Date: Mon, 3 Jul 2017 16:20:08 -0300 Subject: [PATCH] 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 --- ironic/drivers/modules/agent_base_vendor.py | 4 +++ ironic/drivers/modules/pxe.py | 7 +++-- .../drivers/modules/test_agent_base_vendor.py | 19 ++++++++++-- ironic/tests/unit/drivers/modules/test_pxe.py | 30 ++++++++++++------- .../non-persistent-boot-5e3a0cd78e9dc91b.yaml | 7 +++++ 5 files changed, 52 insertions(+), 15 deletions(-) create mode 100644 releasenotes/notes/non-persistent-boot-5e3a0cd78e9dc91b.yaml diff --git a/ironic/drivers/modules/agent_base_vendor.py b/ironic/drivers/modules/agent_base_vendor.py index b69f1f55cf..09f6e9a53d 100644 --- a/ironic/drivers/modules/agent_base_vendor.py +++ b/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 ' diff --git a/ironic/drivers/modules/pxe.py b/ironic/drivers/modules/pxe.py index 833e20ae7c..d6619d00af 100644 --- a/ironic/drivers/modules/pxe.py +++ b/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): diff --git a/ironic/tests/unit/drivers/modules/test_agent_base_vendor.py b/ironic/tests/unit/drivers/modules/test_agent_base_vendor.py index 61cde7bc46..3e1017da92 100644 --- a/ironic/tests/unit/drivers/modules/test_agent_base_vendor.py +++ b/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, diff --git a/ironic/tests/unit/drivers/modules/test_pxe.py b/ironic/tests/unit/drivers/modules/test_pxe.py index bb94b087cc..2a53368b59 100644 --- a/ironic/tests/unit/drivers/modules/test_pxe.py +++ b/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): diff --git a/releasenotes/notes/non-persistent-boot-5e3a0cd78e9dc91b.yaml b/releasenotes/notes/non-persistent-boot-5e3a0cd78e9dc91b.yaml new file mode 100644 index 0000000000..5cf07abc0c --- /dev/null +++ b/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