From aad54c245b195f15b19bbc34a0ca22ea0a3422d4 Mon Sep 17 00:00:00 2001
From: Ilya Etingof <etingof@gmail.com>
Date: Tue, 3 Mar 2020 23:34:10 +0100
Subject: [PATCH] Improve `redfish` set-boot-device behaviour

Makes management interface of `redfish` hardware type not changing
current boot frequency if currently set one is the same as the
desired one. The goal is to avoid touching potentially faulty BMC
option whenever possible.

This should hopefully improve interoperability with Redfish BMCs.

Depends-On: https://review.opendev.org/710751
Change-Id: I380cd3aae410e05a4519c3764140ced121bf7fe7
Story: 2007355
Task: 38937
---
 ironic/drivers/modules/redfish/management.py  | 11 ++++++--
 .../modules/redfish/test_management.py        | 25 +++++++++++++++++++
 ...fish-set-boot-device-e38e9e9442ab5750.yaml |  7 ++++++
 3 files changed, 41 insertions(+), 2 deletions(-)
 create mode 100644 releasenotes/notes/improve-redfish-set-boot-device-e38e9e9442ab5750.yaml

diff --git a/ironic/drivers/modules/redfish/management.py b/ironic/drivers/modules/redfish/management.py
index 3db4b7de2f..4026da48cb 100644
--- a/ironic/drivers/modules/redfish/management.py
+++ b/ironic/drivers/modules/redfish/management.py
@@ -126,10 +126,17 @@ class RedfishManagement(base.ManagementInterface):
         """
         system = redfish_utils.get_system(task.node)
 
+        desired_persistence = BOOT_DEVICE_PERSISTENT_MAP_REV[persistent]
+        current_persistence = system.boot.get('enabled')
+
+        # NOTE(etingof): this can be racy, esp if BMC is not RESTful
+        enabled = (desired_persistence
+                   if desired_persistence != current_persistence else None)
+
         try:
             system.set_system_boot_options(
-                BOOT_DEVICE_MAP_REV[device],
-                enabled=BOOT_DEVICE_PERSISTENT_MAP_REV[persistent])
+                BOOT_DEVICE_MAP_REV[device], enabled=enabled)
+
         except sushy.exceptions.SushyError as e:
             error_msg = (_('Redfish set boot device failed for node '
                            '%(node)s. Error: %(error)s') %
diff --git a/ironic/tests/unit/drivers/modules/redfish/test_management.py b/ironic/tests/unit/drivers/modules/redfish/test_management.py
index 09d903f918..c8a37951a9 100644
--- a/ironic/tests/unit/drivers/modules/redfish/test_management.py
+++ b/ironic/tests/unit/drivers/modules/redfish/test_management.py
@@ -127,6 +127,31 @@ class RedfishManagementTestCase(db_base.DbTestCase):
                 fake_system.set_system_boot_options.reset_mock()
                 mock_get_system.reset_mock()
 
+    @mock.patch.object(redfish_utils, 'get_system', autospec=True)
+    def test_set_boot_device_persistency_no_change(self, mock_get_system):
+        fake_system = mock.Mock()
+        mock_get_system.return_value = fake_system
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=False) as task:
+            expected_values = [
+                (True, sushy.BOOT_SOURCE_ENABLED_CONTINUOUS),
+                (False, sushy.BOOT_SOURCE_ENABLED_ONCE)
+            ]
+
+            for target, expected in expected_values:
+                fake_system.boot.get.return_value = expected
+
+                task.driver.management.set_boot_device(
+                    task, boot_devices.PXE, persistent=target)
+
+                fake_system.set_system_boot_options.assert_called_once_with(
+                    sushy.BOOT_SOURCE_TARGET_PXE, enabled=None)
+                mock_get_system.assert_called_once_with(task.node)
+
+                # Reset mocks
+                fake_system.set_system_boot_options.reset_mock()
+                mock_get_system.reset_mock()
+
     @mock.patch.object(sushy, 'Sushy', autospec=True)
     @mock.patch.object(redfish_utils, 'get_system', autospec=True)
     def test_set_boot_device_fail(self, mock_get_system, mock_sushy):
diff --git a/releasenotes/notes/improve-redfish-set-boot-device-e38e9e9442ab5750.yaml b/releasenotes/notes/improve-redfish-set-boot-device-e38e9e9442ab5750.yaml
new file mode 100644
index 0000000000..a2ff17280d
--- /dev/null
+++ b/releasenotes/notes/improve-redfish-set-boot-device-e38e9e9442ab5750.yaml
@@ -0,0 +1,7 @@
+---
+features:
+  - |
+    Makes management interface of ``redfish`` hardware type not changing
+    current boot frequency if currently set is the same as the desired one.
+    The goal is to avoid touching potentially faulty BMC option whenever
+    possible.