Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[minigraph] Fix parser on PNG DeviceInterfaceLink Bandwidth #1592

Merged
merged 5 commits into from
Apr 12, 2018
Merged

[minigraph] Fix parser on PNG DeviceInterfaceLink Bandwidth #1592

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4e48bfa
[minigraph] Fix parser on PNG DeviceInterfaceLink Bandwidth
qiluo-msft Apr 12, 2018
54a878a
Fix test case due to rebase
qiluo-msft Apr 12, 2018
8e0e2d6
Rename variables
qiluo-msft Apr 12, 2018
3ebedde
Fix StartPort name in test case
qiluo-msft Apr 12, 2018
98d79c5
Remove redundant code, add comment
qiluo-msft Apr 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 23 additions & 9 deletions src/sonic-config-engine/minigraph.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ def parse_png(png, hname):
console_port = ''
mgmt_dev = ''
mgmt_port = ''
port_speeds = {}
for child in png:
if child.tag == str(QName(ns, "DeviceInterfaceLinks")):
for link in child.findall(str(QName(ns, "DeviceLinkBase"))):
Expand All @@ -73,15 +74,21 @@ def parse_png(png, hname):
endport = link.find(str(QName(ns, "EndPort"))).text
startdevice = link.find(str(QName(ns, "StartDevice"))).text
startport = link.find(str(QName(ns, "StartPort"))).text
bandwidth_node = link.find(str(QName(ns, "Bandwidth")))
bandwidth = bandwidth_node.text if bandwidth_node is not None else None

if enddevice == hname:
if port_alias_map.has_key(endport):
endport = port_alias_map[endport]
neighbors[endport] = {'name': startdevice, 'port': startport}
if bandwidth:
port_speeds[endport] = bandwidth
else:
if port_alias_map.has_key(startport):
startport = port_alias_map[startport]
neighbors[startport] = {'name': enddevice, 'port': endport}
if bandwidth:
port_speeds[startport] = bandwidth

if child.tag == str(QName(ns, "Devices")):
for device in child.findall(str(QName(ns, "Device"))):
Expand All @@ -106,7 +113,7 @@ def parse_png(png, hname):
elif node.tag == str(QName(ns, "EndDevice")):
mgmt_dev = node.text

return (neighbors, devices, console_dev, console_port, mgmt_dev, mgmt_port)
return (neighbors, devices, console_dev, console_port, mgmt_dev, mgmt_port, port_speeds)


def parse_dpg(dpg, hname):
Expand Down Expand Up @@ -368,7 +375,8 @@ def parse_xml(filename, platform=None, port_config_file=None):
neighbors = None
devices = None
hostname = None
port_speeds = {}
port_speeds_default = {}
port_speed_png = {}
port_descriptions = {}
syslog_servers = []
dhcp_servers = []
Expand All @@ -395,13 +403,13 @@ def parse_xml(filename, platform=None, port_config_file=None):
elif child.tag == str(QName(ns, "CpgDec")):
(bgp_sessions, bgp_asn, bgp_peers_with_range) = parse_cpg(child, hostname)
elif child.tag == str(QName(ns, "PngDec")):
(neighbors, devices, console_dev, console_port, mgmt_dev, mgmt_port) = parse_png(child, hostname)
(neighbors, devices, console_dev, console_port, mgmt_dev, mgmt_port, port_speed_png) = parse_png(child, hostname)
elif child.tag == str(QName(ns, "UngDec")):
(u_neighbors, u_devices, _, _, _, _) = parse_png(child, hostname)
(u_neighbors, u_devices, _, _, _, _, _) = parse_png(child, hostname)
elif child.tag == str(QName(ns, "MetadataDeclaration")):
(syslog_servers, dhcp_servers, ntp_servers, tacacs_servers, mgmt_routes, erspan_dst, deployment_id) = parse_meta(child, hostname)
elif child.tag == str(QName(ns, "DeviceInfos")):
(port_speeds, port_descriptions) = parse_deviceinfo(child, hwsku)
(port_speeds_default, port_descriptions) = parse_deviceinfo(child, hwsku)

