Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[YANG MODEL] SONiC Yang support for VXLAN #7294
[YANG MODEL] SONiC Yang support for VXLAN #7294
Changes from 14 commits
7370fa8
87ce7d0
ac2c5ac
17dad3c
7680ff8
9f88f96
46a64b6
89bb376
f073f5e
6995781
3bedcb4
68d3c4b
461b045
ca9b3bf
5a1a626
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see corresponding key in tests/vxlan.json. Can you please check and update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was removed in the last commit as the reference to vlan (import vlan) was failing the builds.
The intent is that this be re-added later once the import vlan is fixed. Hence it is being removed only in the
tests and not the tests_config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the limitation from? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for every vlan vni map a vni netdevice of form - is created.
The kernel devices can have a max length of 15 characters.
Keeping aside 5 characters for the "-" we are left with 10.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed explanation! Could you also add it as code comment? the 10 is magic number here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using "-" is a must or the user can choose any letter after "vtep" prefix? if any character, please put max 15 chars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The vni device format is -
Max vid is 4 characters and 1 for hyphen leaving us
with 10 characters for tunnelname.
Added a comment in the yang file for the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we also have "dst_ip" in vxlan tunnel. Please update
VXLAN_TUNNEL|{{tunnel_name}}
"src_ip": {{ip_address}}
"dst_ip": {{ip_address}} (OPTIONAL)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think, we support static vxlan tunnel yet, so dst_ip is not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dst_ip is not handled here.
Even for non BGP P2P tunnel support the configuration will continue to have only the src_ip.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add a common type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a common type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We dont need the list if it's a single entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future expansion. It was recommended that we use the LIST and hence kept it so that the URIs dont have to change later on.