Add GenericLocalLinkConnectionHook processing hook

For Ironic multi-tenant networking support, we need to be able to
discover the nodes local connectivity. To do this IPA can try to pull
LLDP packets for every NIC.

This patch adds a processing hook to handle the data from these packets
in ironic-inspector so that we can populate the correct fields fields in
Ironic. The generic lldp hook only handles the mandatory fields port id
and chassis id, set on port_id and switch_id in local_link_connection.

Further LLDP fields should be handled by additional vendor specific LLDP
processing hooks, that populate the switch_info field in a non-generic
way.

Change-Id: I884eaaa9cc54cd08c21147da438b1dabc10d3a40
Related-Bug: #1526403
Depends-On: Ie655fd59b06de7b84fba3b438d5e4c2ecd8075c3
Depends-On: Idae9b1ede1797029da1bd521501b121957ca1f1a

doc/source/usage.rst

@@ -214,6 +214,12 @@ Here are some plugins that can be additionally enabled:
     then the new format will be stored in the 'extra' key. The 'data' key is
     then deleted from the introspection data, as unless converted it's assumed
     unusable by introspection rules.
+``local_link_connection``
+    Processes LLDP data returned from inspection specifically looking for the
+    port ID and chassis ID, if found it configures the local link connection
+    information on the nodes Ironic ports with that data. To enable LLDP in the
+    inventory from IPA ``ipa-collect-lldp=1`` should be passed as a kernel
+    parameter to the IPA ramdisk.
 
 Refer to :ref:`contributing_link` for information on how to write your
 own plugin.

ironic_inspector/node_cache.py

@@ -216,25 +216,26 @@ class NodeInfo(object):
         self._attributes = None
         self._ironic = None
 
-    def node(self):
+    def node(self, ironic=None):
         """Get Ironic node object associated with the cached node record."""
         if self._node is None:
-            self._node = ir_utils.get_node(self.uuid, ironic=self.ironic)
+            ironic = ironic or self.ironic
+            self._node = ir_utils.get_node(self.uuid, ironic=ironic)
         return self._node
 
-    def create_ports(self, macs):
+    def create_ports(self, macs, ironic=None):
         """Create one or several ports for this node.
 
         A warning is issued if port already exists on a node.
         """
         for mac in macs:
             if mac not in self.ports():
-                self._create_port(mac)
+                self._create_port(mac, ironic)
             else:
                 LOG.warning(_LW('Port %s already exists, skipping'),
                             mac, node_info=self)
 
-    def ports(self):
+    def ports(self, ironic=None):
         """Get Ironic port objects associated with the cached node record.
 
         This value is cached as well, use invalidate_cache() to clean.
@@ -242,13 +243,15 @@ class NodeInfo(object):
         :return: dict MAC -> port object
         """
         if self._ports is None:
+            ironic = ironic or self.ironic
             self._ports = {p.address: p for p in
-                           self.ironic.node.list_ports(self.uuid, limit=0)}
+                           ironic.node.list_ports(self.uuid, limit=0)}
         return self._ports
 
-    def _create_port(self, mac):
+    def _create_port(self, mac, ironic=None):
+        ironic = ironic or self.ironic
         try:
-            port = self.ironic.port.create(node_uuid=self.uuid, address=mac)
+            port = ironic.port.create(node_uuid=self.uuid, address=mac)
         except exceptions.Conflict:
             LOG.warning(_LW('Port %s already exists, skipping'),
                         mac, node_info=self)
@@ -258,14 +261,16 @@ class NodeInfo(object):
         else:
             self._ports[mac] = port
 
-    def patch(self, patches):
+    def patch(self, patches, ironic=None):
         """Apply JSON patches to a node.
 
         Refreshes cached node instance.
 
         :param patches: JSON patches to apply
+        :param ironic: Ironic client to use instead of self.ironic
         :raises: ironicclient exceptions
         """
+        ironic = ironic or self.ironic
         # NOTE(aarefiev): support path w/o ahead forward slash
         # as Ironic cli does
         for patch in patches:
