Transition to graceful restart state on hold timer expiry if applicable

3 files changed, 70 insertions, 15 deletions

diff --git a/pkg/server/fsm.go b/pkg/server/fsm.go
index 39618aa5..b037c0f5 100644
--- a/pkg/server/fsm.go
+++ b/pkg/server/fsm.go
@@ -1788,17 +1788,20 @@ func (h *fsmHandler) established(ctx context.Context) (bgp.FSMState, *fsmStateRe
 			// it now.
 			h.outgoing.In() <- err
 			fsm.lock.RLock()
-			if s := fsm.pConf.GracefulRestart.State; s.Enabled &&
-				(s.NotificationEnabled && err.Type == fsmNotificationRecv ||
+			if s := fsm.pConf.GracefulRestart.State; s.Enabled {
+				if (s.NotificationEnabled && err.Type == fsmNotificationRecv) ||
+					(err.Type == fsmNotificationSent &&
+						err.BGPNotification.Body.(*bgp.BGPNotification).ErrorCode == bgp.BGP_ERROR_HOLD_TIMER_EXPIRED) ||
 					err.Type == fsmReadFailed ||
-					err.Type == fsmWriteFailed) {
-				err = *newfsmStateReason(fsmGracefulRestart, nil, nil)
-				log.WithFields(log.Fields{
-					"Topic": "Peer",
-					"Key":   fsm.pConf.State.NeighborAddress,
-					"State": fsm.state.String(),
-				}).Info("peer graceful restart")
-				fsm.gracefulRestartTimer.Reset(time.Duration(fsm.pConf.GracefulRestart.State.PeerRestartTime) * time.Second)
+					err.Type == fsmWriteFailed {
+					err = *newfsmStateReason(fsmGracefulRestart, nil, nil)
+					log.WithFields(log.Fields{
+						"Topic": "Peer",
+						"Key":   fsm.pConf.State.NeighborAddress,
+						"State": fsm.state.String(),
+					}).Info("peer graceful restart")
+					fsm.gracefulRestartTimer.Reset(time.Duration(fsm.pConf.GracefulRestart.State.PeerRestartTime) * time.Second)
+				}
 			}
 			fsm.lock.RUnlock()
 			return bgp.BGP_FSM_IDLE, &err
@@ -1812,7 +1815,15 @@ func (h *fsmHandler) established(ctx context.Context) (bgp.FSMState, *fsmStateRe
 			fsm.lock.RUnlock()
 			m := bgp.NewBGPNotificationMessage(bgp.BGP_ERROR_HOLD_TIMER_EXPIRED, 0, nil)
 			h.outgoing.In() <- &fsmOutgoingMsg{Notification: m}
-			return bgp.BGP_FSM_IDLE, newfsmStateReason(fsmHoldTimerExpired, m, nil)
+			fsm.lock.RLock()
+			s := fsm.pConf.GracefulRestart.State
+			fsm.lock.RUnlock()
+			// Do not return hold timer expired to server if graceful restart is enabled
+			// Let it fallback to read/write error or fsmNotificationSent handled above
+			// Reference: https://github.com/osrg/gobgp/issues/2174
+			if !s.Enabled {
+				return bgp.BGP_FSM_IDLE, newfsmStateReason(fsmHoldTimerExpired, m, nil)
+			}
 		case <-h.holdTimerResetCh:
 			fsm.lock.RLock()
 			if fsm.pConf.Timers.State.NegotiatedHoldTime != 0 {
diff --git a/pkg/server/fsm_test.go b/pkg/server/fsm_test.go
index 9351c71d..240f5185 100644
--- a/pkg/server/fsm_test.go
+++ b/pkg/server/fsm_test.go
@@ -244,9 +244,53 @@ func TestFSMHandlerEstablish_HoldTimerExpired(t *testing.T) {
 	p.fsm.pConf.Timers.State.NegotiatedHoldTime = 2
 
 	go pushPackets()
-	state, _ := h.established(context.Background())
+	state, fsmStateReason := h.established(context.Background())
 	time.Sleep(time.Second * 1)
 	assert.Equal(bgp.BGP_FSM_IDLE, state)
+	assert.Equal(fsmHoldTimerExpired, fsmStateReason.Type)
+	m.mtx.Lock()
+	lastMsg := m.sendBuf[len(m.sendBuf)-1]
+	m.mtx.Unlock()
+	sent, _ := bgp.ParseBGPMessage(lastMsg)
+	assert.Equal(uint8(bgp.BGP_MSG_NOTIFICATION), sent.Header.Type)
+	assert.Equal(uint8(bgp.BGP_ERROR_HOLD_TIMER_EXPIRED), sent.Body.(*bgp.BGPNotification).ErrorCode)
+}
+
+func TestFSMHandlerEstablish_HoldTimerExpired_GR_Enabled(t *testing.T) {
+	assert := assert.New(t)
+	m := NewMockConnection(t)
+
+	p, h := makePeerAndHandler()
+
+	// push mock connection
+	p.fsm.conn = m
+	p.fsm.h = h
+
+	// set keepalive ticker
+	p.fsm.pConf.Timers.State.NegotiatedHoldTime = 3
+
+	msg := keepalive()
+	header, _ := msg.Header.Serialize()
+	body, _ := msg.Body.Serialize()
+
+	pushPackets := func() {
+		// first keepalive from peer
+		m.setData(header)
+		m.setData(body)
+	}
+
+	// set holdtime
+	p.fsm.pConf.Timers.Config.HoldTime = 2
+	p.fsm.pConf.Timers.State.NegotiatedHoldTime = 2
+
+	// Enable graceful restart
+	p.fsm.pConf.GracefulRestart.State.Enabled = true
+
+	go pushPackets()
+	state, fsmStateReason := h.established(context.Background())
+	time.Sleep(time.Second * 1)
+	assert.Equal(bgp.BGP_FSM_IDLE, state)
+	assert.Equal(fsmGracefulRestart, fsmStateReason.Type)
 	m.mtx.Lock()
 	lastMsg := m.sendBuf[len(m.sendBuf)-1]
 	m.mtx.Unlock()
diff --git a/pkg/server/server.go b/pkg/server/server.go
index 4fbb76ff..fa0f5225 100644
--- a/pkg/server/server.go
+++ b/pkg/server/server.go
@@ -1380,7 +1380,7 @@ func (s *BgpServer) handleFSMMessage(peer *peer, e *fsmMsg) {
 							"Topic":  "Peer",
 							"Key":    peer.ID(),
 							"Family": family,
-						}).Debugf("start LLGR restart timer (%d sec) for %s", t, family)
+						}).Infof("start LLGR restart timer (%d sec) for %s", t, family)
 
 						select {
 						case <-timer.C:
@@ -1389,7 +1389,7 @@ func (s *BgpServer) handleFSMMessage(peer *peer, e *fsmMsg) {
 									"Topic":  "Peer",
 									"Key":    peer.ID(),
 									"Family": family,
-								}).Debugf("LLGR restart timer (%d sec) for %s expired", t, family)
+								}).Infof("LLGR restart timer (%d sec) for %s expired", t, family)
 								s.propagateUpdate(peer, peer.DropAll([]bgp.RouteFamily{family}))
 
 								// when all llgr restart timer expired, stop PeerRestarting
@@ -1403,7 +1403,7 @@ func (s *BgpServer) handleFSMMessage(peer *peer, e *fsmMsg) {
 								"Topic":  "Peer",
 								"Key":    peer.ID(),
 								"Family": family,
-							}).Debugf("stop LLGR restart timer (%d sec) for %s", t, family)
+							}).Infof("stop LLGR restart timer (%d sec) for %s", t, family)
 						}
 					}(f, endCh)
 				}