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: apply route-map for aggregate before attribute comparison #17801

Merged
merged 2 commits into from
Jan 9, 2025

Conversation

enkechen-panw
Copy link
Contributor

Currently when re-evaluating an aggregate route, the full attribute of
the aggregate route is not compared with the existing one in the BGP
table. That can result in unnecessary churns (un-install and then
install) of the aggregate route when a more specific route is added or
deleted, or when the route-map for the aggregate changes. The churn
would impact route installation and route advertisement.

The fix is to apply the route-map for the aggregate first and then
compare the attribute.

…was changed"

This reverts commit ee1986f.

The fix is incomplete, and is no longer needed with the fix that applies
the route-map for an aggregate and then compares the attribute.

Signed-off-by: Enke Chen <[email protected]>
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.

How can we test it in topotests?

@ton31337
Copy link
Member

ton31337 commented Jan 8, 2025

Also, could you fix frrbot issues?

Currently when re-evaluating an aggregate route, the full attribute of
the aggregate route is not compared with the existing one in the BGP
table. That can result in unnecessary churns (un-install and then
install) of the aggregate route when a more specific route is added or
deleted, or when the route-map for the aggregate changes. The churn
would impact route installation and route advertisement.

The fix is to apply the route-map for the aggregate first and then
compare the attribute.

Here is an example of the churn:

debug bgp aggregate prefix 5.5.5.0/24
!
route-map set-comm permit 10
 set community 65004:200
!
router bgp 65001
 address-family ipv4 unicast
  redistribute static
  aggregate-address 5.5.5.0/24 route-map set-comm
!

Step 1:
  ip route 5.5.5.1/32 Null0

Jan  8 10:28:49 enke-vm1 bgpd[285786]: [J7PXJ-A7YA2] bgp_aggregate_install: aggregate 5.5.5.0/24, count 1
Jan  8 10:28:49 enke-vm1 bgpd[285786]: [Y444T-HEVNG]   aggregate 5.5.5.0/24: installed

Step 2:
  ip route 5.5.5.2/32 Null0

Jan  8 10:29:03 enke-vm1 bgpd[285786]: [J7PXJ-A7YA2] bgp_aggregate_install: aggregate 5.5.5.0/24, count 2
Jan  8 10:29:03 enke-vm1 bgpd[285786]: [S2EH5-EQSX6]   aggregate 5.5.5.0/24: existing, removed
Jan  8 10:29:03 enke-vm1 bgpd[285786]: [Y444T-HEVNG]   aggregate 5.5.5.0/24: installed
---

Signed-off-by: Enke Chen <[email protected]>
@enkechen-panw
Copy link
Contributor Author

Also, could you fix frrbot issues?

Done.

@enkechen-panw
Copy link
Contributor Author

enkechen-panw commented Jan 9, 2025

@ton31337 [CI] Build is stuck. Thanks for restarting it.

@ton31337
Copy link
Member

ton31337 commented Jan 9, 2025

ci:rerun ci stuck

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.

LGTM

@ton31337 ton31337 merged commit 003869b into FRRouting:master Jan 9, 2025
10 checks passed
@ton31337
Copy link
Member

ton31337 commented Jan 9, 2025

@Mergifyio backport stable/10.2 stable/10.1 stable/10.0

Copy link

mergify bot commented Jan 9, 2025

backport stable/10.2 stable/10.1 stable/10.0

✅ Backports have been created

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.

2 participants