Skip to content
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

Merged
merged 4 commits into from
May 4, 2018

Conversation

qiluo-msft
Copy link
Collaborator

No description provided.

@qiluo-msft qiluo-msft requested a review from lguohan April 28, 2018 05:55
Copy link
Collaborator

@zhenggen-xu zhenggen-xu left a 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
Copy link
Collaborator

@lguohan lguohan Apr 28, 2018

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

Copy link
Collaborator Author

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)

Copy link
Collaborator

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

Copy link
Collaborator

@lguohan lguohan Apr 28, 2018

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

Copy link
Collaborator Author

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)

Copy link
Collaborator

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
Copy link
Collaborator

@lguohan lguohan Apr 28, 2018

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

Copy link
Collaborator Author

@qiluo-msft qiluo-msft Apr 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I added into docker-base.


In reply to: 184841809 [](ancestors = 184841809)

@qiluo-msft
Copy link
Collaborator Author

qiluo-msft commented Apr 28, 2018

It is an interactive shell through domain socket. The server application is dsserve and syncd. It may be useful for troubleshooting.

root@str-s6100-acs-2:/# socat - UNIX-CONNECT:/var/run/sswsyncd/sswsyncd.socket

drivshell>ps
ps
                 ena/        speed/ link auto    STP                  lrn  inter   max   cut   loop
           port  link  Lns   duplex scan neg?   state   pause  discrd ops   face frame  thru?  back
       xe0(104)  up     2   40G  FD   SW  No   Forward  TX RX   None    F    CR4  9122    No
       xe1(105)  up     2   40G  FD   SW  No   Forward  TX RX   None    F    CR4  9122    No
...
      xe65(100)  !ena   1   10G  FD   SW  No   Forward  TX RX   None   FA    XFI  9412    No

drivshell>^Croot@str-s6100-acs-2:/#

#Closed #Closed

@@ -0,0 +1,3 @@
#!/bin/bash
/usr/bin/socat - UNIX-CONNECT:/var/run/sswsyncd/sswsyncd.socket
Copy link
Collaborator

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

@lguohan
Copy link
Collaborator

lguohan commented Apr 30, 2018

@qiluo-msft , zhenggen's proposal is good.

@@ -0,0 +1,3 @@
#!/bin/bash
/usr/bin/socat - UNIX-CONNECT:/var/run/sswsyncd/sswsyncd.socket
Copy link
Collaborator

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.

Copy link
Collaborator

@lguohan lguohan left a 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>
@qiluo-msft
Copy link
Collaborator Author

qiluo-msft commented May 4, 2018

Update:

  1. Add wrapper in host
  2. No need for host wrapper to communicate by domain socket, following bcmcmd, and keep the real program running inside the docker
  3. Add banner at the beginning of the bcmsh to instruct the keys
  4. Install socat in docker-base, considering the small size and useful usage.


banner="Press Enter to show prompt.
Press Ctrl+C to exit.
Run command 'exit' will shutdown bcm service.
Copy link
Collaborator

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>
@qiluo-msft qiluo-msft merged commit f3f8b9f into sonic-net:master May 4, 2018
@qiluo-msft qiluo-msft deleted the qiluo/socat branch May 4, 2018 16:27
BasimShalata added a commit to BasimShalata/sonic-buildimage that referenced this pull request Aug 4, 2021
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>
lguohan pushed a commit that referenced this pull request Aug 6, 2021
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>
lguohan pushed a commit that referenced this pull request Aug 6, 2021
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>
judyjoseph added a commit that referenced this pull request Aug 7, 2021
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)
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
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>
taras-keryk pushed a commit to taras-keryk/sonic-buildimage that referenced this pull request Apr 28, 2022
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants