Skip to content

Commit

Permalink
Fix tagged VlanInterface if attached to multiple vlan as untagged mem…
Browse files Browse the repository at this point in the history 
…ber (#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.
  • Loading branch information
@qiluo-msft
qiluo-msft authored Apr 19, 2022
1 parent 8b5d908 commit 936d93c
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 11 deletions.
22 changes: 15 additions & 7 deletions src/sonic-config-engine/minigraph.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'}
Expand Down
9 changes: 9 additions & 0 deletions src/sonic-config-engine/tests/sample-graph-resource-type.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,14 @@
<Tag>1000</Tag>
<Subnets>192.168.0.0/27</Subnets>
</VlanInterface>
<VlanInterface>
<Name>ab4</Name>
<AttachTo>fortyGigE0/8</AttachTo>
<DhcpRelays>192.0.0.1;192.0.0.2</DhcpRelays>
<VlanID>1001</VlanID>
<Tag>1001</Tag>
<Subnets>192.168.0.32/27</Subnets>
</VlanInterface>
<VlanInterface>
<Name>kk1</Name>
<AttachTo>fortyGigE0/12</AttachTo>
Expand All @@ -205,6 +213,7 @@
<DhcpRelays>192.0.0.1;192.0.0.2</DhcpRelays>
<VlanID>2000</VlanID>
<Tag>2000</Tag>
<Type>Tagged</Type>
<Subnets>192.168.0.240/27</Subnets>
</VlanInterface>
<VlanInterface>
Expand Down
8 changes: 8 additions & 0 deletions src/sonic-config-engine/tests/sample-graph-subintf.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,14 @@
<Tag>1000</Tag>
<Subnets>192.168.0.0/27</Subnets>
</VlanInterface>
<VlanInterface>
<Name>ab4</Name>
<AttachTo>fortyGigE0/8</AttachTo>
<DhcpRelays>192.0.0.1;192.0.0.2</DhcpRelays>
<VlanID>1001</VlanID>
<Tag>1001</Tag>
<Subnets>192.168.0.32/27</Subnets>
</VlanInterface>
<VlanInterface>
<Name>kk1</Name>
<AttachTo>fortyGigE0/12</AttachTo>
Expand Down
9 changes: 9 additions & 0 deletions src/sonic-config-engine/tests/simple-sample-graph.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,14 @@
<Tag>1000</Tag>
<Subnets>192.168.0.0/27</Subnets>
</VlanInterface>
<VlanInterface>
<Name>ab4</Name>
<AttachTo>fortyGigE0/8</AttachTo>
<DhcpRelays>192.0.0.1;192.0.0.2</DhcpRelays>
<VlanID>1001</VlanID>
<Tag>1001</Tag>
<Subnets>192.168.0.32/27</Subnets>
</VlanInterface>
<VlanInterface>
<Name>kk1</Name>
<AttachTo>fortyGigE0/12</AttachTo>
Expand All @@ -205,6 +213,7 @@
<DhcpRelays>192.0.0.1;192.0.0.2</DhcpRelays>
<VlanID>2000</VlanID>
<Tag>2000</Tag>
<Type>Tagged</Type>
<Subnets>192.168.0.240/27</Subnets>
</VlanInterface>
<VlanInterface>
Expand Down
24 changes: 20 additions & 4 deletions src/sonic-config-engine/tests/test_cfggen.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ def test_var_json_data(self, **kwargs):
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}'
Expand All @@ -160,9 +161,10 @@ def test_var_json_data(self, **kwargs):
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}'
)
)
Expand Down Expand Up @@ -249,6 +251,7 @@ def test_minigraph_vlans(self, **kwargs):
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'}}"
Expand All @@ -265,6 +268,7 @@ def test_minigraph_vlan_members(self, **kwargs):
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'}}"
Expand All @@ -275,9 +279,10 @@ def test_minigraph_vlan_members(self, **kwargs):
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'}}"
)
)

Expand Down Expand Up @@ -704,6 +709,9 @@ def test_minigraph_bgp_voq_chassis_peer(self):
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)

Expand Down Expand Up @@ -737,6 +745,14 @@ def verify_no_vlan_member(self):
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)
Expand Down

0 comments on commit 936d93c

Please sign in to comment.