Mgmt frontend problems in zebra - #21252
Conversation
Greptile SummaryThis PR moves Confidence Score: 5/5PR is safe to merge; all previously identified P0/P1 issues have been resolved and no new blocking issues were found. The two P1 issues raised in earlier review rounds (wrong msg_len in the edit error path, stale route-map data in route_map_destroy) are both correctly addressed. The remaining findings are P2: a stale docstring copy-paste and test entries not cleaned up after the new bad-values test. Neither affects correctness or production behavior. tests/topotests/zebra_rib/test_zebra_import.py — minor cleanup and docstring issues only
|
| Filename | Overview |
|---|---|
| lib/mgmt_fe_client.c | Adds MGMT_MSG_CODE_EDIT error path handling using mgmt_msg_native_get_msg_len(edit_msg) (fixing the previous msg_len bug); success path passes error=0, errstr=NULL — both correct. |
| lib/mgmt_fe_client.h | Extends edit_notify callback signature with int error, const char *errstr — clean, consistent with commit/lock pattern. |
| mgmtd/mgmt_vty_frontend.c | Updated to new edit_notify signature; errors are surfaced to VTY with CMD_WARNING_CONFIG_FAILED and the errstr printed to the user — correct behavior. |
| zebra/zebra_nb_config.c | Implements previously-stubbed northbound callbacks; route_map_destroy is now a no-op delegating to parent apply_finish, which reads committed dnode (fixes the stale-data issue). |
| zebra/zebra_cli.c | Adds YANG-backed DEFPY commands for dplane-limit, zapi-packets, workqueue-timer, and import-table (IPv4 and IPv6); cli_show callbacks added. |
| zebra/zebra_vty.c | Removes legacy DEFUN/DEFPY handlers for commands now owned by zebra_cli.c/northbound. zebra_ip_config simplified to return 0 since YANG handles serialization. |
| yang/frr-zebra.yang | Converts import-kernel-table from container to list keyed on afi-safi+table-id; adds must expression restricting afi-safi to IPv4/IPv6 unicast/multicast; removes old range "1..252" from table-id (OS-agnostic, validated in C callbacks). |
| zebra/zebra_nb.c | Updates northbound node registration from table-id leaf to import-kernel-table list with create/destroy/apply_finish callbacks — correct structural change. |
| tests/topotests/zebra_rib/test_zebra_import.py | Adds check_show_running helper and new test cases verifying show-running output, route-map lifecycle, and test_zebra_import_bad_values for invalid/valid table ID validation via mgmt interface. |
| lib/northbound.c | Comment-only update clarifying that deleted dnodes not found in candidate config fall through to a parent destroy iteration — no logic change. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[VTY: ip import-table N] --> B[zebra_cli.c DEFPY_YANG]
B --> C[nb_cli_enqueue_change NB_OP_CREATE on list entry]
C --> D[nb_cli_apply_changes]
D --> E{NB_EV_VALIDATE zebra_import_kernel_table_create}
E -- valid table_id --> F[NB_EV_APPLY: no-op]
E -- invalid table_id --> G[NB_ERR_VALIDATION Error to VTY]
F --> H[apply_finish zebra_import_kernel_table_apply_finish]
H --> I[zebra_import_kernel_table_apply add=true reads committed dnode]
I --> J[zebra_import_table AFI/SAFI/VRF/table_id/distance/rmap]
K[VTY: no ip import-table N] --> L[no_ip_zebra_import_table_cmd]
L --> M[nb_cli_enqueue_change NB_OP_DESTROY on list entry]
M --> N[zebra_import_kernel_table_destroy NB_EV_APPLY]
N --> O[zebra_import_kernel_table_apply add=false distance=0 rmap=NULL]
O --> J
P[mgmtd EDIT reply success] --> Q[edit_notify error=0 errstr=NULL]
R[mgmtd EDIT reply error] --> S[edit_notify error!=0 errstr set]
Q --> T[vty_mgmt_resume_response CMD_SUCCESS]
S --> U[vty_out errstr / CMD_WARNING_CONFIG_FAILED]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: tests/topotests/zebra_rib/test_zebra_import.py
Line: 545
Comment:
**Stale docstring copied from `test_zebra_mrib_import`**
The function docstring says `"Verify router starts with the initial MRIB"`, but this test actually validates rejection of invalid table IDs and acceptance of valid ones. The docstring was copied from an earlier test and not updated.
```suggestion
"Verify invalid and valid import-table IDs are properly handled"
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: tests/topotests/zebra_rib/test_zebra_import.py
Line: 566-574
Comment:
**Valid table entries not cleaned up after test**
The loop creates three live import-table entries (table IDs 1, 253, 1000 with `ipv4-unicast`) but does not remove them. Since this test is currently last before `test_memory_leak`, the leaked config entries aren't a problem today, but they would pollute state if a new test is inserted after this one. Consider adding a teardown block that removes the created entries:
```python
# Cleanup valid entries
for tid in valids:
r1.vtysh_cmd(f"conf term\n no ip import-table {tid}\n")
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (4): Last reviewed commit: "tests: update test description" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| int zebra_import_kernel_table_route_map_destroy(struct nb_cb_destroy_args *args) | ||
| { | ||
| switch (args->event) { | ||
| case NB_EV_VALIDATE: | ||
| case NB_EV_PREPARE: | ||
| case NB_EV_ABORT: | ||
| case NB_EV_APPLY: | ||
| /* TODO: implement me. */ | ||
| break; | ||
| } | ||
| const struct lyd_node *dnode = | ||
| (const struct lyd_node *)args->dnode->parent; | ||
|
|
||
| if (args->event != NB_EV_APPLY) | ||
| return NB_OK; | ||
|
|
||
| if (zebra_import_kernel_table_apply(dnode, true) < 0) | ||
| return NB_ERR; | ||
|
|
||
| return NB_OK; | ||
| } |
There was a problem hiding this comment.
route_map_destroy re-applies with stale route-map data
In FRR's northbound framework, args->dnode for a destroy callback points to the old (pre-deletion) diff node. Therefore args->dnode->parent is the list entry as it existed before the change, meaning the route-map leaf is still present as a child. As a result, yang_dnode_exists(dnode, "route-map") returns true and the old route-map name is read, so zebra_import_kernel_table_apply(dnode, true) re-applies the import table with the route-map still in effect instead of removing it.
This path is not exercised by the CLI (no ip import-table / no ipv6 import-table always destroys the entire list entry via NB_OP_DESTROY on the entry xpath, triggering zebra_import_kernel_table_destroy), but it is reachable via direct YANG/mgmtd operations that delete only the route-map leaf while keeping the list entry.
The callback needs to call zebra_import_table explicitly with rmap = NULL to apply the updated state without the route-map:
int zebra_import_kernel_table_route_map_destroy(struct nb_cb_destroy_args *args)
{
const struct lyd_node *dnode =
(const struct lyd_node *)args->dnode->parent;
const char *afi_safi = yang_dnode_get_string(dnode, "afi-safi");
uint32_t table_id = yang_dnode_get_uint32(dnode, "table-id");
uint32_t distance = yang_dnode_get_uint32(dnode, "distance");
afi_t afi;
safi_t safi;
if (args->event != NB_EV_APPLY)
return NB_OK;
yang_afi_safi_identity2value(afi_safi, &afi, &safi);
/* Re-apply without the route-map since it is being removed */
if (zebra_import_table(afi, safi, VRF_DEFAULT, table_id, distance,
NULL, true) < 0)
return NB_ERR;
return NB_OK;
}Prompt To Fix With AI
This is a comment left during a code review.
Path: zebra/zebra_nb_config.c
Line: 239-251
Comment:
**`route_map_destroy` re-applies with stale route-map data**
In FRR's northbound framework, `args->dnode` for a destroy callback points to the **old** (pre-deletion) diff node. Therefore `args->dnode->parent` is the list entry as it existed before the change, meaning the route-map leaf is still present as a child. As a result, `yang_dnode_exists(dnode, "route-map")` returns `true` and the old route-map name is read, so `zebra_import_kernel_table_apply(dnode, true)` re-applies the import table *with the route-map still in effect* instead of removing it.
This path is not exercised by the CLI (`no ip import-table` / `no ipv6 import-table` always destroys the entire list entry via `NB_OP_DESTROY` on the entry xpath, triggering `zebra_import_kernel_table_destroy`), but it is reachable via direct YANG/mgmtd operations that delete only the route-map leaf while keeping the list entry.
The callback needs to call `zebra_import_table` explicitly with `rmap = NULL` to apply the updated state without the route-map:
```c
int zebra_import_kernel_table_route_map_destroy(struct nb_cb_destroy_args *args)
{
const struct lyd_node *dnode =
(const struct lyd_node *)args->dnode->parent;
const char *afi_safi = yang_dnode_get_string(dnode, "afi-safi");
uint32_t table_id = yang_dnode_get_uint32(dnode, "table-id");
uint32_t distance = yang_dnode_get_uint32(dnode, "distance");
afi_t afi;
safi_t safi;
if (args->event != NB_EV_APPLY)
return NB_OK;
yang_afi_safi_identity2value(afi_safi, &afi, &safi);
/* Re-apply without the route-map since it is being removed */
if (zebra_import_table(afi, safi, VRF_DEFAULT, table_id, distance,
NULL, true) < 0)
return NB_ERR;
return NB_OK;
}
```
How can I resolve this? If you propose a fix, please make it concise.
|
@choppsv1 -> I need some advice here on what you think is best to do. For the |
I'm going through the changes now, then I'll check out greptile's suggestions again. |
|
I think the simple solution to the multiple calls to |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. @greptileai re-review |
f60bc5b to
242e105
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
242e105 to
88ff01a
Compare
e33c0c2 to
88ff01a
Compare
Route the `zebra dplane limit` CLI through the mgmt-fronted zebra NB path and implement the existing dplane queue limit NB apply callback so the command keeps its behavior after the move. Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Move the `zebra zapi-packets` command to fully use the mgmt frontend. Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Reroute the `zebra work-queue` commands to actually be on the mgmt frontend side instead of the zebra side. Signed-off-by: Donald Sharp <sharpd@nvidia.com>
The `ip import-table ...` commands were not on the mgmt front end side. Move them to it. Signed-off-by: Donald Sharp <sharpd@nvidia.com>
88ff01a to
9c58aa8
Compare
There was a problem hiding this comment.
LGTM, includes a few changes from me as well, in case @donaldsharp you want to review them.
@greptileai re-review
|
@greptile review |
9c58aa8 to
0395396
Compare
|
@greptileai review and please pay attention to the comment I left regarding the YANG range restriction and why it was correctly removed -- that limited range only applied to early versions of linux kernels. Modern linux kernels support a much larger range. Furthermore, that range is totally incorrect for other, non-linux, operating systems that FRR also runs on. |
0395396 to
4b28707
Compare
|
@greptileai review |
|
seems like the greptile didn't care for the extra info, maybe? |
Improve handling of error in frontend client/mgmt vty. Use this to return a more appropriate result (CMD_WARNING_CONFIG_FAILED) back to vtysh mirroring the normal config change path. As a result vtysh will correctly exit with status 1 when running one shot `mgmt edit` config commands. Signed-off-by: Christian Hopps <chopps@labn.net>
Signed-off-by: Christian Hopps <chopps@labn.net>
Better test: a) ip import table b) allow-external.... c) packet read in values for zebra Signed-off-by: Donald Sharp <sharpd@nvidia.com> Signed-off-by: Christian Hopps <chopps@labn.net>
This is a particular type of test we shouldn't mix in other generic config tests. The previous name was too generic as well so update it too. Signed-off-by: Christian Hopps <chopps@labn.net>
4b28707 to
628dccb
Compare
|
@greptile review |
4 participants
See individual commits for more detail:
Move
ip import-table,zebra work-queue,zebra zapi-packets,zebra dplane limitandallow-external-route-updateto their appropriate place in the mgmt frontend.Add tests to show things are working better.