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

[Sonic yang models]: Added the sonic yang models for acl, port, portchannel, vlan, vrf, interface etc #4001

Closed
wants to merge 6 commits into from

Conversation

anand-kumar-subramanian
Copy link
Contributor

- 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)

@anand-kumar-subramanian
Copy link
Contributor Author

Since libyang is there after the following #3710 PR merge, we are fine without these exports

lguohan
lguohan previously approved these changes Jan 9, 2020
@anand-kumar-subramanian anand-kumar-subramanian changed the title [startup] Removing the libYang exports which were pointing to the wrong directory Added the sonic yang models for acl, port, portchannel, vlan, vrf, interface etc Jan 23, 2020
@anand-kumar-subramanian anand-kumar-subramanian changed the title Added the sonic yang models for acl, port, portchannel, vlan, vrf, interface etc [Sonic yang models] Added the sonic yang models for acl, port, portchannel, vlan, vrf, interface etc Jan 23, 2020
@anand-kumar-subramanian anand-kumar-subramanian changed the title [Sonic yang models] Added the sonic yang models for acl, port, portchannel, vlan, vrf, interface etc [Sonic yang models]: Added the sonic yang models for acl, port, portchannel, vlan, vrf, interface etc Jan 23, 2020
type string;
}

leaf PRIORITY {
Copy link
Contributor

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;
Copy link
Contributor

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;
Copy link
Contributor

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 {
Copy link
Contributor

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 {
Copy link
Contributor

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?

Copy link
Member

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;
Copy link
Contributor

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?

Copy link
Member

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"
},

Copy link
Contributor

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 {
Copy link
Contributor

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" {
Copy link
Contributor

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?

praveen-li
praveen-li previously approved these changes Feb 3, 2020
Copy link
Member

@praveen-li praveen-li left a 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.

@praveen-li
Copy link
Member

@anand-kumar-subramanian

It is good to fill below :). Thx.

  • 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)

}
}

leaf-list members {
Copy link
Member

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;
Copy link
Member

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)" {
Copy link
Member

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

}
}

leaf nat_zone {
Copy link
Contributor

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 {
Copy link
Contributor

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;
Copy link
Contributor

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;
Copy link
Contributor

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 {
Copy link
Contributor

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;
Copy link
Contributor

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 {
Copy link
Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
YANG YANG model related changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants