Refactor iDRAC OEM extension manager calls

- Re-usable helper created to avoid duplication.
- Although there is only one manager for system in known iDRAC systems
still iterate through collection for future changes.
- Restructured exception raising and error logging for better feedback.
- Removed some unit tests to avoid duplication that is covered by
method specific unit tests

(cherry picked from commit 39cd751a9063ae04e1649ccbca5496c3754d706e)

Conflicts:
	ironic/tests/unit/drivers/modules/drac/test_boot.py

Change-Id: I03fdb48e47c9557c207a20ee876eccf3f3459d9f

4 files changed, 242 insertions, 145 deletions

diff --git a/ironic/drivers/modules/drac/boot.py b/ironic/drivers/modules/drac/boot.py
index 771dc7f5e..75f9b9427 100644
--- a/ironic/drivers/modules/drac/boot.py
+++ b/ironic/drivers/modules/drac/boot.py
@@ -1,6 +1,6 @@
 # Copyright 2019 Red Hat, Inc.
 # All Rights Reserved.
-# Copyright (c) 2019 Dell Inc. or its subsidiaries.
+# Copyright (c) 2019-2021 Dell Inc. or its subsidiaries.
 #
 # Licensed under the Apache License, Version 2.0 (the "License"); you may
 # not use this file except in compliance with the License. You may obtain
@@ -18,8 +18,7 @@ from oslo_log import log
 from oslo_utils import importutils
 
 from ironic.common import boot_devices
-from ironic.common import exception
-from ironic.common.i18n import _
+from ironic.drivers.modules.drac import utils as drac_utils
 from ironic.drivers.modules.redfish import boot as redfish_boot
 from ironic.drivers.modules.redfish import utils as redfish_utils
 
@@ -102,60 +101,8 @@ class DracRedfishVirtualMediaBoot(redfish_boot.RedfishVirtualMediaBoot):
 
         system = redfish_utils.get_system(task.node)
 
