-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
staticd: yang definition #5460
staticd: yang definition #5460
Conversation
Thanks Vishal for starting this, I am also started working on this. what i defined was..(based on Internal FRR Implementation)
|
@satheeshkarra most of the internal FRR implimeantion can be handled with Yang data model defined by IETF we added more on to that so that all the commands are taken care. |
does above model takes care of negative cases also, some of the options (like, on-link) are available only in certain cases. is the plan to validate them in the callbacks..? from above it looks like parser doesn't restrict on-link for other NH types. |
We can either do this with choice in data model or in validation. We are planning to do that in validation. |
If possible can you please restrict this in data model, because our Idea it to only validate Operational data in the call backs, configuration restrictions can be easily validate in the model. |
Sure. We should be able to do it. |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-9924/ This is a comment from an automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base1 Static Analyzer issues remaining.See details at |
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 take care of Negative cases also
67ac93e
to
255439c
Compare
At present libyang has no support for Schema mount. So, we have modified the ietf models to support the VRF. |
255439c
to
d2d8a29
Compare
|
rw frr-staticd:table-id? & rw frr-staticd:nexthop-vrf? both will be same right..? do we need both...? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Some initial review comments. I didn't spend time on the IETF models this round.
yang/frr-staticd.yang
Outdated
case ifname { | ||
leaf outgoing-interface { | ||
type string { | ||
length "1..16"; |
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.
Any reason this shouldn't be an interface 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.
done
yang/frr-staticd.yang
Outdated
case gateway { | ||
leaf gw-interface { | ||
type string { | ||
length "1..16"; |
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.
Any reason this shouldn't be an interface 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.
done
yang/frr-staticd.yang
Outdated
description | ||
"Name of the outgoing interface."; | ||
} | ||
leaf is-onlink { |
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.
IMO this should be named onlink
and the type should be boolean. I was also of the impression that "onlink" doesn't specify a nexthop so much as it's an attribute of a nexthop. It doesn't mean anything to say "the nexthop for prefix
is onlink" where onlink is a noun (as it is here) and not an adjective.
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.
done
yang/frr-staticd.yang
Outdated
case Null0 { | ||
leaf Null0 { | ||
type boolean; | ||
default 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.
Please remember to use yanglint to format the models
yang/frr-staticd.yang
Outdated
} | ||
} | ||
case Null0 { | ||
leaf Null0 { |
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 a bit of a special case, since Null0
is treated as a "special" interface name. IMO this doesn't need to be represented in the model at all, it's really an anachronism from other vendors, and it's essentially an alias for the blackhole
attribute.
yang/frr-staticd.yang
Outdated
{ | ||
description | ||
"Augment static route configuration 'next-hop-list'."; | ||
uses frr-staticd-nexthop; |
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 feel like this is the wrong way to go about it. We should have a list of nexthops modeled in one place and then other places that need to reference a nexthop can use a leafref. Thoughts?
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.
@qlyoung thanks for review. I think we need to go over call and discuss this? We wanted to keep it as close to IETF as possible keeping extensions in mind if we have to use leafref then we may not be able to use IETF defined model. Though many things in IETF defined yang model are not being used today and we will be using deviate and delete statements. I think if we can go over call and converge faster on 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.
Sounds good
yang/frr-staticd.yang
Outdated
{ | ||
description | ||
"Augment static route configuration 'next-hop-list'."; | ||
uses frr-staticd-nexthop; |
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.
Ditto previous comment regarding leafrefs.
yang/frr-vrf.yang
Outdated
"vrf name."; | ||
leaf name { | ||
type string { | ||
length "1..36"; |
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.
1..16, no? And I believe the name of a vrf should be its own type encapsulating this constraint.
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.
done
I have raised the new PR for common yang modules.
Please refer to this PR.
#5525
} | ||
description | ||
"This type is used by data models that need to reference | ||
interfaces."; |
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 definitely a dup of what exists in the frr-interface
module.
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.
@qlyoung thanks for review. I think again we need to go over call and discussion do we need to use frr-interface or use IETF-data model with deviation.
@@ -0,0 +1,1123 @@ | |||
module ietf-interfaces { |
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 need to decide whether we should augment this module with FRR specific information, or vice versa. In any case this module duplicates the existing frr-interface
module and that needs to be resolved.
Preliminary review: this PR is including modified versions of IETF modules to our tree. We shouldn't do that with any published YANG module since it can easily lead to confusion (e.g. a module named In any case, the idea for now is to reuse as much as possible from the IETF modules by borrowing nodes from them into our native modules instead of implementing the IETF modules directly (this is documented here). Ideally we would implement the IETF data hierarchy directly in FRR, but that would make things much more difficult so it's probably something that can be left for a second moment. I'm open to discuss this decision in the next weekly meeting. Note: we already have a couple of IETF modules in our tree but they are there only because they provide useful YANG types like |
I fully agree with Renato. Reuse published models where sensible and
appropriate, but the goal should be to produce a model tailored for FRR,
not necessarily one as as close as possible to the standards.
…On Fri, Dec 6, 2019, 8:38 PM Renato Westphal ***@***.***> wrote:
Preliminary review: this PR is including modified versions of IETF modules
to our tree. We shouldn't do that with any published YANG module since it
can easily lead to confusion (e.g. a module named
***@***.*** should be the same regardless of where it
was obtained). Modules should only be modified by other modules using
either augmentations or deviations. This has the nice side effect of making
the differences between the original and modified modules easier to
identify.
In any case, the idea for now is to reuse as much as possible from the
IETF modules by borrowing nodes from them into our native modules instead
of implementing the IETF modules directly (this is documented here
<https://github.com/opensourcerouting/frr/wiki/Architecture#yang-models>).
Ideally we would implement the IETF data hierarchy directly in FRR, but
that would make things much more difficult so it's probably something that
can be left for a second moment. I'm open to discuss this decision in the
next weekly meeting.
Note: we already have a couple of IETF modules in our tree but they are
there only because they provide useful YANG types like route-distinguisher.
They are not being implemented in any way.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5460?email_source=notifications&email_token=ABUCX6Z2E3BACJQJ6KZDLMLQXL5ABA5CNFSM4JUYRF72YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGF2TOA#issuecomment-562801080>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABUCX63UA3CXDT7LIW7WAR3QXL5ABANCNFSM4JUYRF7Q>
.
|
Renato - Thanks a lot for your review. I agree with you. While initially we did remove from IETF module then we used deviation and delete which worked well. We will resubmit our patch again which will take care of your comments. |
d2d8a29
to
54bead9
Compare
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 your contribution to FRR!
- One of your commits has a missing or badly formatted
Signed-off-by
line; we can't accept your contribution until all of your commits have one
If you are a new contributor to FRR, please see our contributing guidelines.
54bead9
to
9ab4f01
Compare
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 your contribution to FRR!
- One of your commits has a missing or badly formatted
Signed-off-by
line; we can't accept your contribution until all of your commits have one
If you are a new contributor to FRR, please see our contributing guidelines.
9ab4f01
to
62c7633
Compare
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 your contribution to FRR!
- One of your commits has a missing or badly formatted
Signed-off-by
line; we can't accept your contribution until all of your commits have one
If you are a new contributor to FRR, please see our contributing guidelines.
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedFreeBSD 11 amd64 build: Failed (click for details)Make failed for FreeBSD 11 amd64 build:
FreeBSD 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10171/artifact/CI009BUILD/config.status/config.status Successful on other platforms
|
f9e0eae
to
743cfc2
Compare
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedFreeBSD 11 amd64 build: Failed (click for details)Make failed for FreeBSD 11 amd64 build:
FreeBSD 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10180/artifact/CI009BUILD/config.status/config.status Successful on other platforms
|
743cfc2
to
71c41b7
Compare
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10479/ This is a comment from an automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base1 Static Analyzer issues remaining.See details at |
yang/frr-staticd.yang
Outdated
"Support for a 'static' pseudo-protocol instance | ||
consists of a list of routes."; | ||
|
||
list address-family { |
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 does vrf come into this? I only see unqualified ip prefixes: are the vrfs captured somewhere else?
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.
vrf would come from the frr-routing.yang
It would be like this
module: frr-routing
+--rw routing
+--rw control-plane-protocols
+--rw control-plane-protocol* [type name vrf]
+--rw type identityref
+--rw name string
+--rw vrf frr-vrf:vrf-ref
+--rw frr-staticd:staticd
+--rw frr-staticd:address-family* [address-family]
yang/frr-staticd.yang
Outdated
+ "frr-rt:control-plane-protocol/staticd/address-family/" | ||
+ "ipv4-route/frr-staticd-next-hop/frr-nexthops/nexthops" { | ||
|
||
leaf distance { |
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 both distance and tag are properties of a static route, not of each nexthop in a route? check lib/zclient.h
to see how those attributes are conveyed.
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.
Actually at present in staticd, if you go to the static_route struct, then tag and distance are defined in this struct means part of nexthop. Now when route goes to zebra then these attributes becomes part of route.
IMO routes should not be segmented into containers by AF, there should be a flat list and the AF should be an attribute of the route |
71c41b7
to
b96d61a
Compare
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedFreeBSD 11 amd64 build: Failed (click for details)FreeBSD 11 amd64 build: No useful log foundSuccessful on other platforms
|
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedFreeBSD 11 amd64 build: Failed (click for details)Make failed for FreeBSD 11 amd64 build:
FreeBSD 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10566/artifact/CI009BUILD/config.status/config.status Successful on other platforms
|
b96d61a
to
ef5a6aa
Compare
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedFreeBSD 11 amd64 build: Failed (click for details)Make failed for FreeBSD 11 amd64 build:
FreeBSD 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10600/artifact/CI009BUILD/config.status/config.status Successful on other platforms
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
4491b19
to
8141e54
Compare
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedOpenBSD 6 amd64 build: Failed (click for details)DejaGNU Unittests (make check) failed for OpenBSD 6 amd64 build:
OpenBSD 6 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10620/artifact/CI011BUILD/config.status/config.status Successful on other platforms
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10618/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
8141e54
to
f4ebdbe
Compare
Yang files for staticd to use northbound APIs Co-authored-by: Santosh P K <sapk@vmware.com> Co-authored-by: vishaldhingra <vdhingra@vmware.com> Signed-off-by: vishaldhingra <vdhingra@vmware.com>
f4ebdbe
to
95f1804
Compare
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10656/ This is a comment from an automated CI system. |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10661/ This is a comment from an automated CI system. |
Good to merge after NH model goes in. |
Yang files for staticd to use northbound APIs
Co-authored-by: Santosh P K sapk@vmware.com
Co-authored-by: vishaldhingra vdhingra@vmware.com
Signed-off-by: vishaldhingra vdhingra@vmware.com