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.

Sign up for GitHub

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

Jump to bottom

SONiC NVGRE Tunnel HLD #869

Merged
merged 71 commits into from
Jan 27, 2022
Merged

SONiC NVGRE Tunnel HLD #869

merged 71 commits into from
Jan 27, 2022

Conversation

vadymhlushko-mlnx
Copy link
Contributor

@vadymhlushko-mlnx vadymhlushko-mlnx commented Sep 22, 2021 •
edited
Loading

vadymhlushko-mlnx and others added 30 commits August 22, 2021 10:04
@vadymhlushko-mlnx
Draft
cd664a8 
Signed-off-by: Vadym Hlushko <[email protected]>
@vadymhlushko-mlnx
Added Arch desing and SAI API sections
08efdbf 
Signed-off-by: Vadym Hlushko <[email protected]>
@vadymhlushko-mlnx
fixed internal review comments
05a40b0 
Signed-off-by: Vadym Hlushko <[email protected]>
@vadymhlushko-mlnx
Updated nvgre_orch.svg
bacbe52
@vadymhlushko-mlnx
Added nvgre_tunnel_create_uml.svg
57737c9
@vadymhlushko-mlnx
Updated nvgre_tunnel_create_uml.svg
c104743
@vadymhlushko-mlnx
Added nvgre_tunnel_map_create_uml.svg
7941ea8
@vadymhlushko-mlnx
Added flows
789c2c0 
Signed-off-by: Vadym Hlushko <[email protected]>
@vadymhlushko-mlnx
updated flows description
e9fea45 
Signed-off-by: Vadym Hlushko <[email protected]>
@vadymhlushko-mlnx
Added nvgre_cli_config.svg
ea38da0
@vadymhlushko-mlnx
Updated nvgre_cli_config.svg
24372c4
@vadymhlushko-mlnx
Added nvgre_cli_show.svg
1d1c4cb
@vadymhlushko-mlnx
Added flows diagrams
f65fd8c 
Signed-off-by: Vadym Hlushko <[email protected]>
@vadymhlushko-mlnx
Added CLI section, refactored old sections
23c1be4 
Signed-off-by: Vadym Hlushko <[email protected]>
@vadymhlushko-mlnx
Fixed alignment
687f9cb 
Signed-off-by: Vadym Hlushko <[email protected]>
@vadymhlushko-mlnx
Added VS tests section
852779e 
Signed-off-by: Vadym Hlushko <[email protected]>
@vadymhlushko-mlnx
Fixed minor issues
bed3384 
Signed-off-by: Vadym Hlushko <[email protected]>
@vadymhlushko-mlnx
fixed some internal review comments
68e4934 
Signed-off-by: Vadym Hlushko <[email protected]>
@vadymhlushko-mlnx
Updated the nvgre_tunnel_map_create_uml.svg
be8a214
@vadymhlushko-mlnx
Updated the nvgre_tunnel_create_uml.svg
03f8762
@vadymhlushko-mlnx
Updated the nvgre_tunnel_map_create_uml.svg
c9bfca6
@vadymhlushko-mlnx
Updated the nvgre_tunnel_create_uml.svg
63ae425
@vadymhlushko-mlnx
Updated the nvgre_tunnel_create_uml.svg
8697bc5
@vadymhlushko-mlnx
Fixed tests comments
86c09f7 
Signed-off-by: Vadym Hlushko <[email protected]>
@vadymhlushko-mlnx
Added initial NVGRE YANG model
35be51e 
Signed-off-by: Vadym Hlushko <[email protected]>
@vadymhlushko-mlnx
Fixed table of content
81e4575 
Signed-off-by: Vadym Hlushko <[email protected]>
@vadymhlushko-mlnx
Added nvgre_tunnel_delete_uml.svg
281235f
@vadymhlushko-mlnx
Added nvgre_tunnel_map_delete_uml.svg
0a4c25d
@vadymhlushko-mlnx
Updated the nvgre_tunnel_delete_uml.svg
df06b44
@vadymhlushko-mlnx
Updated the nvgre_cli_config.svg
bc0cbfb
@vadymhlushko-mlnx
fixed cli
8eb7bc6 
Signed-off-by: Vadym Hlushko <[email protected]>
@vadymhlushko-mlnx
Fixed yang model
44346f3 
Signed-off-by: Vadym Hlushko <[email protected]>
venkatmahalingam
venkatmahalingam reviewed Nov 11, 2021
View reviewed changes
doc/nvgre_tunnel/nvgre_tunnel.md Outdated Show resolved Hide resolved
doc/nvgre_tunnel/nvgre_tunnel.md Outdated Show resolved Hide resolved
doc/nvgre_tunnel/nvgre_tunnel.md Outdated Show resolved Hide resolved
doc/nvgre_tunnel/nvgre_tunnel.md Outdated Show resolved Hide resolved
@vadymhlushko-mlnx
Fixed typo
ed4e85f 
Signed-off-by: Vadym Hlushko <[email protected]>
@vadymhlushko-mlnx
Fixed abnf schema
0bbfec6 
Signed-off-by: Vadym Hlushko <[email protected]>
@vadymhlushko-mlnx
Fixed review comments
11055af 
Signed-off-by: Vadym Hlushko <[email protected]>
@liat-grozovik
Copy link
Collaborator

