Skip to content

Commit

Permalink
Fix a corner case with routing instance delete
Browse files Browse the repository at this point in the history 
Sequence of event that causes the crash
   1. Static route config deleted
   2. Static Route maanger triggers resolve_trigger_ to re-evaluate static
      route config
   3. Before the resolve trigger is invoked routing instance is deleted

Resolve trigger calls ProcessStaticRouteConfig to apply any pending static
route config. ProcessStaticRouteConfig accesses the NULL config pointer of
the routing instance

Fix:
1. Check whether the routing instance is deleted in ProcessStaticRouteConfig
2. Reset the resolve_trigger_ in StaticRouteMgr destructor
3. Add API to disable resolve_trigger_ and Add UT to test delayed processing
of resolve_trigger_

Change-Id: Icb1b9bad340ccefc9fbab75188034ade79a6193a
Closes-bug: #1533435
  • Loading branch information
@bailkeri
bailkeri committed Jan 14, 2016
1 parent 17678d0 commit 861864b
Show file tree
Hide file tree
Showing 3 changed files with 148 additions and 0 deletions.
2 changes: 2 additions & 0 deletions src/bgp/routing-instance/static_route.cc
View file
Expand Up @@ -676,6 +676,7 @@ void StaticRouteMgr::RemoveStaticRoutePrefix(const Ip4Prefix &static_route) {

void StaticRouteMgr::ProcessStaticRouteConfig() {
CHECK_CONCURRENCY("bgp::Config");
if (routing_instance()->deleted() || !routing_instance()->config()) return;
const BgpInstanceConfig::StaticRouteList &list =
routing_instance()->config()->static_routes();
typedef BgpInstanceConfig::StaticRouteList::const_iterator iterator_t;
Expand All @@ -690,6 +691,7 @@ bool StaticRouteMgr::ResolvePendingStaticRouteConfig() {
}

StaticRouteMgr::~StaticRouteMgr() {
resolve_trigger_->Reset();
if (static_route_queue_)
delete static_route_queue_;
}
Expand Down
4 changes: 4 additions & 0 deletions src/bgp/routing-instance/static_route.h
View file
Expand Up @@ -75,6 +75,10 @@ class StaticRouteMgr {
RoutingInstance *instance_;
BgpConditionListener *listener_;
StaticRouteMap static_route_map_;

void DisableResolveTrigger() { resolve_trigger_->set_disable(); }
void EnableResolveTrigger() { resolve_trigger_->set_enable(); }

void DisableQueue() { static_route_queue_->set_disable(true); }
void EnableQueue() { static_route_queue_->set_disable(false); }
bool IsQueueEmpty() { return static_route_queue_->IsQueueEmpty(); }
Expand Down
142 changes: 142 additions & 0 deletions src/bgp/test/static_route_test.cc
View file
Expand Up @@ -153,6 +153,19 @@ class StaticRouteTest : public ::testing::Test {
return table->Size();
}

void DisableResolveTrigger(const string &instance_name) {
RoutingInstance *rtinstance =
ri_mgr_->GetRoutingInstance(instance_name);
rtinstance->static_route_mgr()->DisableResolveTrigger();
}

void EnableResolveTrigger(const string &instance_name) {
RoutingInstance *rtinstance =
ri_mgr_->GetRoutingInstance(instance_name);
if (rtinstance)
rtinstance->static_route_mgr()->EnableResolveTrigger();
}

void DisableStaticRouteQ(const string &instance_name) {
RoutingInstance *rtinstance =
ri_mgr_->GetRoutingInstance(instance_name);
Expand Down Expand Up @@ -1607,6 +1620,135 @@ TEST_F(StaticRouteTest, DeleteRoutingInstance) {
task_util::WaitForIdle();
}

//
// Delete the static route config and instance with resolve_trigger disabled
// Allow the routing instance to get deleted with Resolve trigger
//
TEST_F(StaticRouteTest, DeleteRoutingInstance_DisabledResolveTrigger) {
vector<string> instance_names = list_of("blue")("nat");
multimap<string, string> connections;
this->NetworkConfig(instance_names, connections);
task_util::WaitForIdle();

std::auto_ptr<autogen::StaticRouteEntriesType> params =
this->GetStaticRouteConfig(
"controller/src/bgp/testdata/static_route_1.xml");

ifmap_test_util::IFMapMsgPropertyAdd(&this->config_db_, "routing-instance",
"nat", "static-route-entries", params.release(), 0);
task_util::WaitForIdle();

// Add Nexthop Route
this->AddInetRoute(NULL, "nat", "192.168.1.254/32", 100, "2.3.4.5");
task_util::WaitForIdle();

// Check for Static route
TASK_UTIL_WAIT_NE_NO_MSG(this->InetRouteLookup("blue", "192.168.1.0/24"),
NULL, 1000, 10000, "Wait for Static route in blue..");

// Disable resolve trigger
this->DisableResolveTrigger("nat");

// Delete the configuration for the nat instance.
ifmap_test_util::IFMapMsgPropertyDelete(
&this->config_db_, "routing-instance",
"nat", "static-route-entries");

// Delete nexthop route
this->DeleteInetRoute(NULL, "nat", "192.168.1.254/32");
task_util::WaitForIdle();

// Check for Static route
TASK_UTIL_WAIT_EQ_NO_MSG(
this->InetRouteLookup("blue", "192.168.1.0/24"),
NULL, 1000, 10000, "Wait for Static route in blue..");

ifmap_test_util::IFMapMsgUnlink(
&this->config_db_, "routing-instance", "nat",
"virtual-network", "nat", "virtual-network-routing-instance");
ifmap_test_util::IFMapMsgUnlink(&this->config_db_, "routing-instance",
"nat", "route-target", "target:64496:2", "instance-target");
ifmap_test_util::IFMapMsgNodeDelete(
&this->config_db_, "virtual-network", "nat");
ifmap_test_util::IFMapMsgNodeDelete(
&this->config_db_, "routing-instance", "nat");
ifmap_test_util::IFMapMsgNodeDelete(
&this->config_db_, "route-target", "target:64496:2");
task_util::WaitForIdle();

this->EnableResolveTrigger("nat");
}

//
// Delete the static route config and instance with resolve_trigger disabled
// Routing instance is not destroyed when the task trigger is enabled.
// Verify that enabling the task trigger ignores the deleted routing instance
//
TEST_F(StaticRouteTest, DeleteRoutingInstance_DisabledResolveTrigger_1) {
vector<string> instance_names = list_of("blue")("nat");
multimap<string, string> connections;
this->NetworkConfig(instance_names, connections);
task_util::WaitForIdle();

std::auto_ptr<autogen::StaticRouteEntriesType> params =
this->GetStaticRouteConfig(
"controller/src/bgp/testdata/static_route_1.xml");

ifmap_test_util::IFMapMsgPropertyAdd(&this->config_db_, "routing-instance",
"nat", "static-route-entries", params.release(), 0);
task_util::WaitForIdle();

// Add Nexthop Route
this->AddInetRoute(NULL, "nat", "192.168.1.254/32", 100, "2.3.4.5");
task_util::WaitForIdle();

// Check for Static route
TASK_UTIL_WAIT_NE_NO_MSG(this->InetRouteLookup("blue", "192.168.1.0/24"),
NULL, 1000, 10000, "Wait for Static route in blue..");

// Disable resolve trigger
this->DisableResolveTrigger("nat");

// Delete the configuration for the nat instance.
ifmap_test_util::IFMapMsgPropertyDelete(
&this->config_db_, "routing-instance",
"nat", "static-route-entries");

// Check for Static route
TASK_UTIL_WAIT_EQ_NO_MSG(
this->InetRouteLookup("blue", "192.168.1.0/24"),
NULL, 1000, 10000, "Wait for Static route in blue..");

// Delete the nat routing instance
ifmap_test_util::IFMapMsgUnlink(
&this->config_db_, "routing-instance", "nat",
"virtual-network", "nat", "virtual-network-routing-instance");
ifmap_test_util::IFMapMsgUnlink(&this->config_db_, "routing-instance",
"nat", "route-target", "target:64496:2", "instance-target");
ifmap_test_util::IFMapMsgNodeDelete(
&this->config_db_, "virtual-network", "nat");
ifmap_test_util::IFMapMsgNodeDelete(
&this->config_db_, "routing-instance", "nat");
ifmap_test_util::IFMapMsgNodeDelete(
&this->config_db_, "route-target", "target:64496:2");
task_util::WaitForIdle();

RoutingInstance *nat_inst = this->ri_mgr_->GetRoutingInstance("nat");
TASK_UTIL_WAIT_EQ_NO_MSG(nat_inst->deleted(),
true, 1000, 10000, "Wait for nat instance to be marked deleted");
//
// Since the nexthop route is not yet deleted, routing instance is
// not destroyed
//
this->EnableResolveTrigger("nat");

// Delete nexthop route
this->DeleteInetRoute(NULL, "nat", "192.168.1.254/32");
task_util::WaitForIdle();
TASK_UTIL_WAIT_EQ_NO_MSG(this->ri_mgr_->GetRoutingInstance("nat"),
NULL, 1000, 10000, "Wait for nat instance to get destroyed");
}

//
// Add the routing instance that imports the static route after the static
// route has already been added. Objective is to check that the static route
Expand Down

0 comments on commit 861864b

Please sign in to comment.