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

Static Anycast Gateway HLD #837

Merged
merged 7 commits into from
Apr 11, 2024
Merged

Conversation

superchild
Copy link
Contributor

@superchild superchild commented Aug 13, 2021

@ghost
Copy link

ghost commented Aug 13, 2021

CLA assistant check
All CLA requirements met.

doc/sag/sag-HLD.md Outdated Show resolved Hide resolved
doc/sag/sag-HLD.md Outdated Show resolved Hide resolved
doc/sag/sag-HLD.md Outdated Show resolved Hide resolved
doc/sag/sag-HLD.md Outdated Show resolved Hide resolved
doc/sag/sag-HLD.md Outdated Show resolved Hide resolved
doc/sag/sag-HLD.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@liat-grozovik liat-grozovik left a comment

Choose a reason for hiding this comment

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

General comments:

  • The HLD is missing a high level description before even discussing the config db which should be followed by yang model.
    Please check the HLD Template guidelines (https://github.com/Azure/SONiC/blob/master/doc/hld_template.md) it is very helpful to understand the feature and be able to review the code later on.
  • The CLI as described is very high level and the part of the interface does not define and of the checkers expected on the CLI level. The CLI itself IMO should be described in the same way we have in the command reference md file.
  • The flow is very high level and does to explain clearly (although there is a flow image). i don't see any reference to the SAI it self and the parameters used. Missing flow of vlan interface delete, not the sag part.
  • The default behaviour should be clearly mentioned. it is disabled by default.
  • It was mentioned it must go with VXLAN/EVPN. Is there any validation? what will happen if not? what tis the expected behaviour? ignored? error?

doc/sag/sag-HLD.md Outdated Show resolved Hide resolved
doc/sag/sag-HLD.md Outdated Show resolved Hide resolved
doc/sag/sag-HLD.md Outdated Show resolved Hide resolved
doc/sag/sag-HLD.md Outdated Show resolved Hide resolved
doc/sag/sag-HLD.md Outdated Show resolved Hide resolved
doc/sag/sag-HLD.md Outdated Show resolved Hide resolved
doc/sag/sag-HLD.md Outdated Show resolved Hide resolved
doc/sag/sag-HLD.md Outdated Show resolved Hide resolved
doc/sag/sag-HLD.md Outdated Show resolved Hide resolved
doc/sag/sag-HLD.md Outdated Show resolved Hide resolved
@superchild
Copy link
Contributor Author

@kperumalbfn @venkatmahalingam @preetham-singh @hasan-brcm @dgsudharsan @liat-grozovik
All comments are replied and the HLD is updated.
Please help to check again.

@superchild
Copy link
Contributor Author

All related PR's are sent and updated tracking table in this PR.
Please help to review it.

@zhangyanzhao
Copy link
Collaborator

@superchild would you please add the test PR into the code PR list? Thanks.

@superchild
Copy link
Contributor Author

superchild commented Oct 27, 2021

@superchild would you please add the test PR into the code PR list? Thanks.

@zhangyanzhao What is the test PR?
The unit test code is included in the following 3 PRs

sonic-swss-common#540 and sonic-utilities#1887 don't have reviewer yet, could you please assign someone help to review?

Thanks.

@zhangyanzhao
Copy link
Collaborator

Reviewers, can you please help to finish your review on this feature? Thanks.
@gord1306 @kperumalbfn @venkatmahalingam @preetham-singh @hasan-brcm

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 1, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@liat-grozovik
Copy link
Collaborator

@superchild could you please sign the easyCLA?

@superchild
Copy link
Contributor Author

@liat-grozovik thanks for your reminder, already signed.

@zhangyanzhao
Copy link
Collaborator

@liat-grozovik can you please let me know if we can merge this PR? Nvidia is the registered reviewer. Thanks.

@liat-grozovik
Copy link
Collaborator

It was reviewed by Nvidia and approved (@dgsudharsan and @Junchao-Mellanox ). please go a head and merge the HLD.
as for code PRs, need to see the review and feedback

@zhangyanzhao
Copy link
Collaborator

General comments:

  • The HLD is missing a high level description before even discussing the config db which should be followed by yang model.
    Please check the HLD Template guidelines (https://github.com/Azure/SONiC/blob/master/doc/hld_template.md) it is very helpful to understand the feature and be able to review the code later on.
  • The CLI as described is very high level and the part of the interface does not define and of the checkers expected on the CLI level. The CLI itself IMO should be described in the same way we have in the command reference md file.
  • The flow is very high level and does to explain clearly (although there is a flow image). i don't see any reference to the SAI it self and the parameters used. Missing flow of vlan interface delete, not the sag part.
  • The default behaviour should be clearly mentioned. it is disabled by default.
  • It was mentioned it must go with VXLAN/EVPN. Is there any validation? what will happen if not? what tis the expected behaviour? ignored? error?

@superchild can you please address this comment? Thanks.

wangxin pushed a commit to sonic-net/sonic-mgmt that referenced this pull request Jun 15, 2023
How did you do it?
Bases on the SAG HLD sonic-net/SONiC#837, design needed test cases to verify its functionality.

How did you verify/test it?
Executing all SAG sonic-mgmt test cases using edge-core devices

Any platform specific information?
N/A

Supported testbed topology if it's a new test case?
ptf32, ptf64
mrkcmo pushed a commit to Azarack/sonic-mgmt that referenced this pull request Oct 3, 2023
How did you do it?
Bases on the SAG HLD sonic-net/SONiC#837, design needed test cases to verify its functionality.

How did you verify/test it?
Executing all SAG sonic-mgmt test cases using edge-core devices

Any platform specific information?
N/A

Supported testbed topology if it's a new test case?
ptf32, ptf64
@zhangyanzhao
Copy link
Collaborator

HLD has been reviewed in SONiC community on 8/24/2021

@zhangyanzhao
Copy link
Collaborator

HLD was reviewed and approved by reviewers, merge the HLD.

@zhangyanzhao
Copy link
Collaborator

@liat-grozovik NVIDIA is the reviewing company, can you please merge this PR? Thanks.

AharonMalkin pushed a commit to AharonMalkin/sonic-mgmt that referenced this pull request Jan 25, 2024
How did you do it?
Bases on the SAG HLD sonic-net/SONiC#837, design needed test cases to verify its functionality.

How did you verify/test it?
Executing all SAG sonic-mgmt test cases using edge-core devices

Any platform specific information?
N/A

Supported testbed topology if it's a new test case?
ptf32, ptf64
qiluo-msft pushed a commit to sonic-net/sonic-swss-common that referenced this pull request Jan 25, 2024
Add SAG table definition for static anycast gateway.
Static anycast gateway HLD [#837](sonic-net/SONiC#837)
@yanjundeng
Copy link

yanjundeng commented Jun 1, 2024

New Updated PRs:
sonic-swss : sonic-net/sonic-swss#3167
sonic-utility: sonic-net/sonic-utilities#3339
sonic-buildimage: sonic-net/sonic-buildimage#19069

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Status: 🆕 Backlog (not in plan yet)
Development

Successfully merging this pull request may close these issues.