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 <donatas@opensourcerouting.org>

diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c
index a1bf9a4c61ad498113cfa09456b4e4705dfdf45a..8f816eb30decf00cc1989349628abcbeb94a9324 100644 (file)
--- 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);
        }