From b638c70f82e56dbd5a3a86febd42db04e0cdb6fc Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Fri, 18 Mar 2016 10:03:57 +0100 Subject: [PATCH] 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 | 2 +- ironic_inspector/plugins/standard.py | 28 +++++++++---------- ironic_inspector/process.py | 2 +- .../test/test_plugins_discovery.py | 5 +++- ironic_inspector/test/test_process.py | 10 +++++-- ironic_inspector/utils.py | 7 +++++ .../lookup-all-macs-eead528c0b764ad7.yaml | 6 ++++ 7 files changed, 40 insertions(+), 20 deletions(-) create mode 100644 releasenotes/notes/lookup-all-macs-eead528c0b764ad7.yaml diff --git a/ironic_inspector/plugins/discovery.py b/ironic_inspector/plugins/discovery.py index 4de8079e4..2ea441b0a 100644 --- a/ironic_inspector/plugins/discovery.py +++ b/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: diff --git a/ironic_inspector/plugins/standard.py b/ironic_inspector/plugins/standard.py index 2fd714a89..5cf7d66ce 100644 --- a/ironic_inspector/plugins/standard.py +++ b/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 diff --git a/ironic_inspector/process.py b/ironic_inspector/process.py index 05b248b6d..b3a7199fe 100644 --- a/ironic_inspector/process.py +++ b/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: diff --git a/ironic_inspector/test/test_plugins_discovery.py b/ironic_inspector/test/test_plugins_discovery.py index 0c25aea5f..7acb38e90 100644 --- a/ironic_inspector/test/test_plugins_discovery.py +++ b/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, diff --git a/ironic_inspector/test/test_process.py b/ironic_inspector/test/test_process.py index 8765fda61..021a13454 100644 --- a/ironic_inspector/test/test_process.py +++ b/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) diff --git a/ironic_inspector/utils.py b/ironic_inspector/utils.py index d46bef9ad..bf9da0f57 100644 --- a/ironic_inspector/utils.py +++ b/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')] diff --git a/releasenotes/notes/lookup-all-macs-eead528c0b764ad7.yaml b/releasenotes/notes/lookup-all-macs-eead528c0b764ad7.yaml new file mode 100644 index 000000000..eec9db31e --- /dev/null +++ b/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).