Skip to content

Commit

Permalink
Remove redundant CloseInProgress logic from XmppChannelMux class
Browse files Browse the repository at this point in the history
Currently, client such as IFMap registers and unregisters from the XmppChannel
when the READY/NOT_READY notifications are received. BGP also receives these
notifications and handles them to update its data structures.

During GR Helper mode, we can use these notifications (Unregister) from the
client to figure out when it is ready to restart xmpp state machine..
Instead, we use separate APIs which is unnecessary and also does not work
correctly when clients behave differently (such as ifmap and bgp)

Only after all registers clients have unregistered can we go ahead and restart
xmpp state machine for GR to resume.

Also update json to file build agent unit tests when xmpp code is modified.
Agent is an XmppClient and uses Xmpp code in order to communicate with
contrail-control over xmpp

Change-Id: I77521eaf836043a4f1813744072e50edcdbafef8
Closes-Bug: 1646614
  • Loading branch information
ananth-at-camphor-networks committed Dec 2, 2016
1 parent 52c033d commit 9da3c06
Show file tree
Hide file tree
Showing 10 changed files with 23 additions and 59 deletions.
1 change: 1 addition & 0 deletions ci_unittests.json
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@
"source_directories" : [
"controller/src/ksync",
"controller/src/vnsw",
"controller/src/xmpp",
"vrouter"
],
"scons_test_targets" : [
Expand Down
3 changes: 3 additions & 0 deletions src/bgp/bgp_xmpp_channel.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2698,6 +2698,9 @@ void BgpXmppChannelManager::XmppHandleChannelEvent(XmppChannel *channel,
bgp_xmpp_channel = (*it).second;
if (bgp_xmpp_channel->peer_deleted())
return;
channel->RegisterReceive(xmps::BGP,
boost::bind(&BgpXmppChannel::ReceiveUpdate, bgp_xmpp_channel,
_1));
}

bgp_xmpp_channel->eor_sent_ = false;
Expand Down
3 changes: 1 addition & 2 deletions src/bgp/bgp_xmpp_peer_close.cc
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ void BgpXmppPeerClose::CloseComplete() {
channel_->set_peer_closed(false);

// Indicate to Channel that GR Closure is now complete
channel_->channel()->CloseComplete();
channel_->channel()->UnRegisterReceive(xmps::BGP);
}

void BgpXmppPeerClose::Delete() {
Expand All @@ -128,7 +128,6 @@ void BgpXmppPeerClose::Delete() {
void BgpXmppPeerClose::Close(bool graceful) {
if (channel_) {
assert(channel_->peer_deleted());
assert(channel_->channel()->IsCloseInProgress());
if (!IsCloseGraceful())
graceful = false;
GetManager()->Close(graceful);
Expand Down
1 change: 0 additions & 1 deletion src/bgp/test/bgp_xmpp_channel_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ class XmppChannelMock : public XmppChannel {
virtual ~XmppChannelMock() { }
void Close() { }
void CloseComplete() { }
bool IsCloseInProgress() const { return false; }
bool Send(const uint8_t *, size_t, xmps::PeerId, SendReadyCb) {
return true;
}
Expand Down
1 change: 0 additions & 1 deletion src/bgp/test/bgp_xmpp_parse_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ class XmppChannelMock : public XmppChannel {
virtual ~XmppChannelMock() { }
void Close() { }
void CloseComplete() { }
bool IsCloseInProgress() const { return false; }
bool Send(const uint8_t *, size_t, xmps::PeerId, SendReadyCb) {
return true;
}
Expand Down
2 changes: 0 additions & 2 deletions src/vnsw/agent/test/test_cmn_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -482,8 +482,6 @@ class XmppChannelMock : public XmppChannel {
return true;
}
void Close() { }
void CloseComplete() { }
bool IsCloseInProgress() const { return false; }
int GetTaskInstance() const { return 0; }
MOCK_METHOD2(RegisterReceive, void(xmps::PeerId, ReceiveCb));
MOCK_METHOD1(UnRegisterReceive, void(xmps::PeerId));
Expand Down
2 changes: 0 additions & 2 deletions src/xmpp/xmpp_channel.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,6 @@ class XmppChannel {
virtual void RegisterTxMessageTraceCallback(TxMessageTraceCb cb) = 0;
virtual void UnRegisterWriteReady(xmps::PeerId id) = 0;
virtual void Close() = 0;
virtual void CloseComplete() = 0;
virtual bool IsCloseInProgress() const = 0;
virtual const std::string &ToString() const = 0;
virtual const std::string &FromString() const = 0;
virtual std::string StateName() const = 0;
Expand Down
61 changes: 16 additions & 45 deletions src/xmpp/xmpp_channel_mux.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ using namespace xmsm;
XmppChannelMux::XmppChannelMux(XmppConnection *connection)
: connection_(connection), rx_message_trace_cb_(NULL),
tx_message_trace_cb_(NULL) {
closing_count_ = 0;
last_received_ = 0;
last_sent_ = 0;
}
Expand All @@ -24,36 +23,7 @@ XmppChannelMux::~XmppChannelMux() {
}

void XmppChannelMux::Close() {
if (InitializeClosingCount())
connection_->Clear();
}

// Track clients who close gracefully. At the moment, only BGP cares about this.
bool XmppChannelMux::InitializeClosingCount() {
if (closing_count_)
return false;

BOOST_FOREACH(const ReceiveCbMap::value_type &value, rxmap_) {
switch (value.first) {

// Currently, Only BgpXmppChannel client cares about GR.
case xmps::BGP:
closing_count_++;
break;

case xmps::CONFIG:
case xmps::DNS:
case xmps::OTHER:
break;
}
}

return true;
}

// Check if the channel is being closed (Graceful Restart)
bool XmppChannelMux::IsCloseInProgress() const {
return closing_count_ != 0;
connection_->Clear();
}

bool XmppChannelMux::LastReceived(uint64_t durationMsec) const {
Expand All @@ -64,18 +34,6 @@ bool XmppChannelMux::LastSent(uint64_t durationMsec) const {
return (UTCTimestampUsec() - last_sent_) <= durationMsec * 1000;
}

// API for the clients to indicate GR Closure is complete
void XmppChannelMux::CloseComplete() {
assert(closing_count_);
closing_count_--;
if (closing_count_)
return;

// Restart state machine.
if (connection() && connection()->state_machine())
connection()->state_machine()->Initialize();
}

xmps::PeerState XmppChannelMux::GetPeerState() const {
xmsm::XmState st = connection_->GetStateMcState();
return (st == xmsm::ESTABLISHED) ? xmps::READY :
Expand Down Expand Up @@ -122,6 +80,21 @@ void XmppChannelMux::UnRegisterReceive(xmps::PeerId id) {
if (it != rxmap_.end()) {
rxmap_.erase(it);
}

if (ReceiverCount())
return;

XmppServerConnection *server_connection =
dynamic_cast<XmppServerConnection *>(connection_);

// If GracefulRestart helper mode close process is complete, restart the
// state machine to form new session with the client.
if (!connection_->IsDeleted() && server_connection &&
server_connection->server()->IsGRHelperModeEnabled()) {
server_connection->state_machine()->Initialize();
return;
}

connection_->RetryDelete();
}

Expand Down Expand Up @@ -217,8 +190,6 @@ void XmppChannelMux::HandleStateEvent(xmsm::XmState state) {
} else {
// Event to create the peer on server
XmppServer *server = static_cast<XmppServer *>(connection_->server());
if (st == xmps::NOT_READY)
(void) InitializeClosingCount();
server->NotifyConnectionEvent(this, st);
}
}
Expand Down
4 changes: 0 additions & 4 deletions src/xmpp/xmpp_channel_mux.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ class XmppChannelMux : public XmppChannel {
virtual ~XmppChannelMux();

virtual void Close();
virtual bool IsCloseInProgress() const;
virtual void CloseComplete();
virtual bool Send(const uint8_t *msg, size_t msg_size, xmps::PeerId id,
SendReadyCb cb) {
return Send(msg, msg_size, NULL, id, cb);
Expand Down Expand Up @@ -80,7 +78,6 @@ class XmppChannelMux : public XmppChannel {

private:
void RegisterWriteReady(xmps::PeerId, SendReadyCb);
bool InitializeClosingCount();

typedef std::map<xmps::PeerId, SendReadyCb> WriteReadyCbMap;
typedef std::map<xmps::PeerId, ReceiveCb> ReceiveCbMap;
Expand All @@ -92,7 +89,6 @@ class XmppChannelMux : public XmppChannel {
tbb::mutex mutex_;
RxMessageTraceCb rx_message_trace_cb_;
TxMessageTraceCb tx_message_trace_cb_;
tbb::atomic<int> closing_count_;
tbb::atomic<uint64_t> last_received_;
tbb::atomic<uint64_t> last_sent_;
};
Expand Down
4 changes: 2 additions & 2 deletions src/xmpp/xmpp_state_machine.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1381,14 +1381,14 @@ void XmppStateMachine::ProcessStreamHeaderMessage(XmppSession *session,
// check if older connection is under graceful-restart.
if (endpoint && endpoint->connection()) {
if (connection_ != endpoint->connection()) {
XmppChannel *channel = endpoint->connection()->ChannelMux();
XmppChannelMux *channel = endpoint->connection()->ChannelMux();

// If GR is not supported, then close all new connections until old
// one is completely deleted. Even if GR is supported, new
// connection cannot be accepted until old one is fully cleaned up.
bool ready = channel->GetPeerState() == xmps::READY;
if (!xmpp_server->IsPeerCloseGraceful() || ready ||
channel->IsCloseInProgress()) {
channel->ReceiverCount()) {

// Bring down old session if it is still in ESTABLISHED state.
// This is the scenario in which old session's TCP did not learn
Expand Down

0 comments on commit 9da3c06

Please sign in to comment.