zebra: clean up VRF handling by using dataplane provided vrf_id

[maxime-leroy]

Zebra was implicitly assuming that the VRF netdevice ifindex could be used as the VRF identifier, leading to casts from ifindex to vrf_id_t. While this works with the Linux kernel dataplane, it does not respect the dataplane API, which exposes these identifiers as distinct concepts.

The code is updated to use the VRF identifier explicitly provided by the dataplane context and removes the need for casting ifindex to vrf_id_t. In addition, the VRF delete path is clarified by no longer reading unused dataplane fields, avoiding confusion about which identifiers are actually required.

There is no behavior change for the Linux kernel dataplane.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[maxime-leroy]

ci:rerun

[maxime-leroy]

ci:rerun

[vjardin]

thanks, LGTM

[vjardin]

Let's wait for the end of the CI just in case. I'd prefer someone external to ack't+merge it since I am a Maxime's colleague.

[mjstapp]

there's some confusion here - but I'm not sure that this PR isn't making things harder to understand.
I agree that it's confusing to repeatedly cast a value that appears as an ifindex - that's not a great pattern. but I can imagine a very small couple of lines that would correct that in interface.c - I don't see that reorganizing the whole vrf device change flow is helping.

[maxime-leroy]

there's some confusion here - but I'm not sure that this PR isn't making things harder to understand. I agree that it's confusing to repeatedly cast a value that appears as an ifindex - that's not a great pattern. but I can imagine a very small couple of lines that would correct that in interface.c - I don't see that reorganizing the whole vrf device change flow is helping.

There are two separate commits in this PR.

The second commit is the actual fix: interface_vrf_change() was implicitly assuming that the VRF netdevice ifindex could be used as zebra’s vrf_id (via casting).
This holds true for the Linux kernel dataplane (VRF ID == VRF ifindex), so there is no behavior change there, but it is incorrect for non-kernel dataplanes such as Grout (DPDK) where vrf_id != ifindex.
The code now uses the dataplane-provided vrf_id (dplane_ctx_get_ifp_vrf_id()), which fixes VRF handling for Grout.

The first commit is only a cleanup: in the netlink backend ifp_table_id is set for RTM_NEWLINK VRF events but not for RTM_DELLINK,
and VRF deletion does not require table_id anyway. Splitting VRF handling into interface_vrf_update() / interface_vrf_del() avoids relying on an unset dplane field and makes the logic clearer.

If needed, the first commit can be ditched. However, in my view it is a nice-to-have cleanup that clarifies the delete vs update requirements.

[mjstapp]

yes, I understood the two commits, I think.
my point was: your choice to "clean up" what is the common handler function had a significant impact. you could just have ensured that the vrf_id value was conveyed. but instead, you removed the handler, replaced it with two new functions, encountered the lttng traces that are involved. a two-line change turned into 100 lines. that has an impact on the reviewers as well - we have to try to confirm that your changes retain the important logic that you propose moving into separate functions, we have to sort out the traceing api issues.
so: I think you should not try to make that "clean up" change, and just get the vrf_id straight.

If needed, the first commit can be ditched. However, in my view it is a nice-to-have cleanup that clarifies the delete vs update requirements.

[maxime-leroy]

yes, I understood the two commits, I think. my point was: your choice to "clean up" what is the common handler function had a significant impact. you could just have ensured that the vrf_id value was conveyed. but instead, you removed the handler, replaced it with two new functions, encountered the lttng traces that are involved. a two-line change turned into 100 lines. that has an impact on the reviewers as well - we have to try to confirm that your changes retain the important logic that you propose moving into separate functions, we have to sort out the traceing api issues. so: I think you should not try to make that "clean up" change, and just get the vrf_id straight.

Thanks for the detailed feedback — I understand the concern about review surface / churn.

That said, I’d like to clarify the intent and why I still think the split is justified:

• The commits are intentionally split:

  The first commit is cleanup-only, and the second commit contains the actual fix (using the dplane-provided vrf_id).
  This is deliberate to separate “mechanical refactor” from “functional change” and make the review easier.

