From fa454518a372bde543f97093df60fd7025c8e4c8 Mon Sep 17 00:00:00 2001 From: Prakash M Bailkeri Date: Wed, 13 Jan 2016 22:31:23 -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: I3c8a513f1af949aa419594a4214200de73dccaf8 Closes-bug: #1533435 --- src/bgp/routing-instance/istatic_route_mgr.h | 3 + src/bgp/routing-instance/static_route.cc | 2 + src/bgp/routing-instance/static_route.h | 3 + src/bgp/test/static_route_test.cc | 146 +++++++++++++++++++ 4 files changed, 154 insertions(+) diff --git a/src/bgp/routing-instance/istatic_route_mgr.h b/src/bgp/routing-instance/istatic_route_mgr.h index be2dcafb66c..300876a1661 100644 --- a/src/bgp/routing-instance/istatic_route_mgr.h +++ b/src/bgp/routing-instance/istatic_route_mgr.h @@ -26,6 +26,9 @@ class IStaticRouteMgr { private: template friend class StaticRouteTest; + virtual void DisableResolveTrigger() = 0; + virtual void EnableResolveTrigger() = 0; + virtual void DisableQueue() = 0; virtual void EnableQueue() = 0; virtual bool IsQueueEmpty() = 0; diff --git a/src/bgp/routing-instance/static_route.cc b/src/bgp/routing-instance/static_route.cc index 87d9fefdf4b..dfe3034d886 100644 --- a/src/bgp/routing-instance/static_route.cc +++ b/src/bgp/routing-instance/static_route.cc @@ -738,6 +738,7 @@ void StaticRouteMgr::RemoveStaticRoutePrefix(const PrefixT &static_route) { template void StaticRouteMgr::ProcessStaticRouteConfig() { CHECK_CONCURRENCY("bgp::Config"); + if (routing_instance()->deleted() || !routing_instance()->config()) return; const BgpInstanceConfig::StaticRouteList &list = routing_instance()->config()->static_routes(GetFamily()); typedef BgpInstanceConfig::StaticRouteList::const_iterator iterator_t; @@ -754,6 +755,7 @@ bool StaticRouteMgr::ResolvePendingStaticRouteConfig() { template 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 40433d8568c..0234fb68c2f 100644 --- a/src/bgp/routing-instance/static_route.h +++ b/src/bgp/routing-instance/static_route.h @@ -104,6 +104,9 @@ class StaticRouteMgr : public IStaticRouteMgr { bool ResolvePendingStaticRouteConfig(); bool StaticRouteEventCallback(StaticRouteRequest *req); + virtual void DisableResolveTrigger() { resolve_trigger_->set_disable(); } + virtual void EnableResolveTrigger() { resolve_trigger_->set_enable(); } + virtual void DisableQueue() { static_route_queue_->set_disable(true); } virtual void EnableQueue() { static_route_queue_->set_disable(false); } virtual 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 e26b52115bb..29eb3d34fc6 100644 --- a/src/bgp/test/static_route_test.cc +++ b/src/bgp/test/static_route_test.cc @@ -213,6 +213,19 @@ class StaticRouteTest : public ::testing::Test { parser->Receive(&config_db_, netconf.data(), netconf.length(), 0); } + void DisableResolveTrigger(const string &instance_name) { + RoutingInstance *rtinstance = + ri_mgr_->GetRoutingInstance(instance_name); + rtinstance->static_route_mgr(family_)->DisableResolveTrigger(); + } + + void EnableResolveTrigger(const string &instance_name) { + RoutingInstance *rtinstance = + ri_mgr_->GetRoutingInstance(instance_name); + if (rtinstance) + rtinstance->static_route_mgr(family_)->EnableResolveTrigger(); + } + void DisableStaticRouteQ(const string &instance_name) { RoutingInstance *rtinstance = ri_mgr_->GetRoutingInstance(instance_name); @@ -2003,6 +2016,139 @@ TYPED_TEST(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 +// +TYPED_TEST(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->AddRoute(NULL, "nat", this->BuildPrefix("192.168.1.254", 32), + 100, this->BuildNextHopAddress("2.3.4.5")); + task_util::WaitForIdle(); + + // Check for Static route + TASK_UTIL_WAIT_NE_NO_MSG( + this->RouteLookup("blue", this->BuildPrefix("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->DeleteRoute(NULL, "nat", this->BuildPrefix("192.168.1.254", 32)); + task_util::WaitForIdle(); + + // Check for Static route + TASK_UTIL_WAIT_EQ_NO_MSG( + this->RouteLookup("blue", this->BuildPrefix("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 +// +TYPED_TEST(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->AddRoute(NULL, "nat", this->BuildPrefix("192.168.1.254", 32), + 100, this->BuildNextHopAddress("2.3.4.5")); + task_util::WaitForIdle(); + + // Check for Static route + TASK_UTIL_WAIT_NE_NO_MSG( + this->RouteLookup("blue", this->BuildPrefix("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->RouteLookup("blue", this->BuildPrefix("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->DeleteRoute(NULL, "nat", this->BuildPrefix("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