Skip to content
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

bgpd: Ignore routes from evpn if VRF is unknown #16068

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

piotrsuchy
Copy link
Contributor

Fix for a bug, where FRR fails to install route received for an unknown but later-created VRF - detailed description can be found here:

#13708

This is our internal bug fix we introduced that fixed this issue for us shortly after submitting the issue report.

@donaldsharp
Copy link
Member

@ton31337 -> opinions here on needing a new test case in topotests?

@ton31337 ton31337 self-requested a review May 22, 2024 12:16
Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious how it pass this test is_route_matching_for_vrf() in bgp_evpn_route_entry_install_if_vrf_match(). Something weird to me.

@piotrsuchy are you able to reproduce this deterministically?

@riw777
Copy link
Member

riw777 commented May 28, 2024

it seems like this would be a good time to add a topo test here

Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks good, waiting on @ton31337 's question about reproducing this

@riw777 riw777 added the bugfix label Jun 4, 2024
@github-actions github-actions bot added the rebase PR needs rebase label Jun 4, 2024
@piotrsuchy
Copy link
Contributor Author

I'm curious how it pass this test is_route_matching_for_vrf() in bgp_evpn_route_entry_install_if_vrf_match(). Something weird to me.

@piotrsuchy are you able to reproduce this deterministically?

Hello, sorry for the late response. Unfortunately I don't have a minimal deterministic reproduction - back when this issue appeared, we had a test suite on a pretty complex containerlab-like environment with our propriety software and configuration that was able to reproduce this issue, that came down to the reproduction steps mentioned in the linked issue above. The patch in this PR has made it so the issue wasn't reproducible and currently we have this patch running on production. Sadly I don't have a way to mimic this environment easily to reproduce this bug only. In the future when I have more cycles I might get back to this and try to have a minimal reproduction.

@piotrsuchy piotrsuchy requested a review from ton31337 June 17, 2024 14:50
@ton31337
Copy link
Member

Ok, I see I was most likely hooked on bgp_evpn_route_entry_install_if_vrf_match(), but there is install_uninstall_route_in_vrfs() also, that does not have VRF check. Could you at least add a logging to be able to check once we hit it?

@piotrsuchy
Copy link
Contributor Author

Sure! I added the additional log if this condition is hit.
One of the tests in the CI failed, but it's not connected to this change and I can see it is the most common one failing recently, so probably a CI issue. Could you rerun? Thanks!

bgpd/bgp_evpn.c Outdated Show resolved Hide resolved
@ton31337 ton31337 removed the rebase PR needs rebase label Jun 24, 2024
bgpd/bgp_evpn.c Outdated Show resolved Hide resolved
Fix for a bug, where FRR fails to install route received for an unknown but later-created VRF - detailed description can be found here FRRouting#13708

Signed-off-by: Piotr Suchy <[email protected]>
Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, just waiting on other comments/etc.

@ton31337
Copy link
Member

@Mergifyio backport dev/10.1

Copy link

mergify bot commented Jun 25, 2024

backport dev/10.1

✅ Backports have been created

@ton31337 ton31337 merged commit 0727e97 into FRRouting:master Jun 28, 2024
11 checks passed
ton31337 added a commit that referenced this pull request Jun 30, 2024
bgpd: Ignore routes from evpn if VRF is unknown (backport #16068)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants