-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: compare aigp after local route check in bgp_path_info_cmp() #17199
Conversation
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.
LGTM, but IMO we should have a topotest verifying this, agree?
@Mergifyio backport dev/10.2 stable/10.1 stable/10.0 stable/9.1 stable/9.0 |
✅ Backports have been created
|
Sure, will do. Thanks. |
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.
LGTM, but frrbot styling should be fixed before merging.
The topotest verifies that a local route is favored irrespective of its AIGP value. Signed-off-by: Enke Chen <enchen@paloaltonetworks.com>
For consistency between RIB and BGP, the aigp comparison should be made after the local route check in bgp bestpath selection. Signed-off-by: Enke Chen <enchen@paloaltonetworks.com>
f736878
to
6a7049a
Compare
The failed topotest is just not reliable, and is not related to the patch. Here is the failure rate from the weekly report:
I ran it locally with and without the patch, and the results were also inconsistent. |
bgpd: compare aigp after local route check in bgp_path_info_cmp() (backport #17199)
For consistency between RIB and BGP, the aigp comparison should be made after the local route check in bgp bestpath selection.
As a reference, that's exactly what Cisco IOS does:
https://www.cisco.com/c/en/us/support/docs/ip/border-gateway-protocol-bgp/119001-configure-aigp-00.html