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.

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

Parser changes to support parsing of multi-asic device minigraph #4222

Merged
merged 27 commits into from
May 4, 2020
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
3ce843a
[minigraph.py]: Changes added in minigraph and port_config parser
SuvarnaMeenakshi Mar 4, 2020
ff6aa99
[minigraph.py/portconfig.py]: Minor fixes
SuvarnaMeenakshi Mar 4, 2020
b784c81
[minigraph.py]: Updated to make sure that interfaces names are
SuvarnaMeenakshi Mar 13, 2020
9b4039e
Changes to sonic-cfggen for multi-NPU platforms
arlakshm Mar 24, 2020
d4fe8e0
[sonic-cfggen]: Fix spacing.
SuvarnaMeenakshi Apr 7, 2020
53c6693
Minor fixes and UT tests
arlakshm Apr 11, 2020
a9a06a8
Addressing Review comments
arlakshm Apr 16, 2020
9c9f8c2
Merge remote-tracking branch 'remotes/sumeenak/master' into minigraph…
SuvarnaMeenakshi Apr 16, 2020
5b6892e
addressing review comments
arlakshm Apr 17, 2020
78a2bf7
addressing review comments
arlakshm Apr 19, 2020
ce84879
Change from id to asic_id in the device_metadata
arlakshm Apr 23, 2020
882c251
Fix as per review comments.
SuvarnaMeenakshi Apr 25, 2020
4ae22be
Minor fix as per review comment.
SuvarnaMeenakshi Apr 25, 2020
30024ba
Addressing review comments
arlakshm Apr 27, 2020
9744b85
Addressing review comments
arlakshm Apr 27, 2020
4f4be8c
Addressing review comments
arlakshm Apr 27, 2020
0895629
Addressing review comments
arlakshm Apr 29, 2020
0b90dae
Merge remote-tracking branch 'remotes/sumeenak/master' into minigraph…
SuvarnaMeenakshi Apr 29, 2020
15228c1
Addressing review comments
arlakshm May 1, 2020
0432f92
[minigraph.py]: Updated as per review comments.
SuvarnaMeenakshi May 1, 2020
ebf2e8b
[minigraph.py]: Split parse_asic_png function to make it
SuvarnaMeenakshi May 2, 2020
520c43b
[minigraph.py]: Correction in parse_asic_png function
SuvarnaMeenakshi May 2, 2020
110d1b8
Merge branch 'multiasic_minigraph' of github.com:SuvarnaMeenakshi/son…
SuvarnaMeenakshi May 2, 2020
247370b
Minor fixes.
SuvarnaMeenakshi May 2, 2020
cd3a678
Minor fixes as per review comments.
SuvarnaMeenakshi May 2, 2020
06983e4
Addressing review comments
arlakshm May 3, 2020
17a0d04
Fixes from testing
arlakshm May 4, 2020
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
148 changes: 113 additions & 35 deletions src/sonic-config-engine/minigraph.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def parse_device(device):
deployment_id = node.text
return (lo_prefix, mgmt_prefix, name, hwsku, d_type, deployment_id)

