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

[swss] Add support for gearbox phys #1321

Merged
merged 9 commits into from
Jul 2, 2020
Merged

[swss] Add support for gearbox phys #1321

merged 9 commits into from
Jul 2, 2020

Conversation

sydlogan
Copy link
Contributor

  • builds on support for multiple switches in sonic-sairedis
  • adds gearsync daemon
  • parsers for gearbox-related configuration files, plus unit tests
  • gearboxutils for determining if gearbox supported and other functions
  • changes to portsorch and orchagent to support gearbox creation and attributes

What I did

Add support for gearbox phys based on sonic-sairedis support of multiple switches.

Why I did it

Work is done in collaboration with Microsoft engineers.

How I verified it

Builds and tests using VS builds (dependency on sonic-sairedis changes that are pending).

Details if related

* builds on support for multiple switches in sonic-sairedis
* adds gearsync daemon
* parsers for gearbox-related configuration files, plus unit tests
* gearboxutils for determining if gearbox supported and other functions
* changes to portsorch and orchagent to support gearbox creation and attributes

HLD is located at https://github.com/Azure/SONiC/blob/b817a12fd89520d3fd26bbc5897487928e7f6de7/doc/gearbox/gearbox_mgr_design.md

  Signed-off-by: syd.logan@broadcom.com
@sydlogan sydlogan closed this Jun 16, 2020
@sydlogan sydlogan reopened this Jun 16, 2020
@sydlogan sydlogan requested a review from lguohan June 19, 2020 16:13
gearsyncd/gearboxparser.cpp Outdated Show resolved Hide resolved
gearsyncd/gearboxparser.cpp Outdated Show resolved Hide resolved
@sydlogan sydlogan closed this Jun 24, 2020
@sydlogan sydlogan reopened this Jun 24, 2020
gearsyncd/gearboxparser.h Outdated Show resolved Hide resolved
gearsyncd/gearsyncd.cpp Outdated Show resolved Hide resolved
@sydlogan sydlogan closed this Jun 25, 2020
@sydlogan sydlogan reopened this Jun 25, 2020
sydlogan pushed a commit to sydlogan/sonic-buildimage that referenced this pull request Jun 25, 2020
… support VS gearbox phy feature

* scripts and configuration needed to support a second syncd docker (physyncd)
* physyncd supports gearbox device and phy SAI APIs and runs multiple instances of syncd, one per phy in the device
* support for VS target (sonic-sairedis vslib has been extended to support a virtual BCM81724 gearbox PHY).

HLD is located at https://github.com/Azure/SONiC/blob/b817a12fd89520d3fd26bbc5897487928e7f6de7/doc/gearbox/gearbox_mgr_design.md

**- Why I did it**

This work is part of the gearbox phy joint effort between Microsoft and Broadcom, and is based
on multi-switch support in sonic-sairedis.

**- How I did it**

Overall feature was implemented across several projects. The collective pull requests (some in late stages of review at this point):

sonic-net/sonic-utilities#931 - CLI (merged)
sonic-net/sonic-swss-common#347 - Minor changes (merged)
sonic-net/sonic-swss#1321 - gearsyncd, config parsers, changes to orchargent to create gearbox phy on supported systems
sonic-net/sonic-sairedis#624 - physyncd, virtual BCM81724 gearbox phy added to vslib

**- How to verify it**

In a vslib build:

root@sonic:/home/admin# show gearbox interfaces status
  PHY Id    Interface        MAC Lanes    MAC Lane Speed        PHY Lanes    PHY Lane Speed    Line Lanes    Line Lane Speed    Oper    Admin
--------  -----------  ---------------  ----------------  ---------------  ----------------  ------------  -----------------  ------  -------
       1   Ethernet48  121,122,123,124               25G  200,201,202,203               25G       204,205                50G    down     down
       1   Ethernet49  125,126,127,128               25G  206,207,208,209               25G       210,211                50G    down     down
       1   Ethernet50      69,70,71,72               25G  212,213,214,215               25G           216               100G    down     down

In addition, docker ps | grep phy should show a physyncd docker running.

  Signed-off-by: syd.logan@broadcom.com
