zebra: add an approach to keep the NHG ID unchanged for recursive rou… #16693
Conversation
There was a problem hiding this comment.
Before reviewing it further please fix all the styling issues and the commit requirements, as required here: https://github.com/FRRouting/frr/pull/16693/checks?check_run_id=29454500908.
|
|
||
| r1 = tgen.routers()["r1"] | ||
|
|
||
| r1.vtysh_cmd("conf t\nip route 1.1.1.1/24 100.0.0.1") |
There was a problem hiding this comment.
Please avoid using public IP ranges. See here: https://github.com/FRRouting/frr/blob/master/doc/developer/topotests.rst#writing-tests:
Always use IPv4 RFC 5737 (192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24) and IPv6 RFC 3849 (2001:db8::/32) ranges reserved for documentation.
| r1.vtysh_cmd("conf t\nip route 200.0.0.1/24 10.4.4.2") | ||
| r1.vtysh_cmd("conf t\nip route 200.0.0.1/24 10.4.4.2") | ||
|
|
||
| text1 = r1.vtysh_cmd("show ip route 200.0.0.0/24 nexthop-group") |
There was a problem hiding this comment.
Please use just show ip route ... json. It has installedNexthopGroupId. And we can avoid comparing plain-text, instead of JSON.
zebra/zebra_nhg.c
Outdated
| struct nhg_hash_entry *zebra_presvd_nhg_lookup(struct nhg_hash_entry *lookup) | ||
| { | ||
| if (lookup == NULL) | ||
| return lookup; |
There was a problem hiding this comment.
hash_lookup() returns NULL itself, no need to check here. We can just use return hash_lookup(...).
zebra/zebra_nhg.c
Outdated
| if (hash_lookup(zrouter.nhgs_presvd, nhe)) { | ||
| if (IS_ZEBRA_DEBUG_NHG) | ||
| zlog_debug("%s: NHG %pNG already exists", __func__, nhe); | ||
| return -1; |
There was a problem hiding this comment.
I don't see anywhere in the code for the return code being checked or am I missing it?
zebra/zebra_nhg.h
Outdated
| @@ -282,12 +282,17 @@ extern bool zebra_nhg_dependents_is_empty(const struct nhg_hash_entry *nhe); | |||
| /* Lookup ID, doesn't create */ | |||
| extern struct nhg_hash_entry *zebra_nhg_lookup_id(uint32_t id); | |||
|
|
|||
| /* Lookup preserved, doesn't create */ | |||
| extern struct nhg_hash_entry *zebra_presvd_nhg_lookup(struct nhg_hash_entry *lookup); | |||
There was a problem hiding this comment.
Could you name this function without shortening it with something like presvd? I don't see any reason to shorten and then guess what it means.
After discussing with Eddie, we’ve decided not to change the original recursive NHG creation logic for now. Instead of keeping the NHG unchanged, we’ll just update the FPM in |
xingrenwai
force-pushed
the
phoenix_wing
branch
from
d7a3b1c
to
72d4e44
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
xingrenwai
force-pushed
the
phoenix_wing
branch
from
72d4e44
to
aa64002
Compare
…ecursive routes converge This modification requirements comes from the HLD https://github.com/eddieruan-alibaba/SONiC/blob/eruan-recursive/doc/recursive/recursive_route.md Add a new function zebra_rnh_fixup_depends() to make a quick fixup on involved NHGs in FPM
xingrenwai
force-pushed
the
phoenix_wing
branch
from
aa64002
to
246eb4e
Compare
…tes and to quickly fix involved NHGs in the dataplanes when recursive routes converge
This modification requirements comes from the HLD
https://github.com/eddieruan-alibaba/SONiC/blob/eruan-recursive/doc/recursive/recursive_route.md
For achieving this quick fixup, we need the following changes