Merge "Handle route-retracts from the agents during instance deletion…

… correctly"

src/bgp/bgp_xmpp_channel.cc

@@ -671,14 +671,16 @@ bool BgpXmppChannel::GetMembershipInfo(const string &vrf_name,
 //
 bool BgpXmppChannel::VerifyMembership(const string &vrf_name,
     Address::Family family, BgpTable **table,
-    int *instance_id, uint64_t *subscription_gen_id, bool *subscribe_pending) {
+    int *instance_id, uint64_t *subscription_gen_id, bool *subscribe_pending,
+    bool add_change) {
     *table = NULL;
     *subscribe_pending = false;
 
     RoutingInstanceMgr *instance_mgr = bgp_server_->routing_instance_mgr();
     RoutingInstance *rt_instance = instance_mgr->GetRoutingInstance(vrf_name);
-    if (rt_instance != NULL && !rt_instance->deleted()) {
+    if (rt_instance)
         *table = rt_instance->GetTable(family);
+    if (rt_instance != NULL && !rt_instance->deleted()) {
         RequestType req_type;
         if (GetMembershipInfo(*table, instance_id,
                               subscription_gen_id, &req_type)) {
@@ -700,10 +702,11 @@ bool BgpXmppChannel::VerifyMembership(const string &vrf_name,
             }
         }
     } else {
-        // Bail if there's no pending subscribe.
+        // Bail if there's no pending subscribe. route retract can be received
+        // while the instance is marked for deletion.
         if (GetMembershipInfo(vrf_name, instance_id)) {
             *subscribe_pending = true;
-        } else {
+        } else if (add_change || !rt_instance) {
             BGP_LOG_PEER_INSTANCE_CRITICAL(Peer(), vrf_name, BGP_PEER_DIR_IN,
                BGP_LOG_FLAG_ALL, "Received route without pending subscribe");
             return false;
@@ -764,7 +767,7 @@ bool BgpXmppChannel::ProcessMcastItem(string vrf_name,
     uint64_t subscription_gen_id;
     BgpTable *table;
     if (!VerifyMembership(vrf_name, Address::ERMVPN, &table, &instance_id,
-        &subscription_gen_id, &subscribe_pending)) {
+        &subscription_gen_id, &subscribe_pending, add_change)) {
         channel_->Close();
         return false;
     }
@@ -924,7 +927,7 @@ bool BgpXmppChannel::ProcessItem(string vrf_name,
     uint64_t subscription_gen_id;
     BgpTable *table;
     if (!VerifyMembership(vrf_name, Address::INET, &table, &instance_id,
-        &subscription_gen_id, &subscribe_pending)) {
+        &subscription_gen_id, &subscribe_pending, add_change)) {
         channel_->Close();
         return false;
     }
@@ -1130,7 +1133,7 @@ bool BgpXmppChannel::ProcessInet6Item(string vrf_name,
     uint64_t subscription_gen_id;
     BgpTable *table;
     if (!VerifyMembership(vrf_name, Address::INET6, &table, &instance_id,
-        &subscription_gen_id, &subscribe_pending)) {
+        &subscription_gen_id, &subscribe_pending, add_change)) {
         channel_->Close();
         return false;
     }
@@ -1376,7 +1379,7 @@ bool BgpXmppChannel::ProcessEnetItem(string vrf_name,
     uint64_t subscription_gen_id;
     BgpTable *table;
     if (!VerifyMembership(vrf_name, Address::EVPN, &table, &instance_id,
-        &subscription_gen_id, &subscribe_pending)) {
+        &subscription_gen_id, &subscribe_pending, add_change)) {
         channel_->Close();
         return false;
     }

src/bgp/bgp_xmpp_channel.h

@@ -259,7 +259,7 @@ class BgpXmppChannel {
         int *instance_id);
     bool VerifyMembership(const std::string &vrf_name, Address::Family family,
         BgpTable **table, int *instance_id, uint64_t *subscribed_at,
-        bool *subscribe_pending);
+        bool *subscribe_pending, bool add_change);
 
     bool ProcessItem(std::string vrf_name, const pugi::xml_node &node,
                      bool add_change);

src/bgp/test/bgp_xmpp_deferq_test.cc

@@ -1312,7 +1312,7 @@ TEST_F(BgpXmppUnitTest, DuplicateRegisterWithDeletedRoutingInstance2) {
     TASK_UTIL_EXPECT_TRUE(blue->deleted());
 
     // Send unsubscribe for the blue instance.
-    agent_a_->Unsubscribe("blue", -1, -1, true, false);
+    agent_a_->Unsubscribe("blue", -1, true, false);
     TASK_UTIL_EXPECT_FALSE(
         PeerRegistered(bgp_channel_manager_->channel_, "blue", 1));
 
@@ -2585,6 +2585,154 @@ TEST_F(BgpXmppUnitTest, AddDeleteInetRouteWithoutRegister3) {
     agent_a_->SessionDown();
 }
 
+TEST_F(BgpXmppUnitTest, DeleteInetRouteFromDeletedInstance) {
+    Configure();
+    task_util::WaitForIdle();
+
+    // create an XMPP client in server A
+    agent_a_.reset(
+        new test::NetworkAgentMock(&evm_, SUB_ADDR, xs_a_->GetPort()));
+
+    TASK_UTIL_EXPECT_TRUE(bgp_channel_manager_->channel_ != NULL);
+    TASK_UTIL_EXPECT_TRUE(agent_a_->IsEstablished());
+    uint32_t old_flap_count = agent_a_->flap_count();
+
+    agent_a_->SubscribeAll("blue", 1);
+    task_util::WaitForIdle();
+    agent_a_->AddRoute("blue", "10.1.1.2/32");
+    agent_a_->AddInet6Route("blue", "::ffff:1/128");
+    TASK_UTIL_EXPECT_EQ(1, agent_a_->RouteCount());
+    TASK_UTIL_EXPECT_EQ(1, agent_a_->Inet6RouteCount());
+
+    // Pause deletion for blue instance.
+    RoutingInstance *blue = VerifyRoutingInstance("blue");
+    PauseDelete(blue->deleter());
+
+    UnconfigureRoutingInstances();
+    task_util::WaitForIdle();
+
+    agent_a_->DeleteRoute("blue", "10.1.1.2/32");
+    agent_a_->DeleteInet6Route("blue", "::ffff:1/128");
+    task_util::WaitForIdle();
+
+    // Make sure session on agent did not flap.
+    TASK_UTIL_EXPECT_EQ(old_flap_count, agent_a_->flap_count());
+
+    // Resume deletion of the instance but instance should not get deleted
+    // yet because table has not been unsubscribed yet.
+    ResumeDelete(blue->deleter());
+    VerifyRoutingInstance("blue");
+
+    // Unsubscribe from the table and then expect the instance to get deleted.
+    agent_a_->UnsubscribeAll("blue", -1);
+    VerifyNoRoutingInstance("blue");
+    TASK_UTIL_EXPECT_TRUE(
+        PeerNotRegistered(bgp_channel_manager_->channel_, "blue"));
+
+    // Route should have been added now.
+    TASK_UTIL_EXPECT_EQ(0, agent_a_->RouteCount());
+    TASK_UTIL_EXPECT_EQ(0, agent_a_->Inet6RouteCount());
+    agent_a_->SessionDown();
+    task_util::WaitForIdle();
+}
+
+TEST_F(BgpXmppUnitTest, DeleteInetRouteAndUnsubscribeFromDeletedInstance) {
+    Configure();
+    task_util::WaitForIdle();
+
+    // create an XMPP client in server A
+    agent_a_.reset(
+        new test::NetworkAgentMock(&evm_, SUB_ADDR, xs_a_->GetPort()));
+
+    TASK_UTIL_EXPECT_TRUE(bgp_channel_manager_->channel_ != NULL);
+    TASK_UTIL_EXPECT_TRUE(agent_a_->IsEstablished());
+    uint32_t old_flap_count = agent_a_->flap_count();
+
+    agent_a_->SubscribeAll("blue", 1);
+    task_util::WaitForIdle();
+    agent_a_->AddRoute("blue", "10.1.1.2/32");
+    agent_a_->AddInet6Route("blue", "::ffff:1/128");
+    TASK_UTIL_EXPECT_EQ(1, agent_a_->RouteCount());
+    TASK_UTIL_EXPECT_EQ(1, agent_a_->Inet6RouteCount());
+
+    // Pause deletion for blue instance.
+    RoutingInstance *blue = VerifyRoutingInstance("blue");
+    PauseDelete(blue->deleter());
+
+    UnconfigureRoutingInstances();
+    task_util::WaitForIdle();
+
+    // Delete route and unsubscribe from the table together, while the instance
+    // is still under deletion.
+    agent_a_->DeleteRoute("blue", "10.1.1.2/32");
+    agent_a_->DeleteInet6Route("blue", "::ffff:1/128");
+    agent_a_->UnsubscribeAll("blue", -1);
+    task_util::WaitForIdle();
+
+    // Make sure session on agent did not flap.
+    TASK_UTIL_EXPECT_EQ(old_flap_count, agent_a_->flap_count());
+
+    // Resume deletion of the instance and ensure that instance is deleted.
+    ResumeDelete(blue->deleter());
+    VerifyNoRoutingInstance("blue");
+    TASK_UTIL_EXPECT_TRUE(
+        PeerNotRegistered(bgp_channel_manager_->channel_, "blue"));
+
+    // Route should have been added now.
+    TASK_UTIL_EXPECT_EQ(0, agent_a_->RouteCount());
+    TASK_UTIL_EXPECT_EQ(0, agent_a_->Inet6RouteCount());
+    agent_a_->SessionDown();
+    task_util::WaitForIdle();
+}
+
+TEST_F(BgpXmppUnitTest, UnsubscribeFromDeletedInstance) {
+    Configure();
+    task_util::WaitForIdle();
+
+    // create an XMPP client in server A
+    agent_a_.reset(
+        new test::NetworkAgentMock(&evm_, SUB_ADDR, xs_a_->GetPort()));
+
+    TASK_UTIL_EXPECT_TRUE(bgp_channel_manager_->channel_ != NULL);
+    TASK_UTIL_EXPECT_TRUE(agent_a_->IsEstablished());
+    uint32_t old_flap_count = agent_a_->flap_count();
+
+    agent_a_->SubscribeAll("blue", 1);
+    task_util::WaitForIdle();
+    agent_a_->AddRoute("blue", "10.1.1.2/32");
+    agent_a_->AddInet6Route("blue", "::ffff:1/128");
+    TASK_UTIL_EXPECT_EQ(1, agent_a_->RouteCount());
+    TASK_UTIL_EXPECT_EQ(1, agent_a_->Inet6RouteCount());
+
+    // Pause deletion for blue instance.
+    RoutingInstance *blue = VerifyRoutingInstance("blue");
+    PauseDelete(blue->deleter());
+
+    UnconfigureRoutingInstances();
+    task_util::WaitForIdle();
+
+    // Unsubscribe from the instance, without explicitly deleting the routes.
+    // Routes shall get deleted from BgpXmppChannel as part of resulting
+    // table walk from membership manager.
+    agent_a_->UnsubscribeAll("blue", -1, true, false);
+    task_util::WaitForIdle();
+
+    // Make sure session on agent did not flap.
+    TASK_UTIL_EXPECT_EQ(old_flap_count, agent_a_->flap_count());
+
+    // Resume deletion of the instance and ensure that instance is deleted.
+    ResumeDelete(blue->deleter());
+    VerifyNoRoutingInstance("blue");
+    TASK_UTIL_EXPECT_TRUE(
+        PeerNotRegistered(bgp_channel_manager_->channel_, "blue"));
+
+    // Route should have been added now.
+    TASK_UTIL_EXPECT_EQ(0, agent_a_->RouteCount());
+    TASK_UTIL_EXPECT_EQ(0, agent_a_->Inet6RouteCount());
+    agent_a_->SessionDown();
+    task_util::WaitForIdle();
+}
+
 TEST_F(BgpXmppUnitTest, AddDeleteInet6RouteWithoutRegister1) {
     ConfigureWithoutRoutingInstances();
     task_util::WaitForIdle();

src/control-node/test/network_agent_mock.cc

@@ -7,6 +7,7 @@
 #include <boost/algorithm/string.hpp>
 #include <boost/assign/list_of.hpp>
 #include <boost/foreach.hpp>
+#include <tr1/type_traits>
 
 #include "base/logging.h"
 #include "base/util.h"
@@ -788,6 +789,7 @@ NetworkAgentMock::NetworkAgentMock(EventManager *evm, const string &hostname,
             XmppDocumentMock::kNetworkServiceJID));
     inet6_route_mgr_.reset(new InstanceMgr<Inet6RouteEntry>(this,
             XmppDocumentMock::kNetworkServiceJID));
+    inet6_route_mgr_->set_ipv6(true);
     enet_route_mgr_.reset(new InstanceMgr<EnetRouteEntry>(this,
             XmppDocumentMock::kNetworkServiceJID));
     mcast_route_mgr_.reset(new InstanceMgr<McastRouteEntry>(this,
@@ -1205,7 +1207,7 @@ void NetworkAgentMock::AddMcastRoute(const string &network_name,
     xml_document *xdoc = impl_->RouteMcastAddXmlDoc(
             network_name, sg, nexthop, label_range, encap);
     peer->SendDocument(xdoc);
-    enet_route_mgr_->AddOriginated(network_name, sg);
+    mcast_route_mgr_->AddOriginated(network_name, sg);
 }
 
 void NetworkAgentMock::DeleteMcastRoute(const string &network_name,
@@ -1363,8 +1365,20 @@ void NetworkAgentMock::InstanceMgr<T>::Unsubscribe(const std::string &network,
         typename NetworkAgentMock::Instance<T>::OriginatedSet::const_iterator
             iter = rti->originated().begin();
         while (iter != rti->originated().end()) {
-            xml_document *xdoc =
-                parent_->impl_->RouteDeleteXmlDoc(network, *iter);
+            xml_document *xdoc = NULL;
+            if (std::tr1::is_same<T, RouteEntry>::value) {
+                if (ipv6())
+                    xdoc = parent_->impl_->Inet6RouteDeleteXmlDoc(network,
+                                                                  *iter);
+                else
+                    xdoc = parent_->impl_->RouteDeleteXmlDoc(network, *iter);
+            } else if (std::tr1::is_same<T, EnetRouteEntry>::value) {
+                xdoc = parent_->impl_->RouteEnetDeleteXmlDoc(network, *iter);
+            } else if (std::tr1::is_same<T, McastRouteEntry>::value) {
+                xdoc = parent_->impl_->RouteMcastDeleteXmlDoc(network, *iter);
+            } else {
+                assert(false);
+            }
             peer->SendDocument(xdoc);
             iter++;
         }

src/control-node/test/network_agent_mock.h

@@ -330,8 +330,11 @@ class NetworkAgentMock {
             InstanceMgr(NetworkAgentMock *parent, std::string type) {
                 parent_ = parent;
                 type_ = type;
+                ipv6_ = false;
             }
 
+            void set_ipv6 (bool ipv6) { ipv6_ = ipv6; }
+            bool ipv6 () const { return ipv6_; }
             bool HasSubscribed(const std::string &network);
             void Subscribe(const std::string &network, int id = -1,
                            bool wait_for_established = true,
@@ -359,6 +362,7 @@ class NetworkAgentMock {
         private:
             NetworkAgentMock *parent_;
             std::string type_;
+            bool ipv6_;
             InstanceMap instance_map_;
     };
 
@@ -383,11 +387,17 @@ class NetworkAgentMock {
         mcast_route_mgr_->Subscribe(network, id, wait_for_established, false);
     }
     void UnsubscribeAll(const std::string &network, int id = -1,
-                        bool wait_for_established = true) {
-        route_mgr_->Unsubscribe(network, id, wait_for_established, true);
-        inet6_route_mgr_->Unsubscribe(network, id, wait_for_established, false);
-        enet_route_mgr_->Unsubscribe(network, id, wait_for_established, false);
-        mcast_route_mgr_->Unsubscribe(network, id, wait_for_established, false);
+                        bool wait_for_established = true,
+                        bool withdraw_routes = true,
+                        bool send_unsubscribe = true) {
+        inet6_route_mgr_->Unsubscribe(network, id, wait_for_established,
+                                      false, withdraw_routes);
+        enet_route_mgr_->Unsubscribe(network, id, wait_for_established,
+                                     false, withdraw_routes);
+        mcast_route_mgr_->Unsubscribe(network, id, wait_for_established,
+                                      false, withdraw_routes);
+        route_mgr_->Unsubscribe(network, id, wait_for_established,
+                                send_unsubscribe, withdraw_routes);
     }
 
     void Subscribe(const std::string &network, int id = -1,