Skip to content

Commit

Permalink
Merge "Concurrency issue in DBState management" into R2.22.x
Browse files Browse the repository at this point in the history
  • Loading branch information
@opencontrail-ci-admin
Zuul authored and opencontrail-ci-admin committed Mar 22, 2016
2 parents 2fe5825 + 707d7c3 commit b9f7a4a
Show file tree
Hide file tree
Showing 4 changed files with 163 additions and 12 deletions.
142 changes: 142 additions & 0 deletions src/bgp/test/bgp_xmpp_inetvpn_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,61 @@
using namespace std;
using boost::assign::list_of;

static const char *config_2_control_nodes_4vns = "\
<config>\
<bgp-router name=\'X\'>\
<identifier>192.168.0.1</identifier>\
<address>127.0.0.1</address>\
<port>%d</port>\
<session to=\'Y\'>\
<address-families>\
<family>route-target</family>\
<family>inet-vpn</family>\
</address-families>\
</session>\
</bgp-router>\
<bgp-router name=\'Y\'>\
<identifier>192.168.0.2</identifier>\
<address>127.0.0.2</address>\
<port>%d</port>\
<session to=\'X\'>\
<address-families>\
<family>route-target</family>\
<family>inet-vpn</family>\
</address-families>\
</session>\
</bgp-router>\
<virtual-network name='blue'>\
<network-id>1</network-id>\
</virtual-network>\
<routing-instance name='blue'>\
<virtual-network>blue</virtual-network>\
<vrf-target>target:1:1</vrf-target>\
</routing-instance>\
<virtual-network name='pink'>\
<network-id>1</network-id>\
</virtual-network>\
<routing-instance name='pink'>\
<virtual-network>pink</virtual-network>\
<vrf-target>target:1:2</vrf-target>\
</routing-instance>\
<virtual-network name='green'>\
<network-id>1</network-id>\
</virtual-network>\
<routing-instance name='green'>\
<virtual-network>green</virtual-network>\
<vrf-target>target:1:3</vrf-target>\
</routing-instance>\
<virtual-network name='black'>\
<network-id>1</network-id>\
</virtual-network>\
<routing-instance name='black'>\
<virtual-network>black</virtual-network>\
<vrf-target>target:1:4</vrf-target>\
</routing-instance>\
</config>\
";

static const char *config_2_control_nodes = "\
<config>\
<bgp-router name=\'X\'>\
Expand Down Expand Up @@ -224,6 +279,22 @@ class BgpXmppInetvpn2ControlNodeTest : public ::testing::Test {
"instance-target");
}

void AddConnection(BgpServerTestPtr server,
const string &name1, const string &name2) {
ifmap_test_util::IFMapMsgLink(server->config_db(),
"routing-instance", name1,
"routing-instance", name2,
"connection");
}

void RemoveConnection(BgpServerTestPtr server,
const string &name1, const string &name2) {
ifmap_test_util::IFMapMsgUnlink(server->config_db(),
"routing-instance", name1,
"routing-instance", name2,
"connection");
}