-        for manager in system.managers:
-
-            # This call makes Sushy go fishing in the ocean of Sushy
-            # OEM extensions installed on the system. If it finds one
-            # for 'Dell' which implements the 'Manager' resource
-            # extension, it uses it to create an object which
-            # instantiates itself from the OEM JSON. The object is
-            # returned here.
-            #
-            # If the extension could not be found for one manager, it
-            # will not be found for any others until it is installed, so
-            # abruptly exit the for loop. The vendor and resource name,
-            # 'Dell' and 'Manager', respectively, used to search for the
-            # extension are invariant in the loop.
-            try:
-                manager_oem = manager.get_oem_extension('Dell')
-            except sushy.exceptions.OEMExtensionNotFoundError as e:
-                error_msg = (_("Search for Sushy OEM extension Python package "
-                               "'sushy-oem-idrac' failed for node %(node)s. "
-                               "Ensure it is installed. Error: %(error)s") %
-                             {'node': task.node.uuid, 'error': e})
-                LOG.error(error_msg)
-                raise exception.RedfishError(error=error_msg)
-
-            try:
-                manager_oem.set_virtual_boot_device(
-                    device, persistent=persistent, manager=manager,
-                    system=system)
-            except sushy.exceptions.SushyError as e:
-                LOG.debug("Sushy OEM extension Python package "
-                          "'sushy-oem-idrac' failed to set virtual boot "
-                          "device with system %(system)s manager %(manager)s "
-                          "for node %(node)s. Will try next manager, if "
-                          "available. Error: %(error)s",
-                          {'system': system.uuid if system.uuid else
-                           system.identity,
-                           'manager': manager.uuid if manager.uuid else
-                           manager.identity,
-                           'node': task.node.uuid,
-                           'error': e})
-                continue
-
-            LOG.info("Set node %(node)s boot device to %(device)s via OEM",
-                     {'node': task.node.uuid, 'device': device})
-            break
-
-        else:
-            error_msg = (_('iDRAC Redfish set boot device failed for node '
-                           '%(node)s, because system %(system)s has no '
-                           'manager%(no_manager)s.') %
-                         {'node': task.node.uuid,
-                          'system': system.uuid if system.uuid else
-                          system.identity,
-                          'no_manager': '' if not system.managers else
-                          ' which could'})
-            LOG.error(error_msg)
-            raise exception.RedfishError(error=error_msg)
+        drac_utils.execute_oem_manager_method(
+            task, 'set virtual boot device',
+            lambda m, manager: m.set_virtual_boot_device(
+                device, persistent=persistent, system=system,
+                manager=manager), pass_manager=True)
diff --git a/ironic/drivers/modules/drac/utils.py b/ironic/drivers/modules/drac/utils.py
new file mode 100644
index 000000000..21073d56e
--- /dev/null
+++ b/ironic/drivers/modules/drac/utils.py
@@ -0,0 +1,121 @@
+# Copyright (c) 2021 Dell Inc. or its subsidiaries.
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+from oslo_log import log
+from oslo_utils import importutils
+
+from ironic.common import exception
+from ironic.drivers.modules.redfish import utils as redfish_utils
+
+LOG = log.getLogger(__name__)
+
+sushy = importutils.try_import('sushy')
+
+
+def execute_oem_manager_method(
+        task, process_name, lambda_oem_func, pass_manager=False):
+    """Loads OEM manager and executes passed method on it.
+
+    Known iDRAC Redfish systems has only one manager, but as Redfish
+    schema allows a list this method iterates through all values in case
+    this changes in future. If there are several managers, this will
+    try starting from the first in the list until the first success.
+
+    :param task: a TaskManager instance.
+    :param process_name: user friendly name of method to be executed.
+        Used in exception and log messages.
+    :param lambda_oem_func: method to execute as lambda function with
+        input parameter OEM extension manager.
+        Example: lambda m: m.reset_idrac()
+        For older versions also support second input parameter Redfish
+        manager itself when pass_manager set to True.
+    :param pass_manager: whether to pass manager itself to executed
+        OEM extension method. This is for backward compability, new
+        functions must not pass manager, but acquire it internally. Will
+        be removed in future.
+    :returns: Returned value of lambda_oem_func
+    :raises: RedfishError if can't execute OEM function either because
+        there are no managers to the system, failed to load OEM
+        extension or execution of the OEM method failed itself.
+    """
+
+    system = redfish_utils.get_system(task.node)
+
+    if not system.managers:
+        raise exception.RedfishError(
+            "System %(system)s has no managers" %
+            {'system': system.uuid if system.uuid else system.identity})
+
+    oem_error_msgs = []
+    for manager in system.managers:
+        # This call makes Sushy go fishing in the ocean of Sushy
+        # OEM extensions installed on the system. If it finds one
+        # for 'Dell' which implements the 'Manager' resource
+        # extension, it uses it to create an object which
+        # instantiates itself from the OEM JSON. The object is
+        # returned here.
+        #
+        # If the extension could not be found for one manager, it
+        # will not be found for any others until it is installed, so
+        # abruptly exit the for loop. The vendor and resource name,
+        # 'Dell' and 'Manager', respectively, used to search for the
+        # extension are invariant in the loop.
+        try:
+            manager_oem = manager.get_oem_extension('Dell')
+        except sushy.exceptions.OEMExtensionNotFoundError as e:
+            error_msg = (_("Search for Sushy OEM extension Python package "
+                           "'sushy-oem-idrac' failed for node %(node)s. "
+                           "Ensure it is installed. Error: %(error)s") %
+                         {'node': task.node.uuid, 'error': e})
+            LOG.error(error_msg)
+            raise exception.RedfishError(error=error_msg)
+
+        try:
+            if pass_manager:
+                result = lambda_oem_func(manager_oem, manager)
+            else:
+                result = lambda_oem_func(manager_oem)
+            LOG.info("Completed: %(process_name)s with system %(system)s "
+                     "manager %(manager)s for node %(node)s",
+                     {'process_name': process_name,
+                      'system': system.uuid if system.uuid else
+                      system.identity,
+                      'manager': manager.uuid if manager.uuid else
+                      manager.identity,
+                      'node': task.node.uuid})
+            return result
+        except sushy.exceptions.SushyError as e:
+            error_msg = (_("Manager %(manager)s: %(error)s" %
+                         {'manager': manager.uuid if manager.uuid else
+                           manager.identity, 'error': e}))
+            LOG.debug("Failed: %(process_name)s with system %(system)s "
+                      "for node %(node)s. Will try next manager, if "
+                      "available. Error: %(error)s",
+                      {'process_name': process_name,
+                       'system': system.uuid if system.uuid else
+                       system.identity,
+                       'node': task.node.uuid,
+                       'error': error_msg})
+            oem_error_msgs.append(error_msg)
+    else:
+        error_msg = (_('In system %(system)s for node %(node)s all managers '
+                       'failed: %(process_name)s. Errors: %(oem_error_msgs)s' %
+                     {'system': system.uuid if system.uuid else
+                       system.identity,
+                      'node': task.node.uuid,
+                      'process_name': process_name,
+                      'oem_error_msgs': oem_error_msgs if oem_error_msgs else
+                      'unknown'}))
+        LOG.error(error_msg)
+        raise exception.RedfishError(error=error_msg)
diff --git a/ironic/tests/unit/drivers/modules/drac/test_boot.py b/ironic/tests/unit/drivers/modules/drac/test_boot.py
index ec8ae4fe6..481e925e3 100644
--- a/ironic/tests/unit/drivers/modules/drac/test_boot.py
+++ b/ironic/tests/unit/drivers/modules/drac/test_boot.py
@@ -1,6 +1,6 @@
 # Copyright 2019 Red Hat, Inc.
 # All Rights Reserved.