@@ -273,14 +278,16 @@ class NodeInfo(object):
                 patch['path'] = '/' + patch['path']
 
         LOG.debug('Updating node with patches %s', patches, node_info=self)
-        self._node = self.ironic.node.update(self.uuid, patches)
+        self._node = ironic.node.update(self.uuid, patches)
 
-    def patch_port(self, port, patches):
+    def patch_port(self, port, patches, ironic=None):
         """Apply JSON patches to a port.
 
         :param port: port object or its MAC
         :param patches: JSON patches to apply
+        :param ironic: Ironic client to use instead of self.ironic
         """
+        ironic = ironic or self.ironic
         ports = self.ports()
         if isinstance(port, str):
             port = ports[port]
@@ -288,39 +295,45 @@ class NodeInfo(object):
         LOG.debug('Updating port %(mac)s with patches %(patches)s',
                   {'mac': port.address, 'patches': patches},
                   node_info=self)
-        new_port = self.ironic.port.update(port.uuid, patches)
+        new_port = ironic.port.update(port.uuid, patches)
         ports[port.address] = new_port
 
-    def update_properties(self, **props):
+    def update_properties(self, ironic=None, **props):
         """Update properties on a node.
 
         :param props: properties to update
+        :param ironic: Ironic client to use instead of self.ironic
         """
+        ironic = ironic or self.ironic
         patches = [{'op': 'add', 'path': '/properties/%s' % k, 'value': v}
                    for k, v in props.items()]
-        self.patch(patches)
+        self.patch(patches, ironic)
 
-    def update_capabilities(self, **caps):
+    def update_capabilities(self, ironic=None, **caps):
         """Update capabilities on a node.
 
-        :param props: capabilities to update
+        :param caps: capabilities to update
+        :param ironic: Ironic client to use instead of self.ironic
         """
         existing = ir_utils.capabilities_to_dict(
             self.node().properties.get('capabilities'))
         existing.update(caps)
         self.update_properties(
+            ironic=ironic,
             capabilities=ir_utils.dict_to_capabilities(existing))
 
-    def delete_port(self, port):
+    def delete_port(self, port, ironic=None):
         """Delete port.
 
         :param port: port object or its MAC
+        :param ironic: Ironic client to use instead of self.ironic
         """
+        ironic = ironic or self.ironic
         ports = self.ports()
         if isinstance(port, str):
             port = ports[port]
 
-        self.ironic.port.delete(port.uuid)
+        ironic.port.delete(port.uuid)
         del ports[port.address]
 
     def get_by_path(self, path):
