Added Multi-ASIC support for show ip(v6) route #1216
Conversation
|
This pull request introduces 10 alerts when merging e86309d into 42efc03 - view on LGTM.com new alerts:
|
| combined_route.update(copy.deepcopy(new_route)) | ||
|
|
||
| def print_show_ip_route_hdr(): | ||
| # This prints out the show ip route header based on FRR 7.2 version. |
There was a problem hiding this comment.
if you are using frr, why not use its json output. then you do not need to parse anything.
There was a problem hiding this comment.
@lguohan Yes. I am already getting the json output from FRR of each namespace. The parsing is needed if the display option is for front-end where user does not want to see the routes using nexthops that are associated with back-end port/LAG. Also, merging display is required where the same route may have different nexhop depends on which ASIC was obtained and for display option "front-end" we will need to merge these type of routes from all namespaces and present it to the user.
|
please increase the coverage to 95%. |
Yes I will be adding more testcases to improve the coverage. But most likely on another separate PR. |
The sample you asked actually gets filtered out completely as their nexthops are all back-end interface (LAG) so just based on these two it will not show any output when it is merged. I understood what you are looking for and I will provide a sample that shows this merged output. |
show/bgp_common.py
Outdated
| if display is not None: | ||
| print "display option is not applicable for non-multi-asic platform" | ||
| return |
There was a problem hiding this comment.
The display option argument validation is done on the top of the function. This code is duplicate ?
There was a problem hiding this comment.
@arlakshm Yes. it got covered at the top already. Will remove it.
show/bgp_common.py
Outdated
| for name_space, ns_route in sorted(combined_route.items()): | ||
| print "{}:".format(name_space) | ||
| print_ip_routes(ns_route, filter_by_ip) | ||
There was a problem hiding this comment.
can we removed this extra print
There was a problem hiding this comment.
@arlakshm Sure. I was purposely adding a line here so that there is some separation between each ASIC name space region. I will remove this print.
show/bgp_common.py
Outdated
| print_show_ip_route_hdr() | ||
| if print_ns_str: | ||
| for name_space, ns_route in sorted(combined_route.items()): | ||
| print "{}:".format(name_space) |
There was a problem hiding this comment.
can you change all the print statement to be python3 compatible ?
There was a problem hiding this comment.
@arlakshm Sure. Will replace them to be python3 compatible.
show/bgp_common.py
Outdated
| while len(additional_nh_l): | ||
| a_nh = additional_nh_l.pop() | ||
| combined_route[route][j]['nexthops'].append(a_nh) |
There was a problem hiding this comment.
@arlakshm Agreed. will replace the while loop with + operator to combine the nexthop list.
show/bgp_common.py
Outdated
| # The following protocols do not have multi-nexthops. mbrship test with small list is faster than small set | ||
| # If other protocol are also single nexthop not in this list just add them | ||
| single_nh_proto = ["connected"] | ||
| new_info_l = copy.deepcopy(r_info_l) |
There was a problem hiding this comment.
why do need to a deepcopy here ?
There was a problem hiding this comment.
@arlakshm Agreed. there is no need to have deepcopy here. will remove it.
show/bgp_common.py
Outdated
| 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. | ||
| if user specified a namespace, then print everything. | ||
| if user did not specify name space but specified display for all (include backend), then print each namespace | ||
| without any filtering. But if display is for front-end only, then do filter and combine all output(merge same | ||
| routes from all namespace as additional nexthops) | ||
| This code is based on FRR 7.2 branch. If we moved to a new version we may need to change here as well | ||
| ''' |
There was a problem hiding this comment.
can you move this as doc string of the function ?
show/bgp_common.py
Outdated
| filter_back_end = False | ||
| filter_by_ip = False | ||
| asic_cnt = 0 | ||
| if multi_asic.is_multi_asic(): |
There was a problem hiding this comment.
device.get_ns_list_based_on_options() used below handles both single and multi asic devices. I don't think we need this if condition
|
retest default please |
|
retest this please |
|
@gechiang looks like import failures in the test. Can you take a look. |
|
Very interesting... The import bgp_common was fine previously and nothing changed in this from my last commit so not sure why it is failing... bgpcommon.py is a new file that I am adding as part of this PR so perhaps it is somehow not able to include that in the test build? It builds fine with my private build... |
|
@smaheshm The failure was due to sonic-utilities recently got converted to PYTHON3 and my changes still had some python2 stuffs such as the import that python3 did not like. It is fixed now and all tests passed. |
Add Multi-ASIC support to handle "show ip/ipv6 route" on multi-ASIC devices
Signed-off-by: Gen-Hwa Chiang [email protected]
- What I did
Add support for multi ASIC CLI options for "show ip/ipv6 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
show ip route for all routes (include routes that uses internal interface as nexthop) from ALL ASICs in multi ASIC device
show ipv6 route for specific route from all ASICs (include routes that uses internal interface as nexthop) in multi ASIC device
show ip route for specific route from all ASICs and how that is combined when only front-end interface filtering applied
- 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 **
This is the same PR that I raised previously "#1089" which I closed due to design was changed to handle IPV6 as well as the new "merge routes" criteria.