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

Policy Based Hashing HLD #773

Merged
merged 11 commits into from
Jul 27, 2021
Merged

Conversation

nazariig
Copy link
Collaborator

@nazariig nazariig commented Apr 12, 2021

PR title state context
Add PBH YANG model GitHub issue/pull request detail GitHub pull request check contexts
Add PBH DB schema GitHub issue/pull request detail GitHub pull request check contexts
Add PBH OA GitHub issue/pull request detail GitHub pull request check contexts
Add PBH CLI GitHub issue/pull request detail GitHub pull request check contexts

Signed-off-by: Nazarii Hnydyn nazariig@nvidia.com

Signed-off-by: Nazarii Hnydyn <nazariig@nvidia.com>
Signed-off-by: Nazarii Hnydyn <nazariig@nvidia.com>
Signed-off-by: Nazarii Hnydyn <nazariig@nvidia.com>
Signed-off-by: Nazarii Hnydyn <nazariig@nvidia.com>
Signed-off-by: Nazarii Hnydyn <nazariig@nvidia.com>
@nazariig nazariig changed the title SONiC Policy Based Hashing Policy Based Hashing HLD Apr 12, 2021
Signed-off-by: Nazarii Hnydyn <nazariig@nvidia.com>
@nazariig nazariig marked this pull request as draft April 13, 2021 16:34
Signed-off-by: Nazarii Hnydyn <nazariig@nvidia.com>
doc/pbh/pbh-design.md Outdated Show resolved Hide resolved
doc/pbh/pbh-design.md Outdated Show resolved Hide resolved
Copy link
Contributor

@madhupalu madhupalu left a comment

Choose a reason for hiding this comment

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

Provided few comments.

@liat-grozovik
Copy link
Collaborator

liat-grozovik commented Jun 10, 2021 via email

msreddypaluru
msreddypaluru previously approved these changes Jun 15, 2021
Copy link

@msreddypaluru msreddypaluru left a comment

Choose a reason for hiding this comment

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

Thanks for the responses.

Signed-off-by: Nazarii Hnydyn <nazariig@nvidia.com>
Signed-off-by: Nazarii Hnydyn <nazariig@nvidia.com>
@liat-grozovik
Copy link
Collaborator

@anish-n can you please approve the HLD? code is ready and aligned with all the feedback provided offline

Signed-off-by: Nazarii Hnydyn <nazariig@nvidia.com>
anish-n
anish-n previously approved these changes Jul 14, 2021
doc/pbh/pbh-design.md Outdated Show resolved Hide resolved
Signed-off-by: Nazarii Hnydyn <nazariig@nvidia.com>
@nazariig
Copy link
Collaborator Author

@anish-n if no more objections, can we merge this?

@anish-n
Copy link
Contributor

anish-n commented Jul 16, 2021

@nazariig Looks good to me, I don't have merge permission so please request someone with merge permission for the merge.

yxieca pushed a commit to sonic-net/sonic-mgmt that referenced this pull request Jul 19, 2021
Description of PR
Introduces a test similar to the hash_test in SONiC, but for hashing based on the inner packet tuples. The test validates that packets are ECMP'd across multiple nexthops in an balanced way using the inner tuples only.

Summary:

Approach
What is the motivation for this PR?
To test packet hashing based on the inner packet tuples instead of the standard outer packet tuples. The test checks the distribution of packets is as expected, and that each of the inner packet hash parameters(inner src ip, dst ip, src port, dst port and ip proto) leads to a variation of ports hashed to, ie: ECMP spreading. The test also validates, as an optional mode, symmetric hashing: 2 directions of a flow end up on the same next-hop.

Note: The test assumes that inner hashing is configured on DUT prior to the test, in the future once inner hashing becomes a configuration parameter via config db, the test will be enhanced with configuration abilities. Feature which will expose this via configuration: sonic-net/SONiC#773

How did you do it?
PTF test which generates inner packet tuples

How did you verify/test it?
Developed the test and ran it on a DUT configured with inner hashing

Any platform specific information?
Supported on select Mellanox DUTs for now, other platforms may be added as they are tested

Supported testbed topology if it's a new test case?
T0
Other topologies may be added as they are tested.
@nazariig
Copy link
Collaborator Author

@qiluo-msft / @yxieca / @prsunny please help to merge

@liat-grozovik liat-grozovik merged commit 0c7e34b into sonic-net:master Jul 27, 2021
vmittal-msft pushed a commit to vmittal-msft/sonic-mgmt that referenced this pull request Sep 28, 2021
Description of PR
Introduces a test similar to the hash_test in SONiC, but for hashing based on the inner packet tuples. The test validates that packets are ECMP'd across multiple nexthops in an balanced way using the inner tuples only.

Summary:

Approach
What is the motivation for this PR?
To test packet hashing based on the inner packet tuples instead of the standard outer packet tuples. The test checks the distribution of packets is as expected, and that each of the inner packet hash parameters(inner src ip, dst ip, src port, dst port and ip proto) leads to a variation of ports hashed to, ie: ECMP spreading. The test also validates, as an optional mode, symmetric hashing: 2 directions of a flow end up on the same next-hop.

Note: The test assumes that inner hashing is configured on DUT prior to the test, in the future once inner hashing becomes a configuration parameter via config db, the test will be enhanced with configuration abilities. Feature which will expose this via configuration: sonic-net/SONiC#773

How did you do it?
PTF test which generates inner packet tuples

How did you verify/test it?
Developed the test and ran it on a DUT configured with inner hashing

Any platform specific information?
Supported on select Mellanox DUTs for now, other platforms may be added as they are tested

Supported testbed topology if it's a new test case?
T0
Other topologies may be added as they are tested.
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.

7 participants