Merge "Fix RedfishManagement.get_mac_addresses and related functions"

12 files changed, 39 insertions, 42 deletions

diff --git a/ironic/drivers/modules/drac/inspect.py b/ironic/drivers/modules/drac/inspect.py
index aa2ad3dae..8ba0be088 100644
--- a/ironic/drivers/modules/drac/inspect.py
+++ b/ironic/drivers/modules/drac/inspect.py
@@ -68,7 +68,7 @@ class DracRedfishInspect(redfish_inspect.RedfishInspect):
         # versions of the firmware where the port state is not being
         # reported correctly.
 
-        ethernet_interfaces_mac = self._get_mac_address(task)
+        ethernet_interfaces_mac = list(self._get_mac_address(task).values())
         inspect_utils.create_ports_if_not_exist(task, ethernet_interfaces_mac)
         return super(DracRedfishInspect, self).inspect_hardware(task)
 
@@ -76,7 +76,7 @@ class DracRedfishInspect(redfish_inspect.RedfishInspect):
         """Get a list of MAC addresses
 
         :param task: a TaskManager instance.
-        :returns: Returns list of MAC addresses.
+        :returns: a mapping of interface identities to MAC addresses.
         """
         system = redfish_utils.get_system(task.node)
         # Get dictionary of ethernet interfaces
diff --git a/ironic/drivers/modules/ilo/inspect.py b/ironic/drivers/modules/ilo/inspect.py
index 7f79d3bcd..b279bd471 100644
--- a/ironic/drivers/modules/ilo/inspect.py
+++ b/ironic/drivers/modules/ilo/inspect.py
@@ -299,7 +299,8 @@ class IloInspect(base.InspectInterface):
         task.node.save()
 
         # Create ports for the nics detected.
