From 861864bee9578ce688cda1d59234ea6b755ef60f Mon Sep 17 00:00:00 2001 From: Prakash M Bailkeri Date: Wed, 13 Jan 2016 22:22:02 -0800 Subject: [PATCH] Fix a corner case with routing instance delete 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 --- src/bgp/routing-instance/static_route.cc | 2 + src/bgp/routing-instance/static_route.h | 4 + src/bgp/test/static_route_test.cc | 142 +++++++++++++++++++++++ 3 files changed, 148 insertions(+) diff --git a/src/bgp/routing-instance/static_route.cc b/src/bgp/routing-instance/static_route.cc index 3020f0bb322..55e975a13cd 100644 --- a/src/bgp/routing-instance/static_route.cc +++ b/src/bgp/routing-instance/static_route.cc @@ -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; @@ -690,6 +691,7 @@ bool StaticRouteMgr::ResolvePendingStaticRouteConfig() { } StaticRouteMgr::~StaticRouteMgr() { + resolve_trigger_->Reset(); if (static_route_queue_) delete static_route_queue_; } diff --git a/src/bgp/routing-instance/static_route.h b/src/bgp/routing-instance/static_route.h index c04efa6bae9..39e18f071b7 100644 --- a/src/bgp/routing-instance/static_route.h +++ b/src/bgp/routing-instance/static_route.h @@ -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(); } diff --git a/src/bgp/test/static_route_test.cc b/src/bgp/test/static_route_test.cc index bb61f0aef45..2f169c31a48 100644 --- a/src/bgp/test/static_route_test.cc +++ b/src/bgp/test/static_route_test.cc @@ -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); @@ -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 instance_names = list_of("blue")("nat"); + multimap connections; + this->NetworkConfig(instance_names, connections); + task_util::WaitForIdle(); + + std::auto_ptr 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 instance_names = list_of("blue")("nat"); + multimap connections; + this->NetworkConfig(instance_names, connections); + task_util::WaitForIdle(); + + std::auto_ptr 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