From 32c2cf223ea13ea1b3f6c80699a5d6f6f6814f59 Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Sat, 25 Feb 2017 16:04:00 -0800 Subject: [PATCH] Fix race condition in BGPaaS peering creation - take 2 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. The previous fix did not cover the case where the notification for a routing-instance has not been processed when the notification for the instance-bgp-router link is processed. Fix this by modifying dependency tracker policy to re-evaluate all of the bgp-routers for a routing-instance when processing a notification for the routing-instance itself. This in turn causes a re-evaluation of all the bgp-peerings for those bgp-routers. Change-Id: Idc4425c87037aac1eb96f23394bfae536c17546c Closes-Bug: 1662678 --- src/bgp/bgp_config_listener.cc | 1 + src/bgp/test/bgp_config_listener_test.cc | 33 ++++++++++++++---------- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/src/bgp/bgp_config_listener.cc b/src/bgp/bgp_config_listener.cc index 878827e05b6..c8629725901 100644 --- a/src/bgp/bgp_config_listener.cc +++ b/src/bgp/bgp_config_listener.cc @@ -54,6 +54,7 @@ void BgpConfigListener::DependencyTrackerInit() { policy->insert(make_pair("connection", connection_react)); ReactionMap rt_instance_react = map_list_of + ("self", list_of("instance-bgp-router")) ("instance-target", list_of("self")("connection")) ("connection", list_of("self")) ("virtual-network-routing-instance", list_of("self")) diff --git a/src/bgp/test/bgp_config_listener_test.cc b/src/bgp/test/bgp_config_listener_test.cc index 4996e2d3f3f..33270f24db0 100644 --- a/src/bgp/test/bgp_config_listener_test.cc +++ b/src/bgp/test/bgp_config_listener_test.cc @@ -769,7 +769,7 @@ TEST_F(BgpConfigListenerTest, BgpRouterInstanceLinkChange) { TASK_UTIL_EXPECT_EQ(1, GetEdgeListCount()); // Perform propagation and verify change list. - // The bgp-routers are on the change list since the propagate list for + // The bgp-peerings are on the change list since the propagate list for // instance-bgp-router in bgp-router contains bgp-peering. PerformChangeListPropagation(); TASK_UTIL_EXPECT_EQ(3, GetChangeListCount()); @@ -780,11 +780,12 @@ TEST_F(BgpConfigListenerTest, BgpRouterInstanceLinkChange) { } // -// Node event for a routing-instance object without any connections. +// Node event for a routing-instance object with bgp-routers and bgp-peerings. // TEST_F(BgpConfigListenerTest, RoutingInstanceChange1) { - // Initialize config with local router and 3 remote routers. + // Initialize config with local router and 3 remote routers, hence 6 + // peerings. string content = ReadFile("controller/src/bgp/testdata/config_listener_test_1.xml"); EXPECT_TRUE(parser_.Parse(content)); task_util::WaitForIdle(); @@ -795,17 +796,20 @@ TEST_F(BgpConfigListenerTest, RoutingInstanceChange1) { string id_name(BgpConfigManager::kMasterInstance); ifmap_test_util::IFMapNodeNotify(&db_, "routing-instance", id_name); - // The routing-instance should be on the change list but not on the node - // list since there's no entry for self in the reaction map. + // The routing-instance should be on the change list and on the node list + // since there's an entry for self in the reaction map. TASK_UTIL_EXPECT_EQ(1, GetChangeListCount()); - TASK_UTIL_EXPECT_EQ(0, GetNodeListCount()); + TASK_UTIL_EXPECT_EQ(1, GetNodeListCount()); TASK_UTIL_EXPECT_EQ(0, GetEdgeListCount()); // Perform propagation and verify change list. - // Nothing else should get added since the node and edge lists are empty. + // The bgp-peerings are on the change list since the propagate list for + // self in routing-instance contains instance-bgp-router and propagate + // list for instance-bgp-router in bgp-router contains bgp-peering. PerformChangeListPropagation(); - TASK_UTIL_EXPECT_EQ(1, GetChangeListCount()); + TASK_UTIL_EXPECT_EQ(7, GetChangeListCount()); TASK_UTIL_EXPECT_EQ(1, GetChangeListCount("routing-instance")); + TASK_UTIL_EXPECT_EQ(6, GetChangeListCount("bgp-peering")); ResumeChangeListPropagation(); TASK_UTIL_EXPECT_EQ(0, GetChangeListCount()); @@ -833,14 +837,15 @@ TEST_F(BgpConfigListenerTest, RoutingInstanceChange2) { string id_name("red"); ifmap_test_util::IFMapNodeNotify(&db_, "routing-instance", id_name); - // The routing-instance should be on the change list but not on the node - // list since there's no entry for self in the reaction map. + // The routing-instance should be on the change list and on the node list + // since there's an entry for self in the reaction map. TASK_UTIL_EXPECT_EQ(1, GetChangeListCount()); - TASK_UTIL_EXPECT_EQ(0, GetNodeListCount()); + TASK_UTIL_EXPECT_EQ(1, GetNodeListCount()); TASK_UTIL_EXPECT_EQ(0, GetEdgeListCount()); // Perform propagation and verify change list. - // Nothing else should get added since the node and edge lists are empty. + // Nothing else should get added since the routing-instance doesn't have + // any bgp-routers and bgp-peerings. PerformChangeListPropagation(); TASK_UTIL_EXPECT_EQ(1, GetChangeListCount()); TASK_UTIL_EXPECT_EQ(1, GetChangeListCount("routing-instance")); @@ -1659,7 +1664,7 @@ TEST_F(BgpConfigListenerTest, RoutingPolicyUpdate_1) { task_util::WaitForIdle(); TASK_UTIL_EXPECT_EQ(1, GetChangeListCount()); - TASK_UTIL_EXPECT_EQ(0, GetNodeListCount()); + TASK_UTIL_EXPECT_EQ(1, GetNodeListCount()); TASK_UTIL_EXPECT_EQ(0, GetEdgeListCount()); PerformChangeListPropagation(); @@ -1904,7 +1909,7 @@ TEST_F(BgpConfigListenerTest, RoutingPolicyUpdate_8) { task_util::WaitForIdle(); TASK_UTIL_EXPECT_EQ(1, GetChangeListCount()); - TASK_UTIL_EXPECT_EQ(0, GetNodeListCount()); + TASK_UTIL_EXPECT_EQ(1, GetNodeListCount()); TASK_UTIL_EXPECT_EQ(0, GetEdgeListCount()); PerformChangeListPropagation();