bgpd: Add BGP configuration start/end markers

Delay BGP configuration until we receive end-configuration hook to make sure
we don't send partial updates to peer which leads to broken Graceful-Restart.

Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>

diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c
index dea1433f6dfdc92459852e1c8490f5a32024778b..3600e2f0ec3b64724cf4e0f24e936308594c6fc4 100644 (file)
--- a/bgpd/bgp_vty.c
+++ b/bgpd/bgp_vty.c
@@ -17599,11 +17599,67 @@ static const struct cmd_variable_handler bgp_var_peergroup[] = {
        {.tokenname = "PGNAME", .completions = bgp_ac_peergroup},
        {.completions = NULL} };
 
+DEFINE_HOOK(bgp_config_end, (struct bgp *bgp), (bgp));
+
+static struct thread *t_bgp_cfg;
+
+bool bgp_config_inprocess(void)
+{
+       return thread_is_scheduled(t_bgp_cfg);
+}
+
+static void bgp_config_finish(struct thread *t)
+{
+       struct listnode *node;
+       struct bgp *bgp;
+
+       for (ALL_LIST_ELEMENTS_RO(bm->bgp, node, bgp))
+               hook_call(bgp_config_end, bgp);
+}
+
+static void bgp_config_start(void)
+{
+#define BGP_PRE_CONFIG_MAX_WAIT_SECONDS 600
+       THREAD_OFF(t_bgp_cfg);
+       thread_add_timer(bm->master, bgp_config_finish, NULL,
+                        BGP_PRE_CONFIG_MAX_WAIT_SECONDS, &t_bgp_cfg);
+}
+
+/* When we receive a hook the configuration is read,
+ * we start a timer to make sure we postpone sending
+ * EoR before route-maps are processed.
+ * This is especially valid if using `bgp route-map delay-timer`.
+ */
+static void bgp_config_end(void)
+{
+#define BGP_POST_CONFIG_DELAY_SECONDS 1
+       uint32_t bgp_post_config_delay =
+               thread_is_scheduled(bm->t_rmap_update)
+                       ? thread_timer_remain_second(bm->t_rmap_update)
+                       : BGP_POST_CONFIG_DELAY_SECONDS;
+
+       /* If BGP config processing thread isn't running, then
+        * we can return and rely it's properly handled.
+        */
+       if (!bgp_config_inprocess())
+               return;
+
+       THREAD_OFF(t_bgp_cfg);
+
+       /* Start a new timer to make sure we don't send EoR
+        * before route-maps are processed.
+        */
+       thread_add_timer(bm->master, bgp_config_finish, NULL,
+                        bgp_post_config_delay, &t_bgp_cfg);
+}
+
 void bgp_vty_init(void)
 {
        cmd_variable_handler_register(bgp_var_neighbor);
        cmd_variable_handler_register(bgp_var_peergroup);
 
+       cmd_init_config_callbacks(bgp_config_start, bgp_config_end);
+
        /* Install bgp top node. */
        install_node(&bgp_node);
        install_node(&bgp_ipv4_unicast_node);

diff --git a/bgpd/bgp_vty.h b/bgpd/bgp_vty.h
index 93026c663aa3509bbada85c49175c6a238745194..4b393275d6c8f17407bb0fada53d0d6949787d58 100644 (file)
--- a/bgpd/bgp_vty.h
+++ b/bgpd/bgp_vty.h
@@ -164,6 +164,7 @@ extern void bgp_config_write_rpkt_quanta(struct vty *vty, struct bgp *bgp);
 extern void bgp_config_write_listen(struct vty *vty, struct bgp *bgp);
 extern void bgp_config_write_coalesce_time(struct vty *vty, struct bgp *bgp);
 extern int bgp_vty_return(struct vty *vty, int ret);
+extern bool bgp_config_inprocess(void);
 extern struct peer *peer_and_group_lookup_vty(struct vty *vty,
                                              const char *peer_str);
 

diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index 38a106359ea2c6e3e5f2d0fe0e0f19e6f0ac3005..b0be2d70fe7c5230438c1db28159311985ebc93f 100644 (file)
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -1731,6 +1731,8 @@ struct peer *peer_create(union sockunion *su, const char *conf_if,
        peer->v_routeadv = (peer_sort(peer) == BGP_PEER_IBGP)
                                   ? BGP_DEFAULT_IBGP_ROUTEADV
                                   : BGP_DEFAULT_EBGP_ROUTEADV;
+       if (bgp_config_inprocess())
+               peer->shut_during_cfg = true;
 
        peer = peer_lock(peer); /* bgp peer list reference */
        peer->group = group;
@@ -7938,8 +7940,33 @@ void bgp_pthreads_finish(void)
        frr_pthread_stop_all();
 }
 
+static int peer_unshut_after_cfg(struct bgp *bgp)
+{
+       struct listnode *node;
+       struct peer *peer;
+
+       for (ALL_LIST_ELEMENTS_RO(bgp->peer, node, peer)) {
+               if (!peer->shut_during_cfg)
+                       continue;
+
+               if (bgp_debug_neighbor_events(peer))
+                       zlog_debug("%s: released from config-pending hold",
+                                  peer->host);
+
+               peer->shut_during_cfg = false;
+               if (peer_active(peer) && peer->status != Established) {
+                       if (peer->status != Idle)
+                               BGP_EVENT_ADD(peer, BGP_Stop);
+                       BGP_EVENT_ADD(peer, BGP_Start);
+               }
+       }
+
+       return 0;
+}
+
 void bgp_init(unsigned short instance)
 {
+       hook_register(bgp_config_end, peer_unshut_after_cfg);
 
        /* allocates some vital data structures used by peer commands in
         * vty_init */

diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
index a9475f39a76731feb63bd98d1cfdbdfdf5af7677..6eba48924c9444072aa898cfb68ad6bb3c4be132 100644 (file)
--- a/bgpd/bgpd.h
+++ b/bgpd/bgpd.h
@@ -771,6 +771,7 @@ DECLARE_HOOK(bgp_inst_delete, (struct bgp *bgp), (bgp));
 DECLARE_HOOK(bgp_inst_config_write,
                (struct bgp *bgp, struct vty *vty),
                (bgp, vty));
+DECLARE_HOOK(bgp_config_end, (struct bgp *bgp), (bgp));
 
 /* Thread callback information */
 struct afi_safi_info {
@@ -1676,6 +1677,8 @@ struct peer {
        /* Long-lived Graceful Restart */
        struct llgr_info llgr[AFI_MAX][SAFI_MAX];
 
+       bool shut_during_cfg;
+
        QOBJ_FIELDS;
 };
 DECLARE_QOBJ_TYPE(peer);
@@ -1703,9 +1706,10 @@ DECLARE_QOBJ_TYPE(peer);
 
 /* Check if suppress start/restart of sessions to peer. */
 #define BGP_PEER_START_SUPPRESSED(P)                                           \
-       (CHECK_FLAG((P)->flags, PEER_FLAG_SHUTDOWN)                            \
-        || CHECK_FLAG((P)->sflags, PEER_STATUS_PREFIX_OVERFLOW)               \
-        || CHECK_FLAG((P)->bgp->flags, BGP_FLAG_SHUTDOWN))
+       (CHECK_FLAG((P)->flags, PEER_FLAG_SHUTDOWN) ||                         \
+        CHECK_FLAG((P)->sflags, PEER_STATUS_PREFIX_OVERFLOW) ||               \
+        CHECK_FLAG((P)->bgp->flags, BGP_FLAG_SHUTDOWN) ||                     \
+        (P)->shut_during_cfg)
 
 #define PEER_ROUTE_ADV_DELAY(peer)                                            \
        (CHECK_FLAG(peer->thread_flags, PEER_THREAD_SUBGRP_ADV_DELAY))