From a5f7d75ba2067695885c8ba92193e46fbed4d33d Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Mon, 1 Feb 2021 12:44:05 +0100 Subject: [PATCH] Apply force_persistent_boot_device to all boot interfaces For some (likely historical) reasons we only use it for PXE and iPXE, but the same logic applies to any boot interface (since it depends on how the management interface and the BMC work, not on the boot method). This change moves its handling to conductor utils. Change-Id: I948beb4053034d3c1b4c5b7c64100e41f6022739 --- doc/source/admin/interfaces/boot.rst | 9 ++- ironic/conductor/utils.py | 21 +++++- ironic/drivers/modules/ilo/boot.py | 3 +- ironic/drivers/modules/irmc/boot.py | 2 + ironic/drivers/modules/pxe_base.py | 37 +--------- ironic/drivers/modules/redfish/boot.py | 2 + ironic/drivers/utils.py | 14 ++++ ironic/tests/unit/conductor/test_manager.py | 9 +-- ironic/tests/unit/conductor/test_utils.py | 31 ++++++++ .../tests/unit/drivers/modules/test_ipxe.py | 74 ------------------- ironic/tests/unit/drivers/modules/test_pxe.py | 74 ------------------- ...ce-persistent-common-6ef2537f7ccd0dcb.yaml | 5 ++ 12 files changed, 85 insertions(+), 196 deletions(-) create mode 100644 releasenotes/notes/force-persistent-common-6ef2537f7ccd0dcb.yaml diff --git a/doc/source/admin/interfaces/boot.rst b/doc/source/admin/interfaces/boot.rst index 35d5561e36..2b74b17de4 100644 --- a/doc/source/admin/interfaces/boot.rst +++ b/doc/source/admin/interfaces/boot.rst @@ -39,12 +39,15 @@ specific implementations of the PXE boot interface. Additional configuration is required for this boot interface - see :doc:`/install/configure-pxe` for details. +Common options +-------------- + Enable persistent boot device for deploy/clean operation ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -Ironic uses non-persistent boot for cleaning/deploying phases as default, -in PXE interface. For some drivers, a persistent change is far more -costly than a non-persistent one, so this can bring performance improvements. +Ironic uses non-persistent boot for cleaning/deploying phases as default. +For some drivers, a persistent change is far more costly than a non-persistent +one, so this can bring performance improvements. Set the flag ``force_persistent_boot_device`` to ``True`` in the node's ``driver_info``:: diff --git a/ironic/conductor/utils.py b/ironic/conductor/utils.py index d0e2bbd157..32ce3e9d39 100644 --- a/ironic/conductor/utils.py +++ b/ironic/conductor/utils.py @@ -26,6 +26,7 @@ from oslo_log import log from oslo_serialization import jsonutils from oslo_service import loopingcall from oslo_utils import excutils +from oslo_utils import strutils from oslo_utils import timeutils from ironic.common import boot_devices @@ -67,10 +68,22 @@ def node_set_boot_device(task, device, persistent=False): """ task.driver.management.validate(task) - if task.node.provision_state != states.ADOPTING: - task.driver.management.set_boot_device(task, - device=device, - persistent=persistent) + if task.node.provision_state == states.ADOPTING: + return + + force_persistent = task.node.driver_info.get( + 'force_persistent_boot_device') + if force_persistent == 'Always': + persistent = True + elif force_persistent == 'Never': + persistent = False + elif force_persistent not in (None, 'Default'): + # Backward compatibility (used to be a boolean and only True mattered) + if strutils.bool_from_string(force_persistent, strict=False): + persistent = True + + task.driver.management.set_boot_device(task, device=device, + persistent=persistent) def node_get_boot_mode(task): diff --git a/ironic/drivers/modules/ilo/boot.py b/ironic/drivers/modules/ilo/boot.py index c0e0411084..0ed7b190f3 100644 --- a/ironic/drivers/modules/ilo/boot.py +++ b/ironic/drivers/modules/ilo/boot.py @@ -37,6 +37,7 @@ from ironic.drivers.modules.ilo import common as ilo_common from ironic.drivers.modules import image_utils from ironic.drivers.modules import ipxe from ironic.drivers.modules import pxe +from ironic.drivers import utils as driver_utils LOG = logging.getLogger(__name__) @@ -350,7 +351,7 @@ class IloVirtualMediaBoot(base.BootInterface): # TODO(stendulker): COMMON_PROPERTIES should also include rescue # related properties (RESCUE_PROPERTIES). We can add them in Rocky, # when classic drivers get removed. - return COMMON_PROPERTIES + return dict(driver_utils.OPTIONAL_PROPERTIES, **COMMON_PROPERTIES) @METRICS.timer('IloVirtualMediaBoot.validate') def validate(self, task): diff --git a/ironic/drivers/modules/irmc/boot.py b/ironic/drivers/modules/irmc/boot.py index b2bcd9c308..0fcc7263f0 100644 --- a/ironic/drivers/modules/irmc/boot.py +++ b/ironic/drivers/modules/irmc/boot.py @@ -41,6 +41,7 @@ from ironic.drivers.modules import deploy_utils from ironic.drivers.modules.irmc import common as irmc_common from ironic.drivers.modules.irmc import management as irmc_management from ironic.drivers.modules import pxe +from ironic.drivers import utils as driver_utils scci = importutils.try_import('scciclient.irmc.scci') @@ -83,6 +84,7 @@ OPTIONAL_PROPERTIES = { } COMMON_PROPERTIES = REQUIRED_PROPERTIES.copy() +COMMON_PROPERTIES.update(driver_utils.OPTIONAL_PROPERTIES) COMMON_PROPERTIES.update(OPTIONAL_PROPERTIES) diff --git a/ironic/drivers/modules/pxe_base.py b/ironic/drivers/modules/pxe_base.py index 0944e7d4e7..70162a2eec 100644 --- a/ironic/drivers/modules/pxe_base.py +++ b/ironic/drivers/modules/pxe_base.py @@ -17,7 +17,6 @@ from futurist import periodics from ironic_lib import metrics_utils from oslo_config import cfg from oslo_log import log as logging -from oslo_utils import strutils from ironic.common import boot_devices from ironic.common import dhcp_factory @@ -45,18 +44,6 @@ REQUIRED_PROPERTIES = { 'deploy_ramdisk': _("UUID (from Glance) of the ramdisk that is " "mounted at boot time. Required."), } -OPTIONAL_PROPERTIES = { - '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 ' 'is required for rescue mode.'), @@ -65,7 +52,7 @@ RESCUE_PROPERTIES = { 'required for rescue mode.'), } COMMON_PROPERTIES = REQUIRED_PROPERTIES.copy() -COMMON_PROPERTIES.update(OPTIONAL_PROPERTIES) +COMMON_PROPERTIES.update(driver_utils.OPTIONAL_PROPERTIES) COMMON_PROPERTIES.update(RESCUE_PROPERTIES) @@ -211,9 +198,8 @@ class PXEBaseMixin(object): pxe_utils.create_pxe_config(task, pxe_options, pxe_config_template, ipxe_enabled=self.ipxe_enabled) - persistent = self._persistent_ramdisk_boot(node) manager_utils.node_set_boot_device(task, boot_devices.PXE, - persistent=persistent) + persistent=False) if self.ipxe_enabled and CONF.pxe.ipxe_use_swift: kernel_label = '%s_kernel' % mode @@ -328,12 +314,8 @@ class PXEBaseMixin(object): # 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=persistent) + persistent=True) def _validate_common(self, task): node = task.node @@ -436,14 +418,6 @@ class PXEBaseMixin(object): raise exception.UnsupportedDriverExtension( driver=task.node.driver, extension='inspection') - def _persistent_ramdisk_boot(self, node): - """If the ramdisk should be configured as a persistent boot device.""" - value = node.driver_info.get('force_persistent_boot_device', 'Default') - if value in {'Always', 'Default', 'Never'}: - return value == 'Always' - else: - return strutils.bool_from_string(value, False) - _RETRY_ALLOWED_STATES = {states.DEPLOYWAIT, states.CLEANWAIT, states.RESCUEWAIT} @@ -493,11 +467,8 @@ class PXEBaseMixin(object): 'timeout': CONF.pxe.boot_retry_timeout}) manager_utils.node_power_action(task, states.POWER_OFF) - # NOTE(dtantsur): retry even persistent boot setting in case it did not - # work for some reason. - persistent = self._persistent_ramdisk_boot(task.node) manager_utils.node_set_boot_device(task, boot_devices.PXE, - persistent=persistent) + persistent=False) manager_utils.node_power_action(task, states.POWER_ON) diff --git a/ironic/drivers/modules/redfish/boot.py b/ironic/drivers/modules/redfish/boot.py index dbd05ab843..7ffc520812 100644 --- a/ironic/drivers/modules/redfish/boot.py +++ b/ironic/drivers/modules/redfish/boot.py @@ -29,6 +29,7 @@ from ironic.drivers.modules import boot_mode_utils from ironic.drivers.modules import deploy_utils from ironic.drivers.modules import image_utils from ironic.drivers.modules.redfish import utils as redfish_utils +from ironic.drivers import utils as driver_utils LOG = log.getLogger(__name__) @@ -68,6 +69,7 @@ RESCUE_PROPERTIES = { } COMMON_PROPERTIES = REQUIRED_PROPERTIES.copy() +COMMON_PROPERTIES.update(driver_utils.OPTIONAL_PROPERTIES) COMMON_PROPERTIES.update(OPTIONAL_PROPERTIES) COMMON_PROPERTIES.update(RESCUE_PROPERTIES) diff --git a/ironic/drivers/utils.py b/ironic/drivers/utils.py index 07d7d4358b..4aa2335d67 100644 --- a/ironic/drivers/utils.py +++ b/ironic/drivers/utils.py @@ -370,3 +370,17 @@ def collect_ramdisk_logs(node, label=None): LOG.exception('Unknown error when storing logs from the node ' '%(node)s deployment. Error: %(error)s', {'node': node.uuid, 'error': e}) + + +OPTIONAL_PROPERTIES = { + '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."), +} diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index c5fd842797..076963d5c3 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -5718,15 +5718,10 @@ class ManagerTestProperties(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): 'image_http_proxy', 'image_https_proxy', 'image_no_proxy']) if pxe_common: - expected.extend(['force_persistent_boot_device', - 'rescue_kernel', 'rescue_ramdisk']) + expected.extend(['rescue_kernel', 'rescue_ramdisk']) + expected.append('force_persistent_boot_device') self.assertCountEqual(expected, properties) - def test_driver_properties_fake(self): - expected = ['B1', 'B2'] - self._check_driver_properties("fake-hardware", expected, - agent_common=False, pxe_common=False) - def test_driver_properties_ipmi(self): self.config(enabled_hardware_types='ipmi', enabled_power_interfaces=['ipmitool'], diff --git a/ironic/tests/unit/conductor/test_utils.py b/ironic/tests/unit/conductor/test_utils.py index 8d0bf5f35c..5e0b9520ba 100644 --- a/ironic/tests/unit/conductor/test_utils.py +++ b/ironic/tests/unit/conductor/test_utils.py @@ -86,6 +86,37 @@ class NodeSetBootDeviceTestCase(db_base.DbTestCase): conductor_utils.node_set_boot_device(self.task, device='pxe') self.assertFalse(mock_sbd.called) + @mock.patch.object(fake.FakeManagement, 'set_boot_device', autospec=True) + def test_node_set_boot_device_force_default(self, mock_sbd): + # Boolean value False was equivalent to the default + for value in ('false', False, 'Default'): + self.task.node.driver_info['force_persistent_boot_device'] = value + for request in (True, False): + mock_sbd.reset_mock() + conductor_utils.node_set_boot_device(self.task, device='pxe', + persistent=request) + mock_sbd.assert_called_once_with(mock.ANY, self.task, + device='pxe', + persistent=request) + + @mock.patch.object(fake.FakeManagement, 'set_boot_device', autospec=True) + def test_node_set_boot_device_force_always(self, mock_sbd): + for value in ('true', True, 'Always'): + mock_sbd.reset_mock() + self.task.node.driver_info['force_persistent_boot_device'] = value + conductor_utils.node_set_boot_device(self.task, device='pxe', + persistent=False) + mock_sbd.assert_called_once_with(mock.ANY, self.task, + device='pxe', persistent=True) + + @mock.patch.object(fake.FakeManagement, 'set_boot_device', autospec=True) + def test_node_set_boot_device_force_never(self, mock_sbd): + self.task.node.driver_info['force_persistent_boot_device'] = 'Never' + conductor_utils.node_set_boot_device(self.task, device='pxe', + persistent=True) + mock_sbd.assert_called_once_with(mock.ANY, self.task, + device='pxe', persistent=False) + class NodeGetBootModeTestCase(db_base.DbTestCase): diff --git a/ironic/tests/unit/drivers/modules/test_ipxe.py b/ironic/tests/unit/drivers/modules/test_ipxe.py index 4c0536fc51..5229cc2500 100644 --- a/ironic/tests/unit/drivers/modules/test_ipxe.py +++ b/ironic/tests/unit/drivers/modules/test_ipxe.py @@ -343,80 +343,6 @@ class iPXEBootTestCase(db_base.DbTestCase): self.node.save() self._test_prepare_ramdisk() - 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 - self.node.driver_info = driver_info - self.node.save() - self._test_prepare_ramdisk(persistent=True) - - 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(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 self.node.save() diff --git a/ironic/tests/unit/drivers/modules/test_pxe.py b/ironic/tests/unit/drivers/modules/test_pxe.py index 1ccc336993..da1d9da973 100644 --- a/ironic/tests/unit/drivers/modules/test_pxe.py +++ b/ironic/tests/unit/drivers/modules/test_pxe.py @@ -323,80 +323,6 @@ class PXEBootTestCase(db_base.DbTestCase): self.node.save() self._test_prepare_ramdisk() - 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 - self.node.driver_info = driver_info - self.node.save() - self._test_prepare_ramdisk(persistent=True) - - 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(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 self.node.save() diff --git a/releasenotes/notes/force-persistent-common-6ef2537f7ccd0dcb.yaml b/releasenotes/notes/force-persistent-common-6ef2537f7ccd0dcb.yaml new file mode 100644 index 0000000000..569d724900 --- /dev/null +++ b/releasenotes/notes/force-persistent-common-6ef2537f7ccd0dcb.yaml @@ -0,0 +1,5 @@ +--- +features: + - | + The ``force_persistent_boot_device`` parameter now consistently applies + to all boot interfaces, rather than only PXE and iPXE.