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

Multi-ASIC support for show ip route #1089

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
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
237 changes: 229 additions & 8 deletions show/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,21 @@
import re
import subprocess
import sys
import commands
Copy link
Contributor

Choose a reason for hiding this comment

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

imports are generally listed in alphabetical order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Will move it based on alphabetical order



import click
from natsort import natsorted
from pkg_resources import parse_version
from sonic_py_common import device_info
from sonic_py_common import multi_asic
from swsssdk import ConfigDBConnector
from swsssdk import SonicV2Connector
from tabulate import tabulate

import utilities_common.cli as clicommon
from utilities_common.db import Db
from utilities_common import constants
from utilities_common.multi_asic import multi_asic_click_options

import feature
Expand Down Expand Up @@ -768,23 +771,241 @@ def get_bgp_peer():
bgp_peer.setdefault(local_addr, [neighbor_name, neighbor_ip])
return bgp_peer

def print_ip_routes(route_info):
"""
Sample Entry output
B>*172.16.8.2/32 [20/0] via 10.0.0.47, Ethernet92, 03:33:01
B>*192.168.114.96/32 [20/0] via 10.0.0.1, PortChannel0002, 03:30:39
* via 10.0.0.5, PortChannel0005, 03:30:39
* via 10.0.0.9, PortChannel0008, 03:30:39
* via 10.0.0.13, PortChannel0011, 03:30:39
* via 10.0.0.17, PortChannel0014, 03:30:39
* via 10.0.0.21, PortChannel0017, 03:30:39
* via 10.0.0.25, PortChannel0020, 03:30:39
* via 10.0.0.29, PortChannel0023, 03:30:39
B 10.0.107.0/31 [200/0] via 10.0.107.1, inactive 00:10:15
K>*0.0.0.0/0 [0/0] via 10.3.146.1, eth0, 00:25:22
B 0.0.0.0/0 [20/0] via 10.0.0.1, PortChannel0002, 03:31:52
via 10.0.0.5, PortChannel0005, 03:31:52
via 10.0.0.9, PortChannel0008, 03:31:52
via 10.0.0.13, PortChannel0011, 03:31:52
via 10.0.0.17, PortChannel0014, 03:31:52
via 10.0.0.21, PortChannel0017, 03:31:52
via 10.0.0.25, PortChannel0020, 03:31:52
via 10.0.0.29, PortChannel0023, 03:31:52
S 0.0.0.0/0 [200/0] via 10.3.146.1, eth0, 03:35:18
C>*10.0.0.62/31 is directly connected, Ethernet124, 03:34:00
"""
# This method is following what zebra_vty.c does when handling the route parsing printing
# The route_info is a json file which is treated as a dictionary of a bunch of route + info
# This interpretation is based on FRR 7.2 branch. If we later moved on to a new branch, we may
# have to rexamine if there are any changes made that may impact the parsing logic
proto_code = {"system":'X', "kernel":'K', "connected":'C', "static":'S', "rip":'R', "ripng":'R',
"ospf":'O', "ospf6":'O', "isis":'I', "bgp":'B', "pim":'P', "hsls":'H', "olsr":'o', "babel":'A'}
mpls_label_code = { 0:"IPv4 Explicit Null", 1:"Router Alert", 2:"IPv6 Explicit Null", 3:"implicit-null", 4:"Reserved (4)",
5:"Reserved (5)", 6:"Reserved (6)", 7:"Entropy Label Indicator", 8:"Reserved (8)", 9:"Reserved (9)", 10:"Reserved (10)",
11:"Reserved (11)", 12:"Reserved (12)", 13:"Generic Associated Channel", 14:"OAM Alert", 15:"Extension"}
Comment on lines +805 to +807
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's try to maintain 79 chars in a line if possible.

Copy link
Contributor

@jleveque jleveque Sep 2, 2020

Choose a reason for hiding this comment

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