def parse_png(png, hname):
def parse_png(png, hname, device_hostname):
neighbors = {}
devices = {}
console_dev = ''
Expand All @@ -84,6 +84,7 @@ def parse_png(png, hname):
mgmt_port = ''
port_speeds = {}
console_ports = {}
device_neighbors = []
for child in png:
if child.tag == str(QName(ns, "DeviceInterfaceLinks")):
for link in child.findall(str(QName(ns, "DeviceLinkBase"))):
Expand All @@ -101,7 +102,7 @@ def parse_png(png, hname):
'baud_rate': baudrate,
'flow_control': flowcontrol
}
else:
if startdevice.lower() == hname.lower():
lguohan marked this conversation as resolved.
Show resolved Hide resolved
console_ports[startport] = {
'remote_device': enddevice,
'baud_rate': baudrate,
Expand All @@ -118,19 +119,51 @@ def parse_png(png, hname):
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.lower() == hname.lower():
if port_alias_map.has_key(endport):
endport = port_alias_map[endport]
neighbors[endport] = {'name': startdevice, 'port': startport}
if bandwidth:
port_speeds[endport] = bandwidth
# Chassis internal node is used in multi-asic device or chassis minigraph
# where the minigraph will contain the internal asic connectivity and
# external neighbor information. The ChassisInternal node will be used to
# determine if the link is internal to the device or chassis.
chassis_internal_node = link.find(str(QName(ns, "ChassisInternal")))
Copy link
Collaborator

Choose a reason for hiding this comment

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

this part of code needs further explanation. what are the scenario we are dealing with?

I can think of three:

  1. backend asic (all are internal neighbors)
  2. frontend asic (some connection are external neighbors, some are internal neighbors)
  3. whole box. (all connection are external neighbors)

it is becoming complicated and difficult to maintain.

My suggestion is the have a separate parse_asic_png to deal with asic png parsing. asic png does not have management port, no console. easier to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added new function as per comment

chassis_internal = chassis_internal_node.text if chassis_internal_node is not None else "false"

# if hname is ASIC name and the link is an external link
# include the external neighbor inforamtion in ASIC ports table
if chassis_internal == "false" and hname.lower() != device_hostname.lower():
if (enddevice.lower() == device_hostname.lower()):
if ((port_alias_asic_map.has_key(endport)) and
(hname.lower() in port_alias_asic_map[endport].lower())):
endport = port_alias_asic_map[endport]
neighbors[port_alias_map[endport]] = {'name': startdevice, 'port': startport}
device_neighbors.append(startdevice)
if bandwidth:
port_speeds[port_alias_map[endport]] = bandwidth

if (startdevice.lower() == device_hostname.lower()):
if ((port_alias_asic_map.has_key(startport)) and
(hname.lower() in port_alias_asic_map[startport].lower())):
startport = port_alias_asic_map[startport]
neighbors[port_alias_map[startport]] = {'name': enddevice, 'port': endport}
device_neighbors.append(enddevice)
if bandwidth:
port_speeds[port_alias_map[startport]] = 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 ((enddevice.lower() == hname.lower()) and
(startdevice.lower() != device_hostname.lower())):
if port_alias_map.has_key(endport):
endport = port_alias_map[endport]
neighbors[endport] = {'name': startdevice, 'port': startport}
device_neighbors.append(startdevice)
if bandwidth:
port_speeds[endport] = bandwidth

if ((startdevice.lower() == hname.lower()) and
(enddevice.lower() != device_hostname.lower())):
if port_alias_map.has_key(startport):
startport = port_alias_map[startport]
neighbors[startport] = {'name': enddevice, 'port': endport}
device_neighbors.append(enddevice)
if bandwidth:
port_speeds[startport] = bandwidth
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you simplify the code here since it is only for asic png parsing.


if child.tag == str(QName(ns, "Devices")):
for device in child.findall(str(QName(ns, "Device"))):
Expand All @@ -157,10 +190,19 @@ 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, port_speeds, console_ports)
return (neighbors, devices, console_dev, console_port, mgmt_dev, mgmt_port, port_speeds, console_ports, device_neighbors)


Copy link
Collaborator

Choose a reason for hiding this comment

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

remove blank lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

def parse_dpg(dpg, hname, device_hostname):

def parse_dpg(dpg, hname):
#In Multi-NPU platforms the acl intfs are defined for only for the host.
# to get the aclintfs node first.
lguohan marked this conversation as resolved.
Show resolved Hide resolved
for child in dpg:

aclintfs = child.find(str(QName(ns, "AclInterfaces")))
if aclintfs is not None:
break
Copy link
Collaborator

Choose a reason for hiding this comment

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

i do not understand the change here. why do we need two for loop now? if there is no aclinterfaces, then skip aclinterface processing. we can still do it in one pass.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the Multi-asic minigraph, the aclIntf is present for the complete host not per asic, so while parsing to generate the asic configuation, we need to get the aclIntf node first.

Fixed to remove extra loop


