Improve connection backoff logic in xmpp client

Do not reset the connection attempts counter immediately on reaching Established state. Do it only after 3 keepalives have been received. This ensures that the client backs off it's connection attempts even for cases where the state machine goes to Established, but then goes down right away. For example, when the server closes the connection because some higher level requirement is not satisfied. Change-Id: Ie6d68c1bef910faf3b7b6adc7a236127ffc9a0ea Closes-Bug: 1488636 Related-Bug: 1488278
Juniper · Aug 27, 2015 · 1cf76cd · 1cf76cd
1 parent 1c0d9ae
commit 1cf76cd
Show file tree

Hide file tree

Showing 11 changed files with 139 additions and 6 deletions.
diff --git a/src/bgp/test/bgp_server_test_util.cc b/src/bgp/test/bgp_server_test_util.cc
@@ -37,6 +37,7 @@ using namespace std;
 
 int StateMachineTest::hold_time_msecs_ = 0;
 int StateMachineTest::keepalive_time_msecs_ = 0;
+int XmppStateMachineTest::hold_time_msecs_ = 0;
 
 //
 // This is a static data structure that maps client tcp end points to configured

diff --git a/src/bgp/test/bgp_server_test_util.h b/src/bgp/test/bgp_server_test_util.h
@@ -306,6 +306,19 @@ class XmppStateMachineTest : public XmppStateMachine {
             boost::bind(&XmppStateMachine::OpenTimerExpired, this),
             boost::bind(&XmppStateMachine::TimerErrorHandler, this, _1, _2));
     }
