From 9eae95f4406479d49fcb2631096ae56063a96979 Mon Sep 17 00:00:00 2001 From: Prakash Bailkeri Date: Mon, 22 Feb 2016 23:00:25 +0530 Subject: [PATCH] Ignore deleted AggregateRoute object while processing IsBestMatch() Scenario: routing instance is aggregating 1/8 and 1.0.1/24 route. At this point, if contributing route 1.0.1.1/32 is added to routing instance, it will become contributing to 1.0.1/24 and 1.0.1/24 will be contributing route to 1/8. Config update is made to remove 1.0.1/24 aggregate route. In a specific order of delete, 1.0.1.1/32 is notified after it is no longer contributing to 1.0.1/24. During the notification of this route, if the aggregate route object for 1.0.1/24 is not yet removed from aggregate_route_map_ it is not made as contributing to 1/8. Due to this bug, at steady state after the config update, no aggregate route will be published. Fix the issue by ignoring delete marked AggregateRoute object in IsBestMatch() Also added UT to verify the issue and fix Change-Id: Ia107952b876e0ee2a64c817074c677985a74b017 Closes-bug: #1547979 (cherry picked from commit 30b41e3698e2561e5a156c01eb327f8bee813871) --- src/bgp/routing-instance/route_aggregator.cc | 2 +- src/bgp/test/route_aggregator_test.cc | 97 ++++++++++++++++++++ src/bgp/testdata/route_aggregate_4c.xml | 23 +++++ 3 files changed, 121 insertions(+), 1 deletion(-) create mode 100644 src/bgp/testdata/route_aggregate_4c.xml diff --git a/src/bgp/routing-instance/route_aggregator.cc b/src/bgp/routing-instance/route_aggregator.cc index 83474ba975e..c76e3c5276b 100644 --- a/src/bgp/routing-instance/route_aggregator.cc +++ b/src/bgp/routing-instance/route_aggregator.cc @@ -289,7 +289,7 @@ bool AggregateRoute::IsBestMatch(BgpRoute *route) { std::set prefix_list; for (it = manager_->aggregate_route_map().begin(); it != manager_->aggregate_route_map().end(); ++it) { - if (ip_prefix != it->first && + if (!it->second->deleted() && ip_prefix != it->first && ip_prefix.IsMoreSpecific(it->first)) { prefix_list.insert(it->first); } diff --git a/src/bgp/test/route_aggregator_test.cc b/src/bgp/test/route_aggregator_test.cc index cc5fffbff55..851a6b33782 100644 --- a/src/bgp/test/route_aggregator_test.cc +++ b/src/bgp/test/route_aggregator_test.cc @@ -1545,6 +1545,103 @@ TEST_F(RouteAggregatorTest, ConfigUpdate_RemoveOverlappingPrefixes) { task_util::WaitForIdle(); } +// +// With multiple route aggregate config object linked to the routing instance, +// update the config to remove longer prefix +// RA-1: +// 9.0.1.0/24 +// RA-2: +// 9.0.0.0/8 +// 21.1.1.0/24 +// 21.1.0.0/16 +// 21.0.0.0/8 +// Step 1: Attach RA-1 to routing instance +// Step 2: Attach RA-2 to routing instance. Now routing instance has both 9.0/16 +// and 9/8 route aggregate prefix +// Step 3: Remove RA-1. Routing instance is only left with 9/8. +// Ensure valid route aggregate objects after config update +// +TEST_F(RouteAggregatorTest, ConfigUpdate_OverlappingPrefixes_1) { + string content = + FileRead("controller/src/bgp/testdata/route_aggregate_4c.xml"); + EXPECT_TRUE(parser_.Parse(content)); + task_util::WaitForIdle(); + + boost::system::error_code ec; + peers_.push_back( + new BgpPeerMock(Ip4Address::from_string("192.168.0.1", ec))); + + AddRoute(peers_[0], "test.inet.0", "2.2.2.1/32", 100); + AddRoute(peers_[0], "test.inet.0", "9.0.1.1/32", 100); + AddRoute(peers_[0], "test.inet.0", "9.0.1.10/32", 100); + AddRoute(peers_[0], "test.inet.0", "21.1.1.3/32", 100); + AddRoute(peers_[0], "test.inet.0", "21.1.1.4/32", 100); + AddRoute(peers_[0], "test.inet.0", "21.1.1.5/32", 100); + AddRoute(peers_[0], "test.inet.0", "21.1.1.0/24", 100); + task_util::WaitForIdle(); + + // Link the route aggregate config vn_subnet_1 to test + ifmap_test_util::IFMapMsgLink(&config_db_, "routing-instance", "test", + "route-aggregate", "vn_subnet_1", "route-aggregate-routing-instance"); + task_util::WaitForIdle(); + + VERIFY_EQ(8, RouteCount("test.inet.0")); + BgpRoute *rt = RouteLookup("test.inet.0", "9.0.1.0/24"); + ASSERT_TRUE(rt != NULL); + TASK_UTIL_EXPECT_EQ(rt->count(), 2); + TASK_UTIL_EXPECT_TRUE(rt->BestPath() != NULL); + TASK_UTIL_EXPECT_TRUE(rt->BestPath()->IsFeasible()); + + VerifyRouteAggregateSandesh("test"); + + // Link the route aggregate config vn_subnet_2 to test + ifmap_test_util::IFMapMsgLink(&config_db_, "routing-instance", "test", + "route-aggregate", "vn_subnet_2", "route-aggregate-routing-instance"); + task_util::WaitForIdle(); + + + VERIFY_EQ(11, RouteCount("test.inet.0")); + rt = RouteLookup("test.inet.0", "9.0.1.0/24"); + ASSERT_TRUE(rt != NULL); + TASK_UTIL_EXPECT_EQ(rt->count(), 2); + TASK_UTIL_EXPECT_TRUE(rt->BestPath() != NULL); + TASK_UTIL_EXPECT_TRUE(rt->BestPath()->IsFeasible()); + rt = RouteLookup("test.inet.0", "9.0.0.0/8"); + ASSERT_TRUE(rt != NULL); + TASK_UTIL_EXPECT_EQ(rt->count(), 2); + TASK_UTIL_EXPECT_TRUE(rt->BestPath() != NULL); + TASK_UTIL_EXPECT_TRUE(rt->BestPath()->IsFeasible()); + + // Verify the sandesh + VerifyRouteAggregateSandesh("test"); + + + // Unlink the route aggregate config vn_subnet_1 from test + ifmap_test_util::IFMapMsgUnlink(&config_db_, "routing-instance", "test", + "route-aggregate", "vn_subnet_1", "route-aggregate-routing-instance"); + task_util::WaitForIdle(); + + VERIFY_EQ(10, RouteCount("test.inet.0")); + rt = RouteLookup("test.inet.0", "9.0.1.0/24"); + ASSERT_TRUE(rt == NULL); + rt = RouteLookup("test.inet.0", "9.0.0.0/8"); + ASSERT_TRUE(rt != NULL); + TASK_UTIL_EXPECT_EQ(rt->count(), 2); + TASK_UTIL_EXPECT_TRUE(rt->BestPath() != NULL); + TASK_UTIL_EXPECT_TRUE(rt->BestPath()->IsFeasible()); + // Verify the sandesh + VerifyRouteAggregateSandesh("test"); + + DeleteRoute(peers_[0], "test.inet.0", "2.2.2.1/32"); + DeleteRoute(peers_[0], "test.inet.0", "9.0.1.1/32"); + DeleteRoute(peers_[0], "test.inet.0", "9.0.1.10/32"); + DeleteRoute(peers_[0], "test.inet.0", "21.1.1.3/32"); + DeleteRoute(peers_[0], "test.inet.0", "21.1.1.4/32"); + DeleteRoute(peers_[0], "test.inet.0", "21.1.1.5/32"); + DeleteRoute(peers_[0], "test.inet.0", "21.1.1.0/24"); + task_util::WaitForIdle(); +} + // // Update the config to update the prefix len of route-aggregate // Higher prefix len to lower diff --git a/src/bgp/testdata/route_aggregate_4c.xml b/src/bgp/testdata/route_aggregate_4c.xml new file mode 100644 index 00000000000..a948789120c --- /dev/null +++ b/src/bgp/testdata/route_aggregate_4c.xml @@ -0,0 +1,23 @@ + + + + + 9.0.1.0/24 + + 2.2.2.1 + + + + + 9.0.0.0/8 + 21.1.1.0/24 + 21.1.0.0/16 + 21.0.0.0/8 + + 2.2.2.1 + + + + target:1:103 + +