@@ -350,6 +363,7 @@ class NodeInfo(object):
         :raises: KeyError if value is not found and default is not set
         :raises: everything that patch() may raise
         """
+        ironic = kwargs.pop("ironic", None) or self.ironic
         try:
             value = self.get_by_path(path)
             op = 'replace'
@@ -363,7 +377,7 @@ class NodeInfo(object):
         ref_value = copy.deepcopy(value)
         value = func(value)
         if value != ref_value:
-            self.patch([{'op': op, 'path': path, 'value': value}])
+            self.patch([{'op': op, 'path': path, 'value': value}], ironic)
 
 
 def add_node(uuid, **attributes):

ironic_inspector/plugins/local_link_connection.py

@@ -0,0 +1,122 @@
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+# implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Generic LLDP Processing Hook"""
+
+import binascii
+
+from ironicclient import exc as client_exc
+import netaddr
+from oslo_config import cfg
+
+from ironic_inspector.common.i18n import _LW, _LE
+from ironic_inspector.common import ironic
+from ironic_inspector.plugins import base
+from ironic_inspector import utils
+
+LOG = utils.getProcessingLogger(__name__)
+
+# NOTE(sambetts) Constants defined according to IEEE standard for LLDP
+# http://standards.ieee.org/getieee802/download/802.1AB-2009.pdf
+LLDP_TLV_TYPE_CHASSIS_ID = 1
+LLDP_TLV_TYPE_PORT_ID = 2
+PORT_ID_SUBTYPE_MAC = 3
+PORT_ID_SUBTYPE_IFNAME = 5
+PORT_ID_SUBTYPE_LOCAL = 7
+STRING_PORT_SUBTYPES = [PORT_ID_SUBTYPE_IFNAME, PORT_ID_SUBTYPE_LOCAL]
+CHASSIS_ID_SUBTYPE_MAC = 4
+
+CONF = cfg.CONF
+
+REQUIRED_IRONIC_VERSION = '1.19'
+
+
+class GenericLocalLinkConnectionHook(base.ProcessingHook):
+    """Process mandatory LLDP packet fields
+
+    Non-vendor specific LLDP packet fields processed for each NIC found for a
+    baremetal node, port ID and chassis ID. These fields if found and if valid
+    will be saved into the local link connection info port id and switch id
+    fields on the Ironic port that represents that NIC.
+    """
+
+    def _get_local_link_patch(self, tlv_type, tlv_value, port):
+        try:
+            data = bytearray(binascii.unhexlify(tlv_value))
+        except TypeError:
+            LOG.warning(_LW("TLV value for TLV type %d not in correct"
+                            "format, ensure TLV value is in "
+                            "hexidecimal format when sent to "
+                            "inspector"), tlv_type)
+            return
+
+        item = value = None
+        if tlv_type == LLDP_TLV_TYPE_PORT_ID:
+            # Check to ensure the port id is an allowed type
+            item = "port_id"
+            if data[0] in STRING_PORT_SUBTYPES:
+                value = data[1:].decode()
+            if data[0] == PORT_ID_SUBTYPE_MAC:
+                value = str(netaddr.EUI(
+                    binascii.hexlify(data[1:]).decode()))
+        elif tlv_type == LLDP_TLV_TYPE_CHASSIS_ID:
+            # Check to ensure the chassis id is the allowed type
+            if data[0] == CHASSIS_ID_SUBTYPE_MAC:
+                item = "switch_id"
+                value = str(netaddr.EUI(
+                    binascii.hexlify(data[1:]).decode()))
+
+        if item and value:
+            if (not CONF.processing.overwrite_existing and
+                    item in port.local_link_connection):
+                return
+            return {'op': 'add',
+                    'path': '/local_link_connection/%s' % item,
+                    'value': value}
+
+    def before_update(self, introspection_data, node_info, **kwargs):
+        """Process LLDP data and patch Ironic port local link connection"""
+        inventory = utils.get_inventory(introspection_data)
+
+        ironic_ports = node_info.ports()
+
+        for iface in inventory['interfaces']:
+            if iface['name'] not in introspection_data['all_interfaces']:
+                continue
+            port = ironic_ports[iface['mac_address']]
+
+            lldp_data = iface.get('lldp')
+            if lldp_data is None:
+                LOG.warning(_LW("No LLDP Data found for interface %s"), iface)
+                continue
+
+            patches = []
+            for tlv_type, tlv_value in lldp_data:
+                patch = self._get_local_link_patch(tlv_type, tlv_value, port)
+                if patch is not None:
+                    patches.append(patch)
+
+            try:
+                # NOTE(sambetts) We need a newer version of Ironic API for this
+                # transaction, so create a new ironic client and explicitly
+                # pass it into the function.
+                cli = ironic.get_client(api_version=REQUIRED_IRONIC_VERSION)
+                node_info.patch_port(iface['mac_address'], patches, ironic=cli)
+            except client_exc.NotAcceptable:
+                LOG.error(_LE("Unable to set Ironic port local link "
+                              "connection information because Ironic does not "
+                              "support the required version"))
+                # NOTE(sambetts) May as well break out out of the loop here
+                # because Ironic version is not going to change for the other
+                # interfaces.
+                break

ironic_inspector/test/base.py

@@ -71,9 +71,11 @@ class BaseTest(fixtures.TestWithFixtures):
     def assertCalledWithPatch(self, expected, mock_call):
         def _get_patch_param(call):
             try:
-                return call[0][1]
+                if isinstance(call[0][1], list):
+                    return call[0][1]
             except IndexError:
-                return call[0][0]
+                pass
+            return call[0][0]
 
         actual = sum(map(_get_patch_param, mock_call.call_args_list), [])
         self.assertPatchEqual(actual, expected)

