bgpd: fix batch clearing resume to use correct lookup APIs

[miteshkanjariya]

The batch clearing code was using bgp_node_get() to locate the outer/inner route node when resuming a RIB walk. This caused two problems:

1. Double-walking: bgp_node_get() returns the same node that was already processed in the previous iteration. During GR processing, the second visit to an already-stale path takes the else branch in clearing_clear_one_pi() and deletes the route from the RIB, causing traffic loss.

   Example: with 10 routes walked 3 at a time, routes 1-3 are marked stale in the first batch. On resume, bgp_node_get(table, 3) returns route 3 again. Since BGP_PATH_STALE is already set, the GR check fails and bgp_rib_remove() is called instead.

2. Phantom node creation: bgp_node_get() has find-or-create semantics, so it creates empty route nodes for prefixes that were deleted between yields.

Fix this by using the appropriate lookup APIs for each case:

• For the outer/RD table on resume: use bgp_node_lookup() to check if the last RD still exists. If it does, resume from that RD. If it was deleted, fall back to bgp_table_get_next() to advance to the next RD and clear the inner resume state so the new RD's table is walked from the beginning.

• For inner/normal prefix resume: use bgp_table_get_next() to find the in-order successor of the last processed prefix, ensuring we never revisit it.

[greptile-apps]

Greptile Overview

Greptile Summary

This change updates the batch-clearing “resume” behavior in bgpd/bgp_route.c to avoid using bgp_node_get() (find-or-create) when re-establishing position in a RIB walk. For normal prefixes, resume now uses bgp_table_get_next() to continue from the in-order successor and avoid reprocessing the last-cleared prefix. For VPN safis’ outer RD table, resume now uses bgp_node_lookup() to check whether the saved RD still exists, and falls back to bgp_table_get_next() (with inner resume state cleared) when the RD was deleted.

Net effect: prevents double-walking that could remove stale GR paths, and avoids creating empty “phantom” nodes during resume.

Confidence Score: 3/5

• This PR is likely correct functionally, but should not merge until the resume-path lock handling is verified/fixed.
• The behavioral change (using lookup/next instead of get) matches the stated intent and avoids revisiting/creating nodes. However, bgp_node_lookup() is implemented via route_node_lookup() and can take an additional node lock; the new code path does not appear to release that lock, which can leak references over repeated resume operations. Score reflects one concrete, merge-blocking correctness/resource-management concern in the modified logic.
• bgpd/bgp_route.c (resume path using bgp_node_lookup / route_node locks)

Important Files Changed

Filename	Overview
bgpd/bgp_route.c	Changes resume logic in clearing_dest_helper() to use bgp_node_lookup() / bgp_table_get_next() instead of bgp_node_get(). Fixes double-walk/phantom nodes, but introduces a likely route-node lock leak when using bgp_node_lookup() (lookup returns a locked node that is never unlocked).

Sequence Diagram

sequenceDiagram
    participant S as clear_dests_callback()
    participant H as clear_batch_rib_helper()
    participant D as clearing_dest_helper()
    participant T as bgp_table_* / route_table

    S->>H: resume batch clearing (cinfo)
    H->>D: find starting dest (table,cinfo,inner_p)

    alt RESUME flag set
        D->>T: bgp_table_top(table)
        alt outer/RD table (VPN) && saved RD exists
            D->>T: bgp_node_lookup(table,last_pfx)
            note right of D: resumes at same RD
        else outer/RD table && saved RD deleted
            D->>T: bgp_table_get_next(table,last_pfx)
            D->>D: clear INNER + RESUME flags
            note right of D: start new RD inner table from top
        else normal/inner prefix resume
            D->>T: bgp_table_get_next(table,saved_pfx)
            note right of D: successor avoids revisiting last prefix
        end
    else no resume
        D->>T: bgp_table_top(table)
    end

    D-->>H: dest to continue walk
    H-->>S: returns 0 when complete / nonzero to reschedule

Loading

[greptile-apps]

_{1 file reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[mjstapp]

thanks, this looks good.
I wonder if there's still a latent bug during the table-walking: when we break an iteration, we don't unlock the node we are visiting. I'll look into that path.

[mjstapp]

I've put the 10.6 label on this fix.

[mjstapp]

@Mergifyio backport dev/10.6

[mergify]

backport dev/10.6

✅ Backports have been created

Details

• #20748 bgpd: fix batch clearing resume to use correct lookup APIs (backport #20738) has been created for branch dev/10.6