Use all valid MAC's for lookup

Currently we are using only the resulting MAC(s) when doing a node lookup.
In many cases it is the MAC of the PXE-booting NIC. However, it's not necessary
the MAC that people used for enrolling the Ironic node, which will lead to
lookup failures on the virtual environment. This change makes the lookup
procedure use all of the valid MAC's.

Similarly, the enroll node_not_found_hook now checks all MAC's before creating
a node.

Code in the validate_interfaces hook was reordered to ensure we only keep
interfaces with valid MAC's even in the "all_interfaces" list.

Change-Id: Ie7df05d9a7855716fb835c90cfb0ac7fc4cd66df

ironic_inspector/plugins/discovery.py

@@ -52,7 +52,7 @@ def _extract_node_driver_info(introspection_data):
 
 
 def _check_existing_nodes(introspection_data, node_driver_info, ironic):
-    macs = introspection_data.get('macs')
+    macs = utils.get_valid_macs(introspection_data)
     if macs:
         # verify existing ports
         for mac in macs:

ironic_inspector/plugins/standard.py

@@ -191,6 +191,20 @@ class ValidateInterfacesHook(base.ProcessingHook):
                               iface, data=data)
                     continue
 
+                if not mac:
+                    LOG.debug('Skipping interface %s without link information',
+                              name, data=data)
+                    continue
+
+                if not utils.is_valid_mac(mac):
+                    LOG.warning(_LW('MAC %(mac)s for interface %(name)s is '
+                                    'not valid, skipping'),
+                                {'mac': mac, 'name': name},
+                                data=data)
+                    continue
+
+                mac = mac.lower()
+
                 LOG.debug('Found interface %(name)s with MAC "%(mac)s" and '
                           'IP address "%(ip)s"',
                           {'name': name, 'mac': mac, 'ip': ip}, data=data)
@@ -223,20 +237,6 @@ class ValidateInterfacesHook(base.ProcessingHook):
             mac = iface.get('mac')
             ip = iface.get('ip')
 
-            if not mac:
-                LOG.debug('Skipping interface %s without link information',
-                          name, data=data)
-                continue
-
-            if not utils.is_valid_mac(mac):
-                LOG.warning(_LW('MAC %(mac)s for interface %(name)s is not '
-                                'valid, skipping'),
-                            {'mac': mac, 'name': name},
-                            data=data)
-                continue
-
-            mac = mac.lower()
-
             if name == 'lo' or (ip and netaddr.IPAddress(ip).is_loopback()):
                 LOG.debug('Skipping local interface %s', name, data=data)
                 continue

ironic_inspector/process.py

@@ -39,7 +39,7 @@ def _find_node_info(introspection_data, failures):
     try:
         return node_cache.find_node(
             bmc_address=introspection_data.get('ipmi_address'),
-            mac=introspection_data.get('macs'))
+            mac=utils.get_valid_macs(introspection_data))
     except utils.NotFoundInCacheError as exc:
         not_found_hook = plugins_base.node_not_found_hook_manager()
         if not_found_hook is None:

ironic_inspector/test/test_plugins_discovery.py

@@ -102,7 +102,10 @@ class TestEnrollNodeNotFoundHook(test_base.NodeTest):
     def test__check_existing_nodes_existing_mac(self):
         self.ironic.port.list.return_value = [mock.MagicMock(
             address=self.macs[0], uuid='fake_port')]
-        introspection_data = {'macs': self.macs}
+        introspection_data = {
+            'all_interfaces': {'eth%d' % i: {'mac': m}
+                               for i, m in enumerate(self.macs)}
+        }
         node_driver_info = {}
 
         self.assertRaises(utils.Error,

ironic_inspector/test/test_process.py

@@ -54,6 +54,7 @@ class BaseTest(test_base.NodeTest):
         self.all_ports = [mock.Mock(uuid=uuidutils.generate_uuid(),
                                     address=mac) for mac in self.macs]
         self.ports = [self.all_ports[1]]
+        self.all_macs = self.macs + ['DE:AD:BE:EF:DE:AD']
 
 
 @mock.patch.object(process, '_process_node', autospec=True)
@@ -90,7 +91,9 @@ class TestProcess(BaseTest):
         self.assertEqual([self.pxe_mac], self.data['macs'])
 
         pop_mock.assert_called_once_with(bmc_address=self.bmc_address,
-                                         mac=self.data['macs'])
+                                         mac=mock.ANY)
+        actual_macs = pop_mock.call_args[1]['mac']
+        self.assertEqual(sorted(self.all_macs), sorted(actual_macs))
         cli.node.get.assert_called_once_with(self.uuid)
         process_mock.assert_called_once_with(cli.node.get.return_value,
                                              self.data, pop_mock.return_value)
@@ -100,8 +103,9 @@ class TestProcess(BaseTest):
         del self.data['ipmi_address']
         process.process(self.data)
 
-        pop_mock.assert_called_once_with(bmc_address=None,
-                                         mac=self.data['macs'])
+        pop_mock.assert_called_once_with(bmc_address=None, mac=mock.ANY)
+        actual_macs = pop_mock.call_args[1]['mac']
+        self.assertEqual(sorted(self.all_macs), sorted(actual_macs))
         cli.node.get.assert_called_once_with(self.uuid)
         process_mock.assert_called_once_with(cli.node.get.return_value,
                                              self.data, pop_mock.return_value)

ironic_inspector/utils.py

@@ -198,3 +198,10 @@ def get_auth_strategy():
     if CONF.authenticate is not None:
         return 'keystone' if CONF.authenticate else 'noauth'
     return CONF.auth_strategy
+
+
+def get_valid_macs(data):
+    """Get a list of valid MAC's from the introspection data."""
+    return [m['mac']
+            for m in data.get('all_interfaces', {}).values()
+            if m.get('mac')]

releasenotes/notes/lookup-all-macs-eead528c0b764ad7.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - The lookup procedure now uses all valid MAC's, not only the MAC(s) that
+    will be used for creating port(s).
+  - The "enroll" node_not_found_hook now uses all valid MAC's to check node
+    existence, not only the MAC(s) that will be used for creating port(s).