From 0c9447d53b092c1b057998d9a159fbe88a3b881e Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Tue, 16 Jul 2019 21:00:27 -0500 Subject: [PATCH] Active node introspection for nodes not in cache When an active node introspection is performed where an entry already exists in ironic, but inspector has never seen the node before, we enter a state where we should allow introspection to occur, but that we don't have a cache record. As a result of no cache entry, we presently fail hard claiming the node's port already exists. In the case of active nodes, This will always be the case, so we need to add a cache entry and allow the process to... somewhat continue as if normal introspeciton is occuring. Co-Authored-By: Dmitry Tantsur Change-Id: Ib20b4a4e4d46ba23b8a4d87791cf30a957f1f9f9 Story: 2006233 Task: 35834 --- ironic_inspector/common/ironic.py | 82 +++++++++++++++++++ ironic_inspector/node_cache.py | 40 ++++++++- ironic_inspector/plugins/discovery.py | 25 ++---- ironic_inspector/process.py | 19 ++++- ironic_inspector/test/functional.py | 40 +++++++++ .../test/unit/test_common_ironic.py | 82 +++++++++++++++++++ ironic_inspector/test/unit/test_node_cache.py | 34 ++++++++ ironic_inspector/test/unit/test_process.py | 53 +++++++++++- ...ve-node-not-in-cache-b2d7b77603f02a66.yaml | 5 ++ 9 files changed, 353 insertions(+), 27 deletions(-) create mode 100644 releasenotes/notes/active-node-not-in-cache-b2d7b77603f02a66.yaml diff --git a/ironic_inspector/common/ironic.py b/ironic_inspector/common/ironic.py index c8ba563c3..658058b10 100644 --- a/ironic_inspector/common/ironic.py +++ b/ironic_inspector/common/ironic.py @@ -206,3 +206,85 @@ def call_with_retries(func, *args, **kwargs): the func raises again, the exception is propagated to the caller. """ return func(*args, **kwargs) + + +def lookup_node_by_macs(macs, introspection_data=None, + ironic=None, fail=False): + """Find a node by its MACs.""" + if ironic is None: + ironic = get_client() + + nodes = set() + for mac in macs: + ports = ironic.port.list(address=mac) + if not ports: + continue + elif fail: + raise utils.Error( + _('Port %(mac)s already exists, uuid: %(uuid)s') % + {'mac': mac, 'uuid': ports[0].uuid}, data=introspection_data) + else: + nodes.update(p.node_uuid for p in ports) + + if len(nodes) > 1: + raise utils.Error(_('MAC addresses %(macs)s correspond to more than ' + 'one node: %(nodes)s') % + {'macs': ', '.join(macs), + 'nodes': ', '.join(nodes)}, + data=introspection_data) + elif nodes: + return nodes.pop() + + +def lookup_node_by_bmc_addresses(addresses, introspection_data=None, + ironic=None, fail=False): + """Find a node by its BMC address.""" + if ironic is None: + ironic = get_client() + + # FIXME(aarefiev): it's not effective to fetch all nodes, and may + # impact on performance on big clusters + nodes = ironic.node.list(fields=('uuid', 'driver_info'), limit=0) + found = set() + for node in nodes: + bmc_address, bmc_ipv4, bmc_ipv6 = get_ipmi_address(node) + for addr in addresses: + if addr not in (bmc_ipv4, bmc_ipv6): + continue + elif fail: + raise utils.Error( + _('Node %(uuid)s already has BMC address %(addr)s') % + {'addr': addr, 'uuid': node.uuid}, + data=introspection_data) + else: + found.add(node.uuid) + + if len(found) > 1: + raise utils.Error(_('BMC addresses %(addr)s correspond to more than ' + 'one node: %(nodes)s') % + {'addr': ', '.join(addresses), + 'nodes': ', '.join(found)}, + data=introspection_data) + elif found: + return found.pop() + + +def lookup_node(macs=None, bmc_addresses=None, introspection_data=None, + ironic=None): + """Lookup a node in the ironic database.""" + node = node2 = None + + if macs: + node = lookup_node_by_macs(macs, ironic=ironic) + if bmc_addresses: + node2 = lookup_node_by_bmc_addresses(bmc_addresses, ironic=ironic) + + if node and node2 and node != node2: + raise utils.Error(_('MAC addresses %(mac)s and BMC addresses %(addr)s ' + 'correspond to different nodes: %(node1)s and ' + '%(node2)s') % + {'mac': ', '.join(macs), + 'addr': ', '.join(bmc_addresses), + 'node1': node, 'node2': node2}) + + return node or node2 diff --git a/ironic_inspector/node_cache.py b/ironic_inspector/node_cache.py index a6ffbb824..965716b8d 100644 --- a/ironic_inspector/node_cache.py +++ b/ironic_inspector/node_cache.py @@ -911,7 +911,7 @@ def create_node(driver, ironic=None, **attributes): * Sets node_info state to enrolling. :param driver: driver for Ironic node. - :param ironic: ronic client instance. + :param ironic: ironic client instance. :param attributes: dict, additional keyword arguments to pass to the ironic client on node creation. :return: NodeInfo, or None in case error happened. @@ -927,6 +927,44 @@ def create_node(driver, ironic=None, **attributes): return add_node(node.uuid, istate.States.enrolling, ironic=ironic) +def record_node(ironic=None, bmc_addresses=None, macs=None): + """Create a cache record for a known active node. + + :param ironic: ironic client instance. + :param bmc_addresses: list of BMC addresses. + :param macs: list of MAC addresses. + :return: NodeInfo + """ + if not bmc_addresses and not macs: + raise utils.NotFoundInCacheError( + _("Existing node cannot be found since neither MAC addresses " + "nor BMC addresses are present in the inventory")) + + if ironic is None: + ironic = ir_utils.get_client() + + node = ir_utils.lookup_node(macs=macs, bmc_addresses=bmc_addresses, + ironic=ironic) + if not node: + bmc_addresses = ', '.join(bmc_addresses) if bmc_addresses else None + macs = ', '.join(macs) if macs else None + raise utils.NotFoundInCacheError( + _("Existing node was not found by MAC address(es) %(macs)s " + "and BMC address(es) %(addr)s") % + {'macs': macs, 'addr': bmc_addresses}) + + node = ironic.node.get(node, fields=['uuid', 'provision_state']) + # TODO(dtantsur): do we want to allow updates in all states? + if node.provision_state not in ir_utils.VALID_ACTIVE_STATES: + raise utils.Error(_("Node %(node)s is not active, its provision " + "state is %(state)s") % + {'node': node.uuid, + 'state': node.provision_state}) + + return add_node(node.uuid, istate.States.waiting, + manage_boot=False, mac=macs, bmc_address=bmc_addresses) + + def get_node_list(ironic=None, marker=None, limit=None): """Get node list from the cache. diff --git a/ironic_inspector/plugins/discovery.py b/ironic_inspector/plugins/discovery.py index 71c72d406..20f98b1ab 100644 --- a/ironic_inspector/plugins/discovery.py +++ b/ironic_inspector/plugins/discovery.py @@ -15,7 +15,6 @@ from oslo_config import cfg -from ironic_inspector.common.i18n import _ from ironic_inspector.common import ironic as ir_utils from ironic_inspector import node_cache from ironic_inspector import utils @@ -47,14 +46,8 @@ def _extract_node_driver_info(introspection_data): def _check_existing_nodes(introspection_data, node_driver_info, ironic): macs = utils.get_valid_macs(introspection_data) if macs: - # verify existing ports - for mac in macs: - ports = ironic.port.list(address=mac) - if not ports: - continue - raise utils.Error( - _('Port %(mac)s already exists, uuid: %(uuid)s') % - {'mac': mac, 'uuid': ports[0].uuid}, data=introspection_data) + ir_utils.lookup_node_by_macs(macs, introspection_data, ironic=ironic, + fail=True) else: LOG.warning('No suitable interfaces found for discovered node. ' 'Check that validate_interfaces hook is listed in ' @@ -63,17 +56,9 @@ def _check_existing_nodes(introspection_data, node_driver_info, ironic): # verify existing node with discovered ipmi address ipmi_address = node_driver_info.get('ipmi_address') if ipmi_address: - # FIXME(aarefiev): it's not effective to fetch all nodes, and may - # impact on performance on big clusters - nodes = ironic.node.list(fields=('uuid', 'driver_info'), limit=0) - for node in nodes: - bmc_address, bmc_ipv4, bmc_ipv6 = ir_utils.get_ipmi_address(node) - if ipmi_address in (bmc_ipv4, bmc_ipv6): - raise utils.Error( - _('Node %(uuid)s already has BMC address ' - '%(ipmi_address)s, not enrolling') % - {'ipmi_address': ipmi_address, 'uuid': node.uuid}, - data=introspection_data) + ir_utils.lookup_node_by_bmc_addresses([ipmi_address], + introspection_data, + ironic=ironic, fail=True) def enroll_node_not_found_hook(introspection_data, **kwargs): diff --git a/ironic_inspector/process.py b/ironic_inspector/process.py index cabd31f03..71522d03d 100644 --- a/ironic_inspector/process.py +++ b/ironic_inspector/process.py @@ -81,10 +81,23 @@ def _find_node_info(introspection_data, failures): try: address = utils.get_ipmi_address_from_data(introspection_data) v6address = utils.get_ipmi_v6address_from_data(introspection_data) - return node_cache.find_node( - bmc_address=[address, v6address], - mac=utils.get_valid_macs(introspection_data)) + bmc_addresses = list(filter(None, [address, v6address])) + macs = utils.get_valid_macs(introspection_data) + return node_cache.find_node(bmc_address=bmc_addresses, + mac=macs) except utils.NotFoundInCacheError as exc: + if CONF.processing.permit_active_introspection: + try: + return node_cache.record_node(bmc_addresses=bmc_addresses, + macs=macs) + except utils.NotFoundInCacheError: + LOG.debug( + 'Active nodes introspection is enabled, but no node ' + 'was found for MAC(s) %(mac)s and BMC address(es) ' + '%(addr)s; proceeding with discovery', + {'mac': ', '.join(macs) if macs else None, + 'addr': ', '.join(filter(None, bmc_addresses)) or None}) + not_found_hook = plugins_base.node_not_found_hook_manager() if not_found_hook is None: failures.append(_('Look up error: %s') % exc) diff --git a/ironic_inspector/test/functional.py b/ironic_inspector/test/functional.py index ca18f592e..7243b5b10 100644 --- a/ironic_inspector/test/functional.py +++ b/ironic_inspector/test/functional.py @@ -769,6 +769,46 @@ class Test(Base): self.assertEqual(expected_port_id, lldp_out['lldp_processed']['switch_port_id']) + def test_update_unknown_active_node(self): + cfg.CONF.set_override('permit_active_introspection', True, + 'processing') + self.node.provision_state = 'active' + self.cli.node.list_ports.return_value = [ + mock.Mock(address='11:22:33:44:55:66', node_uuid=self.node.uuid) + ] + + # NOTE(dtantsur): we're not starting introspection in this test. + res = self.call_continue(self.data) + self.assertEqual({'uuid': self.uuid}, res) + eventlet.greenthread.sleep(DEFAULT_SLEEP) + + self.cli.node.update.assert_called_with(self.uuid, mock.ANY) + self.assertCalledWithPatch(self.patch, self.cli.node.update) + self.assertFalse(self.cli.port.create.called) + self.assertFalse(self.cli.node.set_boot_device.called) + + status = self.call_get_status(self.uuid) + self.check_status(status, finished=True, state=istate.States.finished) + + def test_update_known_active_node(self): + # Start with a normal introspection as a pre-requisite + self.test_bmc() + + self.cli.node.update.reset_mock() + self.cli.node.set_boot_device.reset_mock() + self.cli.port.create.reset_mock() + # Provide some updates + self.data['inventory']['memory']['physical_mb'] = 16384 + self.patch = [ + {'op': 'add', 'path': '/properties/cpus', 'value': '4'}, + {'path': '/properties/cpu_arch', 'value': 'x86_64', 'op': 'add'}, + {'op': 'add', 'path': '/properties/memory_mb', 'value': '16384'}, + {'path': '/properties/local_gb', 'value': '999', 'op': 'add'} + ] + + # Then continue with active node test + self.test_update_unknown_active_node() + @contextlib.contextmanager def mocked_server(): diff --git a/ironic_inspector/test/unit/test_common_ironic.py b/ironic_inspector/test/unit/test_common_ironic.py index c93530dd1..a20c22218 100644 --- a/ironic_inspector/test/unit/test_common_ironic.py +++ b/ironic_inspector/test/unit/test_common_ironic.py @@ -194,3 +194,85 @@ class TestCallWithRetries(unittest.TestCase): self.call, 'meow', answer=42) self.call.assert_called_with('meow', answer=42) self.assertEqual(5, self.call.call_count) + + +class TestLookupNode(base.NodeTest): + def setUp(self): + super(TestLookupNode, self).setUp() + self.ironic = mock.Mock(spec=['node', 'port'], + node=mock.Mock(spec=['list']), + port=mock.Mock(spec=['list'])) + self.ironic.node.list.return_value = [self.node] + # Simulate only the PXE port enrolled + self.port = mock.Mock(address=self.pxe_mac, node_uuid=self.node.uuid) + self.ironic.port.list.side_effect = [ + [self.port] + ] + [[]] * (len(self.macs) - 1) + + def test_no_input_no_result(self): + self.assertIsNone(ir_utils.lookup_node()) + + def test_lookup_by_mac_only(self): + uuid = ir_utils.lookup_node(macs=self.macs, ironic=self.ironic) + self.assertEqual(self.node.uuid, uuid) + self.ironic.port.list.assert_has_calls([ + mock.call(address=mac) for mac in self.macs + ]) + + def test_lookup_by_mac_duplicates(self): + self.ironic.port.list.side_effect = [ + [self.port], + [mock.Mock(address=self.inactive_mac, node_uuid='another')] + ] + [[]] * (len(self.macs) - 1) + self.assertRaisesRegex(utils.Error, 'more than one node', + ir_utils.lookup_node, + macs=self.macs, ironic=self.ironic) + self.ironic.port.list.assert_has_calls([ + mock.call(address=mac) for mac in self.macs + ]) + + def test_lookup_by_bmc_only(self): + uuid = ir_utils.lookup_node(bmc_addresses=[self.bmc_address, + '42.42.42.42'], + ironic=self.ironic) + self.assertEqual(self.node.uuid, uuid) + self.assertEqual(1, self.ironic.node.list.call_count) + + def test_lookup_by_bmc_duplicates(self): + self.ironic.node.list.return_value = [ + self.node, + mock.Mock(uuid='another', + driver_info={'ipmi_address': '42.42.42.42'}), + ] + self.assertRaisesRegex(utils.Error, 'more than one node', + ir_utils.lookup_node, + bmc_addresses=[self.bmc_address, + '42.42.42.42'], + ironic=self.ironic) + self.assertEqual(1, self.ironic.node.list.call_count) + + def test_lookup_by_both(self): + uuid = ir_utils.lookup_node(bmc_addresses=[self.bmc_address, + self.bmc_v6address], + macs=self.macs, + ironic=self.ironic) + self.assertEqual(self.node.uuid, uuid) + self.ironic.port.list.assert_has_calls([ + mock.call(address=mac) for mac in self.macs + ]) + self.assertEqual(1, self.ironic.node.list.call_count) + + def test_lookup_by_both_duplicates(self): + self.ironic.port.list.side_effect = [ + [mock.Mock(address=self.inactive_mac, node_uuid='another')] + ] + [[]] * (len(self.macs) - 1) + self.assertRaisesRegex(utils.Error, 'correspond to different nodes', + ir_utils.lookup_node, + bmc_addresses=[self.bmc_address, + self.bmc_v6address], + macs=self.macs, + ironic=self.ironic) + self.ironic.port.list.assert_has_calls([ + mock.call(address=mac) for mac in self.macs + ]) + self.assertEqual(1, self.ironic.node.list.call_count) diff --git a/ironic_inspector/test/unit/test_node_cache.py b/ironic_inspector/test/unit/test_node_cache.py index b242aef53..8218a6a5f 100644 --- a/ironic_inspector/test/unit/test_node_cache.py +++ b/ironic_inspector/test/unit/test_node_cache.py @@ -1320,3 +1320,37 @@ class TestIntrospectionDataDbStore(test_base.NodeTest): stored_data = node_cache.get_introspection_data(self.node.uuid, False) self.assertEqual(stored_data, unproc_data) + + +@mock.patch.object(ir_utils, 'lookup_node', autospec=True) +class TestRecordNode(test_base.NodeTest): + def setUp(self): + super(TestRecordNode, self).setUp() + self.node.provision_state = 'active' + self.ironic = mock.Mock(spec=['node'], + node=mock.Mock(spec=['get'])) + self.ironic.node.get.return_value = self.node + + def test_no_lookup_data(self, mock_lookup): + self.assertRaisesRegex(utils.NotFoundInCacheError, + 'neither MAC addresses nor BMC addresses', + node_cache.record_node) + + def test_success(self, mock_lookup): + mock_lookup.return_value = self.uuid + result = node_cache.record_node(macs=self.macs, ironic=self.ironic) + self.assertIsInstance(result, node_cache.NodeInfo) + self.assertEqual(self.uuid, result.uuid) + + def test_not_found(self, mock_lookup): + mock_lookup.return_value = None + self.assertRaises(utils.NotFoundInCacheError, + node_cache.record_node, + macs=self.macs, ironic=self.ironic) + + def test_bad_provision_state(self, mock_lookup): + mock_lookup.return_value = self.uuid + self.node.provision_state = 'deploying' + self.assertRaisesRegex(utils.Error, 'is not active', + node_cache.record_node, + macs=self.macs, ironic=self.ironic) diff --git a/ironic_inspector/test/unit/test_process.py b/ironic_inspector/test/unit/test_process.py index 65963915d..18f5006f4 100644 --- a/ironic_inspector/test/unit/test_process.py +++ b/ironic_inspector/test/unit/test_process.py @@ -102,7 +102,7 @@ class TestProcess(BaseProcessTest): del self.inventory['bmc_v6address'] process.process(self.data) - self.find_mock.assert_called_once_with(bmc_address=[None, None], + self.find_mock.assert_called_once_with(bmc_address=[], mac=mock.ANY) actual_macs = self.find_mock.call_args[1]['mac'] self.assertEqual(sorted(self.all_macs), sorted(actual_macs)) @@ -115,7 +115,7 @@ class TestProcess(BaseProcessTest): self.inventory['bmc_v6address'] = '::/0' process.process(self.data) - self.find_mock.assert_called_once_with(bmc_address=[None, None], + self.find_mock.assert_called_once_with(bmc_address=[], mac=mock.ANY) actual_macs = self.find_mock.call_args[1]['mac'] self.assertEqual(sorted(self.all_macs), sorted(actual_macs)) @@ -129,7 +129,7 @@ class TestProcess(BaseProcessTest): process.process(self.data) self.find_mock.assert_called_once_with( - bmc_address=[None, self.bmc_v6address], + bmc_address=[self.bmc_v6address], mac=mock.ANY) actual_macs = self.find_mock.call_args[1]['mac'] self.assertEqual(sorted(self.all_macs), sorted(actual_macs)) @@ -145,6 +145,53 @@ class TestProcess(BaseProcessTest): self.assertFalse(self.cli.node.get.called) self.assertFalse(self.process_mock.called) + @mock.patch.object(node_cache, 'record_node', autospec=True) + def test_not_found_in_cache_active_introspection(self, mock_record): + CONF.set_override('permit_active_introspection', True, 'processing') + self.find_mock.side_effect = utils.NotFoundInCacheError('not found') + self.cli.node.get.side_effect = exceptions.NotFound('boom') + self.cache_fixture.mock.acquire_lock = mock.Mock() + self.cache_fixture.mock.uuid = '1111' + self.cache_fixture.mock.finished_at = None + self.cache_fixture.mock.node = mock.Mock() + mock_record.return_value = self.cache_fixture.mock + res = process.process(self.data) + + self.assertEqual(self.fake_result_json, res) + self.find_mock.assert_called_once_with( + bmc_address=[self.bmc_address, self.bmc_v6address], + mac=mock.ANY) + actual_macs = self.find_mock.call_args[1]['mac'] + self.assertEqual(sorted(self.all_macs), sorted(actual_macs)) + mock_record.assert_called_once_with( + bmc_addresses=['1.2.3.4', + '2001:1234:1234:1234:1234:1234:1234:1234/64'], + macs=mock.ANY) + actual_macs = mock_record.call_args[1]['macs'] + self.assertEqual(sorted(self.all_macs), sorted(actual_macs)) + self.cli.node.get.assert_not_called() + self.process_mock.assert_called_once_with( + mock.ANY, mock.ANY, self.data) + + def test_found_in_cache_active_introspection(self): + CONF.set_override('permit_active_introspection', True, 'processing') + self.node.provision_state = 'active' + self.cache_fixture.mock.acquire_lock = mock.Mock() + self.cache_fixture.mock.uuid = '1111' + self.cache_fixture.mock.finished_at = None + self.cache_fixture.mock.node = mock.Mock() + res = process.process(self.data) + + self.assertEqual(self.fake_result_json, res) + self.find_mock.assert_called_once_with( + bmc_address=[self.bmc_address, self.bmc_v6address], + mac=mock.ANY) + actual_macs = self.find_mock.call_args[1]['mac'] + self.assertEqual(sorted(self.all_macs), sorted(actual_macs)) + self.cli.node.get.assert_called_once_with(self.uuid) + self.process_mock.assert_called_once_with( + mock.ANY, mock.ANY, self.data) + def test_not_found_in_ironic(self): self.cli.node.get.side_effect = exceptions.NotFound() diff --git a/releasenotes/notes/active-node-not-in-cache-b2d7b77603f02a66.yaml b/releasenotes/notes/active-node-not-in-cache-b2d7b77603f02a66.yaml new file mode 100644 index 000000000..380f9d4ac --- /dev/null +++ b/releasenotes/notes/active-node-not-in-cache-b2d7b77603f02a66.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Corrects introspection of active nodes that are not in the lookup cache, + see `story 2006233 `_.