results = {}
results['DEVICE_METADATA'] = {'localhost': {
Expand Down Expand Up @@ -438,14 +446,20 @@ def parse_xml(filename, platform=None, port_config_file=None):
results['VLAN_INTERFACE'] = vlan_intfs
results['PORTCHANNEL_INTERFACE'] = pc_intfs

for port_name in port_speeds:
for port_name in port_speeds_default:
# ignore port not in port_config.ini
if not ports.has_key(port_name):
continue

ports.setdefault(port_name, {})['speed'] = port_speeds[port_name]
if port_speeds[port_name] == '100000':
ports.setdefault(port_name, {})['fec'] = 'rs'
ports.setdefault(port_name, {})['speed'] = port_speeds_default[port_name]
for port_name in port_speed_png:
Copy link
Collaborator

@lguohan lguohan Apr 12, 2018 •
edited by qiluo-msft
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also check if port_name in the ports or not, if not, we should print a warning here and ignore the port. #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

offline discussed, check the comment for the conclusion. Don't need to treat it as an error here.

In reply to: 181198316 [](ancestors = 181198316)

ports.setdefault(port_name, {})['speed'] = port_speed_png[port_name]
for port_name, port in ports.items():
if port.get('speed') == '100000':
Copy link
Contributor

@taoyl-ms taoyl-ms Apr 12, 2018 •
edited by qiluo-msft
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it guaranteed that 'speed' will be available for all ports? #Resolved

Copy link
Collaborator Author

@qiluo-msft qiluo-msft Apr 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By reading the code, it seems not.

In reply to: 181201283 [](ancestors = 181201283)

port['fec'] = 'rs'
for port_name in port_descriptions:
Copy link
Collaborator

@lguohan lguohan Apr 12, 2018 •
edited by qiluo-msft
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicated. check below #Closed

ports.setdefault(port_name, {})['description'] = port_descriptions[port_name]

for port_name in port_descriptions:
# ignore port not in port_config.ini
if not ports.has_key(port_name):
Expand Down
2 changes: 1 addition & 1 deletion src/sonic-config-engine/tests/sample_output/ports.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[
{
"PORT_TABLE:Ethernet8": {
"speed": "40000",
"speed": "1000",
"description": "Interface description"
},
"OP": "SET"
Expand Down
14 changes: 13 additions & 1 deletion src/sonic-config-engine/tests/simple-sample-graph.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,19 @@
</DeviceDataPlaneInfo>
</DpgDec>
<PngDec>
<DeviceInterfaceLinks/>
<DeviceInterfaceLinks>
<DeviceLinkBase i:type="DeviceInterfaceLink">
<ElementType>DeviceInterfaceLink</ElementType>
<AutoNegotiation>true</AutoNegotiation>
<Bandwidth>1000</Bandwidth>
<EndDevice>ARISTA01T1</EndDevice>
<EndPort>et1</EndPort>
<FlowControl>true</FlowControl>
<StartDevice>switch-t0</StartDevice>
<StartPort>fortyGigE0/8</StartPort>
<Validate>true</Validate>
</DeviceLinkBase>
</DeviceInterfaceLinks>
<Devices>
<Device i:type="ToRRouter">
<Hostname>switch-t0</Hostname>
Expand Down
4 changes: 2 additions & 2 deletions src/sonic-config-engine/tests/test_cfggen.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ def test_minigraph_deployment_id(self):
def test_minigraph_ethernet_interfaces(self):
argument = '-m "' + self.sample_graph_simple + '" -p "' + self.port_config + '" -v "PORT[\'Ethernet8\']"'
output = self.run_script(argument)
self.assertEqual(output.strip(), "{'alias': 'fortyGigE0/8', 'lanes': '37,38,39,40', 'description': 'Interface description', 'speed': '40000'}")
self.assertEqual(output.strip(), "{'alias': 'fortyGigE0/8', 'lanes': '37,38,39,40', 'description': 'Interface description', 'speed': '1000'}")
argument = '-m "' + self.sample_graph_simple + '" -p "' + self.port_config + '" -v "PORT[\'Ethernet12\']"'
output = self.run_script(argument)
self.assertEqual(output.strip(), "{'alias': 'fortyGigE0/12', 'lanes': '33,34,35,36', 'fec': 'rs', 'speed': '100000', 'description': 'Interface description'}")
Expand All @@ -157,7 +157,7 @@ def test_minigraph_extra_ethernet_interfaces(self):
argument = '-m "' + self.sample_graph_simple + '" -p "' + self.port_config + '" -v "PORT"'
output = self.run_script(argument)
self.assertEqual(output.strip(), \
"{'Ethernet8': {'alias': 'fortyGigE0/8', 'lanes': '37,38,39,40', 'description': 'Interface description', 'speed': '40000'}, "
"{'Ethernet8': {'alias': 'fortyGigE0/8', 'lanes': '37,38,39,40', 'description': 'Interface description', 'speed': '1000'}, "
"'Ethernet0': {'alias': 'fortyGigE0/0', 'lanes': '29,30,31,32', 'speed': '10000'}, "
"'Ethernet4': {'alias': 'fortyGigE0/4', 'lanes': '25,26,27,28', 'speed': '25000'}, "
"'Ethernet108': {'alias': 'fortyGigE0/108', 'lanes': '81,82,83,84'}, "
Expand Down