ironic_inspector/test/unit/test_plugins_local_link_connection.py

@@ -0,0 +1,138 @@
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+# implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import mock
+
+from oslo_config import cfg
+
+from ironic_inspector import node_cache
+from ironic_inspector.plugins import local_link_connection
+from ironic_inspector.test import base as test_base
+from ironic_inspector import utils
+
+
+class TestGenericLocalLinkConnectionHook(test_base.NodeTest):
+    hook = local_link_connection.GenericLocalLinkConnectionHook()
+
+    def setUp(self):
+        super(TestGenericLocalLinkConnectionHook, self).setUp()
+        self.data = {
+            'inventory': {
+                'interfaces': [{
+                    'name': 'em1', 'mac_address': '11:11:11:11:11:11',
+                    'ipv4_address': '1.1.1.1',
+                    'lldp': [
+                        (0, ''),
+                        (1, '04885a92ec5459'),
+                        (2, '0545746865726e6574312f3138'),
+                        (3, '0078')]
+                }],
+                'cpu': 1,
+                'disks': 1,
+                'memory': 1
+            },
+            'all_interfaces': {
+                'em1': {},
+            }
+        }
+
+        llc = {
+            'port_id': '56'
+        }
+
+        ports = [mock.Mock(spec=['address', 'uuid', 'local_link_connection'],
+                           address=a, local_link_connection=llc)
+                 for a in ('11:11:11:11:11:11',)]
+        self.node_info = node_cache.NodeInfo(uuid=self.uuid, started_at=0,
+                                             node=self.node, ports=ports)
+
+    @mock.patch.object(node_cache.NodeInfo, 'patch_port')
+    def test_expected_data(self, mock_patch):
+        patches = [
+            {'path': '/local_link_connection/port_id',
+             'value': 'Ethernet1/18', 'op': 'add'},
+            {'path': '/local_link_connection/switch_id',
+             'value': '88-5A-92-EC-54-59', 'op': 'add'},
+        ]
+        self.hook.before_update(self.data, self.node_info)
+        self.assertCalledWithPatch(patches, mock_patch)
+
+    @mock.patch.object(node_cache.NodeInfo, 'patch_port')
+    def test_invalid_chassis_id_subtype(self, mock_patch):
+        # First byte of TLV value is processed to calculate the subtype for the
+        # chassis ID, Subtype 5 ('05...') isn't a subtype supported by this
+        # plugin, so we expect it to skip this TLV.
+        self.data['inventory']['interfaces'][0]['lldp'][1] = (
+            1, '05885a92ec5459')
+        patches = [
+            {'path': '/local_link_connection/port_id',
+             'value': 'Ethernet1/18', 'op': 'add'},
+        ]
+        self.hook.before_update(self.data, self.node_info)
+        self.assertCalledWithPatch(patches, mock_patch)
+
+    @mock.patch.object(node_cache.NodeInfo, 'patch_port')
+    def test_invalid_port_id_subtype(self, mock_patch):
+        # First byte of TLV value is processed to calculate the subtype for the
+        # port ID, Subtype 6 ('06...') isn't a subtype supported by this
+        # plugin, so we expect it to skip this TLV.
+        self.data['inventory']['interfaces'][0]['lldp'][2] = (
+            2, '0645746865726e6574312f3138')
+        patches = [
+            {'path': '/local_link_connection/switch_id',
+             'value': '88-5A-92-EC-54-59', 'op': 'add'}
+        ]
+        self.hook.before_update(self.data, self.node_info)
+        self.assertCalledWithPatch(patches, mock_patch)
+
+    @mock.patch.object(node_cache.NodeInfo, 'patch_port')
+    def test_port_id_subtype_mac(self, mock_patch):
+        self.data['inventory']['interfaces'][0]['lldp'][2] = (
+            2, '03885a92ec5458')
+        patches = [
+            {'path': '/local_link_connection/port_id',
+             'value': '88-5A-92-EC-54-58', 'op': 'add'},
+            {'path': '/local_link_connection/switch_id',
+             'value': '88-5A-92-EC-54-59', 'op': 'add'}
+        ]
+        self.hook.before_update(self.data, self.node_info)
+        self.assertCalledWithPatch(patches, mock_patch)
+
+    @mock.patch.object(node_cache.NodeInfo, 'patch_port')
+    def test_lldp_none(self, mock_patch):
+        self.data['inventory']['interfaces'][0]['lldp'] = None
+        patches = []
+        self.hook.before_update(self.data, self.node_info)
+        self.assertCalledWithPatch(patches, mock_patch)
+
+    @mock.patch.object(node_cache.NodeInfo, 'patch_port')
+    def test_interface_not_in_all_interfaces(self, mock_patch):
+        self.data['all_interfaces'] = {}
+        patches = []
+        self.hook.before_update(self.data, self.node_info)
+        self.assertCalledWithPatch(patches, mock_patch)
+
+    def test_no_inventory(self):
+        del self.data['inventory']
+        self.assertRaises(utils.Error, self.hook.before_update,
+                          self.data, self.node_info)
+
+    @mock.patch.object(node_cache.NodeInfo, 'patch_port')
+    def test_no_overwrite(self, mock_patch):
+        cfg.CONF.set_override('overwrite_existing', False, group='processing')
+        patches = [
+            {'path': '/local_link_connection/switch_id',
+             'value': '88-5A-92-EC-54-59', 'op': 'add'}
+        ]
+        self.hook.before_update(self.data, self.node_info)
+        self.assertCalledWithPatch(patches, mock_patch)

