bgpd: fix various problems with hold/keepalive timers

Problem reported that we weren't adjusting the keepalive timer
correctly when we negotiated a lower hold time learned from a
peer.  While working on this, found we didn't do inheritance
correctly at all.  This fix solves the first problem and also
ensures that the timers are configured correctly based on this
priority order - peer defined > peer-group defined > global config.
This fix also displays the timers as "configured" regardless of
which of the three locations above is used.

Ticket: CM-18408
Signed-off-by: Don Slice <dslice@cumulusnetworks.com>
Reviewed-by: CCR-6807
Testing-performed:  Manual testing successful, fix tested by
submitter, bgp-smoke completed successfully

diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c
index b609abac6994ab47fdf4b674edf29fd45e261350..aa350d3dd4ca65462407051152f6ec7da1db7c0d 100644 (file)
--- a/bgpd/bgp_fsm.c
+++ b/bgpd/bgp_fsm.c
@@ -1096,7 +1096,7 @@ int bgp_stop(struct peer *peer)
                }
 
        /* Reset keepalive and holdtime */
-       if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER)) {
+       if (PEER_OR_GROUP_TIMER_SET(peer)) {
                peer->v_keepalive = peer->keepalive;
                peer->v_holdtime = peer->holdtime;
        } else {

diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c
index 003fbaff632913bdc0d5851da1503277a03e11c6..a545162f81cb33a0296e7ab4efc7804e8377ed6f 100644 (file)
--- a/bgpd/bgp_packet.c
+++ b/bgpd/bgp_packet.c
@@ -529,7 +529,7 @@ void bgp_open_send(struct peer *peer)
        u_int16_t send_holdtime;
        as_t local_as;
 
-       if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER))
+       if (PEER_OR_GROUP_TIMER_SET(peer))
                send_holdtime = peer->holdtime;
        else
                send_holdtime = peer->bgp->default_holdtime;
@@ -1087,7 +1087,7 @@ static int bgp_open_receive(struct peer *peer, bgp_size_t size)
           implementation MAY adjust the rate at which it sends KEEPALIVE
           messages as a function of the Hold Time interval. */
 
-       if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER))
+       if (PEER_OR_GROUP_TIMER_SET(peer))
                send_holdtime = peer->holdtime;
        else
                send_holdtime = peer->bgp->default_holdtime;
@@ -1097,7 +1097,8 @@ static int bgp_open_receive(struct peer *peer, bgp_size_t size)
        else
                peer->v_holdtime = send_holdtime;
 
-       if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER))
+       if ((PEER_OR_GROUP_TIMER_SET(peer))
+           && (peer->keepalive < peer->v_holdtime / 3))
                peer->v_keepalive = peer->keepalive;
        else
                peer->v_keepalive = peer->v_holdtime / 3;

diff --git a/bgpd/bgp_snmp.c b/bgpd/bgp_snmp.c
index 8a9d61f3058ab89370c992df2cb00c441cff399d..484ea7c433b6dc93b633e240eac582d8d7d31f53 100644 (file)
--- a/bgpd/bgp_snmp.c
+++ b/bgpd/bgp_snmp.c
@@ -615,14 +615,14 @@ static u_char *bgpPeerTable(struct variable *v, oid name[], size_t *length,
                break;
        case BGPPEERHOLDTIMECONFIGURED:
                *write_method = write_bgpPeerTable;
-               if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER))
+               if (PEER_OR_GROUP_TIMER_SET(peer))
                        return SNMP_INTEGER(peer->holdtime);
                else
                        return SNMP_INTEGER(peer->v_holdtime);
                break;
        case BGPPEERKEEPALIVECONFIGURED:
                *write_method = write_bgpPeerTable;
-               if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER))
+               if (PEER_OR_GROUP_TIMER_SET(peer))
                        return SNMP_INTEGER(peer->keepalive);
                else
                        return SNMP_INTEGER(peer->v_keepalive);

diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c
index 749c9d25d42d9ce3c471e550c44b993da3dd5070..84dae9b9bcd31ebe1d88ffcbb551213fac7fe153 100644 (file)
--- a/bgpd/bgp_vty.c
+++ b/bgpd/bgp_vty.c
@@ -8272,7 +8272,7 @@ static void bgp_show_peer(struct vty *vty, struct peer *p, u_char use_json,
                                    "bgpTimerKeepAliveIntervalMsecs",
                                    p->v_keepalive * 1000);
 
-               if (CHECK_FLAG(p->config, PEER_CONFIG_TIMER)) {
+               if (PEER_OR_GROUP_TIMER_SET(p)) {
                        json_object_int_add(json_neigh,
                                            "bgpTimerConfiguredHoldTimeMsecs",
                                            p->holdtime * 1000);
@@ -8280,6 +8280,16 @@ static void bgp_show_peer(struct vty *vty, struct peer *p, u_char use_json,
                                json_neigh,
                                "bgpTimerConfiguredKeepAliveIntervalMsecs",
                                p->keepalive * 1000);
+               } else if ((bgp->default_holdtime != BGP_DEFAULT_HOLDTIME)
+                          || (bgp->default_keepalive !=
+                              BGP_DEFAULT_KEEPALIVE)) {
+                       json_object_int_add(json_neigh,
+                                           "bgpTimerConfiguredHoldTimeMsecs",
+                                           bgp->default_holdtime);
+                       json_object_int_add(
+                               json_neigh,
+                               "bgpTimerConfiguredKeepAliveIntervalMsecs",
+                               bgp->default_keepalive);
                }
        } else {
                /* Administrative shutdown. */
@@ -8326,11 +8336,18 @@ static void bgp_show_peer(struct vty *vty, struct peer *p, u_char use_json,
                vty_out(vty,
                        "  Hold time is %d, keepalive interval is %d seconds\n",
                        p->v_holdtime, p->v_keepalive);
-               if (CHECK_FLAG(p->config, PEER_CONFIG_TIMER)) {
+               if (PEER_OR_GROUP_TIMER_SET(p)) {
                        vty_out(vty, "  Configured hold time is %d",
                                p->holdtime);
                        vty_out(vty, ", keepalive interval is %d seconds\n",
                                p->keepalive);
+               } else if ((bgp->default_holdtime != BGP_DEFAULT_HOLDTIME)
+                          || (bgp->default_keepalive !=
+                              BGP_DEFAULT_KEEPALIVE)) {
+                       vty_out(vty, "  Configured hold time is %d",
+                               bgp->default_holdtime);
+                       vty_out(vty, ", keepalive interval is %d seconds\n",
+                               bgp->default_keepalive);
                }
        }
        /* Capability. */

diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index a39ff3b271a06e391228292de2e05f922122dc42..345ceab9ed83a71a3e1d80027afb045badfb566e 100644 (file)
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -2301,6 +2301,7 @@ struct peer_group *peer_group_get(struct bgp *bgp, const char *name)
        group->conf->gtsm_hops = 0;
        group->conf->v_routeadv = BGP_DEFAULT_EBGP_ROUTEADV;
        UNSET_FLAG(group->conf->config, PEER_CONFIG_TIMER);
+       UNSET_FLAG(group->conf->config, PEER_GROUP_CONFIG_TIMER);
        UNSET_FLAG(group->conf->config, PEER_CONFIG_CONNECT);
        group->conf->keepalive = 0;
        group->conf->holdtime = 0;
@@ -4582,20 +4583,30 @@ int peer_timers_set(struct peer *peer, u_int32_t keepalive, u_int32_t holdtime)
                return BGP_ERR_INVALID_VALUE;
 
        /* Set value to the configuration. */
-       SET_FLAG(peer->config, PEER_CONFIG_TIMER);
        peer->holdtime = holdtime;
        peer->keepalive = (keepalive < holdtime / 3 ? keepalive : holdtime / 3);
 
-       if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP))
-               return 0;
-
-       /* peer-group member updates. */
-       group = peer->group;
-       for (ALL_LIST_ELEMENTS(group->peer, node, nnode, peer)) {
+       /* First work on real peers with timers */
+       if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
                SET_FLAG(peer->config, PEER_CONFIG_TIMER);
-               peer->holdtime = group->conf->holdtime;
-               peer->keepalive = group->conf->keepalive;
+               UNSET_FLAG(peer->config, PEER_GROUP_CONFIG_TIMER);
+       } else {
+               /* Now work on the peer-group timers */
+               SET_FLAG(peer->config, PEER_GROUP_CONFIG_TIMER);
+
+               /* peer-group member updates. */
+               group = peer->group;
+               for (ALL_LIST_ELEMENTS(group->peer, node, nnode, peer)) {
+                       /* Skip peers that have their own timers */
+                       if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER))
+                               continue;
+
+                       SET_FLAG(peer->config, PEER_GROUP_CONFIG_TIMER);
+                       peer->holdtime = group->conf->holdtime;
+                       peer->keepalive = group->conf->keepalive;
+               }
        }