-        inspect_utils.create_ports_if_not_exist(task, result['macs'])
+        inspect_utils.create_ports_if_not_exist(
+            task, list(result['macs'].values()))
 
         LOG.debug("Node properties for %(node)s are updated as "
                   "%(properties)s",
diff --git a/ironic/drivers/modules/inspect_utils.py b/ironic/drivers/modules/inspect_utils.py
index 05aae921e..911c5b402 100644
--- a/ironic/drivers/modules/inspect_utils.py
+++ b/ironic/drivers/modules/inspect_utils.py
@@ -21,8 +21,7 @@ from ironic import objects
 LOG = logging.getLogger(__name__)
 
 
-def create_ports_if_not_exist(
-        task, macs, get_mac_address=lambda x: x[1]):
+def create_ports_if_not_exist(task, macs):
     """Create ironic ports from MAC addresses data dict.
 
     Creates ironic ports from MAC addresses data returned with inspection or
@@ -31,14 +30,10 @@ def create_ports_if_not_exist(
     pair.
 
     :param task: A TaskManager instance.
-    :param macs: A dictionary of MAC addresses returned by node inspection.
-    :param get_mac_address: a function to get the MAC address from mac item.
-        A mac item is the dict key-value pair of the previous ``macs``
-        argument.
+    :param macs: A sequence of MAC addresses.
     """
     node = task.node
-    for k_v_pair in macs.items():
-        mac = get_mac_address(k_v_pair)
+    for mac in macs:
         port_dict = {'address': mac, 'node_id': node.id}
         port = objects.Port(task.context, **port_dict)
 
diff --git a/ironic/drivers/modules/inspector.py b/ironic/drivers/modules/inspector.py
index 1b866d0d5..fe27c46e5 100644
--- a/ironic/drivers/modules/inspector.py
+++ b/ironic/drivers/modules/inspector.py
@@ -246,8 +246,7 @@ class Inspector(base.InspectInterface):
         try:
             enabled_macs = task.driver.management.get_mac_addresses(task)
             if enabled_macs:
-                inspect_utils.create_ports_if_not_exist(
-                    task, enabled_macs, get_mac_address=lambda x: x[0])
+                inspect_utils.create_ports_if_not_exist(task, enabled_macs)
             else:
                 LOG.warning("Not attempting to create any port as no NICs "
                             "were discovered in 'enabled' state for node "
diff --git a/ironic/drivers/modules/redfish/inspect.py b/ironic/drivers/modules/redfish/inspect.py
index 4e968e4b3..4c5f7c344 100644
--- a/ironic/drivers/modules/redfish/inspect.py
+++ b/ironic/drivers/modules/redfish/inspect.py
@@ -189,8 +189,7 @@ class RedfishInspect(base.InspectInterface):
     def _create_ports(self, task, system):
         enabled_macs = redfish_utils.get_enabled_macs(task, system)
         if enabled_macs:
-            inspect_utils.create_ports_if_not_exist(
-                task, enabled_macs, get_mac_address=lambda x: x[0])
+            inspect_utils.create_ports_if_not_exist(task, list(enabled_macs))
         else:
             LOG.warning("Not attempting to create any port as no NICs "
                         "were discovered in 'enabled' state for node "
diff --git a/ironic/drivers/modules/redfish/management.py b/ironic/drivers/modules/redfish/management.py
index 29844fae9..5a94d1564 100644
--- a/ironic/drivers/modules/redfish/management.py
+++ b/ironic/drivers/modules/redfish/management.py
@@ -1114,12 +1114,11 @@ class RedfishManagement(base.ManagementInterface):
         :param task: A TaskManager instance containing the node to act on.
         :raises: RedfishConnectionError when it fails to connect to Redfish
         :raises: RedfishError on an error from the Sushy library
-        :returns: a dictionary containing MAC addresses of enabled interfaces
-                  in a {'mac': 'state'} format
+        :returns: A list of MAC addresses for the node
         """
         try:
             system = redfish_utils.get_system(task.node)
-            return redfish_utils.get_enabled_macs(task, system)
+            return list(redfish_utils.get_enabled_macs(task, system))
         except sushy.exceptions.SushyError as exc:
             msg = (_('Failed to get network interface information on node '
                      '%(node)s: %(exc)s')
diff --git a/ironic/drivers/modules/redfish/utils.py b/ironic/drivers/modules/redfish/utils.py
index 607de4366..a590494ae 100644
--- a/ironic/drivers/modules/redfish/utils.py
+++ b/ironic/drivers/modules/redfish/utils.py
@@ -381,7 +381,7 @@ def get_enabled_macs(task, system):
     :param task: a TaskManager instance containing the node to act on.
     :param system: a Redfish System object
     :returns: a dictionary containing MAC addresses of enabled interfaces
-        in a {'mac': 'state'} format
+        in a {'mac': <state>} format, where <state> is a sushy constant
     """
 
     enabled_macs = {}
@@ -398,11 +398,11 @@ def get_enabled_macs(task, system):
                             "reported", {'node': task.node.uuid})
                 continue
             enabled_macs[nic_mac] = nic_state
-        if enabled_macs:
-            return enabled_macs
 
-    LOG.debug("No ethernet interface information is available "
-              "for node %(node)s", {'node': task.node.uuid})
+    if not enabled_macs:
+        LOG.debug("No ethernet interface information is available "
+                  "for node %(node)s", {'node': task.node.uuid})
+    return enabled_macs
 
 
 def wait_until_get_system_ready(node):
diff --git a/ironic/tests/unit/drivers/modules/drac/test_inspect.py b/ironic/tests/unit/drivers/modules/drac/test_inspect.py
index 97dcf30f9..d12adba34 100644
--- a/ironic/tests/unit/drivers/modules/drac/test_inspect.py
+++ b/ironic/tests/unit/drivers/modules/drac/test_inspect.py
@@ -652,7 +652,7 @@ class DracRedfishInspectionTestCase(test_utils.BaseDracTest):
             return_value = task.driver.inspect.inspect_hardware(task)
             self.assertEqual(states.MANAGEABLE, return_value)
             mock_create_ports_if_not_exist.assert_called_once_with(
-                task, ethernet_interfaces_mac)
+                task, ['24:6E:96:70:49:00'])
 
     @mock.patch.object(redfish_utils, 'get_system', autospec=True)
     def test__get_mac_address_with_ethernet_interfaces(self, mock_get_system):
diff --git a/ironic/tests/unit/drivers/modules/ilo/test_inspect.py b/ironic/tests/unit/drivers/modules/ilo/test_inspect.py
index 376c0d601..1a85d5b28 100644
--- a/ironic/tests/unit/drivers/modules/ilo/test_inspect.py
+++ b/ironic/tests/unit/drivers/modules/ilo/test_inspect.py
@@ -81,7 +81,8 @@ class IloInspectTestCase(test_common.BaseIloTest):
                                                        ilo_object_mock)
             get_capabilities_mock.assert_called_once_with(task.node,
                                                           ilo_object_mock)
