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 <dtantsur@redhat.com>
Change-Id: Ib20b4a4e4d46ba23b8a4d87791cf30a957f1f9f9
Story: 2006233
Task: 35834

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

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.
 

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

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)

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

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)

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)

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

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 <https://storyboard.openstack.org/#!/story/2006233>`_.