staticd: nexthop identity as path-list key, and per-route metric - #21296
Conversation
Greptile SummaryThis PR restructures the staticd YANG schema from a two-level All previously flagged issues (metric=0 not transmitted, tag recalculation skipped at zero, tag loss on distance change, stale tag in ECMP move) have been addressed in the current code. Confidence Score: 5/5Safe to merge — all previously flagged P1 issues are resolved and no new defects found. All P0/P1 findings from the prior review rounds (metric=0 omission, tag recalculation on deletion, tag loss on distance change, stale tag in ECMP move) are addressed in the current code. The zebra_rib.c changes correctly extend metric-based route identity to static routes. The flat YANG schema, NB callbacks, CLI encoding, and topotests are internally consistent. No remaining blocking issues identified. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as VTY/CLI
participant NB as Northbound (NB)
participant SR as static_routes.c
participant SZ as static_zebra.c
participant ZR as zebra_rib.c
CLI->>NB: ip route PREFIX NH [distance D] [metric M]
NB->>NB: NB_EV_VALIDATE: ecmp_path_list_validate
NB->>SR: NB_EV_APPLY: static_path_list_create
SR->>SR: static_add_nexthop(pn, nh_type, ...)
Note over NB,SR: On distance/metric MODIFY
NB->>SR: static_path_list_distance_modify / metric_modify
SR->>SR: static_nexthop_move_path(nh, new_dist, new_metric)
Note over SR,ZR: Route install
SR->>SZ: static_zebra_route_add(pn, install=true)
SZ->>ZR: zapi_route send
Note over ZR: Route distinction by metric
ZR->>ZR: rib_compare_routes: STATIC with different metrics are distinct
ZR->>ZR: rib_route_match_ctx: match requires distance AND metric
Reviews (8): Last reviewed commit: "tests: staticd: add topotest for static ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
ab87897 to
26a83bf
Compare
|
Looks astonishingly similar to #20859, but you changed the RIB handling logic as well which I missed since I wasn't aware of its special handling of static routes. This should resolve @donaldsharp 's comment regarding ordering in the other PR. Route removal works worse than in my PR: If distance is specified in the The This PR has way more docs than I had, but where I was a bit on the terse side (and didn't explain the "distance, metric and nexthop are all keys, but you still can't have multiple distances/metrics for one nexthop" topic because I didn't fully understand it yet) this is now on the extremely verbose side. In PR #20859, you mentioned that you used an AI tool, so no surprise here - verbosity is one of the core competencies of AI. Still, something a bit more to the point would probably motivate readers more to actually read the docs 😉 |
26a83bf to
bea9ba4
Compare
|
Hi, @gromit1811:
Thanks for your review. The inconsistent behavior with route deletion is fixed. The doc and topotests are updates. With the fix, it's "lazy deletion" without distance and metric. If either distance or metric is specified, it's exact match for deletion. |
bea9ba4 to
4cdf1a0
Compare
|
Rebased. |
|
ci:rerun |
|
Could you also apply frrbot styling? |
4cdf1a0 to
719f137
Compare
Done. Thanks |
5bca73e to
7d8e9f8
Compare
|
ci:rerun |
|
Disclaimer: Not looked at the latest code, just played with it a bit. RIB processing looks good, config handling feels good as well, with a minor exception:
Shouldn't it rather be exact match only for what's specified? If I do a And you're missing this change to |
|
@greptileai Please continue as long as you are not stuck. You have been doing good. Thanks. |
|
Good progress — all previously flagged bugs (stale tag on ECMP path-move, tag lost on new path, tag recalc guard at zero, metric always sent) appear to be fixed in the latest push. One new issue introduced by the schema flattening:
In the old two-level schema, path-list-destroy called // staticd/static_nb_config.c
static int static_path_list_destroy(struct nb_cb_destroy_args *args)
{
...
case NB_EV_APPLY:
nh = nb_running_unset_entry(args->dnode);
static_delete_nexthop(nh); // removes nh from pn->nexthop_list, unlocks rn for nh
// BUG: pn is now empty but never freed or removed from si->path_list
break;After
For ECMP paths shared by multiple nexthops, the path must only be freed once the list is empty. The fix mirrors what case NB_EV_APPLY: {
struct static_path *pn;
struct route_node *rn;
struct static_route_info *si;
nh = nb_running_unset_entry(args->dnode);
pn = nh->pn;
rn = pn->rn;
si = rn->info;
static_delete_nexthop(nh); /* removes nh, sends route update/delete, unlocks rn for nh */
if (static_nexthop_list_count(&pn->nexthop_list) == 0) {
static_path_list_del(&si->path_list, pn);
route_unlock_node(rn); /* balance the lock from static_add_path */
XFREE(MTYPE_STATIC_PATH, pn);
}
break;
}Lock accounting: |
abfb674 to
834ab83
Compare
|
@greptileai review |
Fixed, thanks.
|
choppsv1
left a comment
choppsv1
left a comment
There was a problem hiding this comment.
The choice to flatten the list created a much larger diff and change to the YANG. Did you consider just moving the distance leaf down into the nexthop structure? It would seem like a much smaller change.
It is probably the right thing to do, flattening the list. I was just wondering about the choice. |
Hi, @choppsv1 The backend structure layout (prefix->path->nexthop) fits well with ECMP, and with zebra interaction. So I was not looking at changing that. I also don't see a need. |
|
I agree that using a single list is better b/c it makes more sense to the user. Previously staticd had to merge multiple path-list entries into the resulting nexthop list for the route so there's no reason for the nested lists IMO. The CLI can still sort based on table-id for the only real user benefit. However, implementing a tag seq-num to maintain bug-for-bug non-idempotent compatibility is sort of crazy. :) Here's a patch that replaces that with the deterministic, idempotent behavior of max-tag-value wins. Claude even helped fix-up all the tests. If you accept these please apply to the respective commits which are the first code commit, the first couple test commits and the doc commit. Don't forget to update commit text if it mentions the tag behavior too. |
|
Hi, @choppsv1 I like the idea of "max-tag-wins"! That's more deterministic than the current "last-write-wins". It took me some effort to match that behavior :-( Thanks for the patch. I will go ahead and apply it. As the branch is behind, let us first rebase/push, and then work on applying your patch. |
edd91f7 to
b3a23b1
Compare
|
Hi, @choppsv1 I have applied your patch for "max-value-wins" for tag, and adjusted commit messages and comments on "last-write-wins". Please review the updated patches. Thanks. |
choppsv1
left a comment
choppsv1
left a comment
There was a problem hiding this comment.
Please go back through the changes and change all the comments that refer to "the flat schema" or anything else about the previous implementation. The comments should speak to the code that is there now, and not refer to something previously there which someone reading the code for the first time has no knowledge of. :)
Motivation ---------- Static routes for a prefix can have multiple nexthops, but each nexthop must be unique for that prefix. The nexthop — identified by the combination of (table-id, nh-type, vrf, gateway, interface) — should therefore be the natural key in the YANG schema for static routes. The previous schema keyed path-list on [table-id, distance] with a nexthop-list nested below. Since distance is not part of nexthop identity, the same nexthop could appear under multiple path-list entries at different distances without YANG detecting any conflict. The CLI carried extra logic to detect and suppress such duplicates, allowing other config methods (e.g., RESTCONF) to create unexpected and unhandled YANG config state. Design ------ Replace the two-level path-list/nexthop hierarchy with a single flat list keyed by full nexthop identity: path-list [table-id, nh-type, vrf, gateway, interface] Each path-list entry represents exactly one nexthop. Distance becomes a non-key leaf attribute (default 1) on the entry. Changing distance on an existing nexthop is now a leaf MODIFY with no DESTROY+CREATE -- the NB callback migrates the nexthop to the appropriate internal struct static_path (keyed by (table-id, distance) in C) without withdrawing the route. Internally, struct static_path groups nexthops that share the same (table-id, distance) for ECMP and RIB installation; this grouping is an implementation detail not exposed in the YANG key. Per-leaf callbacks and apply_finish ------------------------------------ The path-list entry has an apply_finish callback (static_path_list_apply_finish) that calls static_install_nexthop() once after all per-leaf modify callbacks have run. Per-leaf callbacks (tag_modify, distance_modify) update internal state and mark the nexthop with nh->state = STATIC_START rather than calling install helpers directly. This ensures a single consolidated route ADD reaches zebra regardless of how many attributes change in one transaction. Weight and BFD monitoring -------------------------- In the previous schema, weight and bfd-monitoring were per-nexthop attributes added via augment blocks on the nested frr-nexthops/nexthop container. Since each path-list entry is the nexthop, these attributes become direct leaves/containers of the path-list entry in the staticd-prefix-attributes grouping, eliminating the augment indirection. Tag handling ------------ Tag is a per-path-group attribute at the C and RIB level: all nexthops sharing the same distance carry one tag in the RIB. Each nexthop has its own YANG tag leaf, but they share a single pn->tag in the C backend. When nexthops in the same group are configured with different tags, max-value-wins applies: the tag with the highest value across the group's nexthops takes effect. When a nexthop is deleted, if it carried the current maximum tag, pn->tag is recalculated as the maximum of the remaining nexthops' tags. Note: this YANG-vs-RIB divergence for tag is not a new behavior introduced by this schema change. Under the previous [table-id, distance] key, nexthops at the same distance already shared one path-list entry and one tag in the RIB. This schema does not change that behavior. To assign independent tags that are each visible separately in the RIB, configure the nexthops with different administrative distances so they belong to distinct path groups. ECMP validation --------------- Restore the two validation checks that were attached to the old nexthop-list create callback: - Blackhole/non-blackhole mixing: a path-list entry with nh-type STATIC_BLACKHOLE may not coexist with non-blackhole entries in the same ECMP group (same table-id and distance). - ECMP count limit: the number of nexthops in an ECMP group may not exceed zebra_ecmp_count. These checks live in the NB_EV_VALIDATE handlers of static_path_list_create and distance_modify, using path_list_ecmp_iter_cb to iterate sibling path-list entries and accumulate the group's nexthop count and nexthop types. Revalidating on distance change is necessary because a distance change moves the nexthop into a different ECMP group, which may violate the constraints of that target group. YANG revision ------------- A new revision date is recorded in frr-staticd.yang for this backward-incompatible path-list key change. Signed-off-by: Enke Chen <enchen@paloaltonetworks.com>
The staticd path-list key changed from (table-id, distance) to nexthop identity (table-id, nh-type, vrf, gateway, interface), with distance becoming a non-key attribute. Update the hard-coded mgmt set-config xpaths in test_yang_mgmt.py: remove '[distance=1]/frr-nexthops/nexthop[' from each path-list predicate — nh-type, vrf, gateway, and interface are now direct keys on the path-list entry itself. Signed-off-by: Enke Chen <enchen@paloaltonetworks.com>
The staticd path-list key changed from (table-id, distance) to nexthop identity (table-id, nh-type, vrf, gateway, interface), with distance and metric as non-key attributes. Update test_grpc.cpp: - xpath predicate: replace path-list[table-id][distance]/frr-nexthops/ nexthop[nh-type]... with flat path-list[table-id][nh-type]... - Restructure all 13 path-list JSON blocks from the nested frr-nexthops/nexthop format to the flat format where nh-type, vrf, gateway, and interface are direct path-list fields Signed-off-by: Enke Chen <enchen@paloaltonetworks.com>
Add metric to the ZEBRA_ROUTE_STATIC identity checks in zebra so that two static routes at the same distance but different metrics are treated as distinct route_entries: - rib_compare_routes(): add a ZEBRA_ROUTE_STATIC metric check so that an incoming static route at a different metric is not treated as an update to the existing entry. - process_subq_early_route_delete(): extend the existing ZEBRA_ROUTE_KERNEL metric check to also cover ZEBRA_ROUTE_STATIC, matching the rib_compare_routes() logic. - rib_route_match_ctx(): add metric to the ZEBRA_ROUTE_STATIC identity check (alongside distance) in both the is_update and non-update paths, so that dplane results are matched back to the correct route_entry when two static entries coexist at the same distance but different metrics. rib_choose_best() already selects the lower-metric entry as best, giving correct floating-route semantics without further changes. Signed-off-by: Enke Chen <enchen@paloaltonetworks.com>
Add metric as a non-key leaf attribute on the flat path-list entry, mirroring how distance is already handled after the schema refactoring. YANG ---- Add a metric leaf (uint32, default 0) to the staticd-prefix-attributes grouping. Metric is not part of the nexthop-identity key; changing it on an existing nexthop triggers a leaf MODIFY rather than a DESTROY+CREATE. Internal path grouping ---------------------- Extend struct static_path to carry metric alongside distance. The C backend groups nexthops that share (table-id, distance, metric) onto the same path for ECMP and RIB installation. Update static_add_path() to accept and store the metric, and propagate the new parameter through all call sites. NB callback ----------- static_path_list_metric_modify() migrates the nexthop to the appropriate static_path when the metric value changes, using the shared helper static_nexthop_move_path() (introduced here and also used by distance_modify). The helper handles the full old-path teardown and new-path setup, ending with nh->state = STATIC_START so that apply_finish() issues a single consolidated route ADD. VTY --- Extend the ip/ipv6 route CLI to accept an optional metric keyword, and update show commands to display the metric when non-zero. Signed-off-by: Enke Chen <enchen@paloaltonetworks.com>
Add two new sections to doc/user/static.rst: 1. Administrative Distance and Metric: explains that static routes are grouped by (table-id, distance, metric) into path groups; nexthops sharing the same tuple form an ECMP set. Covers floating static routes (nexthops with different (distance, metric) tuples form separate path groups; all groups present in the RIB with the best-preference group selected), nexthop uniqueness / automatic move when distance or metric changes, and lazy deletion keyed on nexthop identity (distance and metric arguments are ignored on 'no ip route'). 2. Route Tag: explains that tag is a per-path-group attribute (not a key), how to assign distinct tags to different path groups via distance or metric, and the limitation that nexthops with the same (distance, metric) always share one tag — the last-configured value wins. Also update all CLI command signatures to include [tag TAG] before DISTANCE and [metric METRIC] after DISTANCE, and add brief inline descriptions of TAG, DISTANCE, and METRIC pointing to the new sections. Signed-off-by: Enke Chen <enchen@paloaltonetworks.com>
Cover the four scenarios introduced by the apply_finish commit:
1. Tag + metric combined change in one transaction (tag_modify and
metric_modify both fire, then apply_finish once).
2. Distance + metric combined change (distance_modify and metric_modify,
then apply_finish once).
3. Tag + distance + metric all three in one transaction (all three
per-leaf callbacks, then apply_finish once).
4. Tag no-op: tag_modify fires but pn->tag is unchanged after
static_path_recalc_tag(); the no-op guard breaks early without
setting nh->state = STATIC_START; apply_finish still fires and
must leave the route correctly installed.
All cases run for both IPv4 and IPv6.
Signed-off-by: Enke Chen <enchen@paloaltonetworks.com>
Add tests/topotests/static_route_distance/ covering: - Per-path administrative distance: AD replacement, ECMP with mixed ADs, lazy deletion keyed on nexthop identity, running-config correctness after each operation. - Per-path tag: basic tag assignment, independent tags per path, tag change in place, combined AD+tag change in one command, max-value-wins recalculation on deletion (delete max-tag holder with one survivor, delete non-max nexthop, delete max-tag holder with two survivors), running-config format with tag+distance+metric, re-add of a previously deleted max-tag holder, and tag recalculation when a nexthop changes distance or metric (max-tag holder moves, non-max nexthop moves, and joining an existing tagged path). - ECMP validation: zebra is started with -e 2 (ECMP limit = 2) to make the count-limit reachable with the three-nexthop topology. Two new test cases verify that configuration-time validation works: blackhole/non-blackhole mixing is rejected, and adding a third nexthop to a full ECMP group is rejected. Both cases check that the error is reported and the existing route is left unchanged. Signed-off-by: Enke Chen <enchen@paloaltonetworks.com>
Add test_static_route_metric.py to tests/topotests/static_route_distance/ covering 7 scenarios × IPv4+IPv6 (14 test functions): 1. Metric replacement: changing a nexthop's metric removes the old-metric RIB entry and installs a new one at the new metric — no stale duplicate is left behind. 2. ECMP at same metric: two nexthops with identical metric are installed as a single ZAPI ADD with two nexthops, producing true ECMP. 3. Floating routes: routes at the same distance but different metrics are kept as separate zebra route_entries; rib_choose_best() selects the lower-metric entry; removing it promotes the higher-metric standby. 4. Metric change promote/demote: adjusting a route's metric causes the correct entry to become active. 5. Nexthop-identity deletion: deletion always uses a lazy search keyed on nexthop identity; distance and metric arguments are ignored. All forms of 'no ip route X/M via Y' remove the route regardless of which distance or metric it was installed with. A two-nexthop case (NH1@metric=100, NH2@metric=200) verifies that zebra's delete-lookup loop removes only the NH1 entry while leaving the NH2 entry intact, directly exercising the metric check in process_subq_early_route_delete. 6. Running-config format: "metric N" appears in show running-config output after the distance, confirming YANG/CLI round-trip. 7. ECMP primaries + metric-based standby: NH1 and NH2 at metric=100 form an ECMP primary group; NH3 at metric=200 is the standby. Removing NH1 shrinks the ECMP group to NH2 alone without promoting NH3. Removing NH2 (the last primary) promotes NH3 to active. Signed-off-by: Enke Chen <enchen@paloaltonetworks.com>
b3a23b1 to
2f81a63
Compare
Done. Thanks. |
|
Thanks so much @choppsv1 for your help and contribution in this project! |
5 participants
This patch series restructures the staticd YANG schema and adds per-route metric support for static routes.
YANG schema (nexthop identity as path-list key)
The previous schema keyed path-list on [table-id, distance] with a nested nexthop-list. Since distance is not part of
nexthop identity, the same nexthop could appear under multiple path-list entries at different distances without YANG
detecting a conflict, creating state that the CLI could not always handle correctly.
The new schema uses a single list keyed by full nexthop identity:
path-list [table-id, nh-type, vrf, gateway, interface]
Distance and metric become non-key leaf attributes (defaults 1 and 0). Changing either on an existing nexthop is now a
leaf MODIFY — the NB callback migrates the nexthop to the appropriate internal struct static_path without withdrawing
the route. Weight and BFD monitoring, previously attached via augment blocks on the nested nexthop container, become direct leaves of the path-list entry.
Per-route metric
metric is added alongside distance and tag as a non-key path-list attribute. Nexthops sharing the same (table-id,
distance, metric) form one ECMP group in the RIB; nexthops with different metrics are separate RIB entries, enabling
metric-based floating static routes. Zebra is extended to treat static routes at the same distance but different
metrics as distinct route entries.
Also included
round-trip)
Please see the individual commits for details.