Added show ip/v6 route summary support for multi-asic platform#1320
Added show ip/v6 route summary support for multi-asic platform#1320gechiang merged 2 commits intosonic-net:masterfrom gechiang:master
Conversation
|
retest default please |
|
retest this please |
| error_msg = output | ||
| print(error_msg) | ||
| return | ||
|
|
There was a problem hiding this comment.
If one of the bgp instances in the middle returns error. Output from the remaining instances wont be displayed ?
There was a problem hiding this comment.
Yes. That is the intention. Most of the case when the FRR of that namespace reject the cmd is due to the parameter the user specified. and if one name space rejects it, all other namespace will reject the same way so it is best to stop on first error. Also, if in case the user thinks the issue is only specific to the failing name space but not on other name space, user can always use -n option to display from which namespace he likes to see.
show/bgp_common.py
Outdated
| back_end_intf_set = None | ||
| # get all the other arguments except json that needs to be the last argument of the cmd if present | ||
| # Handling of multi-asic show ip/v6 route summary will print directly FRR result of each namespace | ||
| check_summary_set = set(["sum", "summ", "summa", "summar", "summary"]) |
There was a problem hiding this comment.
Instead of adding the check for argument is "summary" here. Can we check if the args is empty to aggregate the routes from all namesapces for all other argument we can display the output as-is for each namespace ?
There was a problem hiding this comment.
We also like to aggregate the routes from all namespaces for the case where the user specified specific route or in json format. So for these cases the argument will not be empty but we still like to combine the output from all namespaces and display it. Are you saying that other than these cases we should always default to let each specified namespace FRR to handle those additional parameter options similar to the "summary" case? There will be cases where the "json" option will also be honored and for those we will NOT combine the jason output but have each one displayed based on its own name space which will result to multiple json outputs depends on namespaces involved... Is this acceptable? Or should we handle this "summary" as designed for now and make additional enhancement as we see there is a need to do so?
There was a problem hiding this comment.
@arlakshm I have made the changes to do as you suggested. So other than the specific route specified and the json option for "show ip/v6 route" command such as "summary, table, tables,...etc" will now be all handled by the corresponding namespace FRR directly. This should also be easier when SONiC decided to upgrade to higher FRR version in the future.
show/bgp_common.py
Outdated
| found_json = 1 | ||
| elif str(arg) == "tables": | ||
| found_tables = 1 | ||
| elif set([str(arg)]).issubset(check_summary_set): |
There was a problem hiding this comment.
Not sure I understand the logic. Why not:
if str(arg) in check_summary_set:
or
if "summary".find(str(arg)) == 0:
There was a problem hiding this comment.
The logic is to allow partial matching so that user need not specify the complete word "summary".
This logic is needed so that if user specify something like "umm" it will not be accepted. it needs to start with "sum" and maybe additional matching chars after "sum".
…ow ip/v6 route cmd will default to be handled by the corresponding namespace FRR directly
|
retest this please |
…-net#1320) * Added show ip/v6 route summary support for multi-asic platform by making any additional parameter handling other than specific route cases or json format to be handled by the corresponding namespace FRR directly.
3 participants
- What I did
Added Multi-ASIC support of handling "show ip/v6 route summary"
This was not supported for multi-ASIC platform previously.
When running this cmd under multi-ASIC platform the following output are expected:
- How to verify it
Perform "show ip/v6 route summary" on multi-asic platform to see that this option is now supported.
Before the change:
After the added support: