bgpd: add advertisement-delay to hold route advertisements after startup

[karthikeyav]

Summary

Add a new advertisement-delay CLI that holds route advertisements to peers
for a configured number of seconds after the first peer reaches Established
state. Unlike update-delay, which defers best-path and FIB programming,
advertisement-delay allows best-path selection and FIB installation to
proceed normally -- only peer advertisements are held.
Available as both a global command (bgp advertisement-delay <1-3600>) and
a per-instance command (advertisement-delay <1-3600> under router bgp).

Behavior

• Timer starts when the first peer reaches Established (same trigger as
  update-delay).
• During the delay: routes are received, best-path runs, FIB is programmed.
  Only advertisements to peers are held (pfxSnt remains 0).
• When the timer expires: advertisements are released to all Established peers.
• With both update-delay and advertisement-delay configured: advertisements
  are released at max(update-delay result, T0 + advertisement-delay).
• Re-triggers on clear ip bgp *, following the same pattern as update-delay.
• Not started on the GR restarter path (warm reboot) -- only update-delay/
  advertisement-delay apply to cold start and clear events.
• show bgp summary json shows advertisementDelay, advertisementDelayInProgress,
  advertisementDelayRemainingSeconds, and advertisementDelayResumeTime.
• show bgp router json shows bgpAdvertisementDelayTime.

[enkechen-panw]

What's the motivation for this knob? If the local FIB is already updated, delaying the route advertisement would mean stales routes on the receiver, right?

[greptile-apps]

Greptile Summary

This PR introduces advertisement-delay, a new BGP knob that holds route advertisements to peers for a configured number of seconds after the first peer reaches Established state. Unlike update-delay, best-path selection and FIB programming proceed normally during the hold — only peer advertisements are deferred. The feature is available both globally (bgp advertisement-delay) and per-BGP-instance/VRF (advertisement-delay), re-triggers on clear bgp *, and integrates with update-delay so that advertisements are released at max(update-delay, advertisement-delay).

The core state-machine logic, the dual-timer coordination in bgp_route.c, the clear bgp * re-trigger path, and the JSON/text show output are all implemented correctly. However, one P1 defect was found:

