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

Fix the bcmsh issues. #1761

Merged
merged 1 commit into from
Jun 6, 2018
Merged

Conversation

zhenggen-xu
Copy link
Collaborator

@zhenggen-xu zhenggen-xu commented Jun 1, 2018

-- bcmsh in docker is not executable.
-- bcmsh is not copied to /usr/bin/ at host side

- What I did
Fix the issues with bcmsh:
--bcmsh is not executable today in docker, so if you type bcmsh, it won't work.
--bcmsh is not at /usr/bin so you can not type that command directly.

- How I did it
Fix above two issues.

- How to verify it
bcmsh is working at host and docker.

admin@lnos-x1-a-asw03:~$ bcmsh
Press Enter to show prompt.
Press Ctrl+C to exit.

drivshell>

- Description for the changelog

Fix bcmsh issues (#1761)

- A picture of a cute animal (not mandatory but encouraged)

@lguohan lguohan requested a review from qiluo-msft June 5, 2018 10:53
@@ -20,7 +20,7 @@ debs/{{ deb }}{{' '}}
RUN apt-get install -f kmod

COPY ["files/dsserve", "files/bcmcmd", "start.sh", "bcmsh", "/usr/bin/"]
RUN chmod +x /usr/bin/dsserve /usr/bin/bcmcmd
RUN chmod +x /usr/bin/dsserve /usr/bin/bcmcmd /usr/bin/bcmsh
Copy link
Collaborator

Choose a reason for hiding this comment

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

x [](start = 11, length = 1)

Are you sure? The original bcmsh used in build is executable.

@@ -17,3 +17,4 @@ $(DOCKER_SYNCD_BRCM)_RUN_OPT += -v /var/run/docker-syncd:/var/run/sswsyncd
$(DOCKER_SYNCD_BRCM)_RUN_OPT += -v /etc/sonic:/etc/sonic:ro

$(DOCKER_SYNCD_BRCM)_BASE_IMAGE_FILES += bcmcmd:/usr/bin/bcmcmd
$(DOCKER_SYNCD_BRCM)_BASE_IMAGE_FILES += bcmsh:/usr/bin/bcmsh
Copy link
Collaborator

@qiluo-msft qiluo-msft Jun 5, 2018

Choose a reason for hiding this comment

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

bcmsh [](start = 56, length = 5)

I think bcmsh script will not work in host, because the domain socket is in a different place there. #Closed

Copy link
Collaborator Author

@zhenggen-xu zhenggen-xu Jun 5, 2018

Choose a reason for hiding this comment

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

Disagree here. The script in the change is not the original script inside docker, it is as below:

#!/bin/bash
docker exec -i syncd bcmsh "$@"

It calls the script inside the docker and the same domain socket is used.
Everything works fine from host side, and people don't need go to docker to type the command.
#Closed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! I just found I wrote the wrapper:)


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

Copy link
Collaborator Author

@zhenggen-xu zhenggen-xu Jun 5, 2018

Choose a reason for hiding this comment

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

still marked as "Changes requested", can you remove that? #Closed

Copy link
Collaborator

@qiluo-msft qiluo-msft 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

-- bcmsh is not copied to /usr/bin/ at host side
@zhenggen-xu zhenggen-xu closed this Jun 5, 2018
@zhenggen-xu zhenggen-xu reopened this Jun 5, 2018
@qiluo-msft qiluo-msft merged commit 83d9c7e into sonic-net:master Jun 6, 2018
@zhenggen-xu zhenggen-xu deleted the github-fork-bcmsh branch June 7, 2019 19:35
qiluo-msft pushed a commit that referenced this pull request Sep 1, 2021
#### Why I did it
Fixing issue [[sonic-utilities] Unit test failed when building sonic-utilities #1761](sonic-net/sonic-utilities#1761)

Importing `sonic-acl` caused getting references by `backlinks()` to break, 

#### How I did it
solution is to comment out the importing statement as it is not used anyway.

#### How to verify it
Ran sonic-utilities unit-tests locally after the fix, and all passed.
volodymyrsamotiy added a commit to volodymyrsamotiy/sonic-buildimage that referenced this pull request Sep 3, 2021
* d240291 Update port_rates & rif_rates lua scripts to convert poll_interval to MS (sonic-net#1855)
* a71a5d3 [acl mirror action] Mirror session ref count fix at acl rule attachment (sonic-net#1761)
* 197f427 Fix vs test failure in test_buffer_traditional (sonic-net#1881)
* 8471f42 Revert "[debugcounterorch] check if counter type is supported before querying… (sonic-net#1789)" (sonic-net#1884)

Signed-off-by: Volodymyr Samotiy <[email protected]>
prsunny pushed a commit that referenced this pull request Sep 3, 2021
* d240291 Update port_rates & rif_rates lua scripts to convert poll_interval to MS (#1855)
* a71a5d3 [acl mirror action] Mirror session ref count fix at acl rule attachment (#1761)
* 197f427 Fix vs test failure in test_buffer_traditional (#1881)
* 8471f42 Revert "[debugcounterorch] check if counter type is supported before querying… (#1789)" (#1884)

Signed-off-by: Volodymyr Samotiy <[email protected]>
theasianpianist pushed a commit to theasianpianist/sonic-buildimage that referenced this pull request Feb 5, 2022
…nt (sonic-net#1761)

* Fix mirror session ref count at acl rule attachement
Signed-off-by: wenda.ni <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants