Implement per family prefix-limit for bgp peers

Following changes are implemented:

- Keep track of primary path count per family for each bgp peer
- Trigger a prefix limit check for the peer when limit is exceeded
- Actual check happens in the context of bgp::StateMachine Task
- Clear the peer and send notification with cease subcode MaxPrefixes
- Check prefix limit when peer configuration is changed

Pending - add an idle timeout after clearing the peer

Change-Id: Iefce9a2a692088af2cc69deeaf12e19ca24373d6
Partial-Bug: 1697345
(cherry picked from commit f80f0a3)

src/bgp/bgp_path.cc

@@ -159,13 +159,13 @@ bool BgpPath::PathSameNeighborAs(const BgpPath &rhs) const {
     return (attr_->neighbor_as() == rattr->neighbor_as());
 }
 
-void BgpPath::UpdatePeerRefCount(int count) const {
+void BgpPath::UpdatePeerRefCount(int count, Address::Family family) const {
     if (!peer_)
         return;
     peer_->UpdateTotalPathCount(count);
-    if (source_ != BGP_XMPP || IsReplicated())
+    if (source_ != BGP_XMPP || IsReplicated() || IsResolved())
         return;
-    peer_->UpdatePrimaryPathCount(count);
+    peer_->UpdatePrimaryPathCount(count, family);
 }
 
 string BgpPath::ToString() const {

src/bgp/bgp_path.h

@@ -72,7 +72,7 @@ class BgpPath : public Path {
     const IPeer *GetPeer() const { return peer_; }
     const uint32_t GetPathId() const { return path_id_; }
 
-    void UpdatePeerRefCount(int count) const;
+    void UpdatePeerRefCount(int count, Address::Family family) const;
 
     void SetAttr(const BgpAttrPtr attr, const BgpAttrPtr original_attr) {
         attr_ = attr;

src/bgp/bgp_peer.cc

@@ -170,6 +170,8 @@ class BgpPeer::DeleteActor : public LifetimeActor {
             return false;
         if (!peer_->state_machine_->IsQueueEmpty())
             return false;
+        if (peer_->prefix_limit_trigger_.IsSet())
+            return false;
         return true;
     }
 
@@ -453,6 +455,9 @@ BgpPeer::BgpPeer(BgpServer *server, RoutingInstance *instance,
           trigger_(boost::bind(&BgpPeer::ResumeClose, this),
                    TaskScheduler::GetInstance()->GetTaskId("bgp::StateMachine"),
                    GetTaskInstance()),
+          prefix_limit_trigger_(boost::bind(&BgpPeer::CheckPrefixLimits, this),
+                   TaskScheduler::GetInstance()->GetTaskId("bgp::StateMachine"),
+                   GetTaskInstance()),
           buffer_capacity_(GetBufferCapacity()),
           session_(NULL),
           keepalive_timer_(TimerManager::CreateTimer(*server->ioservice(),
@@ -473,6 +478,7 @@ BgpPeer::BgpPeer(BgpServer *server, RoutingInstance *instance,
           peer_as_(config->peer_as()),
           local_bgp_id_(config->local_identifier()),
           peer_bgp_id_(0),
+          family_primary_path_count_(Address::NUM_FAMILIES),
           peer_type_((config->peer_as() == config->local_as()) ?
                          BgpProto::IBGP : BgpProto::EBGP),
           state_machine_(BgpObjectFactory::Create<StateMachine>(this)),
@@ -520,12 +526,12 @@ BgpPeer::BgpPeer(BgpServer *server, RoutingInstance *instance,
     for (Address::Family family = Address::UNSPEC;
             family < Address::NUM_FAMILIES;
             family = static_cast<Address::Family>(family + 1)) {
+        family_primary_path_count_[family] = 0;
          eor_send_timer_[family] =
              TimerManager::CreateTimer(*server->ioservice(),
                  "BGP EoR Send timer for " + Address::FamilyToString(family),
                  TaskScheduler::GetInstance()->GetTaskId("bgp::StateMachine"),
                  GetTaskInstance());
-
          eor_receive_timer_[family] =
              TimerManager::CreateTimer(*server->ioservice(),
                  "BGP EoR Receive timer for " + Address::FamilyToString(family),
@@ -733,6 +739,28 @@ void BgpPeer::LogInstallAuthKeys(const string &socket_name,
                  BGP_PEER_DIR_NA, logstr);
 }
 
+//
+// Check if the configured prefix limit for any address family has been
+// exceeded. If yes, clear the peer by sending a notification with a cease
+// subcode of MaxPrefixes.
+//
+bool BgpPeer::CheckPrefixLimits() {
+    RetryDelete();
+    if (!IsReady())
+        return true;
+    for (size_t idx = Address::UNSPEC; idx < Address::NUM_FAMILIES; ++idx) {
+        BgpPeerFamilyAttributes *family_attributes =
+            family_attributes_list_[idx];
+        if (!family_attributes || family_attributes->prefix_limit == 0)
+            continue;
+        if (family_primary_path_count_[idx] <= family_attributes->prefix_limit)
+            continue;
+        Clear(BgpProto::Notification::MaxPrefixes);
+        break;
+    }
+    return true;
+}
+
 //
 // Process family attributes configuration and update the family attributes
 // list.
@@ -758,6 +786,7 @@ bool BgpPeer::ProcessFamilyAttributesConfig(const BgpNeighborConfig *config) {
     STLDeleteValues(&family_attributes_list_);
     family_attributes_list_ = family_attributes_list;
     configured_families_ = config->GetAddressFamilies();
+    prefix_limit_trigger_.Set();
     return (ret != 0);
 }
 
@@ -1041,7 +1070,7 @@ const IPeerDebugStats *BgpPeer::peer_stats() const {
 }
 
 void BgpPeer::Clear(int subcode) {
-    CHECK_CONCURRENCY("bgp::Config");
+    CHECK_CONCURRENCY("bgp::Config", "bgp::StateMachine");
     state_machine_->Shutdown(subcode);
 }
 
@@ -1702,6 +1731,18 @@ void BgpPeer::ProcessUpdate(const BgpProto::Update *msg, size_t msgsize) {
     }
 }
 
+void BgpPeer::UpdatePrimaryPathCount(int count, Address::Family family) const {
+    primary_path_count_ += count;
+    if (family == Address::UNSPEC)
+        return;
+    family_primary_path_count_[family] += count;
+    uint32_t limit = 0;
+    if (family_attributes_list_[family])
+        limit = family_attributes_list_[family]->prefix_limit;
+    if (limit && family_primary_path_count_[family] > limit)
+        prefix_limit_trigger_.Set();
+}
+
 void BgpPeer::EndOfRibTimerErrorHandler(string error_name,
                                         string error_message) {
     BGP_LOG_PEER_CRITICAL(Timer, this, BGP_LOG_FLAG_ALL, BGP_PEER_DIR_NA,

src/bgp/bgp_peer.h

@@ -299,9 +299,8 @@ class BgpPeer : public IPeer {
         total_path_count_ += count;
     }
     virtual int GetTotalPathCount() const { return total_path_count_; }
-    virtual void UpdatePrimaryPathCount(int count) const {
-        primary_path_count_ += count;
-    }
+    virtual void UpdatePrimaryPathCount(int count,
+        Address::Family family) const;
     virtual int GetPrimaryPathCount() const { return primary_path_count_; }
 
     void RegisterToVpnTables();
@@ -393,6 +392,7 @@ class BgpPeer : public IPeer {
     void SetInuseAuthKeyInfo(const AuthenticationKey &key, KeyType type);
     void ResetInuseAuthKeyInfo();
 
+    bool CheckPrefixLimits();
     bool ProcessFamilyAttributesConfig(const BgpNeighborConfig *config);
     void ProcessEndpointConfig(const BgpNeighborConfig *config);
 
@@ -422,6 +422,7 @@ class BgpPeer : public IPeer {
     // Global peer index
     int index_;
     TaskTrigger trigger_;
+    mutable TaskTrigger prefix_limit_trigger_;
 
     // The mutex is used to protect the session, keepalive timer and the
     // send ready state.
@@ -468,6 +469,7 @@ class BgpPeer : public IPeer {
     uint32_t local_bgp_id_;     // network order
     uint32_t peer_bgp_id_;      // network order
     FamilyAttributesList family_attributes_list_;
+    mutable std::vector<tbb::atomic<uint32_t> > family_primary_path_count_;
     std::vector<std::string> configured_families_;
     std::vector<std::string> negotiated_families_;
     BgpProto::BgpPeerType peer_type_;

src/bgp/bgp_route.cc

@@ -56,8 +56,9 @@ void BgpRoute::InsertPath(BgpPath *path) {
     Sort(&BgpTable::PathSelection, prev_front);
 
     // Update counters.
-    if (table) table->UpdatePathCount(path, +1);
-    path->UpdatePeerRefCount(+1);
+    if (table)
+        table->UpdatePathCount(path, +1);
+    path->UpdatePeerRefCount(+1, table ? table->family() : Address::UNSPEC);
 }
 
 //
@@ -71,8 +72,9 @@ void BgpRoute::DeletePath(BgpPath *path) {
 
     // Update counters.
     BgpTable *table = static_cast<BgpTable *>(get_table());
-    if (table) table->UpdatePathCount(path, -1);
-    path->UpdatePeerRefCount(-1);
+    if (table)
+        table->UpdatePathCount(path, -1);
+    path->UpdatePeerRefCount(-1, table ? table->family() : Address::UNSPEC);
 
     delete path;
 }

src/bgp/bgp_xmpp_channel.cc

@@ -362,7 +362,8 @@ class BgpXmppChannel::XmppPeer : public IPeer {
     virtual int GetTotalPathCount() const {
         return total_path_count_;
     }
-    virtual void UpdatePrimaryPathCount(int count) const {
+    virtual void UpdatePrimaryPathCount(int count,
+        Address::Family family) const {
         primary_path_count_ += count;
     }
     virtual int GetPrimaryPathCount() const {

src/bgp/ipeer.h

@@ -208,7 +208,8 @@ class IPeer : public IPeerUpdate {
     virtual const std::string GetStateName() const = 0;
     virtual void UpdateTotalPathCount(int count) const = 0;
     virtual int GetTotalPathCount() const = 0;
-    virtual void UpdatePrimaryPathCount(int count) const = 0;
+    virtual void UpdatePrimaryPathCount(int count,
+        Address::Family family = Address::UNSPEC) const = 0;
     virtual int GetPrimaryPathCount() const = 0;
     virtual void MembershipRequestCallback(BgpTable *table) = 0;
     virtual bool MembershipPathCallback(DBTablePartBase *tpart,

src/bgp/l3vpn/test/inetvpn_table_test.cc

@@ -36,7 +36,8 @@ class BgpPeerMock : public IPeer {
     virtual const std::string GetStateName() const { return "UNKNOWN"; }
     virtual void UpdateTotalPathCount(int count) const { }
     virtual int GetTotalPathCount() const { return 0; }
-    virtual void UpdatePrimaryPathCount(int count) const { }
+    virtual void UpdatePrimaryPathCount(int count,
+        Address::Family family) const { }
     virtual int GetPrimaryPathCount() const { return 0; }
     virtual bool IsRegistrationRequired() const { return true; }
     virtual void MembershipRequestCallback(BgpTable *table) { }

src/bgp/test/bgp_evpn_manager_test.cc

@@ -121,7 +121,8 @@ class PeerMock : public IPeer {
     }
     virtual void UpdateTotalPathCount(int count) { }
     virtual int GetTotalPathCount() const { return 0; }
-    virtual void UpdatePrimaryPathCount(int count) const { }
+    virtual void UpdatePrimaryPathCount(int count,
+        Address::Family family) const { }
     virtual int GetPrimaryPathCount() const { return 0; }
     virtual void MembershipRequestCallback(BgpTable *table) { }
     virtual bool MembershipPathCallback(DBTablePartBase *tpart,

src/bgp/test/bgp_ip_test.cc

@@ -52,7 +52,8 @@ class PeerMock : public IPeer {
     virtual const std::string GetStateName() const { return ""; }
     virtual void UpdateTotalPathCount(int count) const { }
     virtual int GetTotalPathCount() const { return 0; }
-    virtual void UpdatePrimaryPathCount(int count) const { }
+    virtual void UpdatePrimaryPathCount(int count,
+        Address::Family family) const { }
     virtual int GetPrimaryPathCount() const { return 0; }
     virtual bool IsRegistrationRequired() const { return true; }
     virtual void MembershipRequestCallback(BgpTable *table) { }
@@ -74,10 +75,12 @@ static const char *cfg_template = "\
         <address>127.0.0.1</address>\
         <port>%d</port>\
         <session to=\'Y\'>\
-            <address-families>\
-                <family>inet</family>\
-                <family>inet6</family>\
-            </address-families>\
+              <family-attributes>\
+                  <address-family>%s</address-family>\
+                  <prefix-limit>\
+                      <maximum>%d</maximum>\
+                  </prefix-limit>\
+              </family-attributes>\
         </session>\
     </bgp-router>\
     <bgp-router name=\'Y\'>\
@@ -86,10 +89,12 @@ static const char *cfg_template = "\
         <address>127.0.0.2</address>\
         <port>%d</port>\
         <session to=\'X\'>\
-            <address-families>\
-                <family>inet</family>\
-                <family>inet6</family>\
-            </address-families>\
+              <family-attributes>\
+                  <address-family>%s</address-family>\
+                  <prefix-limit>\
+                      <maximum>%d</maximum>\
+                  </prefix-limit>\
+              </family-attributes>\
         </session>\
     </bgp-router>\
 </config>\
@@ -141,11 +146,13 @@ class BgpIpTest : public ::testing::Test {
         delete peer2_;
     }
 
-    void Configure() {
+    void Configure(int prefix_limit = 0) {
         char config[4096];
         snprintf(config, sizeof(config), cfg_template,
             bs_x_->session_manager()->GetPort(),
-            bs_y_->session_manager()->GetPort());
+            GetFamily() == Address::INET ? "inet" : "inet6", prefix_limit,
+            bs_y_->session_manager()->GetPort(),
+            GetFamily() == Address::INET ? "inet" : "inet6", prefix_limit);
         bs_x_->Configure(config);
         bs_y_->Configure(config);
     }
@@ -546,6 +553,74 @@ TYPED_TEST(BgpIpTest, MultiplePrefixMultipath) {
     }
 }
 
+//
+// Add multiple prefixes such that the prefix limit configured on Y for
+// the peering to X is exceeded. Verify that the peering keeps flapping.
+// Then increase the prefix limit and verify that all the expected paths
+// exist on Y. Then lower the limit again and verify that the peering
+// keeps flapping again.
+//
+TYPED_TEST(BgpIpTest, PrefixLimit) {
+    BgpServerTestPtr bs_x = this->bs_x_;
+    BgpServerTestPtr bs_y = this->bs_y_;
+    PeerMock *peer1 = this->peer1_;
+    PeerMock *peer2 = this->peer2_;
+    BgpPeer *peer_xy = this->peer_xy_;
+    BgpPeer *peer_yx = this->peer_yx_;
+    const string &master = this->master_;
+
+    this->Configure(DB::PartitionCount() * 2 - 1);
+    task_util::WaitForIdle();
+
+    for (int idx = 1; idx <= DB::PartitionCount() * 2; ++idx) {
+        this->AddRoute(bs_x, peer2, master, this->BuildPrefix(idx),
+            this->BuildNextHopAddress(peer2->ToString()));
+        this->AddRoute(bs_x, peer1, master, this->BuildPrefix(idx),
+            this->BuildNextHopAddress(peer1->ToString()));
+    }
+    for (int idx = 1; idx <= DB::PartitionCount() * 2; ++idx) {
+        this->VerifyPathExists(bs_x, master, this->BuildPrefix(idx),
+            peer2, this->BuildNextHopAddress(peer2->ToString()));
+        this->VerifyPathExists(bs_x, master, this->BuildPrefix(idx),
+            peer1, this->BuildNextHopAddress(peer1->ToString()));
+    }
+
+    TASK_UTIL_EXPECT_TRUE(peer_yx->flap_count() > 3);
+    TASK_UTIL_EXPECT_TRUE(peer_xy->flap_count() > 3);
+
+    this->Configure(DB::PartitionCount() * 2);
+    task_util::WaitForIdle();
+
+    for (int idx = 1; idx <= DB::PartitionCount() * 2; ++idx) {
+        this->VerifyPathExists(bs_x, master, this->BuildPrefix(idx),
+            peer2, this->BuildNextHopAddress(peer2->ToString()));
+        this->VerifyPathExists(bs_x, master, this->BuildPrefix(idx),
+            peer1, this->BuildNextHopAddress(peer1->ToString()));
+        this->VerifyPathExists(bs_y, master, this->BuildPrefix(idx),
+            peer_yx, this->BuildNextHopAddress(peer1->ToString()));
+        this->VerifyPathNoExists(bs_y, master, this->BuildPrefix(idx),
+            peer_yx, this->BuildNextHopAddress(peer2->ToString()));
+    }
+
+    uint64_t flap_count_yx = peer_yx->flap_count();
+    uint64_t flap_count_xy = peer_xy->flap_count();
+
+    this->Configure(DB::PartitionCount() * 2 - 1);
+    task_util::WaitForIdle();
+
+    TASK_UTIL_EXPECT_TRUE(peer_yx->flap_count() > flap_count_yx + 3);
+    TASK_UTIL_EXPECT_TRUE(peer_xy->flap_count() > flap_count_xy + 3);
+
+    for (int idx = 1; idx <= DB::PartitionCount() * 2; ++idx) {
+        this->DeleteRoute(bs_x, peer1, master, this->BuildPrefix(idx));
+        this->DeleteRoute(bs_x, peer2, master, this->BuildPrefix(idx));
+    }
+    for (int idx = 1; idx <= DB::PartitionCount() * 2; ++idx) {
+        this->VerifyRouteNoExists(bs_x, master, this->BuildPrefix(idx));
+        this->VerifyRouteNoExists(bs_y, master, this->BuildPrefix(idx));
+    }
+}
+
 class TestEnvironment : public ::testing::Environment {
     virtual ~TestEnvironment() { }
 };

src/bgp/test/bgp_multicast_test.cc

@@ -86,7 +86,8 @@ class XmppPeerMock : public IPeer {
     virtual bool SendUpdate(const uint8_t *msg, size_t msgsize) { return true; }
     virtual void UpdateTotalPathCount(int count) const { }
     virtual int GetTotalPathCount() const { return 0; }
-    virtual void UpdatePrimaryPathCount(int count) const { }
+    virtual void UpdatePrimaryPathCount(int count,
+        Address::Family family) const { }
     virtual int GetPrimaryPathCount() const { return 0; }
     virtual void MembershipRequestCallback(BgpTable *table) { }
     virtual bool MembershipPathCallback(DBTablePartBase *tpart,

src/bgp/test/bgp_route_test.cc

@@ -50,7 +50,8 @@ class PeerMock : public IPeer {
     virtual uint32_t bgp_identifier() const { return address_.to_ulong(); }
     virtual void UpdateTotalPathCount(int count) const { }
     virtual int GetTotalPathCount() const { return 0; }
-    virtual void UpdatePrimaryPathCount(int count) const { }
+    virtual void UpdatePrimaryPathCount(int count,
+        Address::Family family) const { }
     virtual int GetPrimaryPathCount() const { return 0; }
     virtual bool IsRegistrationRequired() const { return true; }
     virtual void MembershipRequestCallback(BgpTable *table) { }

src/bgp/test/bgp_table_export_test.cc

@@ -85,7 +85,8 @@ class BgpTestPeer : public IPeer {
     virtual const string GetStateName() const { return ""; }
     virtual void UpdateTotalPathCount(int count) const { }
     virtual int GetTotalPathCount() const { return 0; }
-    virtual void UpdatePrimaryPathCount(int count) const { }
+    virtual void UpdatePrimaryPathCount(int count,
+        Address::Family family) const { }
     virtual int GetPrimaryPathCount() const { return 0; }
     virtual bool IsRegistrationRequired() const { return true; }
     virtual void MembershipRequestCallback(BgpTable *table) { }