Fix RedfishManagement.get_mac_addresses and related functions

RedfishManagement.get_mac_addresses has two problems:
* It returns a dict while the base class documents returning a list
* The dict's values are Redfish-specific values

This change fixes this function to return a list and significantly
simplifies the related create_ports_if_not_exists.

Change-Id: I329cabe04662d0d668d4c3e04ecede5b4fdec6c6

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

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",

ironic/drivers/modules/inspect_utils.py

@@ -21,8 +21,7 @@ from ironic import objects
 LOG = logging.getLogger(__name__)
 
 
-def create_ports_if_not_exist(
+def create_ports_if_not_exist(task, macs):
-        task, macs, get_mac_address=lambda x: x[1]):
     """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 macs: A sequence of MAC addresses.
-    :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.
     """
     node = task.node
-    for k_v_pair in macs.items():
+    for mac in macs:
-        mac = get_mac_address(k_v_pair)
         port_dict = {'address': mac, 'node_id': node.id}
         port = objects.Port(task.context, **port_dict)
 

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(
+                inspect_utils.create_ports_if_not_exist(task, enabled_macs)
-                    task, enabled_macs, get_mac_address=lambda x: x[0])
             else:
                 LOG.warning("Not attempting to create any port as no NICs "
                             "were discovered in 'enabled' state for node "

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(
+            inspect_utils.create_ports_if_not_exist(task, list(enabled_macs))
-                task, enabled_macs, get_mac_address=lambda x: x[0])
         else:
             LOG.warning("Not attempting to create any port as no NICs "
                         "were discovered in 'enabled' state for node "

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
+        :returns: A list of MAC addresses for the node
-                  in a {'mac': 'state'} format
         """
         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')

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 "
+    if not enabled_macs:
-              "for node %(node)s", {'node': task.node.uuid})
+        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):

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):

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):

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):

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))

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,
+        macs = {'aa:bb:cc:dd:ee:ff', 'aa:bb:aa:aa:aa:aa'}
-                'aa:bb:aa:aa:aa:aa': sushy.STATE_DISABLED}
         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(
+            utils.create_ports_if_not_exist(task, macs)
-                task, macs, get_mac_address=lambda x: x[0])
             self.assertTrue(log_info_mock.called)
             expected_calls = [mock.call(task.context, **port_dict1),
                               mock.call(task.context, **port_dict2)]