This is debatable now that we all have wider screens. I'm OK with making one exception to the PEP8 standard and extending the max line length to around 100 chars. Using a tool like autopep8 --max-line-length 100 can be helpful. We can discuss this more offline.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, it's not a hardcore requirement. With 80 chars I get more vertical splits in editor :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I will reduce the length shorter to not exceed more than 100 chars.
The code can become harder to read if need to strictly follow this 79 chars limitation.

NH_F_IS_RECURSIVE = 2
NH_F_IS_DUPLICATE = 5
for route, info in route_info.items():
for i in range(0, len(info)):
str_2_print = ""
str_2_print += proto_code[info[i]['protocol']]
if "instance" in info[i]:
str_2_print += "[" + str(info[i]['instance']) + "]"
if "selected" in info[i]:
str_2_print += ">"
else:
str_2_print += " "
for j in range(0, len(info[i]['nexthops'])):
if j != 0:
str_2_print = " "
if "queued" in info[i]:
str_2_print += "q"
elif "failed" in info[i]:
str_2_print += "r"
elif "installed" in info[i]:
if info[i]['nexthops'][j]['flags'] & (1 << NH_F_IS_RECURSIVE) or info[i]['nexthops'][j]['flags'] & (1 << NH_F_IS_DUPLICATE):
str_2_print += " "
else:
str_2_print += "*"
# on 1st nexhop print the prefix and distance/metric if appropriate.
# on all subsequent nexthop replace the prefix and distance/metric by empty spaces only.
if j == 0:
str_2_print += info[i]['prefix']
if info[i]['protocol'] != "connected":
str_2_print += " [" + str(info[i]['distance']) + "/" + str(info[i]['metric']) + "]"
elif info[i]['distance'] > 0 or info[i]['metric'] > 0:
str_2_print += " [" + str(info[i]['distance']) + "/" + str(info[i]['metric']) + "]"
str_length = len(str_2_print)
else:
str_2_print += " "*(str_length - 3)
if "ip" in info[i]['nexthops'][j]:
str_2_print += " via " + info[i]['nexthops'][j]['ip'] + ","
if "interfaceName" in info[i]['nexthops'][j]:
str_2_print += " " + info[i]['nexthops'][j]['interfaceName'] + ","
elif "directlyConnected" in info[i]['nexthops'][j]:
str_2_print += " is directly connected,"
if "interfaceName" in info[i]['nexthops'][j]:
str_2_print += " " + info[i]['nexthops'][j]['interfaceName'] + ","
elif "unreachable" in info[i]['nexthops'][j]:
if "reject" in info[i]['nexthops'][j]:
str_2_print += " (ICMP unreachable)"
elif "admin-prohibited" in info[i]['nexthops'][j]:
str_2_print += " (ICMP admin-prohibited)"
elif "blackhole" in info[i]['nexthops'][j]:
str_2_print += " (blackhole)"
if "vrf" in info[i]['nexthops'][j]:
str_2_print += "(vrf " + info[i]['nexthops'][j]['vrf'] + ", " + info[i]['nexthops'][j]['interfaceName'] + ","
if "active" not in info[i]['nexthops'][j]:
str_2_print += " inactive"
if "onLink" in info[i]['nexthops'][j]:
str_2_print += " onlink"
if "recursive" in info[i]['nexthops'][j]:
str_2_print += " (recursive)"
if "source" in info[i]['nexthops'][j]:
str_2_print += ", src " + info[i]['nexthops'][j]['source']
if "labels" in info[i]['nexthops'][j]:
# MPLS labels are stored as an array (list) in json if present. Need to print through each one in list
str_2_print += ", label "
for k in range(0, len(info[i]['nexthops'][j]['labels'])):
# MPLS labels that has value 15 or lower has special interpretation
if info[i]['nexthops'][j]['labels'][k] > 15:
label_string = str(info[i]['nexthops'][j]['labels'][k])
else:
label_string = mpls_label_code[info[i]['nexthops'][j]['labels'][k]]
if k == 0:
str_2_print += label_string
else:
str_2_print += "/" + label_string
# add uptime at the end of the string
str_2_print += " " + info[i]['uptime']
# print out this string
print str_2_print
Comment on lines +808 to +884
Copy link
Contributor