-# Copyright (c) 2019 Dell Inc. or its subsidiaries.
+# Copyright (c) 2019-2021 Dell Inc. or its subsidiaries.
 #
 # Licensed under the Apache License, Version 2.0 (the "License"); you may
 # not use this file except in compliance with the License. You may obtain
@@ -22,9 +22,8 @@ import mock
 from oslo_utils import importutils
 
 from ironic.common import boot_devices
-from ironic.common import exception
 from ironic.conductor import task_manager
-from ironic.drivers.modules.drac import boot as drac_boot
+from ironic.drivers.modules.redfish import utils as redfish_utils
 from ironic.tests.unit.drivers.modules.drac import utils as test_utils
 from ironic.tests.unit.objects import utils as obj_utils
 
@@ -33,7 +32,7 @@ sushy = importutils.try_import('sushy')
 INFO_DICT = test_utils.INFO_DICT
 
 
-@mock.patch.object(drac_boot, 'redfish_utils', autospec=True)
+@mock.patch.object(redfish_utils, 'get_system', autospec=True)
 class DracBootTestCase(test_utils.BaseDracTest):
 
     def setUp(self):
@@ -41,14 +40,10 @@ class DracBootTestCase(test_utils.BaseDracTest):
         self.node = obj_utils.create_test_node(
             self.context, driver='idrac', driver_info=INFO_DICT)
 
-    def test__set_boot_device_persistent(self, mock_redfish_utils):
-
-        mock_system = mock_redfish_utils.get_system.return_value
-
+    def test__set_boot_device_persistent(self, mock_get_system):
         mock_manager = mock.MagicMock()
-
+        mock_system = mock_get_system.return_value
         mock_system.managers = [mock_manager]
-
         mock_manager_oem = mock_manager.get_oem_extension.return_value
 
         with task_manager.acquire(self.context, self.node.uuid,
@@ -60,14 +55,10 @@ class DracBootTestCase(test_utils.BaseDracTest):
                 'cd', persistent=True, manager=mock_manager,
                 system=mock_system)
 
