[minigraph.py] Update minigraph parsing logic to include only active ports for mirror tables (#3592)

* 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 <daall@microsoft.com>

* Incorporate PR comments
- Update t0 tests to include additional device neighbors
- Refactor xml parsing logic

src/sonic-config-engine/minigraph.py

@@ -37,6 +37,12 @@ VLAN_SUB_INTERFACE_VLAN_ID = '10'
 # Default Virtual Network Index (VNI) 
 vni_default = 8000
 
+###############################################################################
+#
+# Minigraph parsing functions
+#
+###############################################################################
+
 class minigraph_encoder(json.JSONEncoder):
     def default(self, obj):
         if isinstance(obj, (
@@ -278,15 +284,17 @@ def parse_dpg(dpg, hname):
                     if member.lower().startswith('erspanv6'):
                         is_mirror_v6 = True
                     else:
-                        is_mirror = True;
+                        is_mirror = True
-                    # Erspan session will be attached to all front panel ports,
+                    # Erspan session will be attached to all front panel ports
-                    # if panel ports is a member port of LAG, should add the LAG 
+                    # initially. If panel ports is a member port of LAG, then
-                    # to acl table instead of the panel ports
+                    # 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}
@@ -514,6 +522,39 @@ def parse_spine_chassis_fe(results, vni, lo_intfs, phyport_intfs, pc_intfs, pc_m
             # Enslave the port channel interface to a Vnet
             pc_intfs[pc_intf] = {'vnet_name': chassis_vnet}        
 
+###############################################################################
+#
+# 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()
@@ -760,7 +801,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 = {}

src/sonic-config-engine/tests/t0-sample-graph.xml

@@ -383,6 +383,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">

src/sonic-config-engine/tests/test_cfggen.py

@@ -95,6 +95,9 @@ class TestCfgGen(TestCase):
         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)
@@ -103,11 +106,11 @@ class TestCfgGen(TestCase):
                                          "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):
@@ -165,6 +168,9 @@ class TestCfgGen(TestCase):
         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)
@@ -172,6 +178,7 @@ class TestCfgGen(TestCase):
                 "{'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):