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
This commit is contained in:
Don Slice 2017-10-19 14:23:30 -04:00
parent 6d774763d6
commit d25e4efc52
6 changed files with 76 additions and 30 deletions

View File

@ -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 {

View File

@ -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;

View File

@ -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);

View File

@ -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. */

View File

@ -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))

View File

@ -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;