-            create_port_mock.assert_called_once_with(task, macs)
+            create_port_mock.assert_called_once_with(
+                task, ['aa:aa:aa:aa:aa:aa', 'bb:bb:bb:bb:bb:bb'])
 
     @mock.patch.object(ilo_inspect.LOG, 'warning',
                        spec_set=True, autospec=True)
@@ -126,7 +127,8 @@ class IloInspectTestCase(test_common.BaseIloTest):
             self.assertTrue(log_mock.called)
             get_capabilities_mock.assert_called_once_with(task.node,
                                                           ilo_object_mock)
-            create_port_mock.assert_called_once_with(task, macs)
+            create_port_mock.assert_called_once_with(
+                task, ['aa:aa:aa:aa:aa:aa', 'bb:bb:bb:bb:bb:bb'])
 
     @mock.patch.object(ilo_inspect.LOG, 'warning',
                        spec_set=True, autospec=True)
@@ -168,7 +170,8 @@ class IloInspectTestCase(test_common.BaseIloTest):
             self.assertTrue(log_mock.called)
             get_capabilities_mock.assert_called_once_with(task.node,
                                                           ilo_object_mock)
-            create_port_mock.assert_called_once_with(task, macs)
+            create_port_mock.assert_called_once_with(
+                task, ['aa:aa:aa:aa:aa:aa', 'bb:bb:bb:bb:bb:bb'])
 
     @mock.patch.object(ilo_inspect.LOG, 'warning',
                        spec_set=True, autospec=True)
@@ -217,7 +220,8 @@ class IloInspectTestCase(test_common.BaseIloTest):
             self.assertFalse(log_mock.called)
             get_capabilities_mock.assert_called_once_with(task.node,
                                                           ilo_object_mock)
-            create_port_mock.assert_called_once_with(task, macs)
+            create_port_mock.assert_called_once_with(
+                task, ['aa:aa:aa:aa:aa:aa'])
 
     @mock.patch.object(ilo_inspect, '_get_capabilities', spec_set=True,
                        autospec=True)
@@ -256,7 +260,8 @@ class IloInspectTestCase(test_common.BaseIloTest):
                                                        ilo_object_mock)
             get_capabilities_mock.assert_called_once_with(task.node,
                                                           ilo_object_mock)
-            create_port_mock.assert_called_once_with(task, macs)
+            create_port_mock.assert_called_once_with(
+                task, ['aa:aa:aa:aa:aa:aa', 'bb:bb:bb:bb:bb:bb'])
 
     @mock.patch.object(ilo_inspect, '_get_capabilities', spec_set=True,
                        autospec=True)
@@ -295,7 +300,8 @@ class IloInspectTestCase(test_common.BaseIloTest):
                                                        ilo_object_mock)
             get_capabilities_mock.assert_called_once_with(task.node,
                                                           ilo_object_mock)
-            create_port_mock.assert_called_once_with(task, macs)
+            create_port_mock.assert_called_once_with(
+                task, ['aa:aa:aa:aa:aa:aa', 'bb:bb:bb:bb:bb:bb'])
 
     @mock.patch.object(ilo_inspect, '_get_capabilities', spec_set=True,
                        autospec=True)
@@ -341,7 +347,8 @@ class IloInspectTestCase(test_common.BaseIloTest):
                                                        ilo_object_mock)
             get_capabilities_mock.assert_called_once_with(task.node,
                                                           ilo_object_mock)
-            create_port_mock.assert_called_once_with(task, macs)
+            create_port_mock.assert_called_once_with(
+                task, ['aa:aa:aa:aa:aa:aa', 'bb:bb:bb:bb:bb:bb'])
 
 
 class TestInspectPrivateMethods(test_common.BaseIloTest):
diff --git a/ironic/tests/unit/drivers/modules/redfish/test_inspect.py b/ironic/tests/unit/drivers/modules/redfish/test_inspect.py
index de084c341..352b1d4e9 100644
--- a/ironic/tests/unit/drivers/modules/redfish/test_inspect.py
+++ b/ironic/tests/unit/drivers/modules/redfish/test_inspect.py
@@ -118,7 +118,7 @@ class RedfishInspectTestCase(db_base.DbTestCase):
             task.driver.inspect.inspect_hardware(task)
             result = task.driver.management.get_mac_addresses(task)
             inspect_utils.create_ports_if_not_exist.assert_called_once_with(
-                task, result, mock.ANY)
+                task, result)
 
     @mock.patch.object(redfish_utils, 'get_system', autospec=True)
     def test_inspect_hardware_fail_missing_cpu(self, mock_get_system):
