Merge "Apply force_persistent_boot_device to all boot interfaces"

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.