lib,zebra,sharpd: allow recursive resolution of nexthop-groups #16775
Conversation
Re-org/simplify the api used to add nhgs via zapi messages from daemons, using a temporary nhe so we can convey all nhe attributes. Signed-off-by: Mark Stapp <[email protected]>
Add a flags field to the zapi nexthop-group struct; encode and decode the field. Signed-off-by: Mark Stapp <[email protected]>
Add a 'recursive' flag field and corresponding config option for nexthop-groups. Signed-off-by: Mark Stapp <[email protected]>
Update sharpd's internal api so that we carry more config info when processing nhg config callbacks. This is used to allow access to, e.g., the flags field. Signed-off-by: Mark Stapp <[email protected]>
There was a problem hiding this comment.
it is possible to see how to reuse some commits from #16028 ?
As discussed yesterday, this extension has not wanted to control the zebra NHGs. The recursive keyword has been introduced, along with testing and many other zebra elements.
| @@ -91,6 +91,8 @@ struct nexthop_group_cmd { | |||
|
|
|||
| struct nexthop_group nhg; | |||
|
|
|||
| uint32_t flags; | |||
|
|
|||
There was a problem hiding this comment.
very similar to what is proposed 1b743ec
| @@ -483,6 +483,7 @@ struct zapi_nexthop { | |||
| struct zapi_nhg { | |||
| uint16_t proto; | |||
| uint32_t id; | |||
| uint32_t flags; | |||
|
|
|||
There was a problem hiding this comment.
uint32 isnot it too much?
wht other fields do you intend to push in there?
There was a problem hiding this comment.
frankly I would rather have the bit field be large instead of having a problem 2-3 years down the line when we accidently go beyond the bitfield
| @@ -27,3 +27,9 @@ listing of ECMP nexthops used to forward packets. | |||
| will be automatically re-assigned. This cli command must be the first | |||
| command entered currently. Additionally this command only works with linux 5.19 | |||
| kernels or newer. | |||
|
|
|||
| .. clicmd:: recursive | |||
|
|
|||
| * Resolution is going to need some more work. | ||
| */ | ||
| if (CHECK_FLAG(api_nhg.flags, ZAPI_NHG_FLAG_RECURSIVE)) | ||
| SET_FLAG(nhe->flags, NEXTHOP_GROUP_RECURSION_REQ); | ||
|
|
There was a problem hiding this comment.
do you intend to offer this flag for protocol nexthop groups that depend of other protocol nexthop groups (without nexthops) ? Because if it is not the case, the nhe->nhg.nexthop flags can be used to store the ALLOW_RECURSION flag.
If it is the case, do you have a use case in mind to proceed like that ?
|
|
||
| flags = nhgc->flags; | ||
|
|
||
| if (no) |
There was a problem hiding this comment.
is it possible to add a topotest like was proposed in e60cd34 ?
|
What I want to try is to make an incremental change - and avoided "many other ... elements" on purpose. I think it makes most sense to take some careful steps, learn about the issues and impacts as we go. Most of this PR is pretty obvious - we've been talking about this for a long time. But even this small change has raised questions about threshold for nexthop reinstallation, address-family rules, about zapi messaging, about reporting results, about the "contract" that we would expect to exist between zebra and protocol daemons. As we resolve those questions, I think we'll understand better how to take the next step - to use this path to allow zebra to modify an nhg in-place, under some circumstances.
|
Add support for recursive resolution of nexthop-groups: iterate over a group's nexthops, use the zebra validity-checking logic, and update as necessary. This is driven by daemon-owned nhgs, allowing the use of recursive nexthops in those nhgs. Signed-off-by: Mark Stapp <[email protected]>
Add some words about the recursive nhg config option; clean up duplicate doc tags. Signed-off-by: Mark Stapp <[email protected]>
mjstapp
force-pushed
the
nhg_resolve
branch
from
d792691
to
fb655cb
Compare
|
Pushed an update that should help with CI... |
I just made what you requested, ie separate BGP from ZEBRA.
I just wan to avoid sending non tested code in the upstream. |
| @@ -2074,6 +2074,9 @@ static void zread_nhg_add(ZAPI_HANDLER_ARGS) | |||
| nhe->zapi_instance = client->instance; | |||
| nhe->zapi_session = client->session_id; | |||
|
|
|||
| /* TODO -- may need an AFI, so try to derive one */ | |||
| nhe->afi = zebra_nhg_get_afi(nhg, AF_INET); | |||
|
|
|||
There was a problem hiding this comment.
Let us separate AFI move from this pull request => #17049
Add initial support for recursive resolution of nexthop-groups:
I'm making this a draft for now, so we can have some discussion. There are still some gaps - sharpd's groups aren't being uninstalled and deleted properly, for example, so I'd like to clean that up. I'm not sure that all necessary changes to a nexthop are being detected in this path. And I haven't exercised these nexthop-group changes in coordination with routes, so there are some questions and TODOs there also.