From 206134cf58266859fc7b21cd76831955cad72b56 Mon Sep 17 00:00:00 2001 From: Arne Wiebalck Date: Wed, 23 Jan 2019 17:19:55 +0100 Subject: [PATCH] Optionally preserve original system boot order upon instance deployment By default, Ironic makes persistent changes to the system boot order at the end of an instance deployment. This may not be desired in all cases, e.g. when DC policies require to leave the persistent system boot order unchanged. While keeping the persistent approach as the default, this patch proposes to extent the existing 'force_persistent_boot_device' field in the node's driver_info for (i)PXE in a way that allows to have all boot device changes to be non-persistent. Change-Id: If3a19f74fb0dfbcff2cde4cd5a8cc1edf5de3058 Story: #2004846 Task: #29058 --- ironic/drivers/modules/agent_base_vendor.py | 7 +- ironic/drivers/modules/ipxe.py | 17 +++-- ironic/drivers/modules/pxe.py | 18 ++++-- ironic/drivers/modules/pxe_base.py | 14 ++-- .../drivers/modules/test_agent_base_vendor.py | 57 +++++++++++++++-- .../tests/unit/drivers/modules/test_ipxe.py | 64 ++++++++++++++++++- ironic/tests/unit/drivers/modules/test_pxe.py | 64 ++++++++++++++++++- ...rsistent-boot-device-139cf280fb66f4f7.yaml | 17 +++++ 8 files changed, 233 insertions(+), 25 deletions(-) create mode 100644 releasenotes/notes/add-option-persistent-boot-device-139cf280fb66f4f7.yaml diff --git a/ironic/drivers/modules/agent_base_vendor.py b/ironic/drivers/modules/agent_base_vendor.py index 48163f9ea9..0f457f9d41 100644 --- a/ironic/drivers/modules/agent_base_vendor.py +++ b/ironic/drivers/modules/agent_base_vendor.py @@ -735,7 +735,12 @@ class AgentDeployMixin(HeartbeatMixin): log_and_raise_deployment_error(task, msg) try: - deploy_utils.try_set_boot_device(task, boot_devices.DISK) + persistent = True + if node.driver_info.get('force_persistent_boot_device', + 'Default') == 'Never': + persistent = False + deploy_utils.try_set_boot_device(task, boot_devices.DISK, + persistent=persistent) except Exception as e: msg = (_("Failed to change the boot device to %(boot_dev)s " "when deploying node %(node)s. Error: %(error)s") % diff --git a/ironic/drivers/modules/ipxe.py b/ironic/drivers/modules/ipxe.py index 9dca81d732..0a334779a2 100644 --- a/ironic/drivers/modules/ipxe.py +++ b/ironic/drivers/modules/ipxe.py @@ -166,9 +166,14 @@ class iPXEBoot(pxe_base.PXEBaseMixin, base.BootInterface): pxe_utils.create_pxe_config(task, pxe_options, pxe_config_template, ipxe_enabled=True) - persistent = strutils.bool_from_string( - node.driver_info.get('force_persistent_boot_device', - False)) + persistent = False + value = node.driver_info.get('force_persistent_boot_device', + 'Default') + if value in {'Always', 'Default', 'Never'}: + if value == 'Always': + persistent = True + else: + persistent = strutils.bool_from_string(value, False) manager_utils.node_set_boot_device(task, boot_devices.PXE, persistent=persistent) @@ -266,8 +271,12 @@ class iPXEBoot(pxe_base.PXEBaseMixin, 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: + persistent = True + if node.driver_info.get('force_persistent_boot_device', + 'Default') == 'Never': + persistent = False manager_utils.node_set_boot_device(task, boot_device, - persistent=True) + persistent=persistent) @METRICS.timer('iPXEBoot.clean_up_instance') def clean_up_instance(self, task): diff --git a/ironic/drivers/modules/pxe.py b/ironic/drivers/modules/pxe.py index 4ee5c35b2d..d4b0bce8ee 100644 --- a/ironic/drivers/modules/pxe.py +++ b/ironic/drivers/modules/pxe.py @@ -172,9 +172,15 @@ class PXEBoot(pxe_base.PXEBaseMixin, base.BootInterface): pxe_utils.create_pxe_config(task, pxe_options, pxe_config_template, ipxe_enabled=CONF.pxe.ipxe_enabled) - persistent = strutils.bool_from_string( - node.driver_info.get('force_persistent_boot_device', - False)) + + persistent = False + value = node.driver_info.get('force_persistent_boot_device', + 'Default') + if value in {'Always', 'Default', 'Never'}: + if value == 'Always': + persistent = True + else: + persistent = strutils.bool_from_string(value, False) manager_utils.node_set_boot_device(task, boot_devices.PXE, persistent=persistent) @@ -274,8 +280,12 @@ class PXEBoot(pxe_base.PXEBaseMixin, 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: + persistent = True + if node.driver_info.get('force_persistent_boot_device', + 'Default') == 'Never': + persistent = False manager_utils.node_set_boot_device(task, boot_device, - persistent=True) + persistent=persistent) @METRICS.timer('PXEBoot.clean_up_instance') def clean_up_instance(self, task): diff --git a/ironic/drivers/modules/pxe_base.py b/ironic/drivers/modules/pxe_base.py index c9077743d2..39851f5ab3 100644 --- a/ironic/drivers/modules/pxe_base.py +++ b/ironic/drivers/modules/pxe_base.py @@ -31,10 +31,16 @@ REQUIRED_PROPERTIES = { "mounted at boot time. Required."), } OPTIONAL_PROPERTIES = { - 'force_persistent_boot_device': _("True to enable persistent behavior " - "when the boot device is set during " - "deploy and cleaning operations. " - "Defaults to False. Optional."), + 'force_persistent_boot_device': _("Controls the persistency of boot order " + "changes. 'Always' will make all " + "changes persistent, 'Default' will " + "make all but the final one upon " + "instance deployment non-persistent, " + "and 'Never' will make no persistent " + "changes at all. The old values 'True' " + "and 'False' are still supported but " + "deprecated in favor of the new ones." + "Defaults to 'Default'. Optional."), } RESCUE_PROPERTIES = { 'rescue_kernel': _('UUID (from Glance) of the rescue kernel. This value ' 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 cc071bb4cd..fb81cfcf40 100644 --- a/ironic/tests/unit/drivers/modules/test_agent_base_vendor.py +++ b/ironic/tests/unit/drivers/modules/test_agent_base_vendor.py @@ -761,7 +761,7 @@ class AgentDeployMixinTest(AgentDeployMixinBaseTest): task.node.driver_internal_info['is_whole_disk_image'] = False self.deploy.configure_local_boot(task, root_uuid='some-root-uuid') try_set_boot_device_mock.assert_called_once_with( - task, boot_devices.DISK) + task, boot_devices.DISK, persistent=True) install_bootloader_mock.assert_called_once_with( mock.ANY, task.node, root_uuid='some-root-uuid', efi_system_part_uuid=None, prep_boot_part_uuid=None) @@ -779,7 +779,7 @@ class AgentDeployMixinTest(AgentDeployMixinBaseTest): self.deploy.configure_local_boot(task, root_uuid='some-root-uuid', prep_boot_part_uuid='fake-prep') try_set_boot_device_mock.assert_called_once_with( - task, boot_devices.DISK) + task, boot_devices.DISK, persistent=True) install_bootloader_mock.assert_called_once_with( mock.ANY, task.node, root_uuid='some-root-uuid', efi_system_part_uuid=None, prep_boot_part_uuid='fake-prep') @@ -798,7 +798,7 @@ class AgentDeployMixinTest(AgentDeployMixinBaseTest): task, root_uuid='some-root-uuid', efi_system_part_uuid='efi-system-part-uuid') try_set_boot_device_mock.assert_called_once_with( - task, boot_devices.DISK) + task, boot_devices.DISK, persistent=True) install_bootloader_mock.assert_called_once_with( mock.ANY, task.node, root_uuid='some-root-uuid', efi_system_part_uuid='efi-system-part-uuid', @@ -814,7 +814,7 @@ class AgentDeployMixinTest(AgentDeployMixinBaseTest): self.deploy.configure_local_boot(task) self.assertFalse(install_bootloader_mock.called) try_set_boot_device_mock.assert_called_once_with( - task, boot_devices.DISK) + task, boot_devices.DISK, persistent=True) @mock.patch.object(deploy_utils, 'try_set_boot_device', autospec=True) @mock.patch.object(agent_client.AgentClient, 'install_bootloader', @@ -827,7 +827,52 @@ class AgentDeployMixinTest(AgentDeployMixinBaseTest): self.deploy.configure_local_boot(task) self.assertFalse(install_bootloader_mock.called) try_set_boot_device_mock.assert_called_once_with( - task, boot_devices.DISK) + task, boot_devices.DISK, persistent=True) + + @mock.patch.object(deploy_utils, 'try_set_boot_device', autospec=True) + @mock.patch.object(agent_client.AgentClient, 'install_bootloader', + autospec=True) + def test_configure_local_boot_enforce_persistent_boot_device_default( + self, install_bootloader_mock, try_set_boot_device_mock): + with task_manager.acquire(self.context, self.node['uuid'], + shared=False) as task: + driver_info = task.node.driver_info + driver_info['force_persistent_boot_device'] = 'Default' + task.node.driver_info = driver_info + self.deploy.configure_local_boot(task) + self.assertFalse(install_bootloader_mock.called) + try_set_boot_device_mock.assert_called_once_with( + task, boot_devices.DISK, persistent=True) + + @mock.patch.object(deploy_utils, 'try_set_boot_device', autospec=True) + @mock.patch.object(agent_client.AgentClient, 'install_bootloader', + autospec=True) + def test_configure_local_boot_enforce_persistent_boot_device_always( + self, install_bootloader_mock, try_set_boot_device_mock): + with task_manager.acquire(self.context, self.node['uuid'], + shared=False) as task: + driver_info = task.node.driver_info + driver_info['force_persistent_boot_device'] = 'Always' + task.node.driver_info = driver_info + self.deploy.configure_local_boot(task) + self.assertFalse(install_bootloader_mock.called) + try_set_boot_device_mock.assert_called_once_with( + task, boot_devices.DISK, persistent=True) + + @mock.patch.object(deploy_utils, 'try_set_boot_device', autospec=True) + @mock.patch.object(agent_client.AgentClient, 'install_bootloader', + autospec=True) + def test_configure_local_boot_enforce_persistent_boot_device_never( + self, install_bootloader_mock, try_set_boot_device_mock): + with task_manager.acquire(self.context, self.node['uuid'], + shared=False) as task: + driver_info = task.node.driver_info + driver_info['force_persistent_boot_device'] = 'Never' + task.node.driver_info = driver_info + self.deploy.configure_local_boot(task) + self.assertFalse(install_bootloader_mock.called) + try_set_boot_device_mock.assert_called_once_with( + task, boot_devices.DISK, persistent=False) @mock.patch.object(agent_client.AgentClient, 'collect_system_logs', autospec=True) @@ -878,7 +923,7 @@ class AgentDeployMixinTest(AgentDeployMixinBaseTest): mock.ANY, task.node, root_uuid='some-root-uuid', efi_system_part_uuid=None, prep_boot_part_uuid=None) try_set_boot_device_mock.assert_called_once_with( - task, boot_devices.DISK) + task, boot_devices.DISK, persistent=True) collect_logs_mock.assert_called_once_with(mock.ANY, task.node) self.assertEqual(states.DEPLOYFAIL, task.node.provision_state) self.assertEqual(states.ACTIVE, task.node.target_provision_state) diff --git a/ironic/tests/unit/drivers/modules/test_ipxe.py b/ironic/tests/unit/drivers/modules/test_ipxe.py index 2a556c6d6e..e05cdcd47d 100644 --- a/ironic/tests/unit/drivers/modules/test_ipxe.py +++ b/ironic/tests/unit/drivers/modules/test_ipxe.py @@ -308,7 +308,15 @@ class iPXEBootTestCase(db_base.DbTestCase): self.node.save() self._test_prepare_ramdisk() - def test_prepare_ramdisk_force_persistent_boot_device_enabled(self): + def test_prepare_ramdisk_force_persistent_boot_device_true(self): + self.node.provision_state = states.DEPLOYING + driver_info = self.node.driver_info + driver_info['force_persistent_boot_device'] = 'True' + self.node.driver_info = driver_info + self.node.save() + self._test_prepare_ramdisk(persistent=True) + + def test_prepare_ramdisk_force_persistent_boot_device_bool_true(self): self.node.provision_state = states.DEPLOYING driver_info = self.node.driver_info driver_info['force_persistent_boot_device'] = True @@ -316,13 +324,63 @@ class iPXEBootTestCase(db_base.DbTestCase): self.node.save() self._test_prepare_ramdisk(persistent=True) - def test_prepare_ramdisk_force_persistent_boot_device_disabled(self): + def test_prepare_ramdisk_force_persistent_boot_device_sloppy_true(self): + for value in ['true', 't', '1', 'on', 'y', 'YES']: + self.node.provision_state = states.DEPLOYING + driver_info = self.node.driver_info + driver_info['force_persistent_boot_device'] = value + self.node.driver_info = driver_info + self.node.save() + self._test_prepare_ramdisk(persistent=True) + + def test_prepare_ramdisk_force_persistent_boot_device_false(self): + self.node.provision_state = states.DEPLOYING + driver_info = self.node.driver_info + driver_info['force_persistent_boot_device'] = 'False' + self.node.driver_info = driver_info + self.node.save() + self._test_prepare_ramdisk() + + def test_prepare_ramdisk_force_persistent_boot_device_bool_false(self): self.node.provision_state = states.DEPLOYING driver_info = self.node.driver_info driver_info['force_persistent_boot_device'] = False self.node.driver_info = driver_info self.node.save() - self._test_prepare_ramdisk() + self._test_prepare_ramdisk(persistent=False) + + def test_prepare_ramdisk_force_persistent_boot_device_sloppy_false(self): + for value in ['false', 'f', '0', 'off', 'n', 'NO', 'yxz']: + self.node.provision_state = states.DEPLOYING + driver_info = self.node.driver_info + driver_info['force_persistent_boot_device'] = value + self.node.driver_info = driver_info + self.node.save() + self._test_prepare_ramdisk() + + def test_prepare_ramdisk_force_persistent_boot_device_default(self): + self.node.provision_state = states.DEPLOYING + driver_info = self.node.driver_info + driver_info['force_persistent_boot_device'] = 'Default' + self.node.driver_info = driver_info + self.node.save() + self._test_prepare_ramdisk(persistent=False) + + def test_prepare_ramdisk_force_persistent_boot_device_always(self): + self.node.provision_state = states.DEPLOYING + driver_info = self.node.driver_info + driver_info['force_persistent_boot_device'] = 'Always' + self.node.driver_info = driver_info + self.node.save() + self._test_prepare_ramdisk(persistent=True) + + def test_prepare_ramdisk_force_persistent_boot_device_never(self): + self.node.provision_state = states.DEPLOYING + driver_info = self.node.driver_info + driver_info['force_persistent_boot_device'] = 'Never' + self.node.driver_info = driver_info + self.node.save() + self._test_prepare_ramdisk(persistent=False) def test_prepare_ramdisk_rescue(self): self.node.provision_state = states.RESCUING diff --git a/ironic/tests/unit/drivers/modules/test_pxe.py b/ironic/tests/unit/drivers/modules/test_pxe.py index 7121fc18ff..98fe26f7f1 100644 --- a/ironic/tests/unit/drivers/modules/test_pxe.py +++ b/ironic/tests/unit/drivers/modules/test_pxe.py @@ -306,7 +306,15 @@ class PXEBootTestCase(db_base.DbTestCase): self.node.save() self._test_prepare_ramdisk() - def test_prepare_ramdisk_force_persistent_boot_device_enabled(self): + def test_prepare_ramdisk_force_persistent_boot_device_true(self): + self.node.provision_state = states.DEPLOYING + driver_info = self.node.driver_info + driver_info['force_persistent_boot_device'] = 'True' + self.node.driver_info = driver_info + self.node.save() + self._test_prepare_ramdisk(persistent=True) + + def test_prepare_ramdisk_force_persistent_boot_device_bool_true(self): self.node.provision_state = states.DEPLOYING driver_info = self.node.driver_info driver_info['force_persistent_boot_device'] = True @@ -314,13 +322,63 @@ class PXEBootTestCase(db_base.DbTestCase): self.node.save() self._test_prepare_ramdisk(persistent=True) - def test_prepare_ramdisk_force_persistent_boot_device_disabled(self): + def test_prepare_ramdisk_force_persistent_boot_device_sloppy_true(self): + for value in ['true', 't', '1', 'on', 'y', 'YES']: + self.node.provision_state = states.DEPLOYING + driver_info = self.node.driver_info + driver_info['force_persistent_boot_device'] = value + self.node.driver_info = driver_info + self.node.save() + self._test_prepare_ramdisk(persistent=True) + + def test_prepare_ramdisk_force_persistent_boot_device_false(self): + self.node.provision_state = states.DEPLOYING + driver_info = self.node.driver_info + driver_info['force_persistent_boot_device'] = 'False' + self.node.driver_info = driver_info + self.node.save() + self._test_prepare_ramdisk() + + def test_prepare_ramdisk_force_persistent_boot_device_bool_false(self): self.node.provision_state = states.DEPLOYING driver_info = self.node.driver_info driver_info['force_persistent_boot_device'] = False self.node.driver_info = driver_info self.node.save() - self._test_prepare_ramdisk() + self._test_prepare_ramdisk(persistent=False) + + def test_prepare_ramdisk_force_persistent_boot_device_sloppy_false(self): + for value in ['false', 'f', '0', 'off', 'n', 'NO', 'yxz']: + self.node.provision_state = states.DEPLOYING + driver_info = self.node.driver_info + driver_info['force_persistent_boot_device'] = value + self.node.driver_info = driver_info + self.node.save() + self._test_prepare_ramdisk() + + def test_prepare_ramdisk_force_persistent_boot_device_default(self): + self.node.provision_state = states.DEPLOYING + driver_info = self.node.driver_info + driver_info['force_persistent_boot_device'] = 'Default' + self.node.driver_info = driver_info + self.node.save() + self._test_prepare_ramdisk(persistent=False) + + def test_prepare_ramdisk_force_persistent_boot_device_always(self): + self.node.provision_state = states.DEPLOYING + driver_info = self.node.driver_info + driver_info['force_persistent_boot_device'] = 'Always' + self.node.driver_info = driver_info + self.node.save() + self._test_prepare_ramdisk(persistent=True) + + def test_prepare_ramdisk_force_persistent_boot_device_never(self): + self.node.provision_state = states.DEPLOYING + driver_info = self.node.driver_info + driver_info['force_persistent_boot_device'] = 'Never' + self.node.driver_info = driver_info + self.node.save() + self._test_prepare_ramdisk(persistent=False) def test_prepare_ramdisk_rescue(self): self.node.provision_state = states.RESCUING diff --git a/releasenotes/notes/add-option-persistent-boot-device-139cf280fb66f4f7.yaml b/releasenotes/notes/add-option-persistent-boot-device-139cf280fb66f4f7.yaml new file mode 100644 index 0000000000..55e6bcad40 --- /dev/null +++ b/releasenotes/notes/add-option-persistent-boot-device-139cf280fb66f4f7.yaml @@ -0,0 +1,17 @@ +--- +features: + - | + Adds capability to control the persistency of boot order changes during + instance deployment via (i)PXE on a per-node level. The option + 'force_persistent_boot_device' in the node's driver info for the (i)PXE + drivers is extended to allow the values 'Default' (make all changes + but the last one upon deployment non-persistent), 'Always' (make all + changes persistent), and 'Never' (make all boot order changes + non-persistent). +deprecations: + - | + The values 'True'/'False' for the option 'force_persistent_boot_device' + in the node's driver info for the (i)PXE drivers are deprecated and + support for them may be removed in a future release. The former default + value 'False' is replaced by the new value 'Default', the value 'True' + is replaced by 'Always'.