for child in dpg:
hostname = child.find(str(QName(ns, "Hostname")))
if hostname.text.lower() != hname.lower():
Expand Down Expand Up @@ -254,7 +296,7 @@ def parse_dpg(dpg, hname):
vlan_attributes['alias'] = vintfname
vlans[sonic_vlan_name] = vlan_attributes

aclintfs = child.find(str(QName(ns, "AclInterfaces")))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please remove those spaces?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in the latest commit

acls = {}
for aclintf in aclintfs.findall(str(QName(ns, "AclInterface"))):
if aclintf.find(str(QName(ns, "InAcl"))) is not None:
Expand Down Expand Up @@ -369,7 +411,7 @@ def parse_cpg(cpg, hname):
'keepalive': keepalive,
'nhopself': nhopself
}
else:
if start_router.lower() == hname.lower():
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you explain the change here? why not elif?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When parsing for specific asic name, we have to make sure that either start router or end router matches with the asic name because, in multiasic device Minigraph, Cpg will include information of all ASICs.
Changed to "elif".

bgp_sessions[end_peer.lower()] = {
'name': end_router,
'local_addr': start_peer.lower(),
Expand Down Expand Up @@ -419,6 +461,7 @@ def parse_meta(meta, hname):
mgmt_routes = []
erspan_dst = []
deployment_id = None
sub_role = None
device_metas = meta.find(str(QName(ns, "Devices")))
for device in device_metas.findall(str(QName(ns1, "DeviceMetadata"))):
if device.find(str(QName(ns1, "Name"))).text.lower() == hname.lower():
Expand All @@ -441,7 +484,9 @@ def parse_meta(meta, hname):
erspan_dst = value_group
elif name == "DeploymentId":
deployment_id = value
return syslog_servers, dhcp_servers, ntp_servers, tacacs_servers, mgmt_routes, erspan_dst, deployment_id
elif name == "SubRole":
sub_role = value
return syslog_servers, dhcp_servers, ntp_servers, tacacs_servers, mgmt_routes, erspan_dst, deployment_id, sub_role

def parse_deviceinfo(meta, hwsku):
port_speeds = {}
Expand Down Expand Up @@ -518,7 +563,7 @@ def parse_spine_chassis_fe(results, vni, lo_intfs, phyport_intfs, pc_intfs, pc_m
if pc_member[0] == pc_intf:
intf_name = pc_member[1]
break

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please remove spaces?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in the latest commit

Copy link
Collaborator

Choose a reason for hiding this comment

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

i see multiple such instances here, suggest to change your editor setting to automatically remove the trailing spaces.

if intf_name == None:
print >> sys.stderr, 'Warning: cannot find any interfaces that belong to %s' % (pc_intf)
continue
Expand Down Expand Up @@ -564,8 +609,11 @@ def filter_acl_mirror_table_bindings(acls, neighbors, port_channels):
# Main functions
#
###############################################################################

def parse_xml(filename, platform=None, port_config_file=None):
# hostname paramter: To parse minigraph.xml that contains multiple hosts
# information, like in case of multi-asic device or chassis device, this
# parameter can be used to specify the specific hostname for which configuration
# has to be generated.
lguohan marked this conversation as resolved.
Show resolved Hide resolved
def parse_xml(filename, platform=None, port_config_file=None, hostname=None):
root = ET.parse(filename).getroot()
mini_graph_path = filename

Expand All @@ -585,7 +633,7 @@ def parse_xml(filename, platform=None, port_config_file=None):
lo_intfs = None
neighbors = None
devices = None
hostname = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

why delete this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added back

sub_role = None
docker_routing_config_mode = "separated"
port_speeds_default = {}
port_speed_png = {}
Expand All @@ -600,34 +648,47 @@ def parse_xml(filename, platform=None, port_config_file=None):
bgp_peers_with_range = None
deployment_id = None

hwsku_qn = QName(ns, "HwSku")

#hostname is the asic_name, get the asic_id from the asic_name
if hostname is not None:
asic_id = hostname[-1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it always true that asic id will be single digit? can you use regex to match the id?

Copy link
Contributor

Choose a reason for hiding this comment

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

The assumption was the asic_id will be single digit. However as per the comment add a new function to use regex to match the id.

else:
asic_id = None

hwsku_qn = QName(ns, "HwSku")
hostname_qn = QName(ns, "Hostname")
docker_routing_config_mode_qn = QName(ns, "DockerRoutingConfigMode")
for child in root:
if child.tag == str(hwsku_qn):
hwsku = child.text
if child.tag == str(hostname_qn):
hostname = child.text
device_hostname = child.text
if child.tag == str(docker_routing_config_mode_qn):
docker_routing_config_mode = child.text

(ports, alias_map) = get_port_config(hwsku, platform, port_config_file)
if hostname is None:
hostname = device_hostname

(ports, alias_map, alias_asic_map) = get_port_config(hwsku=hwsku, platform=platform, port_config_file=port_config_file, asic=asic_id)
port_alias_map.update(alias_map)
port_alias_asic_map.update(alias_asic_map)

for child in root:
Copy link
Collaborator

@lguohan lguohan May 1, 2020

Choose a reason for hiding this comment

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

suggest to reorg this portion of code for better clarity

if asic_name is None:
    check dpg/cpg/
else:
    check dpg/cpg/...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed as per suggestion

if child.tag == str(QName(ns, "DpgDec")):
(intfs, lo_intfs, mvrf, mgmt_intf, vlans, vlan_members, pcs, pc_members, acls, vni) = parse_dpg(child, hostname)
(intfs, lo_intfs, mvrf, mgmt_intf, vlans, vlan_members, pcs, pc_members, acls, vni) = parse_dpg(child, hostname, device_hostname)
elif child.tag == str(QName(ns, "CpgDec")):
(bgp_sessions, bgp_asn, bgp_peers_with_range, bgp_monitors) = parse_cpg(child, hostname)
elif child.tag == str(QName(ns, "PngDec")):
(neighbors, devices, console_dev, console_port, mgmt_dev, mgmt_port, port_speed_png, console_ports) = parse_png(child, hostname)
(neighbors, devices, console_dev, console_port, mgmt_dev, mgmt_port, port_speed_png, console_ports, device_neighbors) = parse_png(child, hostname, device_hostname)
elif child.tag == str(QName(ns, "UngDec")):
(u_neighbors, u_devices, _, _, _, _, _, _) = parse_png(child, hostname)
(u_neighbors, u_devices, _, _, _, _, _, _, _) = parse_png(child, hostname, device_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)
(syslog_servers, dhcp_servers, ntp_servers, tacacs_servers, mgmt_routes, erspan_dst, deployment_id, sub_role) = parse_meta(child, hostname)
elif child.tag == str(QName(ns, "DeviceInfos")):
(port_speeds_default, port_descriptions) = parse_deviceinfo(child, hwsku)

current_device = [devices[key] for key in devices if key.lower() == hostname.lower()][0]

results = {}
results['DEVICE_METADATA'] = {'localhost': {
'bgp_asn': bgp_asn,
Expand All @@ -638,6 +699,11 @@ def parse_xml(filename, platform=None, port_config_file=None):
'type': current_device['type']
}
}
# for this hostname, if sub_role is defined, add sub_role in
# device_metadata
if sub_role is not None:
current_device['sub_role'] = sub_role
results['DEVICE_METADATA']['localhost']['sub_role'] = sub_role
results['BGP_NEIGHBOR'] = bgp_sessions
results['BGP_MONITORS'] = bgp_monitors
results['BGP_PEER_RANGE'] = bgp_peers_with_range
Expand Down Expand Up @@ -698,9 +764,11 @@ def parse_xml(filename, platform=None, port_config_file=None):

for port_name in port_speed_png:
# not consider port not in port_config.ini
if port_name not in ports:
print >> sys.stderr, "Warning: ignore interface '%s' as it is not in the port_config.ini" % port_name
continue
#If no port_config_file is found ports is empty so ignore this error
if port_config_file is not None:
if port_name not in ports:
print >> sys.stderr, "Warning: ignore interface '%s' as it is not in the port_config.ini" % port_name
continue

ports.setdefault(port_name, {})['speed'] = port_speed_png[port_name]

Expand Down Expand Up @@ -804,11 +872,12 @@ def parse_xml(filename, platform=None, port_config_file=None):
for nghbr in neighbors.keys():
# remove port not in port_config.ini
if nghbr not in ports:
print >> sys.stderr, "Warning: ignore interface '%s' in DEVICE_NEIGHBOR as it is not in the port_config.ini" % nghbr
if port_config_file is not None:
print >> sys.stderr, "Warning: ignore interface '%s' in DEVICE_NEIGHBOR as it is not in the port_config.ini" % nghbr
del neighbors[nghbr]

results['DEVICE_NEIGHBOR'] = neighbors
results['DEVICE_NEIGHBOR_METADATA'] = { key:devices[key] for key in devices if key.lower() != hostname.lower() }
results['DEVICE_NEIGHBOR_METADATA'] = { key:devices[key] for key in devices if key in device_neighbors }
results['SYSLOG_SERVER'] = dict((item, {}) for item in syslog_servers)
results['DHCP_SERVER'] = dict((item, {}) for item in dhcp_servers)
results['NTP_SERVER'] = dict((item, {}) for item in ntp_servers)
Expand Down Expand Up @@ -873,8 +942,17 @@ def parse_device_desc_xml(filename):

return results

def parse_asic_sub_role(filename, asic_name):
if not os.path.isfile(filename):
return None
root = ET.parse(filename).getroot()
for child in root:
if child.tag == str(QName(ns, "MetadataDeclaration")):
_, _, _, _, _, _, _, sub_role = parse_meta(child, asic_name)
lguohan marked this conversation as resolved.
Show resolved Hide resolved
return sub_role

port_alias_map = {}
port_alias_asic_map = {}


def print_parse_xml(filename):
Expand Down
22 changes: 17 additions & 5 deletions src/sonic-config-engine/portconfig.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@
import sys


def get_port_config_file_name(hwsku=None, platform=None):
def get_port_config_file_name(hwsku=None, platform=None, asic=None):
port_config_candidates = []
port_config_candidates.append('/usr/share/sonic/hwsku/port_config.ini')
if hwsku:
if platform:
if asic:
port_config_candidates.append(os.path.join('/usr/share/sonic/device', platform, hwsku, asic,'port_config.ini'))
port_config_candidates.append(os.path.join('/usr/share/sonic/device', platform, hwsku, 'port_config.ini'))
port_config_candidates.append(os.path.join('/usr/share/sonic/platform', hwsku, 'port_config.ini'))
port_config_candidates.append(os.path.join('/usr/share/sonic', hwsku, 'port_config.ini'))
Expand All @@ -17,17 +19,19 @@ def get_port_config_file_name(hwsku=None, platform=None):
return None


def get_port_config(hwsku=None, platform=None, port_config_file=None):

def get_port_config(hwsku=None, platform=None, port_config_file=None, asic=None):
if not port_config_file:
port_config_file = get_port_config_file_name(hwsku, platform)
port_config_file = get_port_config_file_name(hwsku, platform, asic)
if not port_config_file:
return ({}, {})
return ({}, {}, {})
return parse_port_config_file(port_config_file)


def parse_port_config_file(port_config_file):
ports = {}
port_alias_map = {}
port_alias_asic_map = {}
# Default column definition
titles = ['name', 'lanes', 'alias', 'index']
with open(port_config_file) as data:
Expand All @@ -49,6 +53,14 @@ def parse_port_config_file(port_config_file):
data.setdefault('alias', name)
ports[name] = data
port_alias_map[data['alias']] = name
return (ports, port_alias_map)
# asic_port_name to sonic_name mapping also included in
# port_alias_map
if (('asic_port_name' in data) and
(data['asic_port_name'] != name)):
port_alias_map[data['asic_port_name']] = name
# alias to asic_port_name mapping
if 'asic_port_name' in data:
port_alias_asic_map[data['alias']] = data['asic_port_name'].strip()
return (ports, port_alias_map, port_alias_asic_map)


Loading