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
lib: FRR next-hop yang data model. #5578
Conversation
Modified common next-hop and next-hop group for other protocols and zebra to use. For next-hop would look like below with an example if used as leafref Nexthop table: 1 10.10.10.10 Nexthop Group table: 10 1, 2, 3 Route with Nexthop will look like Route with Nexthop group would look like 1.1.1.1/32 nexthop 10 Nexthop tree: |
@qlyoung and @mjstapp I had to pulldown #5547 as i had issues with rebase. I have raised this new PR after addressing both of your comments. Comments that are not addressed are as below.
Both of these points I need a discussion. On group nexthop I wanted to understand how do we want to keep grouping? Right now it is pointing to list of nexthop_ids. |
@qlyoung @mjstapp Can you please also let me know if this looks right here? container gateway-interface { We are saying nexthop type is ipv4-ifindex but we are having leafref which is pointing to interface and not ifindex. I kept it this way as old frr-nexthop was also doing the same. |
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-10190/ 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 |
so ... these flags get set as a result of some processing - in zebra's mind, a nexthop is only "valid" if it meets certain requirements that are part of zebra's RIB processing. it seems confusing for the absence of a flag to mean "enabled".
ok, so maybe we need to disentangle a few cases: there are (a) nexthops that are learned as a result of routing protocol messaging; there are (b) nexthops that are configured explicitly; and there are (c) nexthops that are learned by zebra on some linux/netlink OSes. Some of the attributes of nexthops can be configured in the (b) and (c) paths; some attributes can never be configured and are always "read-only" to admins. the (b) objects are not widely used - but there would be value in having more ability to share those configurations and there's work ongoing to implement that. Further, zebra has recently begun managing (a) objects and (c) objects in the same way - it canonicalizes (a) information that it receives, and uses nexthop-group objects that routes share. those group objects have ids that are unique and stable. and it's likely that in the future we'd like to extend the nexthop-group concept to other daemons, to improve memory footprint, and communication efficiency with zebra. |
|
You are right since this internal flag. We can keep this flag by default as false but thinking aloud @mjstapp should we remove it as it is internal flag and can be augmented in zebra as this zebra only property?
@mjstapp thanks. So in that case how would nexthop group would looks like in tree format? Would it be as I have shown in example of nexthops in previous comment? Since nexthop is a leafref we have ID for each nexthop. For group nexthop we thought of id pointing to multiple Ids in the nexthop table. |
This seems like the right thing to do here, to me... |
yang/frr-nexthop.yang
Outdated
leaf group-id { | ||
type uint32; | ||
description | ||
"Identifies a set of nexthops in a group." |
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.
Needs a semicolon
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 few things:
- duplicate, active, recursive and flags are operational state and need to be readonly
- active should be false by default. That is not set true until it goes through zebra nexthop resolution.
- I'm not sure the
flags
bitfield should be present in model, flags need to be split out
into their own leaves for ones we care about. - I think the mpls-label-stack needs to be defined elsewhere and used as a type for a leaf in a nexthop. I assume other things will need the label stack in their tree?
- @mjstapp what was your reasoning for including
mtu
in the nexthop definition? I'm not aware that's something we track internally with nexthops in frr. Is it not more of aper route
thing? - See my naming comments
yang/frr-nexthop.yang
Outdated
description | ||
"The nexthop-group name."; | ||
} | ||
list group-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.
rename, do we need the group-
prefix? could it just be 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.
+1
yang/frr-nexthop.yang
Outdated
key "group-id"; | ||
description | ||
"A group of nexthops."; | ||
leaf group-id { |
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 we change the name above, this should be renamed similarly right?
yang/frr-nexthop.yang
Outdated
"A nexthop-group, represented as a list of nexthop objects."; | ||
|
||
list group-entry { | ||
key "group-id"; |
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.
rename, maybe nexthop-group-id
?
yang/frr-nexthop.yang
Outdated
description | ||
"A nexthop-group, represented as a list of nexthop objects."; | ||
|
||
list group-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.
So this is where the actual definition of a group begins right?
Could we call this something else besides group-entry
. Maybe even just nexthop-group
will work?
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, this is the global list of nexthop groups, group-entry
isn't descriptive
yang/frr-nexthop.yang
Outdated
type empty; | ||
leaf active { | ||
type boolean; | ||
default true; |
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 should default to 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.
+1, and again Zebra determines this value, so again config false
Another thing probably worth noting here: nexthop-groups and nexthops share ID space internally. What i mean to say is that a Im not sure how to translate that requirement into a yang model but if someone specifies a nexthop with id 9 and puts it into a group specified with id 9 that will break is what I am saying. Zebra does not allow this and the linux kernel does not either. I think going forward we need to make the requirement in our yang models that a nexthop cannot be referenced individual. A route should always reference a nexthop group and that references the nexthops; whether its a single nexthop or not. |
you're quite right @sworleys :
|
yang/frr-nexthop.yang
Outdated
type empty; | ||
leaf active { | ||
type boolean; | ||
default true; |
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, and again Zebra determines this value, so again config false
yang/frr-nexthop.yang
Outdated
description | ||
"The nexthop-group name."; | ||
} | ||
list group-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.
+1
yang/frr-nexthop.yang
Outdated
description | ||
"A nexthop-group, represented as a list of nexthop objects."; | ||
|
||
list group-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.
+1, this is the global list of nexthop groups, group-entry
isn't descriptive
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.
see previous review comments
yang/frr-nexthop.yang
Outdated
type blackhole-type; | ||
description | ||
"A blackhole sub-type, if the nexthop is a blackhole type."; | ||
} | ||
|
||
leaf flags { |
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 know this wasn't touched in the PR, but it should be readonly as well; and maybe not even be a bitfield, it might make more sense to just split the flag values out into booleans
@qlyoung @mjstapp @sworleys Thanks a lot for review. I think most of the operational things in yang is present as we kept them untouched from previous data model. I completely agree that operational attributes are not required here. About Ids for nexthop is something required as yang does not support optional keys. As request last week I am coming up with draft which explains the same. We can discuss in today's call. |
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-10477/artifact/CI009BUILD/config.status/config.status Successful on other platforms
|
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-10478/ 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:
CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base1 Static Analyzer issues remaining.See details at |
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-10496/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:
|
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-10492/ This is a comment from an automated CI system. clang_check |
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-10493/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
clang_check |
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-10495/ 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 |
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-10497/ 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 |
Example of mandatory composite keys and xpath creation. |
@Spantik zebra operational rib model PR 5533 uses Current zebra learned route json output. For yang state data let us decide which fields do we really want to display.
|
In last two meetings we decided to keep operational nexthop separate from that of configuration. We will soon come up with PR for operational. |
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-10632/ 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:
|
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-10658/ 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:
|
Updated Yang with operational grouping ( grouping frr-nexthop-operational) which will be used in zebra operational model. I have added only those flags which are available in nexthop structure today. Please let me know if we need more attributes under operational. |
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-10663/ This is a comment from an automated CI system. |
yang/frr-nexthop.yang
Outdated
type inet:ip-address; | ||
type string { | ||
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.
Shouldn't this be completely empty?
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.
were experimenting with pattern and kept one space. We can keep it empty and will update it.
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 done I have updated it.
A common nexthop and group nexthop yang data model for all protocols in FRR. Co-authored-by: Santosh P K <sapk@vmware.com> Co-authored-by: Vishaldhingra <vdhingra@vmware.com> Signed-off-by: Santosh P K <sapk@vmware.com>
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-10672/ 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:
|
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.
overall changes look good. Need to check operational data based on daemons requirements (zebra etc.)
A common nexthop and group nexthop yang data model
for all protocols in FRR.
Co-authored-by: Santosh P K sapk@vmware.com
Co-authored-by: Vishaldhingra vdhingra@vmware.com
Signed-off-by: Santosh P K sapk@vmware.com