[bgpd] Fix 0.99 shutdown regression, introduce Clearing and Deleted states
2006-09-14 Paul Jakma <paul.jakma@sun.com>
* (general) Fix some niggly issues around 'shutdown' and clearing
by adding a Clearing FSM wait-state and a hidden 'Deleted'
FSM state, to allow deleted peers to 'cool off' and hit 0
references. This introduces a slow memory leak of struct peer,
however that's more a testament to the fragility of the
reference counting than a bug in this patch, cleanup of
reference counting to fix this is to follow.
* bgpd.h: Add Clearing, Deleted states and Clearing_Completed
and event.
* bgp_debug.c: (bgp_status_msg[]) Add strings for Clearing and
Deleted.
* bgp_fsm.h: Don't allow timer/event threads to set anything
for Deleted peers.
* bgp_fsm.c: (bgp_timer_set) Add Clearing and Deleted. Deleted
needs to stop everything.
(bgp_stop) Remove explicit fsm_change_status call, the
general framework handles the transition.
(bgp_start) Log a warning if a start is attempted on a peer
that should stay down, trying to start a peer.
(struct .. FSM) Add Clearing_Completed
events, has little influence except when in state
Clearing to signal wait-state can end.
Add Clearing and Deleted states, former is a wait-state,
latter is a placeholder state to allow peers to disappear
quietly once refcounts settle.
(bgp_event) Try reduce verbosity of FSM state-change debug,
changes to same state are not interesting (Established->Established)
Allow NULL action functions in FSM.
* bgp_packet.c: (bgp_write) Use FSM events, rather than trying
to twiddle directly with FSM state behind the back of FSM.
(bgp_write_notify) ditto.
(bgp_read) Remove the vague ACCEPT_PEER peer_unlock, or else
this patch crashes, now it leaks instead.
* bgp_route.c: (bgp_clear_node_complete) Clearing_Completed
event, to end clearing.
(bgp_clear_route) See extensive comments.
* bgpd.c: (peer_free) should only be called while in Deleted,
peer refcounting controls when peer_free is called.
bgp_sync_delete should be here, not in peer_delete.
(peer_delete) Initiate delete.
Transition to Deleted state manually.
When removing peer from indices that provide visibility of it,
take great care to be idempotent wrt the reference counting
of struct peer through those indices.
Use bgp_timer_set, rather than replicating.
Call to bgp_sync_delete isn't appropriate here, sync can be
referenced while shutting down and finishing deletion.
(peer_group_bind) Take care to be idempotent wrt list references
indexing peers.
diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c
index 770a791..bdb6517 100644
--- a/bgpd/bgp_fsm.c
+++ b/bgpd/bgp_fsm.c
@@ -68,6 +68,11 @@
return ((rand () % (time + 1)) - (time / 2));
}
+/* 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))
+
/* Hook function called after bgp event is occered. And vty's
neighbor command invoke this function after making neighbor
structure. */
@@ -82,10 +87,7 @@
/* First entry point of peer's finite state machine. In Idle
status start timer is on unless peer is shutdown or peer is
inactive. All other timer must be turned off */
- if (CHECK_FLAG (peer->flags, PEER_FLAG_SHUTDOWN)
- || CHECK_FLAG (peer->sflags, PEER_STATUS_PREFIX_OVERFLOW)
- || CHECK_FLAG (peer->sflags, PEER_STATUS_CLEARING)
- || ! peer_active (peer))
+ if (BGP_PEER_START_SUPPRESSED (peer) || ! peer_active (peer))
{
BGP_TIMER_OFF (peer->t_start);
}
@@ -197,6 +199,17 @@
}
BGP_TIMER_OFF (peer->t_asorig);
break;
+ case Deleted:
+ BGP_TIMER_OFF (peer->t_gr_restart);
+ BGP_TIMER_OFF (peer->t_gr_stale);
+ BGP_TIMER_OFF (peer->t_pmax_restart);
+ case Clearing:
+ BGP_TIMER_OFF (peer->t_start);
+ BGP_TIMER_OFF (peer->t_connect);
+ BGP_TIMER_OFF (peer->t_holdtime);
+ BGP_TIMER_OFF (peer->t_keepalive);
+ BGP_TIMER_OFF (peer->t_asorig);
+ BGP_TIMER_OFF (peer->t_routeadv);
}
}
@@ -420,7 +433,6 @@
if (peer->status == Established)
{
peer->dropped++;
- bgp_fsm_change_status (peer, Idle);
/* bgp log-neighbor-changes of neighbor Down */
if (bgp_flag_check (peer->bgp, BGP_FLAG_LOG_NEIGHBOR_CHANGES))
@@ -625,6 +637,14 @@
{
int status;
+ if (BGP_PEER_START_SUPPRESSED (peer))
+ {
+ if (BGP_DEBUG (fsm, FSM))
+ plog_err (peer->log, "%s [FSM] Trying to start suppressed peer"
+ " - this is never supposed to happen!", peer->host);
+ return -1;
+ }
+
/* Scrub some information that might be left over from a previous,
* session
*/
@@ -903,6 +923,7 @@
{bgp_ignore, Idle}, /* Receive_KEEPALIVE_message */
{bgp_ignore, Idle}, /* Receive_UPDATE_message */
{bgp_ignore, Idle}, /* Receive_NOTIFICATION_message */
+ {bgp_ignore, Idle}, /* Clearing_Completed */
},
{
/* Connect */
@@ -919,6 +940,7 @@
{bgp_ignore, Idle}, /* Receive_KEEPALIVE_message */
{bgp_ignore, Idle}, /* Receive_UPDATE_message */
{bgp_stop, Idle}, /* Receive_NOTIFICATION_message */
+ {bgp_ignore, Idle}, /* Clearing_Completed */
},
{
/* Active, */
@@ -935,6 +957,7 @@
{bgp_ignore, Idle}, /* Receive_KEEPALIVE_message */
{bgp_ignore, Idle}, /* Receive_UPDATE_message */
{bgp_stop_with_error, Idle}, /* Receive_NOTIFICATION_message */
+ {bgp_ignore, Idle}, /* Clearing_Completed */
},
{
/* OpenSent, */
@@ -951,6 +974,7 @@
{bgp_ignore, Idle}, /* Receive_KEEPALIVE_message */
{bgp_ignore, Idle}, /* Receive_UPDATE_message */
{bgp_stop_with_error, Idle}, /* Receive_NOTIFICATION_message */
+ {bgp_ignore, Idle}, /* Clearing_Completed */
},
{
/* OpenConfirm, */
@@ -967,22 +991,58 @@
{bgp_establish, Established}, /* Receive_KEEPALIVE_message */
{bgp_ignore, Idle}, /* Receive_UPDATE_message */
{bgp_stop_with_error, Idle}, /* Receive_NOTIFICATION_message */
+ {bgp_ignore, Idle}, /* Clearing_Completed */
},
{
/* Established, */
- {bgp_ignore, Established}, /* BGP_Start */
- {bgp_stop, Idle}, /* BGP_Stop */
- {bgp_stop, Idle}, /* TCP_connection_open */
- {bgp_stop, Idle}, /* TCP_connection_closed */
- {bgp_ignore, Idle}, /* TCP_connection_open_failed */
- {bgp_stop, Idle}, /* TCP_fatal_error */
- {bgp_ignore, Idle}, /* ConnectRetry_timer_expired */
- {bgp_fsm_holdtime_expire, Idle}, /* Hold_Timer_expired */
+ {bgp_ignore, Established}, /* BGP_Start */
+ {bgp_stop, Clearing}, /* BGP_Stop */
+ {bgp_stop, Clearing}, /* TCP_connection_open */
+ {bgp_stop, Clearing}, /* TCP_connection_closed */
+ {bgp_ignore, Clearing}, /* TCP_connection_open_failed */
+ {bgp_stop, Clearing}, /* TCP_fatal_error */
+ {bgp_ignore, Clearing}, /* ConnectRetry_timer_expired */
+ {bgp_fsm_holdtime_expire, Clearing}, /* Hold_Timer_expired */
{bgp_fsm_keepalive_expire, Established}, /* KeepAlive_timer_expired */
- {bgp_stop, Idle}, /* Receive_OPEN_message */
- {bgp_fsm_keepalive, Established}, /* Receive_KEEPALIVE_message */
- {bgp_fsm_update, Established}, /* Receive_UPDATE_message */
- {bgp_stop_with_error, Idle}, /* Receive_NOTIFICATION_message */
+ {bgp_stop, Clearing}, /* Receive_OPEN_message */
+ {bgp_fsm_keepalive, Established}, /* Receive_KEEPALIVE_message */
+ {bgp_fsm_update, Established}, /* Receive_UPDATE_message */
+ {bgp_stop_with_error, Clearing}, /* Receive_NOTIFICATION_message */
+ {bgp_ignore, Idle}, /* Clearing_Completed */
+ },
+ {
+ /* Clearing, */
+ {bgp_ignore, Clearing}, /* BGP_Start */
+ {bgp_ignore, Clearing}, /* BGP_Stop */
+ {bgp_ignore, Clearing}, /* TCP_connection_open */
+ {bgp_ignore, Clearing}, /* TCP_connection_closed */
+ {bgp_ignore, Clearing}, /* TCP_connection_open_failed */
+ {bgp_ignore, Clearing}, /* TCP_fatal_error */
+ {bgp_ignore, Clearing}, /* ConnectRetry_timer_expired */
+ {bgp_ignore, Clearing}, /* Hold_Timer_expired */
+ {bgp_ignore, Clearing}, /* KeepAlive_timer_expired */
+ {bgp_ignore, Clearing}, /* Receive_OPEN_message */
+ {bgp_ignore, Clearing}, /* Receive_KEEPALIVE_message */
+ {bgp_ignore, Clearing}, /* Receive_UPDATE_message */
+ {bgp_ignore, Clearing}, /* Receive_NOTIFICATION_message */
+ {bgp_ignore, Idle }, /* Clearing_Completed */
+ },
+ {
+ /* Deleted, */
+ {bgp_ignore, Deleted}, /* BGP_Start */
+ {bgp_ignore, Deleted}, /* BGP_Stop */
+ {bgp_ignore, Deleted}, /* TCP_connection_open */
+ {bgp_ignore, Deleted}, /* TCP_connection_closed */
+ {bgp_ignore, Deleted}, /* TCP_connection_open_failed */
+ {bgp_ignore, Deleted}, /* TCP_fatal_error */
+ {bgp_ignore, Deleted}, /* ConnectRetry_timer_expired */
+ {bgp_ignore, Deleted}, /* Hold_Timer_expired */
+ {bgp_ignore, Deleted}, /* KeepAlive_timer_expired */
+ {bgp_ignore, Deleted}, /* Receive_OPEN_message */
+ {bgp_ignore, Deleted}, /* Receive_KEEPALIVE_message */
+ {bgp_ignore, Deleted}, /* Receive_UPDATE_message */
+ {bgp_ignore, Deleted}, /* Receive_NOTIFICATION_message */
+ {bgp_ignore, Deleted}, /* Clearing_Completed */
},
};
@@ -1001,14 +1061,15 @@
"Receive_OPEN_message",
"Receive_KEEPALIVE_message",
"Receive_UPDATE_message",
- "Receive_NOTIFICATION_message"
+ "Receive_NOTIFICATION_message",
+ "Clearing_Completed",
};
/* Execute event process. */
int
bgp_event (struct thread *thread)
{
- int ret;
+ int ret = 0;
int event;
int next;
struct peer *peer;
@@ -1019,14 +1080,15 @@
/* Logging this event. */
next = FSM [peer->status -1][event - 1].next_state;
- if (BGP_DEBUG (fsm, FSM))
+ if (BGP_DEBUG (fsm, FSM) && peer->status != next)
plog_debug (peer->log, "%s [FSM] %s (%s->%s)", peer->host,
bgp_event_str[event],
LOOKUP (bgp_status_msg, peer->status),
LOOKUP (bgp_status_msg, next));
/* Call function. */
- ret = (*(FSM [peer->status - 1][event - 1].func))(peer);
+ if (FSM [peer->status -1][event - 1].func)
+ ret = (*(FSM [peer->status - 1][event - 1].func))(peer);
/* When function do not want proceed next job return -1. */
if (ret >= 0)