-    def test__set_boot_device_cd(self, mock_redfish_utils):
-
-        mock_system = mock_redfish_utils.get_system.return_value
-
+    def test__set_boot_device_cd(self, mock_get_system):
+        mock_system = mock_get_system.return_value
         mock_manager = mock.MagicMock()
-
         mock_system.managers = [mock_manager]
-
         mock_manager_oem = mock_manager.get_oem_extension.return_value
 
         with task_manager.acquire(self.context, self.node.uuid,
@@ -78,14 +69,10 @@ class DracBootTestCase(test_utils.BaseDracTest):
                 'cd', persistent=False, manager=mock_manager,
                 system=mock_system)
 
-    def test__set_boot_device_floppy(self, mock_redfish_utils):
-
-        mock_system = mock_redfish_utils.get_system.return_value
-
+    def test__set_boot_device_floppy(self, mock_get_system):
+        mock_system = mock_get_system.return_value
         mock_manager = mock.MagicMock()
-
         mock_system.managers = [mock_manager]
-
         mock_manager_oem = mock_manager.get_oem_extension.return_value
 
         with task_manager.acquire(self.context, self.node.uuid,
@@ -96,72 +83,11 @@ class DracBootTestCase(test_utils.BaseDracTest):
                 'floppy', persistent=False, manager=mock_manager,
                 system=mock_system)
 
-    def test__set_boot_device_disk(self, mock_redfish_utils):
-
-        mock_system = mock_redfish_utils.get_system.return_value
+    def test__set_boot_device_disk(self, mock_get_system):
+        mock_system = mock_get_system.return_value
 
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=False) as task:
             task.driver.boot._set_boot_device(task, boot_devices.DISK)
 
             self.assertFalse(mock_system.called)