ironic_inspector/test/unit/test_plugins_rules.py

@@ -179,7 +179,7 @@ class TestSetCapabilityAction(test_base.NodeTest):
         self.act.apply(self.node_info, self.params)
         mock_patch.assert_called_once_with(
             [{'op': 'add', 'path': '/properties/capabilities',
-              'value': 'cap1:val'}])
+              'value': 'cap1:val'}], mock.ANY)
 
     @mock.patch.object(node_cache.NodeInfo, 'patch')
     def test_apply_with_existing(self, mock_patch):
@@ -203,7 +203,7 @@ class TestExtendAttributeAction(test_base.NodeTest):
     def test_apply(self, mock_patch):
         self.act.apply(self.node_info, self.params)
         mock_patch.assert_called_once_with(
-            [{'op': 'add', 'path': '/extra/value', 'value': [42]}])
+            [{'op': 'add', 'path': '/extra/value', 'value': [42]}], mock.ANY)
 
     @mock.patch.object(node_cache.NodeInfo, 'patch')
     def test_apply_non_empty(self, mock_patch):
@@ -211,7 +211,8 @@ class TestExtendAttributeAction(test_base.NodeTest):
         self.act.apply(self.node_info, self.params)
 
         mock_patch.assert_called_once_with(
-            [{'op': 'replace', 'path': '/extra/value', 'value': [0, 42]}])
+            [{'op': 'replace', 'path': '/extra/value', 'value': [0, 42]}],
+            mock.ANY)
 
     @mock.patch.object(node_cache.NodeInfo, 'patch')
     def test_apply_unique_with_existing(self, mock_patch):

releasenotes/notes/add-lldp-plugin-4645596cb8b39fd3.yaml

@@ -0,0 +1,5 @@
+---
+features:
+  - Added GenericLocalLinkConnectionHook processing plugin to process LLDP data
+    returned during inspection and set port ID and switch ID in an Ironic
+    node's port local link connection information using that data.

setup.cfg

@@ -32,6 +32,7 @@ ironic_inspector.hooks.processing =
     extra_hardware = ironic_inspector.plugins.extra_hardware:ExtraHardwareHook
     raid_device = ironic_inspector.plugins.raid_device:RaidDeviceDetection
     capabilities = ironic_inspector.plugins.capabilities:CapabilitiesHook
+    local_link_connection = ironic_inspector.plugins.local_link_connection:GenericLocalLinkConnectionHook
 ironic_inspector.hooks.node_not_found =
     example = ironic_inspector.plugins.example:example_not_found_hook
     enroll = ironic_inspector.plugins.discovery:enroll_node_not_found_hook