From 3fcb630f60f39992a6ef7d6708abf56048c6ad63 Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Wed, 26 Aug 2015 17:26:35 -0700 Subject: [PATCH] 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 --- src/bgp/test/bgp_server_test_util.cc | 1 + src/bgp/test/bgp_server_test_util.h | 13 ++++++ src/bgp/test/bgp_xmpp_basic_test.cc | 51 +++++++++++++++++++++ src/control-node/test/network_agent_mock.cc | 12 +++++ src/control-node/test/network_agent_mock.h | 2 + src/xmpp/test/xmpp_client_auth_sm_test.cc | 2 +- src/xmpp/test/xmpp_client_sm_test.cc | 36 ++++++++++++++- src/xmpp/xmpp_connection.cc | 8 ++++ src/xmpp/xmpp_connection.h | 2 + src/xmpp/xmpp_state_machine.cc | 9 +++- src/xmpp/xmpp_state_machine.h | 9 +++- 11 files changed, 139 insertions(+), 6 deletions(-) diff --git a/src/bgp/test/bgp_server_test_util.cc b/src/bgp/test/bgp_server_test_util.cc index 3e393bc0ed4..c3ff2407f02 100644 --- 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 index 9f5b1b513e7..f68990a0266 100644 --- 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 index 00abbf53eb9..d27df3b318a 100644 --- 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 index cdbc7c5c474..a80b73350ec 100644 --- a/src/control-node/test/network_agent_mock.cc +++ b/src/control-node/test/network_agent_mock.cc @@ -878,6 +878,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 index deaadad4ee0..2b5f086d223 100644 --- 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 index 411c5131cea..ff5a1b586fa 100644 --- 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 index 892e94953ab..496838975e8 100644 --- 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 index d797ec16551..75ed39bad63 100644 --- 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 index fe9ca64656f..135de092dc4 100644 --- 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 index 9be89a8269d..acba4cfd1b8 100644 --- a/src/xmpp/xmpp_state_machine.cc +++ b/src/xmpp/xmpp_state_machine.cc @@ -264,6 +264,7 @@ struct Active : public sc::state { Active(my_context ctx) : my_base(ctx) { XmppStateMachine *state_machine = &context(); 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(); 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 index 9b3fbbe92c5..c0f3c31d772 100644 --- 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_;