-
-    def test__set_boot_device_missing_oem(self, mock_redfish_utils):
-
-        mock_system = mock_redfish_utils.get_system.return_value
-
-        mock_manager = mock.MagicMock()
-
-        mock_system.managers = [mock_manager]
-
-        mock_manager.get_oem_extension.side_effect = (
-            sushy.exceptions.OEMExtensionNotFoundError)
-
-        with task_manager.acquire(self.context, self.node.uuid,
-                                  shared=False) as task:
-            self.assertRaises(exception.RedfishError,
-                              task.driver.boot._set_boot_device,
-                              task, boot_devices.CDROM)
-
-    def test__set_boot_device_failover(self, mock_redfish_utils):
-
-        mock_system = mock_redfish_utils.get_system.return_value
-
-        mock_manager_fail = mock.MagicMock()
-        mock_manager_ok = mock.MagicMock()
-
-        mock_system.managers = [mock_manager_fail, mock_manager_ok]
-
-        mock_svbd_fail = (mock_manager_fail.get_oem_extension
-                          .return_value.set_virtual_boot_device)
-
-        mock_svbd_ok = (mock_manager_ok.get_oem_extension
-                        .return_value.set_virtual_boot_device)
-
-        mock_svbd_fail.side_effect = sushy.exceptions.SushyError
-
-        with task_manager.acquire(self.context, self.node.uuid,
-                                  shared=False) as task:
-            task.driver.boot._set_boot_device(task, boot_devices.CDROM)
-
-            self.assertFalse(mock_system.called)
-
-        mock_svbd_fail.assert_called_once_with(
-            'cd', manager=mock_manager_fail, persistent=False,
-            system=mock_system)
-
-        mock_svbd_ok.assert_called_once_with(
-            'cd', manager=mock_manager_ok, persistent=False,
-            system=mock_system)
-
-    def test__set_boot_device_no_manager(self, mock_redfish_utils):
-
-        mock_system = mock_redfish_utils.get_system.return_value
-
-        mock_system.managers = []
-
-        with task_manager.acquire(self.context, self.node.uuid,
-                                  shared=False) as task:
-            self.assertRaises(exception.RedfishError,
-                              task.driver.boot._set_boot_device,
-                              task, boot_devices.CDROM)
diff --git a/ironic/tests/unit/drivers/modules/drac/test_utils.py b/ironic/tests/unit/drivers/modules/drac/test_utils.py
new file mode 100644
index 000000000..d66457550
--- /dev/null
+++ b/ironic/tests/unit/drivers/modules/drac/test_utils.py
@@ -0,0 +1,103 @@
+# Copyright (c) 2021 Dell Inc. or its subsidiaries.
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+from unittest import mock
+
+from oslo_utils import importutils
+
+from ironic.common import exception
+from ironic.conductor import task_manager
+from ironic.drivers.modules.drac import utils as drac_utils
+from ironic.drivers.modules.redfish import utils as redfish_utils
+from ironic.tests.unit.drivers.modules.drac import utils as test_utils
+from ironic.tests.unit.objects import utils as obj_utils
+
+sushy = importutils.try_import('sushy')
+
+INFO_DICT = test_utils.INFO_DICT
+
+
+@mock.patch.object(redfish_utils, 'get_system', autospec=True)
+class DracUtilsOemManagerTestCase(test_utils.BaseDracTest):
+
+    def setUp(self):
+        super(DracUtilsOemManagerTestCase, self).setUp()
+        self.node = obj_utils.create_test_node(self.context,
+                                               driver='idrac',
+                                               driver_info=INFO_DICT)
+        self.config(enabled_hardware_types=['idrac'],
+                    enabled_management_interfaces=['idrac-redfish'])
+
+    def test_execute_oem_manager_method(self, mock_get_system):
+        fake_manager_oem = mock.Mock()
+        fake_manager_oem.test_method.return_value = 42
+        fake_manager = mock.Mock()
+        fake_manager.get_oem_extension.return_value = fake_manager_oem
+        mock_get_system.return_value.managers = [fake_manager]
+
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=False) as task:
+            result = drac_utils.execute_oem_manager_method(
+                task, 'test method', lambda m: m.test_method())
+
+            self.assertEqual(42, result)
+
+    def test_execute_oem_manager_method_no_managers(self, mock_get_system):
+        mock_get_system.return_value.managers = []
+
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=False) as task:
+            self.assertRaises(
+                exception.RedfishError,
+                drac_utils.execute_oem_manager_method,
+                task,
+                'test method',
+                lambda m: m.test_method())
+
+    def test_execute_oem_manager_method_oem_not_found(self, mock_get_system):
+        fake_manager = mock.Mock()
+        fake_manager.get_oem_extension.side_effect = (
+            sushy.exceptions.OEMExtensionNotFoundError)
+        mock_get_system.return_value.managers = [fake_manager]
+
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=False) as task:
+            self.assertRaises(
+                exception.RedfishError,
+                drac_utils.execute_oem_manager_method,
+                task,
+                'test method',
+                lambda m: m.test_method())
+
+    def test_execute_oem_manager_method_managers_fail(self, mock_get_system):
+        fake_manager_oem1 = mock.Mock()
+        fake_manager_oem1.test_method.side_effect = (
+            sushy.exceptions.SushyError)
+        fake_manager1 = mock.Mock()
+        fake_manager1.get_oem_extension.return_value = fake_manager_oem1
+        fake_manager_oem2 = mock.Mock()
+        fake_manager_oem2.test_method.side_effect = (
+            sushy.exceptions.SushyError)
+        fake_manager2 = mock.Mock()
+        fake_manager2.get_oem_extension.return_value = fake_manager_oem2
+        mock_get_system.return_value.managers = [fake_manager1, fake_manager2]
+
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=False) as task:
+            self.assertRaises(
+                exception.RedfishError,
+                drac_utils.execute_oem_manager_method,
+                task,
+                'test method',
+                lambda m: m.test_method())