From: Donatas Abraitis Date: Wed, 18 Dec 2024 12:25:58 +0000 (+0200) Subject: bgpd: Withdraw routes without waiting for the coalescing timer to expire X-Git-Tag: base_10.3~104^2 X-Git-Url: https://git.puffer.fish/?a=commitdiff_plain;h=refs%2Fpull%2F17667%2Fhead;p=mirror%2Ffrr.git bgpd: Withdraw routes without waiting for the coalescing timer to expire TL;DR; BGP keeps advertising prefixes which were already removed via `network` command. This was happening randomly, but with this patch, no issues happened. Before: ``` frr# sh ip bgp neighbors 10.0.5.1 advertised-routes BGP table version is 586, local router ID is 44.44.33.43, vrf id 0 Default local pref 100, local AS 2 Status codes: s suppressed, d damped, h history, * valid, > best, = multipath, i internal, r RIB-failure, S Stale, R Removed Nexthop codes: @NNN nexthop's vrf id, < announce-nh-self Origin codes: i - IGP, e - EGP, ? - incomplete RPKI validation codes: V valid, I invalid, N Not found Network Next Hop Metric LocPrf Weight Path *> 15.15.15.0/24 0.0.0.0 0 32768 2 i ============> stale entry *> 15.15.16.0/24 0.0.0.0 0 32768 2 i *> 15.15.17.0/24 0.0.0.0 0 32768 2 i frr# sh ip bgp 15.15.15.0/24 % Network not in table ``` Logs: ``` root@b03d1542-0215-5ecf-013d-af2a7150a4f5:~# cat /log/syslog/frr.log | grep 15.15.15.0 2024/12/08 07:56:58 BGP: [K423X-ETGCQ] group_announce_route_walkcb: afi=IPv4, safi=unicast, p=15.15.15.0/24 2024/12/08 07:56:58 BGP: [T5JFA-13199] subgroup_process_announce_selected: p=15.15.15.0/24, selected=0xde4060 2024/12/08 07:56:58 BGP: [K423X-ETGCQ] group_announce_route_walkcb: afi=IPv4, safi=unicast, p=15.15.15.0/24 2024/12/08 07:56:58 BGP: [T5JFA-13199] subgroup_process_announce_selected: p=15.15.15.0/24, selected=0xde4060 2024/12/08 07:56:58 BGP: [HVRWP-5R9NQ] u78:s77 send UPDATE 15.15.15.0/24 IPv4 unicast ``` Or imagine the situation where withdrawals are postponed for 20-30s., blackholing happens. Signed-off-by: Donatas Abraitis --- diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c index a1bf9a4c61..8f816eb30d 100644 --- a/bgpd/bgp_updgrp_adv.c +++ b/bgpd/bgp_updgrp_adv.c @@ -228,64 +228,67 @@ static int group_announce_route_walkcb(struct update_group *updgrp, void *arg) afi2str(afi), safi2str(safi), ctx->dest); UPDGRP_FOREACH_SUBGRP (updgrp, subgrp) { - /* withdraw stale addpath without waiting for the coalesce timer timeout. - * Otherwise, since adj->addpath_tx_id is overwritten, the code never - * notice anymore it has to do a withdrawal. - */ - if (addpath_capable) - subgrp_withdraw_stale_addpath(ctx, subgrp); - /* - * Skip the subgroups that have coalesce timer running. We will - * walk the entire prefix table for those subgroups when the - * coalesce timer fires. - */ - if (!subgrp->t_coalesce) { - - /* An update-group that uses addpath */ - if (addpath_capable) { - subgrp_announce_addpath_best_selected(ctx->dest, - subgrp); + /* An update-group that uses addpath */ + if (addpath_capable) { + /* Send withdrawals without waiting for coalesting timer + * to expire. + */ + if (subgrp->t_coalesce) { + subgrp_withdraw_stale_addpath(ctx, subgrp); - /* Process the bestpath last so the "show [ip] - * bgp neighbor x.x.x.x advertised" - * output shows the attributes from the bestpath - */ - if (ctx->pi) - subgroup_process_announce_selected( - subgrp, ctx->pi, ctx->dest, afi, - safi, - bgp_addpath_id_for_peer( - peer, afi, safi, - &ctx->pi->tx_addpath)); + goto done; } - /* An update-group that does not use addpath */ - else { - if (ctx->pi) { - subgroup_process_announce_selected( - subgrp, ctx->pi, ctx->dest, afi, - safi, - bgp_addpath_id_for_peer( - peer, afi, safi, - &ctx->pi->tx_addpath)); - } else { - /* Find the addpath_tx_id of the path we - * had advertised and - * send a withdraw */ - RB_FOREACH_SAFE (adj, bgp_adj_out_rb, - &ctx->dest->adj_out, + + subgrp_withdraw_stale_addpath(ctx, subgrp); + subgrp_announce_addpath_best_selected(ctx->dest, subgrp); + + /* Process the bestpath last so the + * "show [ip] bgp neighbor x.x.x.x advertised" output shows + * the attributes from the bestpath. + */ + if (ctx->pi) + subgroup_process_announce_selected( + subgrp, ctx->pi, ctx->dest, afi, safi, + bgp_addpath_id_for_peer(peer, afi, safi, + &ctx->pi->tx_addpath)); + } else { + /* Send withdrawals without waiting for coalesting timer + * to expire. + */ + if (subgrp->t_coalesce) { + if (!ctx->pi || CHECK_FLAG(ctx->pi->flags, BGP_PATH_UNUSEABLE)) { + RB_FOREACH_SAFE (adj, bgp_adj_out_rb, &ctx->dest->adj_out, adj_next) { if (adj->subgroup == subgrp) { subgroup_process_announce_selected( - subgrp, NULL, - ctx->dest, afi, - safi, + subgrp, NULL, ctx->dest, afi, safi, adj->addpath_tx_id); } } } + + goto done; + } + + if (ctx->pi) { + subgroup_process_announce_selected( + subgrp, ctx->pi, ctx->dest, afi, safi, + bgp_addpath_id_for_peer(peer, afi, safi, + &ctx->pi->tx_addpath)); + } else { + RB_FOREACH_SAFE (adj, bgp_adj_out_rb, &ctx->dest->adj_out, + adj_next) { + if (adj->subgroup == subgrp) { + subgroup_process_announce_selected(subgrp, NULL, + ctx->dest, afi, + safi, + adj->addpath_tx_id); + } + } } } +done: /* Notify BGP Conditional advertisement */ bgp_notify_conditional_adv_scanner(subgrp); }