Choose a reason for hiding this comment

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

This too long, lets split it up. There are few types of next hops. Each type can have it's own render function that returns a formatted string. Also these functions can be in utils.py or you can also create a route.py.
show/main.py is the main CLI, this can just call an API that returns a formatted string.

Wondering for formatting if we can use python string formats.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Let me split up at section where it is looking at the same field but with different possibilities.
Unfortunately this logic here is not something sharable to other purpose so will not be moving this to other files...

Copy link
Contributor

Choose a reason for hiding this comment

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

if you use 'tabulate' package python will do the formatting for you.


def print_show_ip_route_hdr():
Copy link
Contributor

Choose a reason for hiding this comment

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

this can also be moved into route.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you are saying now.. basically you want me to move all the helper routines to route.py.
To minimize the impact for now I will do this refactoring on next phase of the changes to move all helper routines to route.py
for this PR, let me keep it here in main.py for now. I will have to work on the mock test cases next which will do this refactoring at that time as well.

# This prints out the show ip route header based on FRR 7.2 version.
# Please note that if we moved to future versions, we may heva to make changes to this
print("Codes: K - kernel route, C - connected, S - static, R - RIP,")
print(" O - OSPF, I - IS-IS, B - BGP, E - EIGRP, N - NHRP,")
print(" T - Table, v - VNC, V - VNC-Direct, A - Babel, D - SHARP,")
print(" F - PBR, f - OpenFabric,")
print(" > - selected route, * - FIB route, q - queued route, r - rejected route\n")

#
# 'route' subcommand ("show ip route")
#

@ip.command()
@click.argument('args', metavar='[IPADDRESS] [vrf <vrf_name>] [...]', nargs=-1, required=False)
@multi_asic_util.multi_asic_click_options
@click.option('--verbose', is_flag=True, help="Enable verbose output")
def route(args, verbose):
def route(args, namespace, display, verbose):
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 also split this up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Will split up to improve code readability

"""Show IP (IPv4) routing table"""
cmd = 'sudo vtysh -c "show ip route'

is_multi_asic = multi_asic.is_multi_asic()
device = multi_asic_util.MultiAsic(display, namespace)
arg_strg = ""
found_json = 0
hdr_printed = False
del_cnt = 0
if is_multi_asic:
# handling multi-ASIC by gathering the output from specified/all name space into a dictionary via
# jason option and then filter out the json entries (by removing those next Hop that are
# back-end ASIC if display is for front-end only). If the entry itself has no more next Hop after filtering,
# then skip over that particular route entry. Once completed, if the user chose "json" option,
# then just print out the dictionary in Json format accordingly. But if no "json" option specified,
# then print out the header and the decoded entry representation for each route accordingly.
# This code is based on FRR 7.2 branch. If we moved to a new version we may need to change here as well
asic_cnt = multi_asic.get_num_asics()
if display == "all":
filter_back_end = False
else:
filter_back_end = True
if str(namespace) == "None":
Copy link
Contributor

Choose a reason for hiding this comment

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

can be simplified to:

filter_back_end = False if display == "all" else True

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! Still in the old "c" programming world... :)
Agreed.

Copy link
Contributor

Choose a reason for hiding this comment

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

why not:

if namespace == None:

Copy link
Contributor

@jleveque jleveque Sep 2, 2020

Choose a reason for hiding this comment

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

Even better:

if not namepsace:

However, this will also catch the empty string (''), which is used as the default namespace. If you expect the default namespace here, then go with @smaheshm's approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Forgot to remove it after a bug fix during unit testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jleveque Yes. the user may not specify any namespace and in that case it will result to empty string.