@sydlogan
Copy link
Contributor Author

@jleveque no known outstanding issues, could you please re-review and consider approval and merge? Thanks!

@jleveque
Copy link
Contributor

@lguohan: Are you OK with the inclusion of CUint here for unit testing? Just wanted to check if we intended to unify all unit tests to use one framework.

@sydlogan
Copy link
Contributor Author

A short comment on cunit for perspective - the tests are not currently run as a part of the build, nor are we including them in any sort of tests at gate. The primary reason for these tests has been to support TDD as we modify the parser. An example, these tests proved invaluable to have a week ago when I ported from cjson to swss::json.hpp to validate the port did not regress anything (yes, we originally were using cjson but prior to the pull request, consulted with @kcudnik and decided we would need to move to swss::json.hpp). cunit was the best tool for the job in my estimation, tar the test directory after a build, scp it to a switch, and run the tests to validate the parser, again in a TDD fashion.

@sydlogan
Copy link
Contributor Author

@jleveque is there anything you are blocked on that I can provide help with? I reached out to @lguohan regarding the cunit usage, not seeing a reply here, possibly he is ok with my explanation.

@jleveque
Copy link
Contributor

jleveque commented Jul 1, 2020

@sydlogan: I'm unsure if we want to check the CUnit files into this repo, or if we should download and copy them at build time. The CUnit files appear to be unmodified by you, is that correct?

@sydlogan
Copy link
Contributor Author

sydlogan commented Jul 1, 2020

@sydlogan: I'm unsure if we want to check the CUnit files into this repo, or if we should download and copy them at build time. The CUnit files appear to be unmodified by you, is that correct?

Correct. Perhaps an example of how this might be done would be helpful. Given the desire to get this Gearbox change into 62020 Release, could we maybe make this a follow on issue for a subsequent pull request?

@sydlogan
Copy link
Contributor Author

sydlogan commented Jul 1, 2020

@sydlogan: I'm unsure if we want to check the CUnit files into this repo, or if we should download and copy them at build time. The CUnit files appear to be unmodified by you, is that correct?

