From 4af672b8493a3580b4ff94d0416da7f839036231 Mon Sep 17 00:00:00 2001 From: Sam Betts Date: Wed, 25 May 2016 15:19:56 +0100 Subject: [PATCH] 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 | 6 + ironic_inspector/node_cache.py | 52 ++++--- .../plugins/local_link_connection.py | 122 ++++++++++++++++ ironic_inspector/test/base.py | 6 +- .../test_plugins_local_link_connection.py | 138 ++++++++++++++++++ .../test/unit/test_plugins_rules.py | 7 +- .../add-lldp-plugin-4645596cb8b39fd3.yaml | 5 + setup.cfg | 1 + 8 files changed, 313 insertions(+), 24 deletions(-) create mode 100644 ironic_inspector/plugins/local_link_connection.py create mode 100644 ironic_inspector/test/unit/test_plugins_local_link_connection.py create mode 100644 releasenotes/notes/add-lldp-plugin-4645596cb8b39fd3.yaml diff --git a/doc/source/usage.rst b/doc/source/usage.rst index 01bbe4fba..790e2f479 100644 --- a/doc/source/usage.rst +++ b/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. diff --git a/ironic_inspector/node_cache.py b/ironic_inspector/node_cache.py index 2b1737597..fe09301ec 100644 --- a/ironic_inspector/node_cache.py +++ b/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): diff --git a/ironic_inspector/plugins/local_link_connection.py b/ironic_inspector/plugins/local_link_connection.py new file mode 100644 index 000000000..3f5480ddc --- /dev/null +++ b/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 diff --git a/ironic_inspector/test/base.py b/ironic_inspector/test/base.py index 9a6bf7a0e..a6c77e1b5 100644 --- a/ironic_inspector/test/base.py +++ b/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) diff --git a/ironic_inspector/test/unit/test_plugins_local_link_connection.py b/ironic_inspector/test/unit/test_plugins_local_link_connection.py new file mode 100644 index 000000000..759c4a7b0 --- /dev/null +++ b/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) diff --git a/ironic_inspector/test/unit/test_plugins_rules.py b/ironic_inspector/test/unit/test_plugins_rules.py index b9f94f49c..71b9c3d43 100644 --- a/ironic_inspector/test/unit/test_plugins_rules.py +++ b/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): diff --git a/releasenotes/notes/add-lldp-plugin-4645596cb8b39fd3.yaml b/releasenotes/notes/add-lldp-plugin-4645596cb8b39fd3.yaml new file mode 100644 index 000000000..eecd28199 --- /dev/null +++ b/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. diff --git a/setup.cfg b/setup.cfg index 2557d1efb..6d64b7226 100644 --- a/setup.cfg +++ b/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