start_range = 0
end_range = asic_cnt
else:
# -n asic0|asic1|...|asicN So skip the first 4 chars to get the namespace number only
start_range = int(namespace[4:])
end_range = start_range + 1
else:
asic_cnt = 1
filter_back_end = False
start_range = 0
end_range = asic_cnt
# get all the other arguments except json that needs to be the last argument of the cmd if present
for arg in args:
cmd += " " + str(arg)

cmd += '"'

run_command(cmd, display_cmd=verbose)
arg_strg += " " + str(arg)
if str(arg) == "json":
found_json = 1
if not found_json:
arg_strg += " json"
Comment on lines 937 to +942
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add 'json' option as 'click' option. Mush easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original design of this handler before my changes purposely left these and many other options as variable arguments. This is done this way so that there is less impact when there are new options added or removed from the Zebra implementation. So in the same spirit we should not be adding this as a 'click' option.


for ns in range(start_range, end_range):
filtered_route = {}
# Need to add "ns" to form bgpX so it is sent to the correct bgpX docker to handle the request
# If not MultiASIC, use space instad of ns number
if is_multi_asic:
ns_str = str(ns)
else:
ns_str = " "
cmd = "sudo docker exec bgp{0} vtysh -c 'show ip route {1}'".format(ns_str, arg_strg)
output = commands.getoutput(cmd)
# in case no output or something went wrong with user specified cmd argument(s) error it out
# error from FRR always start with character "%"
if output == "":
return
if output[0] == "%":
# remove the "json" keyword that was added by this handler so it wont confuse the user
error_msg = output[:-4]
Copy link
Contributor

Choose a reason for hiding this comment

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

what if "json" keywork wasn't specified.

This can be removed if 'json' is passed as click option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See previous comment response in regard to this "optional" argument. which we intent to keep the same way as before.

print error_msg
return
route_info = json.loads(output)
if filter_back_end:
# clean up the dictionary to remove all the nexthops that are back-end interface
for route, info in route_info.items():
Copy link
Contributor

Choose a reason for hiding this comment

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

check_backend_nexthop can be a separate API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

new_info_l = []
new_info_cnt = 0
for i in range(0, len(info)):
new_info = info[i]
new_nhop_l = []
del_cnt = 0
while len(info[i]['nexthops']):
nh = info[i]['nexthops'].pop(0)
if "interfaceName" in nh:
obj_type = constants.PORT_CHANNEL_OBJ
if nh['interfaceName'].find('Ethernet') != -1:
obj_type = constants.PORT_OBJ
if device.skip_display(obj_type, nh['interfaceName']):
del_cnt += 1
else:
new_nhop_l.append(nh)
else:
new_nhop_l.append(nh)
# use the new filtered nhop list if it is not empty. if empty nexthop , this route is filtered out completely
if len(new_nhop_l) > 0:
new_info['nexthops'] = new_nhop_l
new_info_cnt += 1
# in case there are any nexthop that were deleted, we will need to adjust the nexhopt counts as well
if del_cnt > 0:
internalNextHopNum = info[i]['internalNextHopNum'] - del_cnt
new_info['internalNextHopNum'] = internalNextHopNum
internalNextHopActiveNum = info[i]['internalNextHopActiveNum'] - del_cnt
new_info['internalNextHopActiveNum'] = internalNextHopActiveNum
new_info_l.append(new_info)
if new_info_cnt:
filtered_route[route] = new_info_l
else:
filtered_route = route_info
if not found_json:
#print out the header if this is not a json request
if not hdr_printed:
print_show_ip_route_hdr()
hdr_printed = True
print_ip_routes(filtered_route)
Copy link
Contributor

Choose a reason for hiding this comment

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

all ASICs have the same route table, with some routes with different next hops.

It looks like the routing table is being printed for each ASIC independently. We should use prefix as key and merge next hops, and print a prefix only once.

else:
new_string = json.dumps(filtered_route, indent=2)
print(new_string)

#
# 'prefix-list' subcommand ("show ip prefix-list")
Expand Down