@jleveque We could (I think) use the sonic-buildimage slave to install cunit-dev (via apt install, see https://github.com/Azure/sonic-buildimage/blob/master/sonic-slave-buster/Dockerfile.j2#L40 for example of general area of this change). If you agree, I propose we remove cunit sources from swss, disable building the tests, but leave the tests. and we can get swss merged. Then I can come back later to update sonic-buildimage and sonic-swss with the changes needed to install cunit-dev deb package and build the tests, as a separate pull requests. Will that work for you?

lib/gearboxutils.h Outdated Show resolved Hide resolved
@jleveque
Copy link
Contributor

jleveque commented Jul 1, 2020

Looks good to me at this point. Since the PR is quite large, I'd also like @kcudnik to review, who also works more closely with this codebase/repo.

@sydlogan sydlogan closed this Jul 1, 2020
@sydlogan sydlogan reopened this Jul 1, 2020
@sydlogan
Copy link
Contributor Author

sydlogan commented Jul 2, 2020

@jleveque - I think we've got feedback and approval from @kcudnik, is it possible to merge?

@jleveque
Copy link
Contributor

jleveque commented Jul 2, 2020

@sydlogan: I will merge once the vs test build completes successfully.

@jleveque jleveque changed the title swss: Add support to swss for gearbox phys [swss] Add support for gearbox phys Jul 2, 2020
@jleveque jleveque merged commit 10ad70c into sonic-net:master Jul 2, 2020
@sydlogan
Copy link
Contributor Author

sydlogan commented Jul 3, 2020

Yay! Thanks @jleveque @kcudnik @lguohan for the reviews and merge!

@lguohan
Copy link
Contributor

lguohan commented Jul 3, 2020

for the jumping late, i think sonic-swss used googletest for the c++ unit test, can we use that. I do not want to introduce another unit test framework for sonic-swss.

lguohan pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Sep 25, 2020
…gearbox phy feature (#4851)

* buildimage: Add gearbox phy device files and a new physyncd docker to support VS gearbox phy feature

* scripts and configuration needed to support a second syncd docker (physyncd)
* physyncd supports gearbox device and phy SAI APIs and runs multiple instances of syncd, one per phy in the device
* support for VS target (sonic-sairedis vslib has been extended to support a virtual BCM81724 gearbox PHY).

HLD is located at https://github.com/Azure/SONiC/blob/b817a12fd89520d3fd26bbc5897487928e7f6de7/doc/gearbox/gearbox_mgr_design.md

**- Why I did it**

This work is part of the gearbox phy joint effort between Microsoft and Broadcom, and is based
on multi-switch support in sonic-sairedis.

**- How I did it**

Overall feature was implemented across several projects. The collective pull requests (some in late stages of review at this point):

sonic-net/sonic-utilities#931 - CLI (merged)
sonic-net/sonic-swss-common#347 - Minor changes (merged)
sonic-net/sonic-swss#1321 - gearsyncd, config parsers, changes to orchargent to create gearbox phy on supported systems
sonic-net/sonic-sairedis#624 - physyncd, virtual BCM81724 gearbox phy added to vslib

**- How to verify it**

In a vslib build:

root@sonic:/home/admin# show gearbox interfaces status
  PHY Id    Interface        MAC Lanes    MAC Lane Speed        PHY Lanes    PHY Lane Speed    Line Lanes    Line Lane Speed    Oper    Admin
--------  -----------  ---------------  ----------------  ---------------  ----------------  ------------  -----------------  ------  -------
       1   Ethernet48  121,122,123,124               25G  200,201,202,203               25G       204,205                50G    down     down
       1   Ethernet49  125,126,127,128               25G  206,207,208,209               25G       210,211                50G    down     down
       1   Ethernet50      69,70,71,72               25G  212,213,214,215               25G           216               100G    down     down

In addition, docker ps | grep phy should show a physyncd docker running.

  Signed-off-by: syd.logan@broadcom.com
santhosh-kt pushed a commit to santhosh-kt/sonic-buildimage that referenced this pull request Feb 25, 2021
…gearbox phy feature (sonic-net#4851)

* buildimage: Add gearbox phy device files and a new physyncd docker to support VS gearbox phy feature

* scripts and configuration needed to support a second syncd docker (physyncd)
* physyncd supports gearbox device and phy SAI APIs and runs multiple instances of syncd, one per phy in the device
* support for VS target (sonic-sairedis vslib has been extended to support a virtual BCM81724 gearbox PHY).

HLD is located at https://github.com/Azure/SONiC/blob/b817a12fd89520d3fd26bbc5897487928e7f6de7/doc/gearbox/gearbox_mgr_design.md

**- Why I did it**

This work is part of the gearbox phy joint effort between Microsoft and Broadcom, and is based
on multi-switch support in sonic-sairedis.

**- How I did it**

Overall feature was implemented across several projects. The collective pull requests (some in late stages of review at this point):

sonic-net/sonic-utilities#931 - CLI (merged)
sonic-net/sonic-swss-common#347 - Minor changes (merged)
sonic-net/sonic-swss#1321 - gearsyncd, config parsers, changes to orchargent to create gearbox phy on supported systems
sonic-net/sonic-sairedis#624 - physyncd, virtual BCM81724 gearbox phy added to vslib

**- How to verify it**

In a vslib build:

root@sonic:/home/admin# show gearbox interfaces status
  PHY Id    Interface        MAC Lanes    MAC Lane Speed        PHY Lanes    PHY Lane Speed    Line Lanes    Line Lane Speed    Oper    Admin
--------  -----------  ---------------  ----------------  ---------------  ----------------  ------------  -----------------  ------  -------
       1   Ethernet48  121,122,123,124               25G  200,201,202,203               25G       204,205                50G    down     down
       1   Ethernet49  125,126,127,128               25G  206,207,208,209               25G       210,211                50G    down     down
       1   Ethernet50      69,70,71,72               25G  212,213,214,215               25G           216               100G    down     down

In addition, docker ps | grep phy should show a physyncd docker running.

  Signed-off-by: syd.logan@broadcom.com
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.

5 participants