Skip to content

Commit

Permalink
Fix race condition in BGPaaS peering creation
Browse files Browse the repository at this point in the history 
If the internal ifmap configuration object for the routing-instance
for the bgp-routers that comprise a bgp-peering does not exist when
processing the bgp-peering, the ifmap config manager does create the
internal ifmap configuration object for the bgp-peering.

This causes the control node to not create the internal ifmap config
object for some bgp-peerings when the control node gets restarted in
a scaled bgpaas configuration. This happens for bgp-peerings which
get processed before the corresponding routing-instance has been
processed.

Fix this by modifying dependency tracker policy to re-evaluate all of
the bgp-peerings for a bgp-router when processing a notification for
the instance-bgp-router edge on the bgp-router.

Change-Id: Id74dda2573491dd7d8df8d9c09d881dc6aa0e95c
Closes-Bug: 1662678
  • Loading branch information
Nischal Sheth committed Feb 9, 2017
1 parent c1d88f5 commit fd55ab3
Show file tree
Hide file tree
Showing 3 changed files with 116 additions and 48 deletions.
3 changes: 2 additions & 1 deletion src/bgp/bgp_config_listener.cc
View file
Expand Up @@ -44,7 +44,8 @@ void BgpConfigListener::DependencyTrackerInit() {
policy->insert(make_pair("bgp-peering", bgp_peering_react));

ReactionMap bgp_router_react = map_list_of<string, PropagateList>
("self", list_of("bgp-peering"));
("self", list_of("bgp-peering"))
("instance-bgp-router", list_of("bgp-peering"));
policy->insert(make_pair("bgp-router", bgp_router_react));

ReactionMap connection_react = map_list_of<string, PropagateList>
Expand Down
61 changes: 14 additions & 47 deletions src/bgp/test/bgp_config_listener_test.cc
View file
Expand Up @@ -738,12 +738,12 @@ TEST_F(BgpConfigListenerTest, BgpRouterChange2) {
}

//
// Link event for an uninteresting link of a bgp-router object.
// Link event for the instance-bgp-router link of a bgp-router object.
//
TEST_F(BgpConfigListenerTest, BgpRouterUninterestingLinkChange) {
TEST_F(BgpConfigListenerTest, BgpRouterInstanceLinkChange) {

// Initialize config with local router and add link to master instance.
string content = ReadFile("controller/src/bgp/testdata/config_listener_test_0.xml");
// Initialize config with local router and 3 remote routers.
string content = ReadFile("controller/src/bgp/testdata/config_listener_test_1.xml");
EXPECT_TRUE(parser_.Parse(content));
string instance(BgpConfigManager::kMasterInstance);
string router = instance + ":local";
Expand All @@ -759,17 +759,21 @@ TEST_F(BgpConfigListenerTest, BgpRouterUninterestingLinkChange) {
"bgp-router", router, "routing-instance", instance);
task_util::WaitForIdle();

// Edge list should be empty since both edges are not interesting.
// Reaction map for bgp-router has no entry for instance-bgp-router.
// Reaction map for routing-instance has no entry for instance-bgp-router.
// Only one edge should get added to the edge list.
// This is the instance-bgp-router edge from bgp-router.
// The instance-bgp-router edge from the routing-instance is not
// interesting since the reaction map for routing-instance doesn't
// have an entry for instance-bgp-router.
TASK_UTIL_EXPECT_EQ(0, GetChangeListCount());
TASK_UTIL_EXPECT_EQ(0, GetNodeListCount());
TASK_UTIL_EXPECT_EQ(0, GetEdgeListCount());
TASK_UTIL_EXPECT_EQ(1, GetEdgeListCount());

// Perform propagation and verify change list.
// Nothing should get added since the node and edge lists are empty.
// The bgp-routers are on the change list since the propagate list for
// instance-bgp-router in bgp-router contains bgp-peering.
PerformChangeListPropagation();
TASK_UTIL_EXPECT_EQ(0, GetChangeListCount());
TASK_UTIL_EXPECT_EQ(3, GetChangeListCount());
TASK_UTIL_EXPECT_EQ(3, GetChangeListCount("bgp-peering"));

ResumeChangeListPropagation();
TASK_UTIL_EXPECT_EQ(0, GetChangeListCount());
Expand Down Expand Up @@ -1439,43 +1443,6 @@ TEST_F(BgpConfigListenerTest, RoutingInstanceVirtualNetworkChange3) {
TASK_UTIL_EXPECT_EQ(0, GetChangeListCount());
}

//
// Link event for an uninteresting link of a routing-instance object.
//
TEST_F(BgpConfigListenerTest, RoutingInstanceUninterestingLinkChange) {

// Initialize config with local router and add link to master instance.
string content = ReadFile("controller/src/bgp/testdata/config_listener_test_0.xml");
EXPECT_TRUE(parser_.Parse(content));
string instance(BgpConfigManager::kMasterInstance);
string router = instance + ":local";
ifmap_test_util::IFMapMsgLink(&db_, "routing-instance", instance,
"bgp-router", router, "instance-bgp-router");
task_util::WaitForIdle();
TASK_UTIL_EXPECT_EQ(0, GetChangeListCount());

// Pause propagation, notify instance-bgp-router link.
PauseChangeListPropagation();
ifmap_test_util::IFMapLinkNotify(&db_, &graph_, "instance-bgp-router",
"routing-instance", instance, "bgp-router", router);
task_util::WaitForIdle();

// Edge list should be empty since both edges are not interesting.
// Reaction map for bgp-router has no entry for instance-bgp-router.
// Reaction map for routing-instance has no entry for instance-bgp-router.
TASK_UTIL_EXPECT_EQ(0, GetChangeListCount());
TASK_UTIL_EXPECT_EQ(0, GetNodeListCount());
TASK_UTIL_EXPECT_EQ(0, GetEdgeListCount());

// Perform propagation and verify change list.
// Nothing should get added since the node and edge lists are empty.
PerformChangeListPropagation();
TASK_UTIL_EXPECT_EQ(0, GetChangeListCount());

ResumeChangeListPropagation();
TASK_UTIL_EXPECT_EQ(0, GetChangeListCount());
}

//
// Node event for a virtual-network object associated with routing-instances.
// There are no other virtual-networks or routing-instances.
Expand Down
100 changes: 100 additions & 0 deletions src/bgp/test/bgp_ifmap_config_manager_test.cc
View file
Expand Up @@ -1872,6 +1872,106 @@ TEST_F(BgpIfmapConfigManagerShowTest, ShowBGPaaSPeerings) {
TASK_UTIL_EXPECT_EQ(0, db_graph_.vertex_count());
}

TEST_F(BgpIfmapConfigManagerTest, AddBgpPeeringBeforeInstanceBgpRouterLink1) {
// Add client bgp-router with parameters.
string bgp_router_id1 = string("test") + ":client";
autogen::BgpRouterParams *params1 = new autogen::BgpRouterParams;
params1->Clear();
params1->autonomous_system = 100;
params1->identifier = "10.1.1.100";
params1->address = "127.0.0.100";
params1->router_type = "bgpaas-client";
ifmap_test_util::IFMapMsgPropertyAdd(&db_, "bgp-router", bgp_router_id1,
"bgp-router-parameters", params1);
task_util::WaitForIdle();

// Add server bgp-router with parameters.
string bgp_router_id2 = string("test") + ":server";
autogen::BgpRouterParams *params2 = new autogen::BgpRouterParams;
params2->Clear();
params2->autonomous_system = 100;
params2->router_type = "bgpaas-server";
ifmap_test_util::IFMapMsgPropertyAdd(&db_, "bgp-router", bgp_router_id2,
"bgp-router-parameters", params2);
task_util::WaitForIdle();

// Now add a bgp-peering link between the bgp-routers.
// Verify that's there's no peering config created as there's no parent
// routing instance.
string peering = "attr(" + bgp_router_id1 + "," + bgp_router_id2 + ")";
ifmap_test_util::IFMapMsgLink(&db_,
"bgp-router", bgp_router_id1, "bgp-router", bgp_router_id2,
"bgp-peering", 0, new autogen::BgpPeeringAttributes());
task_util::WaitForIdle();
TASK_UTIL_EXPECT_EQ(0, GetPeeringCount());
TASK_UTIL_EXPECT_TRUE(FindPeeringConfig(peering) == NULL);

// Now add a link between the bgp-routers and the routing-instance.
// Verify that's the peering config gets created due to notification
// of the instance-bgp-router links.
ifmap_test_util::IFMapMsgLink(&db_, "routing-instance", "test",
"bgp-router", bgp_router_id1, "instance-bgp-router");
ifmap_test_util::IFMapMsgLink(&db_, "routing-instance", "test",
"bgp-router", bgp_router_id2, "instance-bgp-router");
task_util::WaitForIdle();
TASK_UTIL_EXPECT_EQ(1, GetPeeringCount());
TASK_UTIL_EXPECT_TRUE(FindPeeringConfig(peering) != NULL);
}

TEST_F(BgpIfmapConfigManagerTest, AddBgpPeeringBeforeInstanceBgpRouterLink2) {
// Add client bgp-router with parameters.
string bgp_router_id1 = string("test") + ":client";
autogen::BgpRouterParams *params1 = new autogen::BgpRouterParams;
params1->Clear();
params1->autonomous_system = 100;
params1->identifier = "10.1.1.100";
params1->address = "127.0.0.100";
params1->router_type = "bgpaas-client";
ifmap_test_util::IFMapMsgPropertyAdd(&db_, "bgp-router", bgp_router_id1,
"bgp-router-parameters", params1);
task_util::WaitForIdle();

// Add server bgp-router with parameters.
string bgp_router_id2 = string("test") + ":server";
autogen::BgpRouterParams *params2 = new autogen::BgpRouterParams;
params2->Clear();
params2->autonomous_system = 100;
params2->router_type = "bgpaas-server";
ifmap_test_util::IFMapMsgPropertyAdd(&db_, "bgp-router", bgp_router_id2,
"bgp-router-parameters", params2);
task_util::WaitForIdle();

// Now add a bgp-peering link between the bgp-routers.
// Verify that's there's no peering config created as there's no parent
// routing instance.
string peering = "attr(" + bgp_router_id1 + "," + bgp_router_id2 + ")";
ifmap_test_util::IFMapMsgLink(&db_,
"bgp-router", bgp_router_id1, "bgp-router", bgp_router_id2,
"bgp-peering", 0, new autogen::BgpPeeringAttributes());
task_util::WaitForIdle();
TASK_UTIL_EXPECT_EQ(0, GetPeeringCount());
TASK_UTIL_EXPECT_TRUE(FindPeeringConfig(peering) == NULL);

// Now add the parent routing-instance.
// Verify that's there's no peering config created sine there's no
// notification to re-evaluate the bgp-peering.
ifmap_test_util::IFMapMsgNodeAdd(&db_, "routing-instance", "test");
task_util::WaitForIdle();
TASK_UTIL_EXPECT_EQ(0, GetPeeringCount());
TASK_UTIL_EXPECT_TRUE(FindPeeringConfig(peering) == NULL);

// Now add a link between the bgp-routers and the routing-instance.
// Verify that's the peering config gets created due to notification
// of the instance-bgp-router links.
ifmap_test_util::IFMapMsgLink(&db_, "routing-instance", "test",
"bgp-router", bgp_router_id1, "instance-bgp-router");
ifmap_test_util::IFMapMsgLink(&db_, "routing-instance", "test",
"bgp-router", bgp_router_id2, "instance-bgp-router");
task_util::WaitForIdle();
TASK_UTIL_EXPECT_EQ(1, GetPeeringCount());
TASK_UTIL_EXPECT_TRUE(FindPeeringConfig(peering) != NULL);
}

TEST_F(BgpIfmapConfigManagerTest, AddBgpRouterBeforeParentLink) {
string bgp_router_id = string(BgpConfigManager::kMasterInstance) + ":local";
// Add bgp-router with parameters.
Expand Down

0 comments on commit fd55ab3

Please sign in to comment.