+
        return 0;
 }
 
@@ -4604,20 +4615,32 @@ int peer_timers_unset(struct peer *peer)
        struct peer_group *group;
        struct listnode *node, *nnode;
 
-       /* Clear configuration. */
-       UNSET_FLAG(peer->config, PEER_CONFIG_TIMER);
-       peer->keepalive = 0;
-       peer->holdtime = 0;
-
-       if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP))
-               return 0;
-
-       /* peer-group member updates. */
-       group = peer->group;
-       for (ALL_LIST_ELEMENTS(group->peer, node, nnode, peer)) {
+       /* First work on real peers vs the peer-group */
+       if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
                UNSET_FLAG(peer->config, PEER_CONFIG_TIMER);
-               peer->holdtime = 0;
                peer->keepalive = 0;
+               peer->holdtime = 0;
+
+               if (peer->group && peer->group->conf->holdtime) {
+                       SET_FLAG(peer->config, PEER_GROUP_CONFIG_TIMER);
+                       peer->keepalive = peer->group->conf->keepalive;
+                       peer->holdtime = peer->group->conf->holdtime;
+               }
+       } else {
+               /* peer-group member updates. */
+               group = peer->group;
+               for (ALL_LIST_ELEMENTS(group->peer, node, nnode, peer)) {
+                       if (!CHECK_FLAG(peer->config, PEER_CONFIG_TIMER)) {
+                               UNSET_FLAG(peer->config,
+                                          PEER_GROUP_CONFIG_TIMER);
+                               peer->holdtime = 0;
+                               peer->keepalive = 0;
+                       }
+               }
+
+               UNSET_FLAG(group->conf->config, PEER_GROUP_CONFIG_TIMER);
+               group->conf->holdtime = 0;
+               group->conf->keepalive = 0;
        }
 
        return 0;
@@ -6535,7 +6558,7 @@ static void bgp_config_write_peer_global(struct vty *vty, struct bgp *bgp,
        }
 
        /* timers */
-       if (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER)
+       if ((PEER_OR_GROUP_TIMER_SET(peer))
            && ((!peer_group_active(peer)
                 && (peer->keepalive != BGP_DEFAULT_KEEPALIVE
                     || peer->holdtime != BGP_DEFAULT_HOLDTIME))

diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
index 9c38a65b40023e91e73cbf3db3e666cf7b5701e0..69ffe086d282d79d87950415528af43232ea98ad 100644 (file)
--- a/bgpd/bgpd.h
+++ b/bgpd/bgpd.h
@@ -771,6 +771,11 @@ struct peer {
 #define PEER_CONFIG_TIMER             (1 << 0) /* keepalive & holdtime */
 #define PEER_CONFIG_CONNECT           (1 << 1) /* connect */
 #define PEER_CONFIG_ROUTEADV          (1 << 2) /* route advertise */
+#define PEER_GROUP_CONFIG_TIMER       (1 << 3) /* timers from peer-group */
+
+#define PEER_OR_GROUP_TIMER_SET(peer)                                        \
+       (CHECK_FLAG(peer->config, PEER_CONFIG_TIMER)                         \
+        || CHECK_FLAG(peer->config, PEER_GROUP_CONFIG_TIMER))
 
        u_int32_t holdtime;
        u_int32_t keepalive;