• No functional change is introduced by the first commit:

  This is explicitly stated in the commit log and PR description, and I also mentioned during the weekly meeting that the first commit was only a split/cleanup with no functional change. The Linux kernel dataplane behavior is unchanged.

• The cleanup is not only cosmetic:

  In the netlink backend, ifp_table_id is set for RTM_NEWLINK VRF events but not for RTM_DELLINK. The delete path does not require table_id, and reading/tracing it on delete can produce misleading trace data (e.g., a delete event always carrying table_id == 0). Splitting the flow makes the required inputs explicit: update uses table_id, delete does not. This avoids consuming an unset dataplane field and emitting invalid trace payloads.

• About the LTTng issue:
  While working on this PR, I introduced a compilation issue related to the new tracepoints in zebra/interface.c during a rebase. Sorry about that — I was not familiar with LTTng tracing at the time. I am working on fixing this in the current series.

On the positive side, this led to adding CI coverage for --enable-lttng (thanks to @vjardin), which also uncovered an existing LTTng compilation issue already present on master. I fixed that master issue as well.

So I agree the diff is larger than the minimal vrf_id accessor change, but it is not arbitrary churn: it makes the delete/update requirements explicit and avoids relying on/tracing unset dataplane fields. The actual behavior change remains isolated in the second commit.

[vjardin]

