-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[syncd]: Add socat and bcmsh wrapper #1657
Conversation
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 provide examples here what to expect from this socat?
@@ -0,0 +1,3 @@ | |||
#!/bin/bash | |||
/usr/bin/socat - UNIX-CONNECT:/var/run/sswsyncd/sswsyncd.socket |
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'd like you to add in the base image. #Resolved
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 domain socket file is inside docker-syncd-brcm.
There is no domain socket at all on other platform.
So this wrapper is only useful here.
In reply to: 184841807 [](ancestors = 184841807)
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.
under /var/run/docker-syncd
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 have another wrapper script in base image to do docker exec -it .... #Resolved
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 realized that you are talking about debian base image. I prefer keep it inside brcm-syncd since it is a local specific troubleshooting tool, and not expected to be exposed to host users. The same idea is applied to platform provided dump/dbg/... tools.
In reply to: 184862817 [](ancestors = 184862817)
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 we think this socket is only available in brcm platform, we should keep it in docker-syncd-brcm only.
and if we need make the command available at host side, we should add bcmsh to
platform/broadcom/docker-syncd-brcm/base_image_files/
with wrap command in it:
docker exec -i syncd xx
@@ -19,7 +19,9 @@ debs/{{ deb }}{{' '}} | |||
## TODO: add kmod into Depends | |||
RUN apt-get install -f kmod | |||
|
|||
COPY ["files/dsserve", "files/bcmcmd", "start.sh", "/usr/bin/"] | |||
RUN apt-get install socat |
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.
add into base image. #Resolved
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 an interactive shell through domain socket. The server application is dsserve and syncd. It may be useful for troubleshooting.
#Closed #Closed |
@@ -0,0 +1,3 @@ | |||
#!/bin/bash | |||
/usr/bin/socat - UNIX-CONNECT:/var/run/sswsyncd/sswsyncd.socket |
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 we think this socket is only available in brcm platform, we should keep it in docker-syncd-brcm only.
and if we need make the command available at host side, we should add bcmsh to
platform/broadcom/docker-syncd-brcm/base_image_files/
with wrap command in it:
docker exec -i syncd xx
@qiluo-msft , zhenggen's proposal is good. |
@@ -0,0 +1,3 @@ | |||
#!/bin/bash | |||
/usr/bin/socat - UNIX-CONNECT:/var/run/sswsyncd/sswsyncd.socket |
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.
in the drivshell, can we type quit or exit will that exit bcm shell? if yes, then we need to print out warning to warn user certain action will terminate syncd.
what is the proper way to exit? we should print a banner to that too.
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.
Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>
Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>
Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>
Update:
|
|
||
banner="Press Enter to show prompt. | ||
Press Ctrl+C to exit. | ||
Run command 'exit' will shutdown bcm service. |
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.
Based on my test, exit/quit won't exit the service, this is good so we don't accidentally shut down syncd. but we should remove this line of message
/usr/bin/socat - UNIX-CONNECT:/var/run/sswsyncd/sswsyncd.socket
Enter 'quit' to exit the application.
drivshell>exit
exit
Failed to execute the diagnostic command. Error: Invalid parameter.
drivshell>quit
quit
Exiting the application.
Hit enter to get drivshell prompt..
Enter 'quit' to exit the application.
drivshell>
Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>
This PR contains the following commits: 54b74a2 2021-08-04 [LLDP] Fix lldpshow script to enable display multiple MAC addresses on the same remote physical interface (sonic-net#1657) 0d53b7a 2021-08-03 [sonic_installer] don't print errors when installing an image not supporting app ext (sonic-net#1719) 394e2fb 2021-08-03 Implement script null_route_helper (sonic-net#1737) dd01b56 2021-08-02 disk_check updates: (sonic-net#1736) 8a74d03 2021-07-30 [CLI][show][bgp] Fix the show ip bgp network command (sonic-net#1733) 679a4ba 2021-07-30 [MACsec]: Allow upgrade-docker for macsec container (sonic-net#1716) e9c73e8 2021-07-28 [CLI][MPLS][Show] Added multi ASIC support for 'show mpls command'. Signed-off-by: Basim Shalata <basims@nvidia.com>
This PR is to update sonic-utilities for master branch Changes including ``` 54b74a2 [LLDP] Fix lldpshow script to enable display multiple MAC addresses on the same remote physical interface (#1657) 0d53b7a [sonic_installer] don't print errors when installing an image not supporting app ext (#1719) 394e2fb Implement script null_route_helper (#1737) ``` Signed-off-by: bingwang <bingwang@microsoft.com>
This PR contains the following commits: 54b74a2 2021-08-04 [LLDP] Fix lldpshow script to enable display multiple MAC addresses on the same remote physical interface (#1657) 0d53b7a 2021-08-03 [sonic_installer] don't print errors when installing an image not supporting app ext (#1719) 394e2fb 2021-08-03 Implement script null_route_helper (#1737) dd01b56 2021-08-02 disk_check updates: (#1736) 8a74d03 2021-07-30 [CLI][show][bgp] Fix the show ip bgp network command (#1733) 679a4ba 2021-07-30 [MACsec]: Allow upgrade-docker for macsec container (#1716) e9c73e8 2021-07-28 [CLI][MPLS][Show] Added multi ASIC support for 'show mpls command'. Signed-off-by: Basim Shalata <basims@nvidia.com>
8b149a3 Load the database global_db only once for show cli (#1712) cd0e560 [config][interface][speed] Fixed the config interface speed in multiasic issue (#1739) b595ba6 [fast-reboot] revert the change of disabling counter polling before fast-reboot (#1744) 8518820 [minigraph] Donot enable PFC watchdog for MgmtTsToR (#1734) 2213774 [CLI][show][bgp] Fix the show ip bgp network command (#1733) 3526507 [configlet] Python3 compatible syntax for extracting a key from the dict (#1721) 5b56b97 [sonic_installer] don't print errors when installing an image not supporting app ext (#1719) a581955 [LLDP] Fix lldpshow script to enable display multiple MAC addresses on the same remote physical interface (#1657)
This PR is to update sonic-utilities for master branch Changes including ``` 54b74a2 [LLDP] Fix lldpshow script to enable display multiple MAC addresses on the same remote physical interface (sonic-net#1657) 0d53b7a [sonic_installer] don't print errors when installing an image not supporting app ext (sonic-net#1719) 394e2fb Implement script null_route_helper (sonic-net#1737) ``` Signed-off-by: bingwang <bingwang@microsoft.com>
…n the same remote physical interface (sonic-net#1657) Scenario: 1- remote interface has 2 MACs on the same physical interface. 2- "show lldp table" command displays one entry for only one MAC address Root cause: "show lldp table" command uses lldpshow script to get, parse and display data from the lldp open source package (lldpctl script). lldpctl script returns a proper info about the 2 MACs but the issue is with the lldpshow script parser where it built a dictionary which its key is the local physical interface. Therefore when having 2 MACs, lldpctl will return 2 entries but the lldpshow parser will overwrite the first enrty. Fix: Change the key to be a string of "interface#MAC". This will enable having 2 entries for 2 different MAC addresses. In addition: - update display_sum()-->get_summary_output() to return a string instead of printing it directly. this to allow checking the returned value inside the new unit test. - add a new unit test for this scenario. Signed-off-by: Basim Shalata <basims@nvidia.com>
No description provided.