bgpd: fix import of evpn routes into vni/vrf/esi after nexthop update#10372
bgpd: fix import of evpn routes into vni/vrf/esi after nexthop update#10372louis-6wind wants to merge 2 commits into
Conversation
louis-6wind
force-pushed
the
evpn-nht-update
branch
from
a2354be to
078cfb3
Compare
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-2824/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-2825/ This is a comment from an automated CI system. |
louis-6wind
force-pushed
the
evpn-nht-update
branch
2 times, most recently
from
8a086f6 to
2641ab9
Compare
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedDebian 11 amd64 build: Failed (click for details)Debian 11 amd64 build: No useful log foundSuccessful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsDebian 11 amd64 build: Failed (click for details)Debian 11 amd64 build: No useful log found |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-2842/ This is a comment from an automated CI system. |
|
PR status? |
It is ready and needs a review |
|
@ton31337 needs anything additional? who can review this? we have crit infra affected by this |
donaldsharp
left a comment
donaldsharp
left a comment
There was a problem hiding this comment.
I am not sure I understand the change in evaluate_paths. Can you go into more detail what you expect to work here?
louis-6wind
force-pushed
the
evpn-nht-update
branch
4 times, most recently
from
330d979 to
f3fc37a
Compare
If the igp metric changes, I want to update nexthop on EVPN routes. bgp_evpn_import_route() can also do updates on existing routes, not only import.
|
louis-6wind
force-pushed
the
evpn-nht-update
branch
from
f3fc37a to
91936d6
Compare
|
To reproduce the issue: Apply the configuration PE1 to 3. PE2 PE3 PE3 PE1 PE3 But the routes remain the same. Expected output that we get after the patches |
louis-6wind
changed the title
louis-6wind
force-pushed
the
evpn-nht-update
branch
2 times, most recently
from
3d3a0d1 to
e28687d
Compare
louis-6wind
changed the title
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-3329/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDTest incomplete. See below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: IncompleteAddresssanitizer topotests part 4: Incomplete(check logs for details)Successful on other platforms/tests
|
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 i386 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9U18I386-3330/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 9 Successful on other platforms/tests
|
|
ci:rerun |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 amd64 part 1: Failed (click for details)Topotests Ubuntu 18.04 amd64 part 1: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14403/artifact/TP1U1804AMD64/TopotestDetails/Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TP1U1804AMD64-14403/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 1 Topotests Ubuntu 18.04 i386 part 8: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO8U18I386-14403/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 8 Topotests Ubuntu 18.04 arm8 part 9: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 9: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14403/artifact/TOPO9U18ARM8/TopotestDetails/ Topotests Ubuntu 18.04 arm8 part 9: No useful log foundSuccessful on other platforms/tests
|
|
ci:rerun |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
louis-6wind
force-pushed
the
evpn-nht-update
branch
from
b7b5f90 to
7d75a34
Compare
|
@Mergifyio rebase |
✅ Branch has been successfully rebased |
ton31337
force-pushed
the
evpn-nht-update
branch
from
7d75a34 to
fb0da4e
Compare
|
@ton31337 it was rebased yesterday |
|
Could you fix the styling also? |
louis-6wind
force-pushed
the
evpn-nht-update
branch
from
fb0da4e to
51488f3
Compare
|
done |
|
@greptile review |
Greptile SummaryThis PR fixes EVPN type-5 routes not being updated in VRFs after a BGP nexthop IGP metric change (e.g., underlay metric update). The root cause was a two-part gap:
Confidence Score: 4/5The core logic change is small and targeted; the main risk is the new NHT else-if branch triggering redundant unimports when a nexthop is already invalid. Both changed code paths are narrow and well-contained. The bgp_evpn.c change simply widens the condition under which the early-return is bypassed, and the bgp_nht.c addition mirrors the existing validity-change handling pattern. The test suite is extended with concrete ECMP and metric selection assertions. Minor cosmetic issues (misleading comment, missing exit-address-family in test configs) do not affect correctness. bgpd/bgp_nht.c — the new else-if branch and its comment; r2/bgpd.conf and r3/bgpd.conf — missing exit-address-family in the ipv4 unicast block. Important Files Changed
Sequence DiagramsequenceDiagram
participant Zebra
participant NHT as bgp_nht.c (evaluate_paths)
participant EVPN as bgp_evpn.c (import_route)
participant VRF as install_evpn_route_entry_in_vrf
Zebra->>NHT: Nexthop update (metric changed)
NHT->>NHT: SET BGP_PATH_IGP_CHANGED on path
NHT->>NHT: "old_path_valid == bnc_is_valid_nexthop?"
Note over NHT: YES - new else-if branch fires
NHT->>EVPN: bgp_evpn_import_route(path)
EVPN->>VRF: install_evpn_route_entry_in_vrf(parent_pi)
VRF->>VRF: CHECK_FLAG(parent_pi, BGP_PATH_IGP_CHANGED)?
Note over VRF: YES - skip attrhash_cmp early-return
VRF->>VRF: Update attr, trigger bgp_process
NHT->>NHT: bgp_process(dest) [BGP_NEXTHOP_METRIC_CHANGED]
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
bgpd/bgp_nht.c:1505-1507
**Inaccurate comment in the new else-if branch**
The comment says "import the EVPN routes if the path validity changed or update existing routes," but this `else if` is only reached when `old_path_valid == bnc_is_valid_nexthop` — i.e., validity did **not** change. The "path validity changed" part is misleading; the branch solely handles the metric-update case where the nexthop validity is unchanged.
### Issue 2 of 3
tests/topotests/bgp_path_selection/test_bgp_path_selection.py:168-169
**Potentially confusing "r5" in test function names**
`test_bgp_path_selection_evpn_r5_ecmp` and `test_bgp_path_selection_evpn_r5_metric` use the suffix `r5`, which in this topology context (routers r1/r2/r3) looks like a reference to a non-existent router. The intent is EVPN route type 5 (IP prefix). Consider renaming to `_evpn_type5_ecmp` / `_evpn_type5_metric` to avoid confusion with router names.
### Issue 3 of 3
tests/topotests/bgp_path_selection/r2/bgpd.conf:31-34
**Missing `exit-address-family` for `address-family ipv4 unicast`**
The `address-family ipv4 unicast` block in the new `router bgp 65002 vrf vrf2` stanza has no closing `exit-address-family` before the next `address-family l2vpn evpn` entry. The same pattern applies in `r3/bgpd.conf`. While FRR's parser may handle an implicit exit, this is inconsistent with the rest of the config file and could cause unexpected parsing behaviour across FRR versions.
Reviews (1): Last reviewed commit: "tests: add an evpn RT5 path selection to..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (bnc_is_valid_nexthop) | ||
| /* import the EVPN routes if the path validity | ||
| * changed or update existing routes. |
There was a problem hiding this comment.
The comment says "import the EVPN routes if the path validity changed or update existing routes," but this else if is only reached when old_path_valid == bnc_is_valid_nexthop — i.e., validity did not change. The "path validity changed" part is misleading; the branch solely handles the metric-update case where the nexthop validity is unchanged.
Prompt To Fix With AI
This is a comment left during a code review.
Path: bgpd/bgp_nht.c
Line: 1505-1507
Comment:
**Inaccurate comment in the new else-if branch**
The comment says "import the EVPN routes if the path validity changed or update existing routes," but this `else if` is only reached when `old_path_valid == bnc_is_valid_nexthop` — i.e., validity did **not** change. The "path validity changed" part is misleading; the branch solely handles the metric-update case where the nexthop validity is unchanged.
How can I resolve this? If you propose a fix, please make it concise.| assert result is None, "Failed to see BGP prefixes on R1" | ||
|
|
There was a problem hiding this comment.
test_bgp_path_selection_evpn_r5_ecmp and test_bgp_path_selection_evpn_r5_metric use the suffix r5, which in this topology context (routers r1/r2/r3) looks like a reference to a non-existent router. The intent is EVPN route type 5 (IP prefix). Consider renaming to _evpn_type5_ecmp / _evpn_type5_metric to avoid confusion with router names.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/topotests/bgp_path_selection/test_bgp_path_selection.py
Line: 168-169
Comment:
**Potentially confusing "r5" in test function names**
`test_bgp_path_selection_evpn_r5_ecmp` and `test_bgp_path_selection_evpn_r5_metric` use the suffix `r5`, which in this topology context (routers r1/r2/r3) looks like a reference to a non-existent router. The intent is EVPN route type 5 (IP prefix). Consider renaming to `_evpn_type5_ecmp` / `_evpn_type5_metric` to avoid confusion with router names.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| bgp router-id 192.0.2.2 | ||
| no bgp ebgp-requires-policy | ||
| address-family ipv4 unicast | ||
| redistribute connected |
There was a problem hiding this comment.
exit-address-family for address-family ipv4 unicast
The address-family ipv4 unicast block in the new router bgp 65002 vrf vrf2 stanza has no closing exit-address-family before the next address-family l2vpn evpn entry. The same pattern applies in r3/bgpd.conf. While FRR's parser may handle an implicit exit, this is inconsistent with the rest of the config file and could cause unexpected parsing behaviour across FRR versions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/topotests/bgp_path_selection/r2/bgpd.conf
Line: 31-34
Comment:
**Missing `exit-address-family` for `address-family ipv4 unicast`**
The `address-family ipv4 unicast` block in the new `router bgp 65002 vrf vrf2` stanza has no closing `exit-address-family` before the next `address-family l2vpn evpn` entry. The same pattern applies in `r3/bgpd.conf`. While FRR's parser may handle an implicit exit, this is inconsistent with the rest of the config file and could cause unexpected parsing behaviour across FRR versions.
How can I resolve this? If you propose a fix, please make it concise.
|
Deprecation notice: This pull request comes from a fork and was rebased using |
✅ Branch has been successfully rebased |
louis-6wind
force-pushed
the
evpn-nht-update
branch
from
d3e98cb to
fda6532
Compare
|
ci:rerun |
Related to issue FRRouting#10271. When importing prefixes from EVPN, any changes to the BGP nexthop associated with those prefixes, such as a change in the route metric to the peer, will not trigger a re-evaluation of the imported prefixes. In evaluate_path(), the import is only triggered when the validity of the path changes, which is not the case when, for example, the route metric to the peer changes. Request import or unimport when BGP_PATH_IGP_CHANGED is set but the path validity is unchanged. In install_evpn_route_entry_in_vrf(), do not skip VRF import when BGP_PATH_IGP_CHANGED is set. Fixes: 34ea39b ("bgpd: Check NHT change for triggering EVPN import or unimport") Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Add an evpn RT5 path selection to bgp_path_selection Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
louis-6wind
force-pushed
the
evpn-nht-update
branch
from
fda6532 to
0635e1b
Compare
|
ci:rerun |
1 similar comment
|
ci:rerun |
9 participants
Related to issue #10271
After an update of the BGP nexthop (e.g. update of the igp metric), the
EVPN routes are not updated in VNIs, VRFs and ESIs.
Fix the issue.
Signed-off-by: Louis Scalbert louis.scalbert@6wind.com