@mjstapp: Fair point on the size, but the split makes the contracts explicit (delete doesn't need table_id/ns_id, update does). With commit 1 being pure refactor and commit 2 the isolated fix, I think it lands in a better place for future maintenance.

[maxime-leroy]

@mjstapp I added the extra {} blocks in interface_vrf_update() / interface_vrf_del()
in the first commit to preserve the original indentation and minimize the diff,
so it should be easier to review.

I’ve added a follow-up commit that removes these redundant blocks and runs
clang-format.

Hope this makes the review easier on your side.

[maxime-leroy]

ci:rerun

[vjardin]

@donaldsharp and @mjstapp : I had direct discussion with @maxime-leroy about it. I do not see additional issues with this pull request. Can you argue if there is a strong concern ? if not, let's merge, it is a hurt-less clean up that helps to have a generic support.

[mjstapp]

I think I've been pretty clear about the issue. A useful 10-line change has turned into a 100-line change ... and that just seems like something that's unnecessary.

@donaldsharp and @mjstapp : I had direct discussion with @maxime-leroy about it. I do not see additional issues with this pull request. Can you argue if there is a strong concern ? if not, let's merge, it is a hurt-less clean up that helps to have a generic support.

[maxime-leroy]

I think I've been pretty clear about the issue. A useful 10-line change has turned into a 100-line change ... and that just seems like something that's unnecessary.

@donaldsharp and @mjstapp : I had direct discussion with @maxime-leroy about it. I do not see additional issues with this pull request. Can you argue if there is a strong concern ? if not, let's merge, it is a hurt-less clean up that helps to have a generic support.

@mjstapp , I hear you about keeping this small. While working on this PR I noticed two related issues in the VRF delete path:

• table-id is not set on delete, yet we still read it via dplane_ctx_get_ifp_table_id(ctx)
• the delete trace always reports table-id = 0, which is misleading

The function split was my way to avoid using/printing a meaningless value there.
You’re right that if the PR only focused on using the correct API to get vrf_id, it would be ~10 lines.
When I notice additional issues in the code I’m touching, I usually try to fix them as well.

If you prefer, I can instead keep the change minimal and simply stop tracing table-id on delete.
Note that with this minimal change, table-id would still be retrieved by the caller (via dplane_ctx_get_ifp_table_id(ctx)) and passed to interface_vrf_change() even on delete, even though it has no meaning there.

[maxime-leroy]

I've reworked the patch as requested. The diff is now +30/-38 lines compared to the previous +97/-75, which is roughly a 60% reduction in diff size.

I still think that splitting interface_vrf_change() into two functions could result in cleaner code, but I understand that this may not be the preferred approach for this particular fix. For reference, I’ve pushed an alternative version here:
https://github.com/maxime-leroy/frr/commits/zebra_vrf_id_cleanup_split_version/

Thanks for the review and feedback.

[maxime-leroy]

@greptileai

[greptile-apps]

Greptile Overview

Greptile Summary

This PR refactors VRF handling in zebra to properly respect the dataplane abstraction by using the VRF identifier explicitly provided by dplane_ctx_get_ifp_vrf_id() instead of casting ifindex to vrf_id_t.

Key changes:

• interface_vrf_change() signature updated to accept a VRF pointer (for DELETE) or vrf_id parameter (for UPDATE), eliminating the implicit assumption that VRF ifindex equals vrf_id
• For DELETE operations: VRF pointer is captured before if_delete_update() and passed directly, avoiding redundant vrf_lookup_by_id() call
• For UPDATE operations: Uses dataplane-provided vrf_id from dplane_ctx_get_ifp_vrf_id(ctx) instead of casting ifindex
• Tracepoint updated to use vrf_id_t instead of ifindex_t for semantic correctness

Impact:

• No behavior change for Linux kernel dataplane (where vrf_id == VRF ifindex)
• Enables support for non-kernel dataplanes like Grout (DPDK) where VRF ID ≠ VRF interface ifindex
• Improves code clarity by making VRF ID handling explicit rather than implicit

Confidence Score: 5/5

• This PR is safe to merge with no risks identified
• The refactoring is well-structured across three logical commits, maintains backward compatibility with the Linux kernel dataplane while enabling support for alternative dataplanes. The changes are purely semantic - replacing implicit ifindex-to-vrf_id casts with explicit dataplane-provided values. All call sites are properly updated, and the VRF pointer handling in the delete path is protected by a null check at the call site.
• No files require special attention

Important Files Changed

Filename	Overview
zebra/interface.c	Refactored VRF handling to use dataplane-provided vrf_id and pass VRF pointer in delete path to eliminate redundant lookup
zebra/zebra_trace.h	Updated tracepoint to use vrf_id instead of ifindex, aligning with semantic changes in interface_vrf_change

Sequence Diagram

sequenceDiagram
    participant DP as Dataplane
    participant ZIDH as zebra_if_dplane_ifp_handling
    participant IVC as interface_vrf_change
    participant VRF as VRF subsystem

    alt VRF Interface Delete
        DP->>ZIDH: DPLANE_OP_INTF_DELETE
        Note over ZIDH: zif_type == ZEBRA_IF_VRF
        ZIDH->>ZIDH: vrf = ifp->vrf (capture before delete)
        ZIDH->>ZIDH: if_delete_update(&ifp)
        ZIDH->>IVC: interface_vrf_change(op, vrf, 0, NULL, 0, ns_id)
        Note over IVC: Use vrf->vrf_id, vrf->name, vrf->data.l.table_id
        IVC->>VRF: vrf_delete(vrf)
    else VRF Interface Update
        DP->>ZIDH: DPLANE_OP_INTF_UPDATE
        Note over ZIDH: zif_type == ZEBRA_IF_VRF
        ZIDH->>ZIDH: vrf_id = dplane_ctx_get_ifp_vrf_id(ctx)
        ZIDH->>ZIDH: tableid = dplane_ctx_get_ifp_table_id(ctx)
        ZIDH->>IVC: interface_vrf_change(op, NULL, vrf_id, name, tableid, ns_id)
        Note over IVC: Use vrf_id from dataplane (no ifindex cast)
        IVC->>VRF: vrf_update(vrf_id, name)
        IVC->>VRF: vrf_enable(vrf)
    end

Loading

[maxime-leroy]

ci:rerun

[maxime-leroy]

Hi @mjstapp, gentle ping whenever you have a moment. The patch has been reduced to the minimal version you asked for (+30/-38) since January 27 and is sitting ready.

FWIW, greptile (configured by @mwinter-osr on an unrelated FRR PR) also recommends splitting this kind of function: opensourcerouting#234 (comment)

Thanks!

[mjstapp]

Thanks, looks good to me now

[vjardin]

Ok