-
Notifications
You must be signed in to change notification settings - Fork 1.3k
BGP evpn testing and bug fixes related to non default EVPN backbone #18358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
a0d102f
topotests: add bgp_evpn_rt5_vrf_underlay with backbone in non default…
pguibert6WIND ef00a51
topotests: fix pylint code style in test_bgp_evpn.py file
pguibert6WIND 39f6549
zebra: fix heap-after-free on shutdown in netns mode
louis-6wind 4656368
bgpd: do not set default bgp instance at bgp evpn at creation
pguibert6WIND c27a4aa
bgpd: fix remove too verbose error message
pguibert6WIND 19707c8
topotests: add bgp route importation from ce to l3vpn evpn network
pguibert6WIND fb4bae1
bgpd: fix allow to reconfigure advertise-all-vni on non default vrf
pguibert6WIND f58a727
topotests: bgp_evpn_rt5, add check on reconfigure evpn instance
pguibert6WIND File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| interface loop141 vrf vrf-141 | ||
| ip address 192.168.116.61/32 | ||
| ipv6 address fd00::116/128 | ||
| ! | ||
| interface loop142 vrf vrf-142 | ||
| ip address 192.168.126.61/32 | ||
| ipv6 address fd00::126/128 | ||
| ! | ||
| interface eth-r2.141 vrf vrf-141 | ||
| ip address 192.168.106.61/24 | ||
| ! | ||
| interface eth-r2.142 vrf vrf-142 | ||
| ip address 192.168.105.61/24 | ||
| ! | ||
| router bgp 65001 vrf vrf-142 | ||
| bgp router-id 192.168.105.61 | ||
| bgp log-neighbor-changes | ||
| no bgp ebgp-requires-policy | ||
| neighbor 192.168.105.41 remote-as 65000 | ||
| neighbor 192.168.105.41 capability extended-nexthop | ||
| address-family ipv4 unicast | ||
| network 192.168.126.61/32 | ||
| exit-address-family | ||
| address-family ipv6 unicast | ||
| neighbor 192.168.105.41 activate | ||
| network fd00::126/128 | ||
| exit-address-family | ||
| ! | ||
| router bgp 65001 vrf vrf-141 | ||
| bgp router-id 192.168.106.61 | ||
| bgp log-neighbor-changes | ||
| no bgp ebgp-requires-policy | ||
| neighbor 192.168.106.41 remote-as 65000 | ||
| neighbor 192.168.106.41 capability extended-nexthop | ||
| address-family ipv4 unicast | ||
| network 192.168.116.61/32 | ||
| exit-address-family | ||
| address-family ipv6 unicast | ||
| neighbor 192.168.106.41 activate | ||
| network fd00::116/128 | ||
| exit-address-family | ||
| ! |
127 changes: 127 additions & 0 deletions
127
tests/topotests/bgp_evpn_rt5/r1/bgp_l2vpn_evpn_routes_source_vrf_all.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,127 @@ | ||
| { | ||
| "65000:2":{ | ||
| "rd":"65000:2", | ||
| "[5]:[0]:[32]:[192.168.116.61]":{ | ||
| "prefix":"[5]:[0]:[32]:[192.168.116.61]", | ||
| "prefixLen":352, | ||
| "paths":[ | ||
| { | ||
| "valid":true, | ||
| "bestpath":true, | ||
| "selectionReason":"First path received", | ||
| "pathFrom":"internal", | ||
| "routeType":5, | ||
| "ethTag":0, | ||
| "ipLen":32, | ||
| "ip":"192.168.116.61", | ||
| "metric":0, | ||
| "locPrf":100, | ||
| "weight":0, | ||
| "peerId":"192.168.1.101", | ||
| "path":"65001", | ||
| "origin":"IGP", | ||
| "nexthops":[ | ||
| { | ||
| "ip":"192.168.2.2", | ||
| "hostname":"rr", | ||
| "afi":"ipv4", | ||
| "used":true | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| }, | ||
| "[5]:[0]:[32]:[192.168.126.61]":{ | ||
| "prefix":"[5]:[0]:[32]:[192.168.126.61]", | ||
| "prefixLen":352, | ||
| "paths":[ | ||
| { | ||
| "valid":true, | ||
| "bestpath":true, | ||
| "selectionReason":"First path received", | ||
| "pathFrom":"internal", | ||
| "routeType":5, | ||
| "ethTag":0, | ||
| "ipLen":32, | ||
| "ip":"192.168.126.61", | ||
| "metric":0, | ||
| "locPrf":100, | ||
| "weight":0, | ||
| "peerId":"192.168.1.101", | ||
| "path":"65001", | ||
| "origin":"IGP", | ||
| "nexthops":[ | ||
| { | ||
| "ip":"192.168.2.2", | ||
| "hostname":"rr", | ||
| "afi":"ipv4", | ||
| "used":true | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| }, | ||
| "[5]:[0]:[128]:[fd00::116]":{ | ||
| "prefix":"[5]:[0]:[128]:[fd00::116]", | ||
| "prefixLen":352, | ||
| "paths":[ | ||
| { | ||
| "valid":true, | ||
| "bestpath":true, | ||
| "selectionReason":"First path received", | ||
| "pathFrom":"internal", | ||
| "routeType":5, | ||
| "ethTag":0, | ||
| "ipLen":128, | ||
| "ip":"fd00::116", | ||
| "metric":0, | ||
| "locPrf":100, | ||
| "weight":0, | ||
| "peerId":"192.168.1.101", | ||
| "path":"65001", | ||
| "origin":"IGP", | ||
| "nexthops":[ | ||
| { | ||
| "ip":"192.168.2.2", | ||
| "hostname":"rr", | ||
| "afi":"ipv4", | ||
| "used":true | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| }, | ||
| "[5]:[0]:[128]:[fd00::126]":{ | ||
| "prefix":"[5]:[0]:[128]:[fd00::126]", | ||
| "prefixLen":352, | ||
| "paths":[ | ||
| { | ||
| "valid":true, | ||
| "bestpath":true, | ||
| "selectionReason":"First path received", | ||
| "pathFrom":"internal", | ||
| "routeType":5, | ||
| "ethTag":0, | ||
| "ipLen":128, | ||
| "ip":"fd00::126", | ||
| "metric":0, | ||
| "locPrf":100, | ||
| "weight":0, | ||
| "peerId":"192.168.1.101", | ||
| "path":"65001", | ||
| "origin":"IGP", | ||
| "nexthops":[ | ||
| { | ||
| "ip":"192.168.2.2", | ||
| "hostname":"rr", | ||
| "afi":"ipv4", | ||
| "used":true | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| }, | ||
| "numPrefix":14, | ||
| "totalPrefix":14 | ||
| } |
67 changes: 67 additions & 0 deletions
67
tests/topotests/bgp_evpn_rt5/r1/bgp_l2vpn_evpn_routes_source_vrf_filtering.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| { | ||
| "65000:2":{ | ||
| "rd":"65000:2", | ||
| "[5]:[0]:[32]:[192.168.116.61]":{ | ||
| "prefix":"[5]:[0]:[32]:[192.168.116.61]", | ||
| "prefixLen":352, | ||
| "paths":[ | ||
| { | ||
| "valid":true, | ||
| "bestpath":true, | ||
| "selectionReason":"First path received", | ||
| "pathFrom":"internal", | ||
| "routeType":5, | ||
| "ethTag":0, | ||
| "ipLen":32, | ||
| "ip":"192.168.116.61", | ||
| "metric":0, | ||
| "locPrf":100, | ||
| "weight":0, | ||
| "peerId":"192.168.1.101", | ||
| "path":"65001", | ||
| "origin":"IGP", | ||
| "nexthops":[ | ||
| { | ||
| "ip":"192.168.2.2", | ||
| "hostname":"rr", | ||
| "afi":"ipv4", | ||
| "used":true | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| }, | ||
| "[5]:[0]:[128]:[fd00::126]":{ | ||
| "prefix":"[5]:[0]:[128]:[fd00::126]", | ||
| "prefixLen":352, | ||
| "paths":[ | ||
| { | ||
| "valid":true, | ||
| "bestpath":true, | ||
| "selectionReason":"First path received", | ||
| "pathFrom":"internal", | ||
| "routeType":5, | ||
| "ethTag":0, | ||
| "ipLen":128, | ||
| "ip":"fd00::126", | ||
| "metric":0, | ||
| "locPrf":100, | ||
| "weight":0, | ||
| "peerId":"192.168.1.101", | ||
| "path":"65001", | ||
| "origin":"IGP", | ||
| "nexthops":[ | ||
| { | ||
| "ip":"192.168.2.2", | ||
| "hostname":"rr", | ||
| "afi":"ipv4", | ||
| "used":true | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| }, | ||
| "numPrefix":10, | ||
| "totalPrefix":10 | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like a significant change: why is it necessary? if the explicit config in some other instance works, then this is benign. and if the config change doesn't work ... then that's would be something to fix.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjstapp , without this fix, the order of BGP instance configs matters. And this is an issue of having this lock.
About removing that, please find below the discussion I had with @Tuetuopay:
Ca va trigger pour tout le monde un souci que j'ai eu en prod et que j'ai commencé à fix.
Un RR EVPN n'est pas vtep (donc bm->bgp_evpn n'a pas de raison d'être set).
Sauf que dans le cas d'un rr, frr tente quand même d'import les routes, ne trouve pas de vrf evpn, et log en boucle
bgpd[781] [J51AF-GQ2HJ][EC 33554468] vrf import rt lookup - evpn instance not created yet
actuellement ça ne se produit que si
ce patch (ne pas mettre default dans bm->bgp_evpn) est à combiner avec ce fix de logging. (sur ma prod j'ai ce message des centaines de fois par seconde)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so ... that exchange isn't very helpful to me, since it's in French?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for translation, the explanation is the following one:
"
It was a RR EVPN setup made for production. Initially, there were a lot of log messages flooded.
(the ones I propose to remove). Those messages were displayed because there was no backbone defined, because 'advertise-all-vni' command was not used.
This control was a workaround to avoid having log messages.
So a default evpn backbone has been defined.
Those log message are displayed on two cases:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me chime in (in English, this time).
The issue at hand is setting
bm->bgp_evpnakabgp_master.bgp_evpnblindly when creating a non-vrf BGP instance, instead of setting it only when FRR is a VTEP (i.e.advertise-all-vnienabled). It makes the whole behavior inconsistent between having a default instance and not having one.Say, for example, we make a route-reflector out of FRR using only the non-default VRF. Such a setup does not need
advertise-all-vni, and it would be detrimental as it would increase the processing done for every route passing through FRR. As a matter of fact, there are a lot of stuff that FRR tries to do whenbm->bgp_evpnis not NULL, which will never succeed anyways ifadvertise-all-vniis not enabled.The issue I describe in the French extract above is about log messages that Philippe also disables. The setup described above (route-reflector in a non-default vrf, with no default vrf instance) will log the
bgpd[781] [J51AF-GQ2HJ][EC 33554468] vrf import rt lookup - evpn instance not created yetmessage for every single route received by FRR. In production, I have EVPN route-reflectors that received over 250 EVPN routes/s, which translates to this log message flooding everywhere.The proper behavior overall is to set this field when enabling the
advertise-all-vniknob, and only then. This will save work overall because my setup shows that quite a bit of the EVPN code is guarded withif (bm->bgp_evpn), and that having it set to NULL is not detrimental if the speaker is not a VTEP. As a testament to the stability of such a change: overall I have more than 50 RRs in production with this setup, and they've been rock-solid.