-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Conversation
Added tests for sonic-vxlan.yang
Dependent on #6861 for build to go through |
@srj102 please help to address the conflict and build failure |
What is the specific table name for this YANG model? vxlan? |
config db - VXLAN_TUNNEL, VXLAN_TUNNEL_MAP, VXLAN_EVPN_NVO |
enhancement for previous vxlan YANG tables |
error-app-tag invalid-vtep-name; | ||
} | ||
type string { | ||
length 1..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.
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.
} | ||
} | ||
} | ||
container VXLAN_FDB_TABLE{ |
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 can't find UT for this table
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.
Do we have any infra support for this ? I see a sample_config_db.json but not a sample_app_db or sample_state_db..
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.
No support yet. Could you add this support?
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 have removed the APP and STATE DB table changes and kept only the CONFIG DB changes. Will add the remaining once the support for these tables comes in in the framework.
} | ||
|
||
leaf type { | ||
type string; |
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.
Does this type has any pattern?
|
||
} | ||
|
||
leaf vni { |
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
} | ||
} | ||
} | ||
container VXLAN_TUNNEL_TABLE { |
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.
Can't find UT for this
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.
Removed APP and STATE DB related containers and will support later.
} | ||
} | ||
} | ||
container VXLAN_REMOTE_VNI_TABLE { |
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.
Can't find UT?
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.
Removed APP and STATE DB related containers and will support later.
} | ||
|
||
leaf vni { | ||
type string; |
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.
VXLAN_TUNNEL_MAP defines vni before, same pattern?
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.
Removed APP and STATE DB related containers and will support later.
/azpw run |
/AzurePipelines run |
You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list. |
import sonic-vlan { | ||
prefix svlan; | ||
} | ||
import sonic-types { |
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.
Please correct alignment #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.
The reason is mixing tab and spaces. So it seems aligned in some editor.
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.
Removed a few tabs which were present.
max-elements 1; | ||
|
||
leaf name { | ||
must "(starts-with(../name, 'vtep'))" |
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 think this restriction exists today. Please remove this. Might break backward compatibility
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.
Sure. Have removed the same.
leaf src_ip { | ||
type inet:ipv4-address; | ||
} | ||
} |
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.
} | ||
} | ||
container VXLAN_REMOTE_VNI_TABLE { | ||
sonic-ext:db-name "APPL_DB"; |
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.
If i am not wrong currently we define yang models only for config_db and no other DBs. @praveen-li have we decided to use yang in other DBs?
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.
Removed APP and STATE DB related containers and will support later.
max-elements 1; | ||
|
||
leaf name { | ||
must "(starts-with(../name, 'vtep'))" |
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.
Please use the string pattern to enforce this.
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.
Why do we need a list if the number of entries itself is one?
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.
Removed this restriction of having to start with vtep.
@qiluo-msft would you please help to merge this PR? Thanks. |
/azpw run Azure.sonic-buildimage |
/AzurePipelines run Azure.sonic-buildimage |
Azure Pipelines successfully started running 1 pipeline(s). |
import sonic-extension { | ||
prefix sonic-ext; | ||
} | ||
import sonic-vlan { |
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 tests are failing in Sonic-utils specifically GCU because of yang known issue: #9312
Please comment out sonic-vlan
import and add a note
// Comment sonic-vlan import here until libyang back-links issue is resolved for VLAN leaf reference.
//import sonic-vlan {
// prefix vlan;
//}
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.
@qiluo-msft for viz
Remove vlan leafref test
@dgsudharsan Please review again. |
@srj102 @venkatmahalingam would you like to review again? any more comments? |
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.
Can you please update the schema in configuration doc?
https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-yang-models/doc/Configuration.md
src/sonic-yang-models/tests/yang_model_tests/tests_config/vxlan.json
Outdated
Show resolved
Hide resolved
} | ||
} | ||
}, | ||
"VXLAN_MAP_WITHOUT_VLAN": { |
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.
done. |
@srj102 PR conflicts with 202205 branch |
@srj102 there is cherry-picking conflict, can you help manually cherry-pick this one? |
Will backport by Feb 23. |
Why I did it
SONiC Yang support for VXLAN
How I did it
Added a new sonic-vxlan.yang file.
Please refer to EVPN VXLAN HLD for DB details
https://github.com/Azure/SONiC/tree/master/doc/vxlan/EVPN
How to verify it
Added tests for sonic vxlan yang.
Which release branch to backport (provide reason below if selected)
Description for the changelog
A picture of a cute animal (not mandatory but encouraged)