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

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

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

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

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)
 
 

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)
 
 

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)
 

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."),
+}

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'],

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

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

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

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.