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

12 files changed, 85 insertions, 196 deletions

diff --git a/doc/source/admin/interfaces/boot.rst b/doc/source/admin/interfaces/boot.rst
index 35d5561e3..2b74b17de 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 d0e2bbd15..32ce3e9d3 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 c0e041108..0ed7b190f 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 b2bcd9c30..0fcc7263f 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 0944e7d4e..70162a2ee 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 dbd05ab84..7ffc52081 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 07d7d4358..4aa2335d6 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 c5fd84279..076963d5c 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 8d0bf5f35..5e0b9520b 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 4c0536fc5..5229cc250 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 1ccc33699..da1d9da97 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 000000000..569d72490
--- /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.