diff --git a/ironic/tests/unit/drivers/modules/redfish/test_management.py b/ironic/tests/unit/drivers/modules/redfish/test_management.py
index 3e7d1a1d4..f67efa9f0 100644
--- a/ironic/tests/unit/drivers/modules/redfish/test_management.py
+++ b/ironic/tests/unit/drivers/modules/redfish/test_management.py
@@ -1362,13 +1362,11 @@ class RedfishManagementTestCase(db_base.DbTestCase):
 
     @mock.patch.object(redfish_utils, 'get_system', autospec=True)
     def test_get_mac_addresses_success(self, mock_get_system):
-        expected_properties = {'00:11:22:33:44:55': 'enabled'}
-
         self.init_system_mock(mock_get_system.return_value)
 
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=True) as task:
-            self.assertEqual(expected_properties,
+            self.assertEqual(['00:11:22:33:44:55'],
                              task.driver.management.get_mac_addresses(task))
 
     @mock.patch.object(redfish_utils, 'get_system', autospec=True)
@@ -1378,4 +1376,5 @@ class RedfishManagementTestCase(db_base.DbTestCase):
         system_mock.ethernet_interfaces.summary = None
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=True) as task:
-            self.assertIsNone(task.driver.management.get_mac_addresses(task))
+            self.assertEqual([],
+                             task.driver.management.get_mac_addresses(task))
diff --git a/ironic/tests/unit/drivers/modules/test_inspect_utils.py b/ironic/tests/unit/drivers/modules/test_inspect_utils.py
index aa7bd79be..3c443c3fc 100644
--- a/ironic/tests/unit/drivers/modules/test_inspect_utils.py
+++ b/ironic/tests/unit/drivers/modules/test_inspect_utils.py
@@ -39,7 +39,7 @@ class InspectFunctionTestCase(db_base.DbTestCase):
     @mock.patch.object(utils.LOG, 'info', spec_set=True, autospec=True)
     @mock.patch.object(objects, 'Port', spec_set=True, autospec=True)
     def test_create_ports_if_not_exist(self, port_mock, log_mock):
-        macs = {'Port 1': 'aa:aa:aa:aa:aa:aa', 'Port 2': 'bb:bb:bb:bb:bb:bb'}
+        macs = {'aa:aa:aa:aa:aa:aa', 'bb:bb:bb:bb:bb:bb'}
         node_id = self.node.id
         port_dict1 = {'address': 'aa:aa:aa:aa:aa:aa', 'node_id': node_id}
         port_dict2 = {'address': 'bb:bb:bb:bb:bb:bb', 'node_id': node_id}
@@ -61,7 +61,7 @@ class InspectFunctionTestCase(db_base.DbTestCase):
                                                      create_mock,
                                                      log_mock):
         create_mock.side_effect = exception.MACAlreadyExists('f')
-        macs = {'Port 1': 'aa:aa:aa:aa:aa:aa', 'Port 2': 'bb:bb:bb:bb:bb:bb'}
+        macs = {'aa:aa:aa:aa:aa:aa', 'bb:bb:bb:bb:bb:bb'}
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=False) as task:
             utils.create_ports_if_not_exist(task, macs)
@@ -71,15 +71,13 @@ class InspectFunctionTestCase(db_base.DbTestCase):
     @mock.patch.object(objects, 'Port', spec_set=True, autospec=True)
     def test_create_ports_if_not_exist_attempts_port_creation_blindly(
             self, port_mock, log_info_mock):
-        macs = {'aa:bb:cc:dd:ee:ff': sushy.STATE_ENABLED,
-                'aa:bb:aa:aa:aa:aa': sushy.STATE_DISABLED}
+        macs = {'aa:bb:cc:dd:ee:ff', 'aa:bb:aa:aa:aa:aa'}
         node_id = self.node.id
         port_dict1 = {'address': 'aa:bb:cc:dd:ee:ff', 'node_id': node_id}
         port_dict2 = {'address': 'aa:bb:aa:aa:aa:aa', 'node_id': node_id}
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=False) as task:
-            utils.create_ports_if_not_exist(
-                task, macs, get_mac_address=lambda x: x[0])
+            utils.create_ports_if_not_exist(task, macs)
             self.assertTrue(log_info_mock.called)
             expected_calls = [mock.call(task.context, **port_dict1),
                               mock.call(task.context, **port_dict2)]