Multi-ASIC support for show ip route #1089
Conversation
gechiang
requested review from
jleveque,
arlakshm,
prsunny,
smaheshm,
rlhui and
pavel-shirshov
and removed request for
jleveque and
prsunny
| @@ -7,18 +7,21 @@ | |||
| import re | |||
| import subprocess | |||
| import sys | |||
| import commands | |||
There was a problem hiding this comment.
imports are generally listed in alphabetical order.
There was a problem hiding this comment.
Sure. Will move it based on alphabetical order
| 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"} |
There was a problem hiding this comment.
Let's try to maintain 79 chars in a line if possible.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sure, it's not a hardcore requirement. With 80 chars I get more vertical splits in editor :).
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
if you use 'tabulate' package python will do the formatting for you.
| # print out this string | ||
| print str_2_print | ||
|
|
||
| def print_show_ip_route_hdr(): |
There was a problem hiding this comment.
this can also be moved into route.py
There was a problem hiding this comment.
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.
| filter_back_end = False | ||
| else: | ||
| filter_back_end = True | ||
| if str(namespace) == "None": |
There was a problem hiding this comment.
can be simplified to:
filter_back_end = False if display == "all" else True
There was a problem hiding this comment.
Nice! Still in the old "c" programming world... :)
Agreed.
| filter_back_end = False | ||
| else: | ||
| filter_back_end = True | ||
| if str(namespace) == "None": |
There was a problem hiding this comment.
why not:
if namespace == None:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agreed. Forgot to remove it after a bug fix during unit testing.
There was a problem hiding this comment.
@jleveque Yes. the user may not specify any namespace and in that case it will result to empty string.
| @click.option('--verbose', is_flag=True, help="Enable verbose output") | ||
| def route(args, verbose): | ||
| def route(args, namespace, display, verbose): |
There was a problem hiding this comment.
can you also split this up.
There was a problem hiding this comment.
Agreed. Will split up to improve code readability
| 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" |
There was a problem hiding this comment.
let's add 'json' option as 'click' option. Mush easier.
There was a problem hiding this comment.
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.
| 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(): |
There was a problem hiding this comment.
check_backend_nexthop can be a separate API.
| return | ||
| if output[0] == "%": | ||
| # remove the "json" keyword that was added by this handler so it wont confuse the user | ||
| error_msg = output[:-4] |
There was a problem hiding this comment.
what if "json" keywork wasn't specified.
This can be removed if 'json' is passed as click option.
There was a problem hiding this comment.
See previous comment response in regard to this "optional" argument. which we intent to keep the same way as before.
| if not hdr_printed: | ||
| print_show_ip_route_hdr() | ||
| hdr_printed = True | ||
| print_ip_routes(filtered_route) |
There was a problem hiding this comment.
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.
|
Many changes was made to support "merging" and IPV6. |
Add Multi-ASIC support to handle "show ip route" on multi-ASIC devices
Depends on the following PRs
Signed-off-by: Gen-Hwa Chiang gechiang@microsoft.com
- What I did
Add support for multi ASIC CLI options for "show ip route"
2 new options have added
[-n, --namespace] to allow user to display the information for given namespaces (ASIC)
If this option is not present the information from all the namespaces will be displayed
[-d, --display] to allow user to display ip routes related with nexthop that are going through both internal and external interfaces
If this option is not present only ip routes with external interfaces as its nexthop will be display
On single ASIC platform, this options are not valid, so the behavior remains unchanged
- How I did it
- How to verify it
Help menu
show ip route for all routes (include routes that uses internal interface as nexthop) from ASIC0 in multi ASIC device
show ip route (exclude routes that uses internal interface as nexthop) from ASIC0 in multi ASIC device
show ip route (exclude routes that uses internal interface as nexthop) from ASIC5 in multi ASIC device in json format
show ip route (exclude routes that uses internal interface as nexthop) from ASIC5 in multi ASIC device
show ip route for all routes (exclude routes that uses internal interface as nexthop) from ALL ASICs in multi ASIC device
- Previous command output (if the output of a command-line utility has changed)
- New command output (if the output of a command-line utility has changed)
** Please Note **
The test case changes will be submitted with a separate PR later so that we can take advantage of some other multi-ASIC CLI PR that have already created the test modules that can be reused without too much duplication of work.
Note 2
This is the same PR that I raised previously "#1060 (review)" which I closed due to messed up my last update commits thus recreating with all the update made based on previous comments in this new PR.