Skip to content

Commit

Permalink
[minigraph.py] Update minigraph parsing logic to include only active …
Browse files Browse the repository at this point in the history 
…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 <[email protected]>

* Incorporate PR comments
- Update t0 tests to include additional device neighbors
- Refactor xml parsing logic
  • Loading branch information
@daall @renukamanavalan
daall authored and renukamanavalan committed Oct 24, 2019
1 parent bd13aad commit d1c1e7f
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 8 deletions.
54 changes: 48 additions & 6 deletions src/sonic-config-engine/minigraph.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, (
Expand Down Expand Up @@ -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}
Expand Down Expand Up @@ -413,6 +421,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
Expand Down Expand Up @@ -623,7 +665,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 = {}
Expand Down
7 changes: 7 additions & 0 deletions src/sonic-config-engine/tests/t0-sample-graph.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,13 @@
<StartPort>Ethernet1/33</StartPort>
<Validate>true</Validate>
</DeviceLinkBase>
<DeviceLinkBase>
<ElementType>DeviceInterfaceLink</ElementType>
<EndDevice>Servers0</EndDevice>
<EndPort>eth0</EndPort>
<StartDevice>switch-t0</StartDevice>
<StartPort>fortyGigE0/4</StartPort>
</DeviceLinkBase>
</DeviceInterfaceLinks>
<Devices>
<Device i:type="ToRRouter">
Expand Down
11 changes: 9 additions & 2 deletions src/sonic-config-engine/tests/test_cfggen.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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):
Expand Down Expand Up @@ -145,13 +148,17 @@ 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)
self.assertEqual(output.strip(), \
"{'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):
Expand Down

0 comments on commit d1c1e7f

Please sign in to comment.