• no advertisement-delay leaves peers permanently unable to receive advertisements — when the timer is running and the config is removed, event_cancel stops the timer but bgp->main_peers_update_hold is never reset to 0. Because the only two code paths that clear that flag are the timer callback (cancelled) and the update-delay EOIU path (not triggered if update-delay isn't configured), peers stop receiving route advertisements until a clear bgp * is issued. Both no_bgp_advertisement_delay and no_bgp_global_advertisement_delay are affected. The topotest doesn't catch this because test_bgp_no_advertisement_delay waits for the delay to fully complete before issuing the no command.

• updateDelayPeerUpdateResume appears in show bgp summary json for advertisement-delay-only instances — bgp_start_routeadv() unconditionally stamps bgp->update_delay_peers_resume_time, so the suppression condition added in the display code is never actually effective after the first advertisement-delay cycle fires.

Confidence Score: 4/5

Not safe to merge as-is: removing advertisement-delay while the timer is active permanently blocks peer route advertisements until a hard clear

One P1 defect: the no advertisement-delay / no bgp advertisement-delay handlers cancel the timer but do not reset main_peers_update_hold, leaving peer advertisements permanently blocked. All other paths (timer expiry, clear-bgp re-trigger, dual-timer ordering) are correct. A targeted fix to the two deconfiguration handlers resolves the issue.

bgpd/bgp_vty.c — both no_bgp_advertisement_delay and no_bgp_global_advertisement_delay need to release main_peers_update_hold when the timer is still in-flight

Important Files Changed

Filename	Overview
bgpd/bgp_fsm.c	Adds advertisement-delay timer/begin/applicable/active/configured helpers; timer callback doesn't handle mid-run config removal (hold stuck P1)
bgpd/bgp_vty.c	Adds global and per-instance advertisement-delay CLI; no/deconfiguration path cancels timer but omits release of main_peers_update_hold (P1 bug)
bgpd/bgp_route.c	End-of-initial-update path correctly defers hold release when advertisement-delay timer is still active; both timers handled correctly
bgpd/bgpd.h	Adds advertisement-delay fields to bgp_master and bgp structs; straightforward additions
tests/topotests/bgp_advertisement_delay/test_bgp_advertisement_delay.py	8 test cases covering core scenarios; test_bgp_no_advertisement_delay waits for delay to finish before removing config, missing coverage of mid-run deconfiguration

Sequence Diagram

sequenceDiagram
    participant Peer as First Peer
    participant FSM as bgp_fsm.c
    participant Timer as advertisement-delay timer
    participant Route as bgp_route.c
    participant Adv as Route Advertisements

    Note over FSM: startup / clear ip bgp *

    Peer->>FSM: reaches Established
    FSM->>FSM: bgp_advertisement_delay_begin()<br/>main_peers_update_hold = 1
    FSM->>Timer: start timer (v_advertisement_delay seconds)

    Note over Route: (update-delay also running? update-delay end fires first)
    Route->>Route: bgp_process_main_one() EOIU path<br/>advertisement-delay still active?<br/>YES → keep holding

    Timer->>FSM: bgp_advertisement_delay_timer() fires
    alt update-delay still active or main_zebra_update_hold
        FSM-->>Route: set advertisement_delay_over=1, return<br/>(release deferred to EOIU path)
        Route->>Route: EOIU: advertisement_delay_active=false<br/>→ main_peers_update_hold=0
        Route->>Adv: bgp_start_routeadv()
    else advertisement-delay is the last hold
        FSM->>FSM: main_peers_update_hold = 0
        FSM->>Adv: bgp_start_routeadv()
    end

    Note over FSM,Adv: BUG: no advertisement-delay cancels timer<br/>but leaves main_peers_update_hold=1 → Adv permanently blocked

Loading

Comments Outside Diff (2)

1. bgpd/bgp_vty.c, line 237-252 (link)

   main_peers_update_hold not released on no advertisement-delay

   When advertisement-delay is actively running (bgp_advertisement_delay_begin has already set bgp->main_peers_update_hold = 1) and the user issues no advertisement-delay, the timer is cancelled and the flag fields are cleared — but bgp->main_peers_update_hold is never reset to 0. Since the only code paths that clear that flag are:

   1. bgp_advertisement_delay_timer (callback – but the timer was just cancelled), and
   2. the end-of-initial-update path in bgp_process_main_one (only reachable when update-delay is also configured),

   peers will permanently stop receiving route advertisements until a clear bgp * is issued to restart the cycle. The same problem exists in no_bgp_global_advertisement_delay which iterates all BGP instances.

   The fix is to release the hold when the timer is still in-flight at deconfiguration time, provided update-delay post-processing is not also holding things:

   /* Per-instance no handler */
   bgp->v_advertisement_delay = 0;
   if (bgp->advertisement_delay_started && !bgp->advertisement_delay_over) {
       event_cancel(&bgp->t_advertisement_delay);
       bgp->advertisement_delay_started = 0;
       bgp->advertisement_delay_over = 0;
       if (!bgp_update_delay_active(bgp) && !bgp->main_zebra_update_hold) {
           bgp->main_peers_update_hold = 0;
           bgp_start_routeadv(bgp);
       }
   } else {
       event_cancel(&bgp->t_advertisement_delay);
       bgp->advertisement_delay_started = 0;
       bgp->advertisement_delay_over = 0;
   }

   The same pattern should be applied to each instance inside no_bgp_global_advertisement_delay.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: bgpd/bgp_vty.c
   Line: 237-252
   
   Comment:
   **`main_peers_update_hold` not released on `no advertisement-delay`**
   
   When advertisement-delay is actively running (`bgp_advertisement_delay_begin` has already set `bgp->main_peers_update_hold = 1`) and the user issues `no advertisement-delay`, the timer is cancelled and the flag fields are cleared — but `bgp->main_peers_update_hold` is never reset to 0. Since the only code paths that clear that flag are:
   
   1. `bgp_advertisement_delay_timer` (callback – but the timer was just cancelled), and
   2. the end-of-initial-update path in `bgp_process_main_one` (only reachable when `update-delay` is also configured),
   
   peers will permanently stop receiving route advertisements until a `clear bgp *` is issued to restart the cycle. The same problem exists in `no_bgp_global_advertisement_delay` which iterates all BGP instances.
   
   The fix is to release the hold when the timer is still in-flight at deconfiguration time, provided `update-delay` post-processing is not also holding things:
   
   ```c
   /* Per-instance no handler */
   bgp->v_advertisement_delay = 0;
   if (bgp->advertisement_delay_started && !bgp->advertisement_delay_over) {
       event_cancel(&bgp->t_advertisement_delay);
       bgp->advertisement_delay_started = 0;
       bgp->advertisement_delay_over = 0;
       if (!bgp_update_delay_active(bgp) && !bgp->main_zebra_update_hold) {
           bgp->main_peers_update_hold = 0;
           bgp_start_routeadv(bgp);
       }
   } else {
       event_cancel(&bgp->t_advertisement_delay);
       bgp->advertisement_delay_started = 0;
       bgp->advertisement_delay_over = 0;
   }
   ```
   
   The same pattern should be applied to each instance inside `no_bgp_global_advertisement_delay`.
   
   How can I resolve this? If you propose a fix, please make it concise.

2. bgpd/bgp_vty.c, line 283-289 (link)

   updateDelayPeerUpdateResume shown for advertisement-delay-only case

   bgp_start_routeadv() unconditionally stamps bgp->update_delay_peers_resume_time (see bgp_fsm.c). When advertisement-delay fires without any update-delay configured, bgp_start_routeadv is still called and populates that field. The guard added here:

   if (!bgp_advertisement_delay_configured(bgp) ||
       bgp->update_delay_peers_resume_time[0] != '\0')

   …evaluates to true (field is non-empty after the delay fires), so updateDelayPeerUpdateResume is emitted in show bgp summary json even when no update-delay was ever configured. This can be confusing to operators parsing the JSON output.

   Consider clearing update_delay_peers_resume_time alongside the other resume fields when only advertisement-delay is in use, or stamp a dedicated field in bgp_start_routeadv when called from the advertisement-delay path.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: bgpd/bgp_vty.c
   Line: 283-289
   
   Comment:
   **`updateDelayPeerUpdateResume` shown for advertisement-delay-only case**
   
   `bgp_start_routeadv()` unconditionally stamps `bgp->update_delay_peers_resume_time` (see `bgp_fsm.c`). When advertisement-delay fires without any `update-delay` configured, `bgp_start_routeadv` is still called and populates that field. The guard added here:
   
   ```c
   if (!bgp_advertisement_delay_configured(bgp) ||
       bgp->update_delay_peers_resume_time[0] != '\0')
   ```
   
   …evaluates to `true` (field is non-empty after the delay fires), so `updateDelayPeerUpdateResume` is emitted in `show bgp summary json` even when no `update-delay` was ever configured. This can be confusing to operators parsing the JSON output.
   
   Consider clearing `update_delay_peers_resume_time` alongside the other resume fields when only advertisement-delay is in use, or stamp a dedicated field in `bgp_start_routeadv` when called from the advertisement-delay path.
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: bgpd/bgp_vty.c
Line: 237-252

Comment:
**`main_peers_update_hold` not released on `no advertisement-delay`**

When advertisement-delay is actively running (`bgp_advertisement_delay_begin` has already set `bgp->main_peers_update_hold = 1`) and the user issues `no advertisement-delay`, the timer is cancelled and the flag fields are cleared — but `bgp->main_peers_update_hold` is never reset to 0. Since the only code paths that clear that flag are:

1. `bgp_advertisement_delay_timer` (callback – but the timer was just cancelled), and
2. the end-of-initial-update path in `bgp_process_main_one` (only reachable when `update-delay` is also configured),

peers will permanently stop receiving route advertisements until a `clear bgp *` is issued to restart the cycle. The same problem exists in `no_bgp_global_advertisement_delay` which iterates all BGP instances.

The fix is to release the hold when the timer is still in-flight at deconfiguration time, provided `update-delay` post-processing is not also holding things:

```c
/* Per-instance no handler */
bgp->v_advertisement_delay = 0;
if (bgp->advertisement_delay_started && !bgp->advertisement_delay_over) {
    event_cancel(&bgp->t_advertisement_delay);
    bgp->advertisement_delay_started = 0;
    bgp->advertisement_delay_over = 0;
    if (!bgp_update_delay_active(bgp) && !bgp->main_zebra_update_hold) {
        bgp->main_peers_update_hold = 0;
        bgp_start_routeadv(bgp);
    }
} else {
    event_cancel(&bgp->t_advertisement_delay);
    bgp->advertisement_delay_started = 0;
    bgp->advertisement_delay_over = 0;
}
```

The same pattern should be applied to each instance inside `no_bgp_global_advertisement_delay`.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: bgpd/bgp_vty.c
Line: 283-289

Comment:
**`updateDelayPeerUpdateResume` shown for advertisement-delay-only case**

`bgp_start_routeadv()` unconditionally stamps `bgp->update_delay_peers_resume_time` (see `bgp_fsm.c`). When advertisement-delay fires without any `update-delay` configured, `bgp_start_routeadv` is still called and populates that field. The guard added here:

```c
if (!bgp_advertisement_delay_configured(bgp) ||
    bgp->update_delay_peers_resume_time[0] != '\0')
```

…evaluates to `true` (field is non-empty after the delay fires), so `updateDelayPeerUpdateResume` is emitted in `show bgp summary json` even when no `update-delay` was ever configured. This can be confusing to operators parsing the JSON output.

Consider clearing `update_delay_peers_resume_time` alongside the other resume fields when only advertisement-delay is in use, or stamp a dedicated field in `bgp_start_routeadv` when called from the advertisement-delay path.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "tests: add topotest for bgp advertisemen..." | Re-trigger Greptile}

