-
Notifications
You must be signed in to change notification settings - Fork 670
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
[show]add'show ip bgp summary enhanced' #287
base: master
Are you sure you want to change the base?
Conversation
from show.main import * | ||
|
||
def parse_bgp_summary(summ): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parse_bgp_summary [](start = 4, length = 17)
Where is the unit test?
from show.main import * | ||
|
||
def parse_bgp_summary(summ): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def [](start = 0, length = 3)
Suggest move command functions to https://github.com/Azure/sonic-py-swsssdk, so sonic-snmpagent can also access.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, will sync with Joe on this. #Pending
from natsort import natsorted | ||
from tabulate import tabulate | ||
from natsort import natsorted | ||
from tabulate import tabulate | ||
from show.main import * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- [](start = 22, length = 1)
Don't import wildcard in prod code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did not changed wildcard import on this PR. Will sync with Joe on this
proc = subprocess.Popen(command, | ||
stdout=subprocess.PIPE, | ||
shell=True, | ||
stderr=subprocess.STDOUT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STDOUT [](start = 50, length = 6)
Why mix it with stdout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reused the logic of the function from main.py, will check with Joe on this
stderr=subprocess.STDOUT) | ||
stdout = proc.communicate()[0] | ||
proc.wait() | ||
result = stdout.rstrip('\n') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
result [](start = 8, length = 6)
also check return code and stderr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reused the logic of the function from main.py, will check with Joe on this
proc.wait() | ||
result = stdout.rstrip('\n') | ||
except OSError, e: | ||
raise OSError("Error running command") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OSError [](start = 14, length = 7)
raise the same type of exception, no benefit added, and lose the inner exception info.
neighbor_adv_dict = {} | ||
# get advestise prefix per neighbor | ||
for item in neighborIP: | ||
pfxadv = run_command_save('sudo vtysh -c "show ip bgp nei {0} advertised-routes" | grep Total'.format(item)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| [](start = 92, length = 2)
add '| tail -1'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will add
neighbor_adv_dict = {} | ||
# get advestise prefix per neighbor | ||
for item in neighborIP: | ||
pfxadv = run_command_save('sudo vtysh -c "show ip bgp nei {0} advertised-routes" | grep Total'.format(item)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
run_command_save [](start = 21, length = 16)
What if some run_command fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plan to have exception caught on the run_command
neighbor_adv_dict = {} | ||
# get advestise prefix per neighbor | ||
for item in neighborIP: | ||
pfxadv = run_command_save('sudo vtysh -c "show ip bgp nei {0} advertised-routes" | grep Total'.format(item)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pfxadv [](start = 12, length = 6)
Since you invoke the command in sequence, it may have time consistence issue. Does it make sense to collect them and finally show in a table?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is indeed the case, the per peer output is stored on neighbor_adv_dict. Then output all togther
p = subprocess.Popen(bgp_neighbor_cmd, shell=True, stdout=subprocess.PIPE) | ||
bgp_neighbor_dict = json.loads(p.stdout.read()) | ||
|
||
header = ['NeighborIP', 'V', 'AS', 'MsgRcvd', 'MsgSent', 'TblVer', 'InQ', 'OutQ', 'Up/Down', 'State/PfxRcd', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NeighborIP [](start = 19, length = 10)
Suggest keep original name 'Neighbor'. Many vendors use this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As comments
Description The heath metric in ssd_generic for innodisk SSDs is too lazy. Fix to match the entire health number rather than just the first digit. How Has This Been Tested? Manual testing on Mellanox MSN2100
- What I did
Add Device Name for each BGP peer and number of Advertised-route tp 'show ip bgp summary'
- How I did it
- How to verify it
run 'show ip bgp summary enhanced'
- Previous command output (if the output of a command-line utility has changed)
None
- New command output (if the output of a command-line utility has changed)
run show ip ( to see the option)
run show ip bgp summary (No change)
run newly added command 'show ip bgp summary enhanced' ( 2 new columns added at the end)
-->