From aa6adc13848e800a4afd55df5a58e151c632462f Mon Sep 17 00:00:00 2001 From: Danny Allen <52468448+daall@users.noreply.github.com> Date: Wed, 23 Oct 2019 10:01:48 -0700 Subject: [PATCH] [minigraph.py] Update minigraph parsing logic to include only active ports for mirror tables (#3592) (#3634) * Update minigraph.py to filter out front-panel ports that are not active * Update cfggen tests to reflect new behavior Signed-off-by: Danny Allen * Incorporate PR comments - Update t0 tests to include additional device neighbors - Refactor xml parsing logic --- src/sonic-config-engine/minigraph.py | 54 ++++++++++++++++--- .../tests/t0-sample-graph.xml | 7 +++ src/sonic-config-engine/tests/test_cfggen.py | 11 +++- 3 files changed, 64 insertions(+), 8 deletions(-) diff --git a/src/sonic-config-engine/minigraph.py b/src/sonic-config-engine/minigraph.py index 98fbd79f83b8..3042cb077c4c 100644 --- a/src/sonic-config-engine/minigraph.py +++ b/src/sonic-config-engine/minigraph.py @@ -26,6 +26,12 @@ ns2 = "Microsoft.Search.Autopilot.NetMux" ns3 = "http://www.w3.org/2001/XMLSchema-instance" +############################################################################### +# +# Minigraph parsing functions +# +############################################################################### + class minigraph_encoder(json.JSONEncoder): def default(self, obj): if isinstance(obj, ( @@ -251,15 +257,17 @@ def parse_dpg(dpg, hname): if member.lower().startswith('erspanv6'): is_mirror_v6 = True else: - is_mirror = True; - # Erspan session will be attached to all front panel ports, - # if panel ports is a member port of LAG, should add the LAG - # to acl table instead of the panel ports + is_mirror = True + # Erspan session will be attached to all front panel ports + # initially. If panel ports is a member port of LAG, then + # the LAG will be added to acl table instead of the panel + # ports. Non-active ports will be removed from this list + # later after the rest of the minigraph has been parsed. acl_intfs = pc_intfs[:] for panel_port in port_alias_map.values(): if panel_port not in intfs_inpc: acl_intfs.append(panel_port) - break; + break if acl_intfs: acls[aclname] = {'policy_desc': aclname, 'ports': acl_intfs} @@ -415,6 +423,40 @@ def parse_deviceinfo(meta, hwsku): port_speeds[port_alias_map.get(alias, alias)] = speed return port_speeds, port_descriptions +############################################################################### +# +# Post-processing functions +# +############################################################################### + +def filter_acl_mirror_table_bindings(acls, neighbors, port_channels): + """ + Filters out inactive front-panel ports from the binding list for mirror + ACL tables. We define an "active" port as one that is a member of a + port channel or one that is connected to a neighboring device. + """ + + for acl_table, group_params in acls.iteritems(): + group_type = group_params.get('type', None) + + if group_type != 'MIRROR' and group_type != 'MIRRORV6': + continue + + active_ports = [ port for port in group_params.get('ports', []) if port in neighbors.keys() or port in port_channels ] + + if not active_ports: + print >> sys.stderr, 'Warning: mirror table {} in ACL_TABLE does not have any ports bound to it'.format(acl_table) + + acls[acl_table]['ports'] = active_ports + + return acls + +############################################################################### +# +# Main functions +# +############################################################################### + def parse_xml(filename, platform=None, port_config_file=None): root = ET.parse(filename).getroot() mini_graph_path = filename @@ -627,7 +669,7 @@ def parse_xml(filename, platform=None, port_config_file=None): results['NTP_SERVER'] = dict((item, {}) for item in ntp_servers) results['TACPLUS_SERVER'] = dict((item, {'priority': '1', 'tcp_port': '49'}) for item in tacacs_servers) - results['ACL_TABLE'] = acls + results['ACL_TABLE'] = filter_acl_mirror_table_bindings(acls, neighbors, pcs) # Do not configure the minigraph's mirror session, which is currently unused # mirror_sessions = {} diff --git a/src/sonic-config-engine/tests/t0-sample-graph.xml b/src/sonic-config-engine/tests/t0-sample-graph.xml index 180895928fbd..0c641107da06 100644 --- a/src/sonic-config-engine/tests/t0-sample-graph.xml +++ b/src/sonic-config-engine/tests/t0-sample-graph.xml @@ -383,6 +383,13 @@ Ethernet1/33 true + + DeviceInterfaceLink + Servers0 + eth0 + switch-t0 + fortyGigE0/4 + diff --git a/src/sonic-config-engine/tests/test_cfggen.py b/src/sonic-config-engine/tests/test_cfggen.py index 1ba59eb7c97a..83ed97f47f71 100644 --- a/src/sonic-config-engine/tests/test_cfggen.py +++ b/src/sonic-config-engine/tests/test_cfggen.py @@ -75,6 +75,9 @@ def test_render_template(self): output = self.run_script(argument) self.assertEqual(output.strip(), 'value1\nvalue2') + # FIXME: This test depends heavily on the ordering of the interfaces and + # it is not at all intuitive what that ordering should be. Could make it + # more robust by adding better parsing logic. def test_minigraph_acl(self): argument = '-m "' + self.sample_graph_t0 + '" -p "' + self.port_config + '" -v ACL_TABLE' output = self.run_script(argument, True) @@ -83,11 +86,11 @@ def test_minigraph_acl(self): "Warning: ignore interface 'fortyGigE0/2' in DEVICE_NEIGHBOR as it is not in the port_config.ini\n" "{'DATAACL': {'type': 'L3', 'policy_desc': 'DATAACL', 'ports': ['PortChannel01', 'PortChannel02', 'PortChannel03', 'PortChannel04']}, " "'NTP_ACL': {'services': ['NTP'], 'type': 'CTRLPLANE', 'policy_desc': 'NTP_ACL'}, " - "'EVERFLOW': {'type': 'MIRROR', 'policy_desc': 'EVERFLOW', 'ports': ['PortChannel01', 'PortChannel02', 'PortChannel03', 'PortChannel04', 'Ethernet24', 'Ethernet40', 'Ethernet20', 'Ethernet44', 'Ethernet48', 'Ethernet28', 'Ethernet96', 'Ethernet92', 'Ethernet76', 'Ethernet72', 'Ethernet52', 'Ethernet80', 'Ethernet56', 'Ethernet32', 'Ethernet16', 'Ethernet36', 'Ethernet12', 'Ethernet60', 'Ethernet8', 'Ethernet4', 'Ethernet0', 'Ethernet64', 'Ethernet68', 'Ethernet84', 'Ethernet88', 'Ethernet108', 'Ethernet104', 'Ethernet100']}, " + "'EVERFLOW': {'type': 'MIRROR', 'policy_desc': 'EVERFLOW', 'ports': ['PortChannel01', 'PortChannel02', 'PortChannel03', 'PortChannel04', 'Ethernet4']}, " "'ROUTER_PROTECT': {'services': ['SSH', 'SNMP'], 'type': 'CTRLPLANE', 'policy_desc': 'ROUTER_PROTECT'}, " "'SNMP_ACL': {'services': ['SNMP'], 'type': 'CTRLPLANE', 'policy_desc': 'SNMP_ACL'}, " "'SSH_ACL': {'services': ['SSH'], 'type': 'CTRLPLANE', 'policy_desc': 'SSH_ACL'}, " - "'EVERFLOWV6': {'type': 'MIRRORV6', 'policy_desc': 'EVERFLOWV6', 'ports': ['PortChannel01', 'PortChannel02', 'PortChannel03', 'PortChannel04', 'Ethernet24', 'Ethernet40', 'Ethernet20', 'Ethernet44', 'Ethernet48', 'Ethernet28', 'Ethernet96', 'Ethernet92', 'Ethernet76', 'Ethernet72', 'Ethernet52', 'Ethernet80', 'Ethernet56', 'Ethernet32', 'Ethernet16', 'Ethernet36', 'Ethernet12', 'Ethernet60', 'Ethernet8', 'Ethernet4', 'Ethernet0', 'Ethernet64', 'Ethernet68', 'Ethernet84', 'Ethernet88', 'Ethernet108', 'Ethernet104', 'Ethernet100']}}") + "'EVERFLOWV6': {'type': 'MIRRORV6', 'policy_desc': 'EVERFLOWV6', 'ports': ['PortChannel01', 'PortChannel02', 'PortChannel03', 'PortChannel04', 'Ethernet4']}}") # everflow portion is not used # def test_minigraph_everflow(self): @@ -145,6 +148,9 @@ def test_minigraph_neighbors(self): output = self.run_script(argument) self.assertEqual(output.strip(), "{'name': 'ARISTA04T1', 'port': 'Ethernet1/1'}") + # FIXME: This test depends heavily on the ordering of the interfaces and + # it is not at all intuitive what that ordering should be. Could make it + # more robust by adding better parsing logic. def test_minigraph_extra_neighbors(self): argument = '-m "' + self.sample_graph_t0 + '" -p "' + self.port_config + '" -v DEVICE_NEIGHBOR' output = self.run_script(argument) @@ -152,6 +158,7 @@ def test_minigraph_extra_neighbors(self): "{'Ethernet116': {'name': 'ARISTA02T1', 'port': 'Ethernet1/1'}, " "'Ethernet124': {'name': 'ARISTA04T1', 'port': 'Ethernet1/1'}, " "'Ethernet112': {'name': 'ARISTA01T1', 'port': 'Ethernet1/1'}, " + "'Ethernet4': {'name': 'Servers0', 'port': 'eth0'}, " "'Ethernet120': {'name': 'ARISTA03T1', 'port': 'Ethernet1/1'}}") def test_minigraph_port_description(self):