ospf6d: Remove ospf6 route when connected wins - #21476
Conversation
|
Note for me the ospf6_point_to_multipoint test fails infrequently ( between 1-8 times when running in parallel with itself 64 times ). Now it never fails. The test was failing with a unexpected ospf6 route in the zebra rib. From my perspective this is also more of a |
Greptile SummaryThis PR fixes a bug in Key changes:
The fix is well-targeted and addresses a race condition seen in the Confidence Score: 5/5This PR is safe to merge; the fix is well-scoped and correctly handles the stale-route edge case. The change is small, targeted, and addresses a real bug. Prior review concerns (GR guard, send return value) have been resolved per maintainer guidance. The logic in ospf6_route_add correctly guards with scope type and connected/non-connected checks. No P0 or P1 findings remain. No files require special attention.
|
| Filename | Overview |
|---|---|
| ospf6d/ospf6_route.c | Adds connected-wins detection in ospf6_route_add; condition correctly checks route->connected && !next->connected with GLOBAL scope guard before calling new delete helper. |
| ospf6d/ospf6_zebra.c | Adds ospf6_zebra_route_delete_prefix; sends ZEBRA_ROUTE_DELETE by prefix/type without nexthops, with error logging on failure — consistent with zebra deletion semantics. |
| ospf6d/ospf6_zebra.h | Adds extern declaration for ospf6_zebra_route_delete_prefix; straightforward header update. |
Reviews (2): Last reviewed commit: "ospf6d: Remove ospf6 route when connecte..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| void ospf6_zebra_route_delete_prefix(struct ospf6_route *route, struct ospf6 *ospf6) | ||
| { | ||
| struct zapi_route api; | ||
|
|
||
| if (IS_OSPF6_DEBUG_ZEBRA(SEND)) | ||
| zlog_debug("Send delete for route: %pFX a connected in Zebra now wins", | ||
| &route->prefix); | ||
|
|
||
| zapi_route_init(&api); | ||
| api.vrf_id = ospf6->vrf_id; | ||
| api.type = ZEBRA_ROUTE_OSPF6; | ||
| api.safi = SAFI_UNICAST; | ||
| api.prefix = route->prefix; | ||
| zclient_route_send(ZEBRA_ROUTE_DELETE, ospf6_zclient, &api); |
There was a problem hiding this comment.
ospf6_zebra_route_delete_prefix sends a ZEBRA_ROUTE_DELETE to zebra without checking the GR state, while both ospf6_zebra_route_update_remove and ospf6_zebra_route_update_add explicitly bail out during graceful restart to preserve forwarding continuity. Deleting the OSPF6 route during GR would break the in-progress restart's forwarding plane state.
The same guard used by the other removal path should be applied here:
void ospf6_zebra_route_delete_prefix(struct ospf6_route *route, struct ospf6 *ospf6)
{
struct zapi_route api;
if (ospf6->gr_info.restart_in_progress
|| ospf6->gr_info.prepare_in_progress) {
if (IS_DEBUG_OSPF6_GR)
zlog_debug(
"Zebra: Graceful Restart in progress -- not deleting %pFX",
&route->prefix);
return;
}
if (IS_OSPF6_DEBUG_ZEBRA(SEND))
zlog_debug("Send delete for route: %pFX a connected in Zebra now wins",
&route->prefix);
zapi_route_init(&api);
api.vrf_id = ospf6->vrf_id;
api.type = ZEBRA_ROUTE_OSPF6;
api.safi = SAFI_UNICAST;
api.prefix = route->prefix;
zclient_route_send(ZEBRA_ROUTE_DELETE, ospf6_zclient, &api);
}Prompt To Fix With AI
This is a comment left during a code review.
Path: ospf6d/ospf6_zebra.c
Line: 542-555
Comment:
**Missing Graceful Restart guard**
`ospf6_zebra_route_delete_prefix` sends a `ZEBRA_ROUTE_DELETE` to zebra without checking the GR state, while both `ospf6_zebra_route_update_remove` and `ospf6_zebra_route_update_add` explicitly bail out during graceful restart to preserve forwarding continuity. Deleting the OSPF6 route during GR would break the in-progress restart's forwarding plane state.
The same guard used by the other removal path should be applied here:
```c
void ospf6_zebra_route_delete_prefix(struct ospf6_route *route, struct ospf6 *ospf6)
{
struct zapi_route api;
if (ospf6->gr_info.restart_in_progress
|| ospf6->gr_info.prepare_in_progress) {
if (IS_DEBUG_OSPF6_GR)
zlog_debug(
"Zebra: Graceful Restart in progress -- not deleting %pFX",
&route->prefix);
return;
}
if (IS_OSPF6_DEBUG_ZEBRA(SEND))
zlog_debug("Send delete for route: %pFX a connected in Zebra now wins",
&route->prefix);
zapi_route_init(&api);
api.vrf_id = ospf6->vrf_id;
api.type = ZEBRA_ROUTE_OSPF6;
api.safi = SAFI_UNICAST;
api.prefix = route->prefix;
zclient_route_send(ZEBRA_ROUTE_DELETE, ospf6_zclient, &api);
}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
It's not necessary to do this. The code is only removing the route if a ospf6 route was already installed. We cannot be in graceful restart situation if this is happening.
| api.type = ZEBRA_ROUTE_OSPF6; | ||
| api.safi = SAFI_UNICAST; | ||
| api.prefix = route->prefix; | ||
| zclient_route_send(ZEBRA_ROUTE_DELETE, ospf6_zclient, &api); |
There was a problem hiding this comment.
zclient_route_send return value ignored
Every other direct zclient_route_send call in this file (e.g. in ospf6_zebra_route_update around line 499–507) checks the return code and calls flog_err on ZCLIENT_SEND_FAILURE. The new function silently drops any send error, making failures invisible in logs.
| zclient_route_send(ZEBRA_ROUTE_DELETE, ospf6_zclient, &api); | |
| int ret = zclient_route_send(ZEBRA_ROUTE_DELETE, ospf6_zclient, &api); | |
| if (ret == ZCLIENT_SEND_FAILURE) | |
| flog_err(EC_LIB_ZAPI_SOCKET, | |
| "zclient_route_send() delete failed: %s", | |
| safe_strerror(errno)); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: ospf6d/ospf6_zebra.c
Line: 555
Comment:
**`zclient_route_send` return value ignored**
Every other direct `zclient_route_send` call in this file (e.g. in `ospf6_zebra_route_update` around line 499–507) checks the return code and calls `flog_err` on `ZCLIENT_SEND_FAILURE`. The new function silently drops any send error, making failures invisible in logs.
```suggestion
int ret = zclient_route_send(ZEBRA_ROUTE_DELETE, ospf6_zclient, &api);
if (ret == ZCLIENT_SEND_FAILURE)
flog_err(EC_LIB_ZAPI_SOCKET,
"zclient_route_send() delete failed: %s",
safe_strerror(errno));
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
seems reasonable. change made
Currently when ospf6 installs a route that is later covered by a connected, the ospf6 route is never removed. Modify the code to detect the case where the new route that `wins` in ospf6 notices that it is connected and the old route that was in was a ospf6 route. If so send a specific route deletion for the ospf6 route. This failure is happening infrequently in the ospf6_point_to_multipoint test due to timing issues on how connected routes are received into ospf6. Signed-off-by: Donald Sharp <sharpd@nvidia.com>
cfbdcf4 to
7acbf6d
Compare
|
@greptile review |
|
@Mergifyio backport stable/10.6 stable/10.5 |
✅ Backports have been createdDetails
|
2 participants
Currently when ospf6 installs a route that is later covered by a connected, the ospf6 route is never removed. Modify the code to detect the case where the new route that
winsin ospf6 notices that it is connected and the old route that was in was a ospf6 route. If so send a specific route deletion for the ospf6 route.This failure is happening infrequently in the ospf6_point_to_multipoint test due to timing issues on how connected routes are received into ospf6.