-
Notifications
You must be signed in to change notification settings - Fork 666
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] enhance 'show ip[v6] bgp summary' command #754
[show] enhance 'show ip[v6] bgp summary' command #754
Conversation
show/bgp_frr_v6.py
Outdated
@@ -19,7 +19,7 @@ def bgp(): | |||
@bgp.command() | |||
def summary(): | |||
"""Show summarized information of IPv6 BGP state""" | |||
run_command('sudo vtysh -c "show bgp ipv6 summary"') | |||
get_bgp_summary_extended('sudo vtysh -c "show ip bgp summary"') |
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.
ipv6 changed to ip ?
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.
Thanks, changed to show bgp ipv6 summary in next iteration
show/bgp_quagga_v6.py
Outdated
@@ -19,7 +19,7 @@ def bgp(): | |||
@bgp.command() | |||
def summary(): | |||
"""Show summarized information of IPv6 BGP state""" | |||
run_command('sudo vtysh -c "show ipv6 bgp summary"') | |||
get_bgp_summary_extended('sudo vtysh -c "show ip bgp summary"') |
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.
ipv6 changed to ip ?
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.
Thanks, changed to show ipv6 bgp summary in next iteration
show/main.py
Outdated
:param command: command to get bgp summary | ||
""" | ||
try: | ||
p = subprocess.Popen(command, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) |
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.
If you can consider : can we modifiy run_command to take a bool arg and return the output but not echo if True, then you can reuse the it at many places.:
def run_command(command, display_cmd=False, return_cmd=False):
if output and return_cmd:
return output
elif output:
click.echo(output.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.
good one. modified in next iteration
show/main.py
Outdated
device_output = p.stdout.read() | ||
modified_output = [] | ||
my_list = iter(device_output.splitlines()) | ||
for element in my_list: |
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.
You can simplify logic. and avoid nested for loops.
import re
data_list = data.splitlines()
new_data = []
for i in data_list:
if i.startswith("Neighbor "):
new_i = "{}\tNeighborName".format(i)
new_data.append(new_i)
elif re.match(r"([0-9A-Fa-f]{1,4}:?|\d+.\d+.\d+.\d+)", i.split(" ")[0]):
new_data.append(i + "\tNameOfNeighbor")
elif i.startswith("Total number "):
new_data.append(i)
else:
new_data.append(i)
for i in new_data:
print(i)
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.
The command output lines that we modify are not always sequential.
for eg.
sometimes the ipv6 summary output have ipv6 in one line and rest parameters comes in another line.
Neighbor V AS MsgRcvd MsgSent TblVer InQ OutQ Up/Down State/PfxRcd
afc0::20:1a2:1:1a
4 64802 506 504 0 0 0 08:17:29 7
So used iterator and nested for loops.
will check if I can simplify this.
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.
removed nested for loops in next iteration
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
show/main.py
Outdated
@@ -201,6 +201,8 @@ def run_command(command, display_cmd=False): | |||
proc = subprocess.Popen(command, shell=True, stdout=subprocess.PIPE) | |||
|
|||
while True: | |||
if return_cmd: | |||
return proc.stdout.read() |
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.
This will not work if output has a lot of data.
You need to use communicate() for that. https://docs.python.org/2/library/subprocess.html#subprocess.Popen.communicate
Also
Warning Use communicate() rather than .stdin.write, .stdout.read or .stderr.read to avoid deadlocks due to any of the other OS pipe buffers filling up and blocking the child process.
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.
Thanks Pavel. Using communicate() from next iteration
show/main.py
Outdated
modified_output.append(element) | ||
elif not element or element.startswith("Total number "): | ||
modified_output.append(element) | ||
elif re.match(r"([0-9A-Fa-f]{1,4}:|\d+.\d+.\d+.\d+)", element.split()[0]): |
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.
your regexp doesn't have '*' prefix, which we have for dynamic neighbors.
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.
fixed in next iteration
name = bgp_neighbor_ip_to_name(ip) | ||
if len(element.split()) == 1: | ||
modified_output.append(element) | ||
element = next(my_list) |
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.
I don't feel safe with next() here. I'm not sure it's a good practice.
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.
element = next(my_list)
The show ipv6 bgp summary
command output have ipv6 address in one line and the other datas in second line.
acb0::10:182:1:10e
4 64802 548 546 0 0 0 08:59:22 7
as the motive of this PR is to add the Neighbor Name at the end of default data, we have to move to the line and append at the end. That is the reason I chose the next method.
:return: string | ||
""" | ||
config_db = ConfigDBConnector() | ||
config_db.connect() |
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.
Can you open that connection only once?
Otherwise you'll open a connection for every neighbor
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.
changed in next iteration
please retest again |
retest this please |
retest this please |
Merge remote-tracking branch 'origin/master' into bgpSummaryWithHostname
retest this please |
2 similar comments
retest this please |
retest this please |
* [show] enhance 'show ip[v6] bgp summary' command * changing ipaddr to ipaddress
6cfb3ecb0248768da0a91e5f7fb4477c5da7eb4e (HEAD -> 201911, origin/201911) [build]: allow to use extra inc/lib location to build the package (sonic-net#595) 40d34872d3b7f354adac67f084eebf6ee467f779 Merge pull request sonic-net#846 from xumia/azp-201911 76ac50f147a7d820b19d8d7628a67f2fe4f5159b Disable the build test 6c9cf655b8b5b152cab1d578e05eddf8238b81b0 Fix branch reference error ca8d81d37a9b0294098f161b036d330d9ff461e0 [ci]: download artifacts from master branch (sonic-net#768) 0cbf4d55c67a9f8f52715f95536f3588acf06c4a [ci]: use sonicbld pool (sonic-net#766) b6f1265ee9bd86f8a5e909a6f1e9b2384497c906 [ci]: add build for arm64 and armhf (sonic-net#757) 9ec0a7da64d479b124815edc5b505fb88b2532a0 CI: add azure pipeline CI/CD (sonic-net#754) 1436dbe02cd3c56f796c6b3398d4075cd05d97e0 Fix RIF issue (sonic-net#835) Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
- What I did
Enhanced show ip bgp summary and show ipv6 bgp summary commands to display neighbor host name
- How I did it
By getting the neighbor info from the CONFIG DB BGP_NEIGHBOR table
- How to verify it
Run show ip bgp summary or show ipv6 bgp summary in the hardware box
- Previous command output (if the output of a command-line utility has changed)
show ip bgp summary
show ipv6 bgp summary
- New command output (if the output of a command-line utility has changed)
show ip bgp summary
show ipv6 bgp summary