Skip to content

Commit

Permalink
Ignore deleted AggregateRoute object while processing IsBestMatch()
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@bailkeri
bailkeri committed Feb 22, 2016
1 parent b2b0819 commit 30b41e3
Show file tree
Hide file tree
Showing 3 changed files with 121 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/bgp/routing-instance/route_aggregator.cc
View file
Expand Up @@ -289,7 +289,7 @@ bool AggregateRoute<T>::IsBestMatch(BgpRoute *route) {
std::set<PrefixT> 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);
}
Expand Down
97 changes: 97 additions & 0 deletions src/bgp/test/route_aggregator_test.cc
View file
Expand Up @@ -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<InetDefinition>(peers_[0], "test.inet.0", "2.2.2.1/32", 100);
AddRoute<InetDefinition>(peers_[0], "test.inet.0", "9.0.1.1/32", 100);
AddRoute<InetDefinition>(peers_[0], "test.inet.0", "9.0.1.10/32", 100);
AddRoute<InetDefinition>(peers_[0], "test.inet.0", "21.1.1.3/32", 100);
AddRoute<InetDefinition>(peers_[0], "test.inet.0", "21.1.1.4/32", 100);
AddRoute<InetDefinition>(peers_[0], "test.inet.0", "21.1.1.5/32", 100);
AddRoute<InetDefinition>(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<InetDefinition>("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<InetDefinition>("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<InetDefinition>("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<InetDefinition>("test.inet.0", "9.0.1.0/24");
ASSERT_TRUE(rt == NULL);
rt = RouteLookup<InetDefinition>("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<InetDefinition>(peers_[0], "test.inet.0", "2.2.2.1/32");
DeleteRoute<InetDefinition>(peers_[0], "test.inet.0", "9.0.1.1/32");
DeleteRoute<InetDefinition>(peers_[0], "test.inet.0", "9.0.1.10/32");
DeleteRoute<InetDefinition>(peers_[0], "test.inet.0", "21.1.1.3/32");
DeleteRoute<InetDefinition>(peers_[0], "test.inet.0", "21.1.1.4/32");
DeleteRoute<InetDefinition>(peers_[0], "test.inet.0", "21.1.1.5/32");
DeleteRoute<InetDefinition>(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
Expand Down
23 changes: 23 additions & 0 deletions src/bgp/testdata/route_aggregate_4c.xml
View file
@@ -0,0 +1,23 @@
<?xml version="1.0" encoding="utf-8"?>
<config>
<route-aggregate name='vn_subnet_1'>
<aggregate-route-entries>
<route>9.0.1.0/24</route>
</aggregate-route-entries>
<nexthop>2.2.2.1</nexthop>
</route-aggregate>

<route-aggregate name='vn_subnet_2'>
<aggregate-route-entries>
<route>9.0.0.0/8</route>
<route>21.1.1.0/24</route>
<route>21.1.0.0/16</route>
<route>21.0.0.0/8</route>
</aggregate-route-entries>
<nexthop>2.2.2.1</nexthop>
</route-aggregate>

<routing-instance name="test">
<vrf-target>target:1:103</vrf-target>
</routing-instance>
</config>

0 comments on commit 30b41e3

Please sign in to comment.