Merge "Active node introspection for nodes not in cache"

ironic_inspector/common/ironic.py

@@ -207,3 +207,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

@@ -727,6 +727,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>`_.