Skip to content

Commit

Permalink
Walk on deleted route table when VRF is deleted.
Browse files Browse the repository at this point in the history 
Problem:
When VRF is marked for delete, route tables are deleted from agent however the
pointer is not made NULL(for debugging it is retained). This happens via
Onzerorefcount, where vrf entry(db entry) delete is enqueued later. There can be
a walk going on Vrftable and since this Vrf is not yet deleted and is in queue,
it can be picked up. Once this happens further route table walks in this vrf
will be started. Since route table were deleted these pointer(table) will be
invalid resulting in crash.

Solution:
On route table walk identify if table is deleted and walk need not be done.
To fix use bitmap and reset it once table is deleted.

Change-Id: I3ca5e6eb0eb4fb54ec873242b57a4389448c5279
Closes-bug: 1471101
  • Loading branch information
@manishsing
manishsing committed Jul 16, 2015
1 parent e6e07d4 commit 436914e
Show file tree
Hide file tree
Showing 6 changed files with 79 additions and 54 deletions.
9 changes: 8 additions & 1 deletion src/vnsw/agent/oper/agent_route_walker.cc
View file
Expand Up @@ -167,7 +167,7 @@ void AgentRouteWalker::StartRouteWalk(VrfEntry *vrf) {

void AgentRouteWalker::StartRouteWalkInternal(const VrfEntry *vrf) {
DBTableWalker *walker = agent_->db()->GetWalker();
DBTableWalker::WalkId walkid;
DBTableWalker::WalkId walkid = DBTableWalker::kInvalidWalkerId;
uint32_t vrf_id = vrf->vrf_id();
AgentRouteTable *table = NULL;

Expand All @@ -180,6 +180,13 @@ void AgentRouteWalker::StartRouteWalkInternal(const VrfEntry *vrf) {
table_type++) {
table = static_cast<AgentRouteTable *>
(vrf->GetRouteTable(table_type));
if (table == NULL) {
AGENT_DBWALK_TRACE(AgentRouteWalkerTrace,
"Route table walk SKIPPED for vrf", walk_type_,
(vrf != NULL) ? vrf->GetName() : "Unknown",
vrf_walkid_, table_type, "", walkid);
continue;
}
walkid = walker->WalkTable(table, NULL,
boost::bind(&AgentRouteWalker::RouteWalkNotify,
this, _1, _2),
Expand Down
3 changes: 2 additions & 1 deletion src/vnsw/agent/oper/nexthop.cc
View file
Expand Up @@ -836,7 +836,8 @@ bool TunnelNH::Change(const DBRequest *req) {
void TunnelNH::Delete(const DBRequest *req) {
InetUnicastAgentRouteTable *rt_table =
(GetVrf()->GetInet4UnicastRouteTable());
rt_table->RemoveUnresolvedNH(this);
if (rt_table)
rt_table->RemoveUnresolvedNH(this);
}

void TunnelNH::SendObjectLog(AgentLogEvent::type event) const {
Expand Down
86 changes: 39 additions & 47 deletions src/vnsw/agent/oper/vrf.cc
View file
Expand Up @@ -103,44 +103,41 @@ void VrfEntry::CreateTableLabel() {
}
}

void VrfEntry::CreateRouteTables() {
DB *db = get_table()->database();
VrfTable *table = static_cast<VrfTable *>(get_table());

for (uint8_t type = (Agent::INVALID + 1); type < Agent::ROUTE_TABLE_MAX; type++) {
rt_table_db_[type] = static_cast<AgentRouteTable *>
(db->CreateTable(name_ +
AgentRouteTable::GetSuffix(Agent::RouteTableType(type))));
rt_table_db_[type]->SetVrf(this);
table->dbtree_[type].insert(VrfTable::VrfDbPair(name_, rt_table_db_[type]));
}
}

void VrfEntry::DeleteRouteTables() {
for (int table_type = (Agent::INVALID + 1);
table_type < Agent::ROUTE_TABLE_MAX; table_type++) {
VrfTable *vrf_table = ((VrfTable *) get_table());
AgentRouteTable *route_table =
vrf_table->GetRouteTable(name_, table_type);
if (route_table) {
vrf_table->DeleteFromDbTree(table_type, name_);
vrf_table->database()->RemoveTable(route_table);
delete route_table;
}
}
}

void VrfEntry::PostAdd() {
VrfTable *table = static_cast<VrfTable *>(get_table());
Agent *agent = table->agent();
// get_table() would return NULL in Add(), so move dependent functions and
// initialization to PostAdd
deleter_.reset(new DeleteActor(this));
// Create the route-tables and insert them into dbtree_
Agent::RouteTableType type = Agent::INET4_UNICAST;
DB *db = get_table()->database();
rt_table_db_[type] = static_cast<AgentRouteTable *>
(db->CreateTable(name_ + AgentRouteTable::GetSuffix(type)));
rt_table_db_[type]->SetVrf(this);
table->dbtree_[type].insert(VrfTable::VrfDbPair(name_, rt_table_db_[type]));

type = Agent::INET4_MULTICAST;
rt_table_db_[type] = static_cast<AgentRouteTable *>
(db->CreateTable(name_ + AgentRouteTable::GetSuffix(type)));
rt_table_db_[type]->SetVrf(this);
table->dbtree_[type].insert(VrfTable::VrfDbPair(name_, rt_table_db_[type]));

type = Agent::EVPN;
rt_table_db_[type] = static_cast<AgentRouteTable *>
(db->CreateTable(name_ + AgentRouteTable::GetSuffix(type)));
rt_table_db_[type]->SetVrf(this);
((VrfTable *)get_table())->dbtree_[type].insert(VrfTable::VrfDbPair(name_,
rt_table_db_[type]));

type = Agent::BRIDGE;
rt_table_db_[type] = static_cast<AgentRouteTable *>
(db->CreateTable(name_ + AgentRouteTable::GetSuffix(type)));
rt_table_db_[type]->SetVrf(this);
table->dbtree_[type].insert(VrfTable::VrfDbPair(name_, rt_table_db_[type]));

type = Agent::INET6_UNICAST;
rt_table_db_[type] = static_cast<AgentRouteTable *>
(db->CreateTable(name_ + AgentRouteTable::GetSuffix(type)));
rt_table_db_[type]->SetVrf(this);
table->dbtree_[type].insert(VrfTable::VrfDbPair(name_, rt_table_db_[type]));
CreateRouteTables();

uint32_t vxlan_id = VxLanTable::kInvalidvxlan_id;
if (vn_) {
Expand Down Expand Up @@ -238,29 +235,27 @@ void VrfEntry::SetRouteTableDeleted(uint8_t table_type) {
}

AgentRouteTable *VrfEntry::GetRouteTable(uint8_t table_type) const {
return rt_table_db_[table_type];
return (RouteTableDeleted(table_type) ? NULL : rt_table_db_[table_type]);
}

InetUnicastAgentRouteTable *VrfEntry::GetInet4UnicastRouteTable() const {
return static_cast<InetUnicastAgentRouteTable *>
(rt_table_db_[Agent::INET4_UNICAST]);
return static_cast<InetUnicastAgentRouteTable *>(GetRouteTable(Agent::INET4_UNICAST));
}

AgentRouteTable *VrfEntry::GetInet4MulticastRouteTable() const {
return rt_table_db_[Agent::INET4_MULTICAST];
return GetRouteTable(Agent::INET4_MULTICAST);
}

AgentRouteTable *VrfEntry::GetEvpnRouteTable() const {
return rt_table_db_[Agent::EVPN];
return GetRouteTable(Agent::EVPN);
}

AgentRouteTable *VrfEntry::GetBridgeRouteTable() const {
return rt_table_db_[Agent::BRIDGE];
return GetRouteTable(Agent::BRIDGE);
}

InetUnicastAgentRouteTable *VrfEntry::GetInet6UnicastRouteTable() const {
return static_cast<InetUnicastAgentRouteTable *>
(rt_table_db_[Agent::INET6_UNICAST]);
return static_cast<InetUnicastAgentRouteTable *>(GetRouteTable(Agent::INET6_UNICAST));
}

bool VrfEntry::DBEntrySandesh(Sandesh *sresp, std::string &name) const {
Expand Down Expand Up @@ -464,14 +459,7 @@ void VrfTable::VrfReuse(const std::string name) {
void VrfTable::OnZeroRefcount(AgentDBEntry *e) {
VrfEntry *vrf = static_cast<VrfEntry *>(e);
if (e->IsDeleted()) {
int table_type;
for (table_type = (Agent::INVALID + 1);
table_type < Agent::ROUTE_TABLE_MAX; table_type++) {
database()->RemoveTable(vrf->GetRouteTable(table_type));
dbtree_[table_type].erase(vrf->GetName());
delete vrf->GetRouteTable(table_type);
}

vrf->DeleteRouteTables();
name_tree_.erase(vrf->GetName());
vrf->CancelDeleteTimer();
}
Expand Down Expand Up @@ -589,6 +577,10 @@ void VrfTable::DeleteStaticVrf(const string &name) {
DeleteVrfReq(name);
}

void VrfTable::DeleteFromDbTree(int table_type, const std::string &vrf_name) {
dbtree_[table_type].erase(vrf_name);
}

void VrfTable::Input(DBTablePartition *partition, DBClient *client,
DBRequest *req) {

Expand Down
6 changes: 5 additions & 1 deletion src/vnsw/agent/oper/vrf.h
View file
Expand Up @@ -111,12 +111,15 @@ class VrfEntry : AgentRefCount<VrfEntry>, public AgentDBEntry {
InetUnicastAgentRouteTable *GetInet6UnicastRouteTable() const;
AgentRouteTable *GetRouteTable(uint8_t table_type) const;
void CreateTableLabel();

bool AllRouteTableDeleted() const;
bool RouteTableDeleted(uint8_t table_type) const;
void SetRouteTableDeleted(uint8_t table_type);
void DeleteRouteTables();

private:
friend class VrfTable;
void CreateRouteTables();

class DeleteActor;
string name_;
uint32_t id_;
Expand Down Expand Up @@ -210,6 +213,7 @@ class VrfTable : public AgentDBTable {

void DeleteRoutes();
void Shutdown();
void DeleteFromDbTree(int table_type, const std::string &vrf_name);
private:
friend class VrfEntry;

Expand Down
2 changes: 1 addition & 1 deletion src/vnsw/agent/test/SConscript
View file
Expand Up @@ -114,7 +114,7 @@ test_global_vrouter_config = AgentEnv.MakeTestCmd(env,
agent_suite)
test_tunnel_encap = AgentEnv.MakeTestCmd(env, 'test_tunnel_encap', flaky_agent_suite)
test_agent_route_walker = AgentEnv.MakeTestCmd(env, 'test_agent_route_walker',
flaky_agent_suite)
agent_suite)
test_xmpp_hv = AgentEnv.MakeTestCmd(env, 'test_xmpp_hv', flaky_agent_suite)
test_scale_walk = AgentEnv.MakeTestCmd(env, 'test_scale_walk', flaky_agent_suite)
service_instance_test = AgentEnv.MakeTestCmd(env, 'service_instance_test',
Expand Down
27 changes: 24 additions & 3 deletions src/vnsw/agent/test/test_agent_route_walker.cc
View file
Expand Up @@ -189,6 +189,7 @@ class AgentRouteWalkerTest : public AgentRouteWalker, public ::testing::Test {
AgentRouteWalker::VrfWalkDone(part);
if (is_vrf_walk_done_ &&
!route_table_walk_started_ &&
(queued_walk_done_count() == AgentRouteWalker::kInvalidWalkCount) &&
AreAllWalksDone())
assert(0);
}
Expand Down Expand Up @@ -250,6 +251,9 @@ class SetupTask : public Task {
test_->WalkDoneCallback(boost::bind(&SetupTask::AllWalkDone,
test_));
test_->StartVrfWalk();
} else if (test_name_ ==
"walk_on_deleted_vrf_with_deleted_route_table") {
test_->StartVrfWalk();
}
return true;
}
Expand All @@ -272,7 +276,7 @@ TEST_F(AgentRouteWalkerTest, walk_all_routes_wih_1_vrf) {
client->Reset();
SetupEnvironment(1);
StartVrfWalk();
VerifyNotifications(17, 2, 1, ((Agent::ROUTE_TABLE_MAX - 1) * 2));
VerifyNotifications(19, 2, 1, ((Agent::ROUTE_TABLE_MAX - 1) * 2));
EXPECT_TRUE(walk_task_context_mismatch_ == false);
walk_task_context_mismatch_ = true;
DeleteEnvironment(1);
Expand All @@ -282,7 +286,7 @@ TEST_F(AgentRouteWalkerTest, walk_all_routes_with_2_vrf) {
client->Reset();
SetupEnvironment(2);
StartVrfWalk();
VerifyNotifications(26, 3, 1, ((Agent::ROUTE_TABLE_MAX - 1) * 3));
VerifyNotifications(30, 3, 1, ((Agent::ROUTE_TABLE_MAX - 1) * 3));
EXPECT_TRUE(walk_task_context_mismatch_ == false);
walk_task_context_mismatch_ = true;
DeleteEnvironment(2);
Expand All @@ -292,7 +296,7 @@ TEST_F(AgentRouteWalkerTest, walk_all_routes_with_3_vrf) {
client->Reset();
SetupEnvironment(3);
StartVrfWalk();
VerifyNotifications(35, 4, 1, ((Agent::ROUTE_TABLE_MAX - 1) * 4));
VerifyNotifications(41, 4, 1, ((Agent::ROUTE_TABLE_MAX - 1) * 4));
EXPECT_TRUE(walk_task_context_mismatch_ == false);
walk_task_context_mismatch_ = true;
DeleteEnvironment(3);
Expand Down Expand Up @@ -333,6 +337,23 @@ TEST_F(AgentRouteWalkerTest, vrf_state_deleted) {
DeleteEnvironment(1);
}

TEST_F(AgentRouteWalkerTest, walk_on_deleted_vrf_with_deleted_route_table) {
client->Reset();
SetupEnvironment(1);
SetupTask * task = new SetupTask(this,
"walk_on_deleted_vrf_with_deleted_route_table");
VrfEntryRef vrf = VrfGet("vrf1");
EXPECT_TRUE(vrf != NULL);
DeleteEnvironment(1);
EXPECT_TRUE(vrf->IsDeleted());
Agent::GetInstance()->vrf_table()->OnZeroRefcount(vrf.get());
TaskScheduler::GetInstance()->Enqueue(task);
WAIT_FOR(1000, 1000, IsWalkCompleted() == true);
SetupEnvironment(1);
vrf = NULL;
DeleteEnvironment(1);
}

//TODO REMAINING TESTS
// - based on walktype - unicast/multicast/all
//
Expand Down

0 comments on commit 436914e

Please sign in to comment.