[karthikeyav]

What's the motivation for this knob? If the local FIB is already updated, delaying the route advertisement would mean stales routes on the receiver, right?

With update-delay, both FIB programming and advertisements are deferred -- this means the restarting router cannot forward local traffic until the delay completes. advertisement-delay allows FIB programming to proceed normally so local forwarding works immediately, while holding only the advertisements to prevent the router from attracting remote traffic before it has a complete routing state.

Regarding stale routes: after a non-graceful restart, peers detect the session loss and withdraw routes to this router, so the receiver has no routes -- not stale ones. The advertisement-delay controls when this router re-announces itself, ensuring it only attracts traffic once it has fully converged. When graceful-restart is active, advertisement-delay is not started -- the GR restarter path handles route retention separately.

[ton31337]

Please update the documentation on what is the motivation for this new command (like you answered in the comments).

[ton31337]

Any reason is this a char?

[karthikeyav]

Followed same pattern as the existing update_delay_*_time fields (bgpd.h#L673-L676) -- char[64] populated by frr_timestamp() for display in show bgp summary.

[ton31337]

Same, put it under zlog_debug().

[karthikeyav]

This fires once when the first peer reaches Established, same pattern as update-delay's zlog_info("Begin read-only mode ..."). Can I keep this at zlog_info? I'd like it in frr.log without requiring debugs.

[karthikeyav]

Please update the documentation on what is the motivation for this new command (like you answered in the comments).

Updated in the bgp.rst documentation section.

[ton31337]

In general, I like this "feature", and I agree we should rename an existing "update-delay" to something else more reflective in the future.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[riw777]

looks good ... this is a good feature