-
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
[Sonic yang models]: Added the sonic yang models for acl, port, portchannel, vlan, vrf, interface etc #4001
[Sonic yang models]: Added the sonic yang models for acl, port, portchannel, vlan, vrf, interface etc #4001
Conversation
Since libyang is there after the following #3710 PR merge, we are fine without these exports |
Merge master
type string; | ||
} | ||
|
||
leaf PRIORITY { |
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.
Documenting is required. But that does not justify us to be random.
Let us follow a standard.
Agree with @praveen-li, let us go for all caps.
@anand-kumar-subramanian, please ensure new SONiC schema files in buzznic follows this.
Please update the doc too.
} | ||
|
||
leaf RULE_DESCRIPTION { | ||
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.
I see the length restriction dropped (compared to PR #18.
Is this intentional?
enum FORWARD; | ||
enum DROP; | ||
enum REDIRECT; | ||
enum INT_INSERT; |
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.
What are these new action types, "INT_INSERT" & "INT_DELETE"?
Does SONiC support today ?
Is this platform constrained?
description | ||
"SONIC VLAN"; | ||
|
||
revision 2019-05-15 { |
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.
Not sure, if you would like to update revision date.
} | ||
} | ||
|
||
leaf-list members { |
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 this field, when there is a separate table for VLAN_MEMBERS?
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 think, this is for backward compatibility.
With change in SONIC Config, The challange is to ask NetOPS to change few tools which are responsible to deploy config on thousands of switches. So if we were using it in data Center previously then usually we try to be backward compatible.
|
||
|
||
container VLAN_TABLE { | ||
config false; |
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.
Which DB is this from?
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.
Yeah, same ques. We need VLAN VRF entry as below [I think, as per VRF HLD]. But VLAN_table, I am not sure.
"VLAN_INTERFACE": {
"Vlan100": {},
"Vlan777": {},
"Vlan100|2a04:....::1/64": {
"family": "IPv6",
"scope": "global"
},
"Vlan100|10...../26": {
"family": "IPv4",
"scope": "global"
},
"Vlan100|fe80::..../10": {
"family": "IPv6",
"scope": "local"
},
"Vlan777|2a04:..../64": {
"family": "IPv6",
"scope": "global"
},
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 is not Vlan table. Its similar to LAG/PORT Tables. L3 is binded to another table which is suffex-ed with _INTERFACE which has VRF info. In the current context, I think it is the VLAN table that's referenced here. @renukamanavalan , its in APPL_DB
key "name ifname"; | ||
|
||
leaf name { | ||
type leafref { |
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.
Not sure, you need to declare as leafref for a read-only data. Redundant ? More like, 'unused' ?
|
||
leaf vni { | ||
type uint32 { | ||
range "0..16777215" { |
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.
Constraint to ensure that this ID is not reused?
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.
Without ietf yang models, it looks good to me. Approving,
@anand-kumar-subramanian : kindly address other comments from Renuka, So that we can merge this. Thanks.
It is good to fill below :). Thx.
|
} | ||
} | ||
|
||
leaf-list members { |
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 think, this is for backward compatibility.
With change in SONIC Config, The challange is to ask NetOPS to change few tools which are responsible to deploy config on thousands of switches. So if we were using it in data Center previously then usually we try to be backward compatible.
|
||
|
||
container VLAN_TABLE { | ||
config false; |
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.
Yeah, same ques. We need VLAN VRF entry as below [I think, as per VRF HLD]. But VLAN_table, I am not sure.
"VLAN_INTERFACE": {
"Vlan100": {},
"Vlan777": {},
"Vlan100|2a04:....::1/64": {
"family": "IPv6",
"scope": "global"
},
"Vlan100|10...../26": {
"family": "IPv4",
"scope": "global"
},
"Vlan100|fe80::..../10": {
"family": "IPv6",
"scope": "local"
},
"Vlan777|2a04:..../64": {
"family": "IPv6",
"scope": "global"
},
} | ||
} | ||
|
||
must "not (src_port) or not (dst_port) or (dst_port != src_port)" { |
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.
Due to many must and pattern condition we thought of writing test cases which make sure that these conditions are working as expected.
I strongly think, we should have some test case results for these Or we should remove them right now. When I put test case infra then put them back with appropriate test cases.
} | ||
} | ||
|
||
leaf MIRROR_ACTION { |
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.
Yes
} | ||
} | ||
|
||
leaf nat_zone { |
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.
nat supported in latest master
} | ||
} | ||
|
||
leaf unnumbered { |
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.
unnumbered is not supported in Sonic now
} | ||
|
||
leaf ip_prefix { | ||
type inet:ip-prefix; |
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.
In general yes, but if they are in two different VRFs, it is ok to have same ip address
|
||
leaf direction { | ||
type enumeration { | ||
enum rx; |
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.
a more common wording is "ingress" and "egress". E.g. mirror-ingress
type string; | ||
} | ||
|
||
leaf dst_mac { |
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.
how about src_mac?
|
||
|
||
container VLAN_TABLE { | ||
config false; |
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 is not Vlan table. Its similar to LAG/PORT Tables. L3 is binded to another table which is suffex-ed with _INTERFACE which has VRF info. In the current context, I think it is the VLAN table that's referenced here. @renukamanavalan , its in APPL_DB
"Enalbe/disable fallback feature which is useful for specified VRF user to access internet through global/main route."; | ||
} | ||
|
||
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.
what is VNI? is it for tunnel? can you give some more description?
…tabase, swss and syncd to After list
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)