+
+    virtual int hold_time_msecs() const {
+        if (hold_time_msecs_)
+            return hold_time_msecs_;
+        return XmppStateMachine::hold_time_msecs();
+    }
+
+    static void set_hold_time_msecs(int hold_time_msecs) {
+        hold_time_msecs_ = hold_time_msecs;
+    }
+
+private:
+    static int hold_time_msecs_;
 };
 
 class XmppLifetimeManagerTest : public XmppLifetimeManager {

diff --git a/src/bgp/test/bgp_xmpp_basic_test.cc b/src/bgp/test/bgp_xmpp_basic_test.cc
@@ -592,6 +592,57 @@ TEST_P(BgpXmppBasicParamTest, NoSelfBgpConfiguration2) {
     DestroyAgents();
 }
 
+TEST_P(BgpXmppBasicParamTest, ClientConnectionBackoff) {
+    XmppStateMachineTest::set_hold_time_msecs(300);
+    Unconfigure();
+    task_util::WaitForIdle();
+    TASK_UTIL_EXPECT_FALSE(bs_x_->HasSelfConfiguration());
+
+    CreateAgents();
+
+    uint32_t client_flap_a = agent_a_->flap_count();
+    uint32_t client_flap_b = agent_b_->flap_count();
+    uint32_t client_flap_c = agent_c_->flap_count();
+
+    size_t client_conn_attempts_a = agent_a_->get_sm_connect_attempts();
+    size_t client_conn_attempts_b = agent_b_->get_sm_connect_attempts();
+    size_t client_conn_attempts_c = agent_c_->get_sm_connect_attempts();
+
+    TASK_UTIL_EXPECT_TRUE(agent_a_->flap_count() > client_flap_a + 3);
+    TASK_UTIL_EXPECT_TRUE(agent_b_->flap_count() > client_flap_b + 3);
+    TASK_UTIL_EXPECT_TRUE(agent_c_->flap_count() > client_flap_c + 3);
+
+    TASK_UTIL_EXPECT_TRUE(
+        agent_a_->get_sm_connect_attempts() > client_conn_attempts_a + 3);
+    TASK_UTIL_EXPECT_TRUE(
+        agent_b_->get_sm_connect_attempts() > client_conn_attempts_b + 3);
+    TASK_UTIL_EXPECT_TRUE(
+        agent_c_->get_sm_connect_attempts() > client_conn_attempts_c + 3);
+
+    TASK_UTIL_EXPECT_TRUE(agent_a_->get_sm_keepalive_count() <= 1);
+    TASK_UTIL_EXPECT_TRUE(agent_b_->get_sm_keepalive_count() <= 1);
+    TASK_UTIL_EXPECT_TRUE(agent_c_->get_sm_keepalive_count() <= 1);
+
+    Configure(bgp_config_template, 64512, 64512);
+    task_util::WaitForIdle();
+    TASK_UTIL_EXPECT_TRUE(bs_x_->HasSelfConfiguration());
+
+    TASK_UTIL_EXPECT_TRUE(agent_a_->IsEstablished());
+    TASK_UTIL_EXPECT_TRUE(agent_b_->IsEstablished());
+    TASK_UTIL_EXPECT_TRUE(agent_c_->IsEstablished());
+
+    TASK_UTIL_EXPECT_TRUE(agent_a_->get_sm_keepalive_count() >= 3);
+    TASK_UTIL_EXPECT_TRUE(agent_b_->get_sm_keepalive_count() >= 3);
+    TASK_UTIL_EXPECT_TRUE(agent_c_->get_sm_keepalive_count() >= 3);
+
+    TASK_UTIL_EXPECT_EQ(0, agent_a_->get_sm_connect_attempts());
+    TASK_UTIL_EXPECT_EQ(0, agent_b_->get_sm_connect_attempts());
+    TASK_UTIL_EXPECT_EQ(0, agent_c_->get_sm_connect_attempts());
+
+    DestroyAgents();
+    XmppStateMachineTest::set_hold_time_msecs(0);
+}
+
 TEST_P(BgpXmppBasicParamTest, ShutdownServer1) {
 
     // Create agents and wait for them to come up.

diff --git a/src/control-node/test/network_agent_mock.cc b/src/control-node/test/network_agent_mock.cc
@@ -879,6 +879,18 @@ void NetworkAgentMock::SessionUp() {
         connection->SetAdminState(false);
 }
 
+size_t NetworkAgentMock::get_sm_connect_attempts() {
+    XmppConnection *connection =
+        client_->FindConnection("network-control@contrailsystems.com");
+    return (connection ? connection->get_sm_connect_attempts() : 0);
+}
+
+size_t NetworkAgentMock::get_sm_keepalive_count() {
+    XmppConnection *connection =
+        client_->FindConnection("network-control@contrailsystems.com");
+    return (connection ? connection->get_sm_keepalive_count() : 0);
+}
+
 size_t NetworkAgentMock::get_connect_error() {
     XmppConnection *connection =
         client_->FindConnection("network-control@contrailsystems.com");

diff --git a/src/control-node/test/network_agent_mock.h b/src/control-node/test/network_agent_mock.h
@@ -457,6 +457,8 @@ class NetworkAgentMock {
 
     bool ProcessRequest(Request *request);
 
+    size_t get_sm_connect_attempts();
+    size_t get_sm_keepalive_count();
     size_t get_connect_error();
     size_t get_session_close();
     uint32_t flap_count();

diff --git a/src/xmpp/test/xmpp_client_auth_sm_test.cc b/src/xmpp/test/xmpp_client_auth_sm_test.cc
@@ -168,7 +168,7 @@ class XmppStateMachineTest : public ::testing::Test {
             EXPECT_TRUE(HoldTimerRunning());
             EXPECT_TRUE(sm_->session() != NULL);
             EXPECT_TRUE(connection_->session() != NULL);
-            EXPECT_TRUE(sm_->GetConnectTime() == 0);
+            EXPECT_TRUE(sm_->get_connect_attempts() != 0);
             break;
         default:
             ASSERT_TRUE(false);

diff --git a/src/xmpp/test/xmpp_client_sm_test.cc b/src/xmpp/test/xmpp_client_sm_test.cc
@@ -135,7 +135,7 @@ class XmppStateMachineTest : public ::testing::Test {
             EXPECT_TRUE(HoldTimerRunning());
             EXPECT_TRUE(sm_->session() != NULL);
             EXPECT_TRUE(connection_->session() != NULL);
-            EXPECT_TRUE(sm_->GetConnectTime() == 0);
+            EXPECT_TRUE(sm_->get_connect_attempts() != 0);
             break;
         default:
             ASSERT_TRUE(false);
@@ -635,6 +635,40 @@ TEST_F(XmppStateMachineTest, Connect_EvTcpConnectFailed_EvTcpConnected) {
     VerifyState(xmsm::OPENSENT);
 }
 
+TEST_F(XmppStateMachineTest, ConnectionBackoff) {
+    sm_->connect_attempts_inc(); // set attempts as we do not
+                                 // want connection timer to expire
+    sm_->connect_attempts_inc(); // set attempts as we do not
+                                 // want connection timer to expire
+    VerifyState(xmsm::IDLE);
+    EvStart();
+    VerifyState(xmsm::ACTIVE);
+
+    for (int idx = 0; idx <= 6; ++idx) {
+        EvConnectTimerExpired();
+        VerifyState(xmsm::CONNECT);
+
+        EvTcpConnected();
+        VerifyState(xmsm::OPENSENT);
+
+        EvXmppOpen();
+        VerifyState(xmsm::ESTABLISHED);
+
+        if (idx == 6)
+            break;
+
+        EvTcpClose();
+        VerifyState(xmsm::ACTIVE);
+    }
+
+    TASK_UTIL_EXPECT_TRUE(sm_->get_connect_attempts() >= 6);
+    EvXmppKeepalive();
+    EvXmppKeepalive();
+    EvXmppKeepalive();
+    TASK_UTIL_EXPECT_EQ(3, sm_->get_keepalive_count());
+    TASK_UTIL_EXPECT_EQ(0, sm_->get_connect_attempts());
+}
+
 }
 
 static void SetUp() {

diff --git a/src/xmpp/xmpp_connection.cc b/src/xmpp/xmpp_connection.cc
@@ -487,6 +487,14 @@ size_t XmppConnection::get_handshake_failure() {
     return error_stats_.handshake_fail;
 }
 
+size_t XmppConnection::get_sm_connect_attempts() {
+    return state_machine_->get_connect_attempts();
+}
+
+size_t XmppConnection::get_sm_keepalive_count() {
+    return state_machine_->get_keepalive_count();
+}
+
 void XmppConnection::ReceiveMsg(XmppSession *session, const string &msg) {
     XmppStanza::XmppMessage *minfo = XmppDecode(msg);
 

diff --git a/src/xmpp/xmpp_connection.h b/src/xmpp/xmpp_connection.h
@@ -215,6 +215,8 @@ class XmppConnection {
     size_t get_open_fail();
     size_t get_stream_feature_fail();
     size_t get_handshake_failure();
+    size_t get_sm_connect_attempts();
+    size_t get_sm_keepalive_count();
 
     static const char *kAuthTypeNil;
     static const char *kAuthTypeTls;

diff --git a/src/xmpp/xmpp_state_machine.cc b/src/xmpp/xmpp_state_machine.cc
@@ -264,6 +264,7 @@ struct Active : public sc::state<Active, XmppStateMachine> {
     Active(my_context ctx) : my_base(ctx) {
         XmppStateMachine *state_machine = &context<XmppStateMachine>();
         SM_LOG(state_machine, "(Xmpp Active State)");
+        state_machine->keepalive_count_clear();
         bool flap = (state_machine->get_state() == ESTABLISHED);
         state_machine->set_state(ACTIVE);
         if (flap) {
@@ -951,7 +952,6 @@ struct XmppStreamEstablished :
     XmppStreamEstablished(my_context ctx) : my_base(ctx) {
         XmppStateMachine *state_machine = &context<XmppStateMachine>();
         SM_LOG(state_machine, "(XMPP Established)");
-        state_machine->connect_attempts_clear();
         XmppConnection *connection = state_machine->connection();
         state_machine->StartHoldTimer();
         state_machine->set_state(ESTABLISHED);
@@ -984,6 +984,10 @@ struct XmppStreamEstablished :
         if (event.session != state_machine->session()) {
             return discard_event();
         }
+        state_machine->keepalive_count_inc();
+        if (state_machine->get_keepalive_count() == 3) {
+            state_machine->connect_attempts_clear();
+        }
         state_machine->StartHoldTimer();
         return discard_event();
     }
@@ -1118,6 +1122,7 @@ XmppStateMachine::XmppStateMachine(XmppConnection *connection, bool active,
           TaskScheduler::GetInstance()->GetTaskId("xmpp::StateMachine"), 0)),
       hold_time_(GetConfiguredHoldTime()),
       attempts_(0),
+      keepalive_count_(0),
       deleted_(false),
       in_dequeue_(false),
       is_active_(active),
@@ -1479,7 +1484,7 @@ void XmppStateMachine::AssignSession() {
 const int XmppStateMachine::kConnectInterval;
 
 int XmppStateMachine::GetConnectTime() const {
-    int backoff = min(attempts_, 6);
+    int backoff = attempts_ > 6 ? 6 : attempts_;
     return std::min(backoff ? 1 << (backoff - 1) : 0, kConnectInterval);
 }
 

diff --git a/src/xmpp/xmpp_state_machine.h b/src/xmpp/xmpp_state_machine.h
@@ -141,7 +141,11 @@ class XmppStateMachine :
 
     void connect_attempts_inc() { attempts_++; }
     void connect_attempts_clear() { attempts_ = 0; }
-    int get_connect_attempts() { return attempts_; }
+    int get_connect_attempts() const { return attempts_; }
+
+    void keepalive_count_inc() { keepalive_count_++; }
+    void keepalive_count_clear() { keepalive_count_ = 0; }
+    int get_keepalive_count() const { return keepalive_count_; }
 
     int hold_time() const { return hold_time_; }
     virtual int hold_time_msecs() const { return hold_time_ * 1000; }
@@ -180,7 +184,8 @@ class XmppStateMachine :
     Timer *open_timer_;
     Timer *hold_timer_;
     int hold_time_;
-    int attempts_;
+    uint32_t attempts_;
+    uint32_t keepalive_count_;
     bool deleted_;
     bool in_dequeue_;
     bool is_active_;