size_t GetExportRouteTargetListSize(BgpServerTestPtr server,
const string &instance) {
TASK_UTIL_EXPECT_NE(static_cast<RoutingInstance *>(NULL),
Expand Down Expand Up @@ -631,6 +702,77 @@ TEST_F(BgpXmppInetvpn2ControlNodeTest, RouteFlap2) {
agent_b_->SessionDown();
}

//
// Agent flaps routes with same prefix on two connected VRFs repeatedly.
//
TEST_F(BgpXmppInetvpn2ControlNodeTest, RouteFlap_ConnectedVRF) {
Configure(config_2_control_nodes_4vns);
AddConnection(bs_x_, "blue", "pink");
AddConnection(bs_x_, "blue", "green");
AddConnection(bs_x_, "pink", "black");

AddConnection(bs_y_, "blue", "pink");
AddConnection(bs_y_, "blue", "green");
AddConnection(bs_y_, "pink", "black");
task_util::WaitForIdle();

// Create XMPP Agent A connected to XMPP server X.
agent_a_.reset(
new test::NetworkAgentMock(&evm_, "agent-a", xs_x_->GetPort(),
"127.0.0.1", "127.0.0.1"));
TASK_UTIL_EXPECT_TRUE(agent_a_->IsEstablished());

// Create XMPP Agent B connected to XMPP server Y.
agent_b_.reset(
new test::NetworkAgentMock(&evm_, "agent-b", xs_y_->GetPort(),
"127.0.0.2", "127.0.0.2"));
TASK_UTIL_EXPECT_TRUE(agent_b_->IsEstablished());

// Register to blue instance
agent_a_->Subscribe("blue", 1);
agent_b_->Subscribe("blue", 1);
agent_a_->Subscribe("pink", 2);
agent_b_->Subscribe("pink", 2);
agent_a_->Subscribe("green", 3);
agent_b_->Subscribe("green", 3);
agent_a_->Subscribe("black", 4);
agent_b_->Subscribe("black", 4);

// Add and delete route from agent A repeatedly.
stringstream route_a;
route_a << "10.1.1.1/32";
for (int idx = 0; idx < 64; ++idx) {
agent_a_->AddRoute("blue", route_a.str(), "192.168.1.1");
agent_a_->AddRoute("pink", route_a.str(), "192.168.1.1");
usleep(500);
agent_a_->DeleteRoute("blue", route_a.str());
agent_a_->DeleteRoute("pink", route_a.str());
}

// Verify that route is deleted at agents A and B.
VerifyRouteNoExists(agent_a_, "blue", route_a.str());
VerifyRouteNoExists(agent_b_, "blue", route_a.str());
VerifyRouteNoExists(agent_a_, "pink", route_a.str());
VerifyRouteNoExists(agent_b_, "pink", route_a.str());


agent_a_->Unsubscribe("blue");
agent_b_->Unsubscribe("blue");
agent_a_->Unsubscribe("pink");
agent_b_->Unsubscribe("pink");
agent_a_->Unsubscribe("green");
agent_b_->Unsubscribe("green");
agent_a_->Unsubscribe("black");
agent_b_->Unsubscribe("black");

// Close the sessions.
agent_a_->SessionDown();
agent_b_->SessionDown();

TASK_UTIL_EXPECT_FALSE(agent_a_->IsEstablished());
TASK_UTIL_EXPECT_FALSE(agent_b_->IsEstablished());
}

//
// Multiple routes are exchanged correctly.
//
Expand Down
15 changes: 9 additions & 6 deletions src/db/db_entry.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,13 +74,12 @@ void DBEntryBase::ClearState(DBTableBase *tbl_base, ListenerId listener) {
DBTablePartBase *tpart = tbl_base->GetTablePartition(this);
tbb::mutex::scoped_lock lock(tpart->dbstate_mutex());

if (state_.erase(listener) != 0) {
// Account for state removal for this listener.
tbl_base->AddToDBStateCount(listener, -1);
}
assert(state_.erase(listener) != 0);

// Account for state removal for this listener.
tbl_base->AddToDBStateCount(listener, -1);

if (state_.empty() && IsDeleted() && !is_onlist()) {
assert(!IsOnRemoveQ());
if (state_.empty() && IsDeleted() && !is_onlist() && !IsOnRemoveQ()) {
tbl_base->EnqueueRemove(this);
}
}
Expand All @@ -90,6 +89,10 @@ bool DBEntryBase::is_state_empty(DBTablePartBase *tpart) {
return state_.empty();
}

bool DBEntryBase::is_state_empty_unlocked(DBTablePartBase *tpart) {
return state_.empty();
}

void DBEntryBase::set_last_change_at_to_now() {
last_change_at_ = UTCTimestampUsec();
}
Expand Down
1 change: 1 addition & 0 deletions src/db/db_entry.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ class DBEntryBase {
const DBState *GetState(const DBTableBase *tbl_base,
ListenerId listener) const;
bool is_state_empty(DBTablePartBase *tpart);
bool is_state_empty_unlocked(DBTablePartBase *tpart);

void MarkDelete() { flags |= DeleteMarked; }
void ClearDelete() { flags &= ~DeleteMarked; }
Expand Down
17 changes: 11 additions & 6 deletions src/db/db_partition.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,12 +169,17 @@ class DBPartition::QueueRunner : public Task {

RemoveQueueEntry *rm_entry = NULL;
while (queue_->DequeueRemove(&rm_entry)) {
if (rm_entry->db_entry->IsDeleted() &&
!rm_entry->db_entry->is_onlist() &&
rm_entry->db_entry->is_state_empty(rm_entry->tpart)) {
rm_entry->tpart->Remove(rm_entry->db_entry);
} else {
rm_entry->db_entry->ClearOnRemoveQ();
DBEntryBase *db_entry = rm_entry->db_entry;
{
tbb::mutex::scoped_lock lock(rm_entry->tpart->dbstate_mutex());
if (!db_entry->IsDeleted() || db_entry->is_onlist() ||
!db_entry->is_state_empty_unlocked(rm_entry->tpart)) {
db_entry->ClearOnRemoveQ();
db_entry = NULL;
}
}
if (db_entry) {
rm_entry->tpart->Remove(db_entry);
}
delete rm_entry;
if (++count == kMaxIterations) {
Expand Down

0 comments on commit b9f7a4a

Please sign in to comment.