From 936d93cbcd25b3997f38f4278eae8d0db012c258 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Tue, 19 Apr 2022 15:47:07 -0700 Subject: [PATCH] Fix tagged VlanInterface if attached to multiple vlan as untagged member (#8927) #### Why I did it Fix several bugs: 1. If one vlan member belongs to multiple vlans, and if any of the vlans is "Tagged" type, we respect the tagged type 2. If one vlan member belongs to multiple vlans, and all of the vlans have no "Tagged" type, we override it to be a tagged member 3. make sure `vlantype_name` is assigned correctly in each iteration #### How to verify it 1. Test the command line to parse a minigraph and make sure the output does not change. ``` ./sonic-cfggen -m minigraph.mlnx20.xml ``` The minigraph is for HwSKU Mellanox-SN2700-D40C8S8. 2. Test on a DUT with HwSKU Mellanox-SN2700-D40C8S8 ``` sudo config load_minigraph show vlan brief ``` Checked the "Port Tagging" column in the output. --- src/sonic-config-engine/minigraph.py | 22 +++++++++++------ .../tests/sample-graph-resource-type.xml | 9 +++++++ .../tests/sample-graph-subintf.xml | 8 +++++++ .../tests/simple-sample-graph.xml | 9 +++++++ src/sonic-config-engine/tests/test_cfggen.py | 24 +++++++++++++++---- 5 files changed, 61 insertions(+), 11 deletions(-) diff --git a/src/sonic-config-engine/minigraph.py b/src/sonic-config-engine/minigraph.py index b4f1f3acbd..35a6f56456 100644 --- a/src/sonic-config-engine/minigraph.py +++ b/src/sonic-config-engine/minigraph.py @@ -536,28 +536,36 @@ def parse_dpg(dpg, hname): vlan_members = {} vlan_member_list = {} dhcp_relay_table = {} - vlantype_name = "" - intf_vlan_mbr = defaultdict(list) + # Dict: vlan member (port/PortChannel) -> set of VlanID, in which the member if an untagged vlan member + untagged_vlan_mbr = defaultdict(set) for vintf in vlanintfs.findall(str(QName(ns, "VlanInterface"))): vlanid = vintf.find(str(QName(ns, "VlanID"))).text + vlantype = vintf.find(str(QName(ns, "Type"))) + if vlantype is None: + vlantype_name = "" + else: + vlantype_name = vlantype.text vintfmbr = vintf.find(str(QName(ns, "AttachTo"))).text vmbr_list = vintfmbr.split(';') - for i, member in enumerate(vmbr_list): - intf_vlan_mbr[member].append(vlanid) + if vlantype_name != "Tagged": + for member in vmbr_list: + untagged_vlan_mbr[member].add(vlanid) for vintf in vlanintfs.findall(str(QName(ns, "VlanInterface"))): vintfname = vintf.find(str(QName(ns, "Name"))).text vlanid = vintf.find(str(QName(ns, "VlanID"))).text vintfmbr = vintf.find(str(QName(ns, "AttachTo"))).text vlantype = vintf.find(str(QName(ns, "Type"))) - if vlantype != None: - vlantype_name = vintf.find(str(QName(ns, "Type"))).text + if vlantype is None: + vlantype_name = "" + else: + vlantype_name = vlantype.text vmbr_list = vintfmbr.split(';') for i, member in enumerate(vmbr_list): vmbr_list[i] = port_alias_map.get(member, member) sonic_vlan_member_name = "Vlan%s" % (vlanid) if vlantype_name == "Tagged": vlan_members[(sonic_vlan_member_name, vmbr_list[i])] = {'tagging_mode': 'tagged'} - elif len(intf_vlan_mbr[member]) > 1: + elif len(untagged_vlan_mbr[member]) > 1: vlan_members[(sonic_vlan_member_name, vmbr_list[i])] = {'tagging_mode': 'tagged'} else: vlan_members[(sonic_vlan_member_name, vmbr_list[i])] = {'tagging_mode': 'untagged'} diff --git a/src/sonic-config-engine/tests/sample-graph-resource-type.xml b/src/sonic-config-engine/tests/sample-graph-resource-type.xml index 9ba4f1e702..62c2dfd64f 100644 --- a/src/sonic-config-engine/tests/sample-graph-resource-type.xml +++ b/src/sonic-config-engine/tests/sample-graph-resource-type.xml @@ -190,6 +190,14 @@ 1000 192.168.0.0/27 + + ab4 + fortyGigE0/8 + 192.0.0.1;192.0.0.2 + 1001 + 1001 + 192.168.0.32/27 + kk1 fortyGigE0/12 @@ -205,6 +213,7 @@ 192.0.0.1;192.0.0.2 2000 2000 + Tagged 192.168.0.240/27 diff --git a/src/sonic-config-engine/tests/sample-graph-subintf.xml b/src/sonic-config-engine/tests/sample-graph-subintf.xml index d5fd2d461c..a23b668c2c 100644 --- a/src/sonic-config-engine/tests/sample-graph-subintf.xml +++ b/src/sonic-config-engine/tests/sample-graph-subintf.xml @@ -208,6 +208,14 @@ 1000 192.168.0.0/27 + + ab4 + fortyGigE0/8 + 192.0.0.1;192.0.0.2 + 1001 + 1001 + 192.168.0.32/27 + kk1 fortyGigE0/12 diff --git a/src/sonic-config-engine/tests/simple-sample-graph.xml b/src/sonic-config-engine/tests/simple-sample-graph.xml index a8bd8b0b46..8d7800686c 100644 --- a/src/sonic-config-engine/tests/simple-sample-graph.xml +++ b/src/sonic-config-engine/tests/simple-sample-graph.xml @@ -190,6 +190,14 @@ 1000 192.168.0.0/27 + + ab4 + fortyGigE0/8 + 192.0.0.1;192.0.0.2 + 1001 + 1001 + 192.168.0.32/27 + kk1 fortyGigE0/12 @@ -205,6 +213,7 @@ 192.0.0.1;192.0.0.2 2000 2000 + Tagged 192.168.0.240/27 diff --git a/src/sonic-config-engine/tests/test_cfggen.py b/src/sonic-config-engine/tests/test_cfggen.py index e1f6844dd5..29773dffc6 100644 --- a/src/sonic-config-engine/tests/test_cfggen.py +++ b/src/sonic-config-engine/tests/test_cfggen.py @@ -151,6 +151,7 @@ class TestCfgGen(TestCase): utils.to_dict(output.strip()), utils.to_dict( '{\n "Vlan1000|Ethernet8": {\n "tagging_mode": "tagged"\n },' + ' \n "Vlan1001|Ethernet8": {\n "tagging_mode": "tagged"\n },' ' \n "Vlan2000|Ethernet12": {\n "tagging_mode": "tagged"\n },' ' \n "Vlan2001|Ethernet12": {\n "tagging_mode": "tagged"\n },' ' \n "Vlan2020|Ethernet12": {\n "tagging_mode": "tagged"\n }\n}' @@ -160,9 +161,10 @@ class TestCfgGen(TestCase): self.assertEqual( utils.to_dict(output.strip()), utils.to_dict( - '{\n "Vlan1000|Ethernet8": {\n "tagging_mode": "untagged"\n },' + '{\n "Vlan1000|Ethernet8": {\n "tagging_mode": "tagged"\n },' + ' \n "Vlan1001|Ethernet8": {\n "tagging_mode": "tagged"\n },' ' \n "Vlan2000|Ethernet12": {\n "tagging_mode": "tagged"\n },' - ' \n "Vlan2001|Ethernet12": {\n "tagging_mode": "tagged"\n },' + ' \n "Vlan2001|Ethernet12": {\n "tagging_mode": "untagged"\n },' ' \n "Vlan2020|Ethernet12": {\n "tagging_mode": "tagged"\n }\n}' ) ) @@ -249,6 +251,7 @@ class TestCfgGen(TestCase): utils.to_dict(output.strip()), utils.to_dict( "{'Vlan1000': {'alias': 'ab1', 'dhcp_servers': ['192.0.0.1', '192.0.0.2'], 'vlanid': '1000'}, " + "'Vlan1001': {'alias': 'ab4', 'dhcp_servers': ['192.0.0.1', '192.0.0.2'], 'vlanid': '1001'}," "'Vlan2001': {'alias': 'ab3', 'dhcp_servers': ['192.0.0.1', '192.0.0.2'], 'vlanid': '2001'}," "'Vlan2000': {'alias': 'ab2', 'dhcp_servers': ['192.0.0.1', '192.0.0.2'], 'vlanid': '2000'}," "'Vlan2020': {'alias': 'kk1', 'dhcp_servers': ['192.0.0.1', '192.0.0.2'], 'vlanid': '2020'}}" @@ -265,6 +268,7 @@ class TestCfgGen(TestCase): utils.to_dict(output.strip()), utils.to_dict( "{('Vlan2000', 'Ethernet12'): {'tagging_mode': 'tagged'}, " + "('Vlan1001', 'Ethernet8'): {'tagging_mode': 'tagged'}, " "('Vlan1000', 'Ethernet8'): {'tagging_mode': 'tagged'}, " "('Vlan2020', 'Ethernet12'): {'tagging_mode': 'tagged'}, " "('Vlan2001', 'Ethernet12'): {'tagging_mode': 'tagged'}}" @@ -275,9 +279,10 @@ class TestCfgGen(TestCase): utils.to_dict(output.strip()), utils.to_dict( "{('Vlan2000', 'Ethernet12'): {'tagging_mode': 'tagged'}, " - "('Vlan1000', 'Ethernet8'): {'tagging_mode': 'untagged'}, " + "('Vlan1000', 'Ethernet8'): {'tagging_mode': 'tagged'}, " + "('Vlan1001', 'Ethernet8'): {'tagging_mode': 'tagged'}, " "('Vlan2020', 'Ethernet12'): {'tagging_mode': 'tagged'}, " - "('Vlan2001', 'Ethernet12'): {'tagging_mode': 'tagged'}}" + "('Vlan2001', 'Ethernet12'): {'tagging_mode': 'untagged'}}" ) ) @@ -704,6 +709,9 @@ class TestCfgGen(TestCase): def test_minigraph_sub_port_interfaces(self, check_stderr=True): self.verify_sub_intf(check_stderr=check_stderr) + def test_minigraph_sub_port_intf_resource_type_non_backend_tor(self, check_stderr=True): + self.verify_sub_intf_non_backend_tor(graph_file=self.sample_resource_graph, check_stderr=check_stderr) + def test_minigraph_sub_port_intf_resource_type(self, check_stderr=True): self.verify_sub_intf(graph_file=self.sample_resource_graph, check_stderr=check_stderr) @@ -737,6 +745,14 @@ class TestCfgGen(TestCase): output = self.run_script(argument) self.assertEqual(output.strip(), "{}") + def verify_sub_intf_non_backend_tor(self, **kwargs): + graph_file = kwargs.get('graph_file', self.sample_graph_simple) + + # All the other tables stay unchanged + self.test_var_json_data(graph_file=graph_file) + self.test_minigraph_vlans(graph_file=graph_file) + self.test_minigraph_vlan_members(graph_file=graph_file) + def verify_sub_intf(self, **kwargs): graph_file = kwargs.get('graph_file', self.sample_graph_simple) check_stderr = kwargs.get('check_stderr', True)