@kperumalbfn could you please help to review and check if your comments were addressed?

@vadymhlushko-mlnx
Copy link
Contributor Author

@venkatmahalingam, do you have more review comments, or you can approve this PR?

@vadymhlushko-mlnx
Copy link
Contributor Author

@kperumalbfn kind reminder, could you please help to review this PR

@vadymhlushko-mlnx
Copy link
Contributor Author

@tzack000, do you have more review comments, or you can approve this PR?

mickeyspiegel
mickeyspiegel reviewed Nov 19, 2021
View reviewed changes
doc/nvgre_tunnel/nvgre_tunnel.md
### SAI API

The NVGRE Tunnel feature require at least `SAI 1.9` or above.

Copy link

Choose a reason for hiding this comment

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

Some aspects of the SAI API in the initial implementation are not described in the HLD. They should be described:

  • SAI_TUNNEL_ATTR_PEER_MODE_P2MP (default)
  • Only decap mapper entries are created, not encap mapper entries

More importantly, the implementation does not conform to normal SAI tunnel usage. While the implementation seems to be focused on decap, there does not seem to be any code creating sai_tunnel_term_table entries. As far as I know the SAI tunnel definitions, I do not believe any packets will be consumed without entries in the sai_tunnel_term_table. Just creation of sai_tunnel objects is not enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the tunnel termination entry type and fixed implementation.
Remove the encap mappers.

Copy link

Choose a reason for hiding this comment

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

@svshah-intel Please review as well. Thanks.

@liat-grozovik
Copy link
Collaborator

@venkatmahalingam, @tzack000, @mickeyspiegel and @kperumalbfn could you please review and see if your concern is addressed and we can approve this PR?

@venkatmahalingam
Copy link
Collaborator

@venkatmahalingam, @tzack000, @mickeyspiegel and @kperumalbfn could you please review and see if your concern is addressed and we can approve this PR?

Sure, will do this week.

@liat-grozovik
Copy link
Collaborator

@venkatmahalingam any update?

@liat-grozovik liat-grozovik merged commit 9091931 into sonic-net:master Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reshmaintel reshmaintel reshmaintel left review comments

@mickeyspiegel mickeyspiegel mickeyspiegel left review comments

@kperumalbfn kperumalbfn kperumalbfn left review comments

@tzack000 tzack000 tzack000 left review comments

@liat-grozovik liat-grozovik liat-grozovik approved these changes

@venkatmahalingam venkatmahalingam venkatmahalingam approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@vadymhlushko-mlnx @liat-grozovik @venkatmahalingam @tzack000 @kperumalbfn @mickeyspiegel @reshmaintel