Implement knob to always compare med

Always compare med even if paths have an empty AS path or are learnt from different neighbor ASes if the knob is set. Change-Id: Ia831ae01ec7deb1359e9842a0220264923788fa3 Closes-Bug: 1680005
Juniper · Jun 10, 2017 · ccfd6d7 · ccfd6d7
1 parent 2de3241
commit ccfd6d7
Show file tree

Hide file tree

Showing 14 changed files with 209 additions and 6 deletions.
diff --git a/src/bgp/bgp_attr.h b/src/bgp/bgp_attr.h
@@ -942,6 +942,7 @@ class BgpAttrDB : public BgpPathAttributeDB<BgpAttr, BgpAttrPtr, BgpAttrSpec,
     BgpAttrPtr ReplaceNexthopAndLocate(const BgpAttr *attr,
                                        const IpAddress &addr);
     BgpServer *server() { return server_; }
+    const BgpServer *server() const { return server_; }
 
 private:
     BgpServer *server_;

diff --git a/src/bgp/bgp_config.h b/src/bgp/bgp_config.h
@@ -531,14 +531,15 @@ class BgpProtocolConfig {
     DISALLOW_COPY_AND_ASSIGN(BgpProtocolConfig);
 };
 
-// Route Policy configuration.
+// Global system configuration.
 class BgpGlobalSystemConfig {
 public:
     static const int kEndOfRibTime = 300; // seconds
     BgpGlobalSystemConfig() :
             last_change_at_(0), gr_time_(0), llgr_time_(0),
             end_of_rib_timeout_(kEndOfRibTime), gr_enable_(false),
-            gr_bgp_helper_(false), gr_xmpp_helper_(false) {
+            gr_bgp_helper_(false), gr_xmpp_helper_(false),
+            always_compare_med_(false) {
     }
     ~BgpGlobalSystemConfig() { }
 
@@ -556,6 +557,10 @@ class BgpGlobalSystemConfig {
     void set_gr_xmpp_helper(bool helper) { gr_xmpp_helper_ = helper; }
     bool gr_enable() const { return gr_enable_; }
     void set_gr_enable(bool enable) { gr_enable_ = enable; }
+    bool always_compare_med() const { return always_compare_med_; }
+    void set_always_compare_med(bool always_compare_med) {
+        always_compare_med_ = always_compare_med;
+    }
 
 private:
     mutable uint64_t last_change_at_;
@@ -565,6 +570,8 @@ class BgpGlobalSystemConfig {
     bool gr_enable_;
     bool gr_bgp_helper_;
     bool gr_xmpp_helper_;
+    bool always_compare_med_;
+
     DISALLOW_COPY_AND_ASSIGN(BgpGlobalSystemConfig);
 };
 

diff --git a/src/bgp/bgp_config_ifmap.cc b/src/bgp/bgp_config_ifmap.cc
@@ -1844,6 +1844,11 @@ bool BgpIfmapGlobalSystemConfig::Update(BgpIfmapConfigManager *manager,
         changed |= true;
     }
 
+    if (data_.always_compare_med() != system->bgp_always_compare_med()) {
+        data_.set_always_compare_med(system->bgp_always_compare_med());
+        changed |= true;
+    }
+
     return changed;
 }
 

diff --git a/src/bgp/bgp_config_ifmap.h b/src/bgp/bgp_config_ifmap.h
@@ -334,11 +334,11 @@ class BgpIfmapGlobalSystemConfig {
     const BgpGlobalSystemConfig *config() const { return &data_; }
     bool Update(BgpIfmapConfigManager *manager,
                 const autogen::GlobalSystemConfig *system);
+
 private:
     BgpGlobalSystemConfig data_;
 };
 
-
 //
 // BgpConfigData contains all the configuration data that's relevant to a
 // node. The BgpConfigManager has a pointer to BgpConfigData.

diff --git a/src/bgp/bgp_config_parser.cc b/src/bgp/bgp_config_parser.cc
@@ -655,6 +655,18 @@ bool BgpConfigParser::ParseGlobalSystemConfig(const xml_node &node,
                                        "graceful-restart-parameters", requests);
             }
         }
+        if (strcmp(child.name(), "bgp-always-compare-med") == 0) {
+            auto_ptr<autogen::GlobalSystemConfig::OolProperty> property(
+                new autogen::GlobalSystemConfig::OolProperty);
+            property->data = (string(child.child_value()) == "true");
+            if (add_change) {
+                MapObjectSetProperty("global-system-config", "",
+                    "bgp-always-compare-med", property.release(), requests);
+            } else {
+                MapObjectClearProperty("global-system-config", "",
+                    "bgp-always-compare-med", requests);
+            }
+        }
     }
     return true;
 }

diff --git a/src/bgp/bgp_path.cc b/src/bgp/bgp_path.cc
@@ -6,8 +6,9 @@
 
 #include <boost/foreach.hpp>
 
-#include "bgp/bgp_route.h"
 #include "bgp/bgp_peer.h"
+#include "bgp/bgp_route.h"
+#include "bgp/bgp_server.h"
 #include "net/community_type.h"
 
 using std::string;
@@ -83,9 +84,14 @@ int BgpPath::PathCompare(const BgpPath &rhs, bool allow_ecmp) const {
 
     KEY_COMPARE(attr_->origin(), rattr->origin());
 
-    // Compare med if both paths are learnt from the same neighbor as.
-    if (attr_->neighbor_as() && attr_->neighbor_as() == rattr->neighbor_as())
+    // Compare med if always compare med knob is enabled or if both paths are
+    // learnt from the same neighbor as.
+    const BgpServer *server = attr_->attr_db()->server();
+    if (server->global_config()->always_compare_med() ||
+        (attr_->neighbor_as() &&
+         attr_->neighbor_as() == rattr->neighbor_as())) {
         KEY_COMPARE(attr_->med(), rattr->med());
+    }
 
     // For ECMP paths, above checks should suffice.
     if (allow_ecmp)

diff --git a/src/bgp/bgp_server.cc b/src/bgp/bgp_server.cc
@@ -75,6 +75,14 @@ class BgpServer::ConfigUpdater {
         server_->global_config()->set_gr_bgp_helper(
                 system->gr_bgp_helper());
 
+        // Clear peers if there's a change in always-compare-med knob.
+        if (server_->global_config()->always_compare_med() !=
+            system->always_compare_med()) {
+            server_->global_config()->set_always_compare_med(
+                system->always_compare_med());
+            clear_peers = true;
+        }
+
         if (!clear_peers)
             return;
         RoutingInstanceMgr *ri_mgr = server_->routing_instance_mgr();

diff --git a/src/bgp/bgp_server.h b/src/bgp/bgp_server.h
@@ -246,6 +246,9 @@ class BgpServer {
     uint32_t GetStaticRouteCount() const;
     uint32_t GetDownStaticRouteCount() const;
     BgpGlobalSystemConfig *global_config() { return global_config_.get(); }
+    const BgpGlobalSystemConfig *global_config() const {
+        return global_config_.get();
+    }
     bool gr_helper_disable() const { return gr_helper_disable_; }
     void set_gr_helper_disable(bool gr_helper_disable) {
         gr_helper_disable_ = gr_helper_disable;

diff --git a/src/bgp/test/bgp_config_test.cc b/src/bgp/test/bgp_config_test.cc
@@ -3581,6 +3581,34 @@ TEST_F(BgpConfigTest, BgpRouterLongLivedGracefulRestartTimeChange) {
     TASK_UTIL_EXPECT_EQ(0, db_graph_.vertex_count());
 }
 
+TEST_F(BgpConfigTest, BgpAlwaysCompareMedChange) {
+    string content_a =
+        FileRead("controller/src/bgp/testdata/config_test_46a.xml");
+    EXPECT_TRUE(parser_.Parse(content_a));
+    task_util::WaitForIdle();
+    TASK_UTIL_EXPECT_FALSE(server_.global_config()->always_compare_med());
+
+    string content_b =
+        FileRead("controller/src/bgp/testdata/config_test_46b.xml");
+    EXPECT_TRUE(parser_.Parse(content_b));
+    task_util::WaitForIdle();
+    TASK_UTIL_EXPECT_TRUE(server_.global_config()->always_compare_med());
+
+    string content_c =
+        FileRead("controller/src/bgp/testdata/config_test_46c.xml");
+    EXPECT_TRUE(parser_.Parse(content_c));
+    task_util::WaitForIdle();
+    TASK_UTIL_EXPECT_FALSE(server_.global_config()->always_compare_med());
+
+    boost::replace_all(content_c, "<config>", "<delete>");
+    boost::replace_all(content_c, "</config>", "</delete>");
+    EXPECT_TRUE(parser_.Parse(content_c));
+    task_util::WaitForIdle();
+
+    TASK_UTIL_EXPECT_EQ(0, db_graph_.edge_count());
+    TASK_UTIL_EXPECT_EQ(0, db_graph_.vertex_count());
+}
+
 int main(int argc, char **argv) {
     bgp_log_test::init();
     ControlNode::SetDefaultSchedulingPolicy();

diff --git a/src/bgp/test/bgp_route_test.cc b/src/bgp/test/bgp_route_test.cc
@@ -4,6 +4,7 @@
 
 
 #include "base/test/task_test_util.h"
+#include "bgp/bgp_config.h"
 #include "bgp/bgp_log.h"
 #include "bgp/bgp_server.h"
 #include "bgp/inet/inet_route.h"
@@ -657,6 +658,88 @@ TEST_F(BgpRouteTest, PathCompareMed4) {
     EXPECT_EQ(0, path2.PathCompare(path1, true));
 }
 
+//
+// Paths with empty as path are compared w.r.t med if always compare med knob
+// is set.
+// Path with lower med is better.
+// Both paths have empty as paths.
+// Paths with different MEDs are not considered ECMP.
+//
+TEST_F(BgpRouteTest, PathCompareMed5) {
+    boost::system::error_code ec;
+    server_.global_config()->set_always_compare_med(true);
+    BgpAttrDB *db = server_.attr_db();
+
+    BgpAttrSpec spec1;
+    BgpAttrMultiExitDisc med_spec1(100);
+    spec1.push_back(&med_spec1);
+    AsPathSpec aspath_spec1;
+    spec1.push_back(&aspath_spec1);
+    BgpAttrPtr attr1 = db->Locate(spec1);
+    PeerMock peer1(BgpProto::IBGP, Ip4Address::from_string("10.1.1.2", ec));
+    BgpPath path1(&peer1, BgpPath::BGP_XMPP, attr1, 0, 0);
+
+    BgpAttrSpec spec2;
+    BgpAttrMultiExitDisc med_spec2(200);
+    spec2.push_back(&med_spec2);
+    AsPathSpec aspath_spec2;
+    spec2.push_back(&aspath_spec2);
+    BgpAttrPtr attr2 = db->Locate(spec2);
+    PeerMock peer2(BgpProto::IBGP, Ip4Address::from_string("10.1.1.1", ec));
+    BgpPath path2(&peer2, BgpPath::BGP_XMPP, attr2, 0, 0);
+
+    EXPECT_EQ(-1, path1.PathCompare(path2, false));
+    EXPECT_EQ(1, path2.PathCompare(path1, false));
+    EXPECT_EQ(-1, path1.PathCompare(path2, true));
+    EXPECT_EQ(1, path2.PathCompare(path1, true));
+}
+
+//
+// Paths with different neighbor as are compared w.r.t med if always compare
+// med knob is set.
+// Path with lower med is better.
+// Both paths have as paths, but the leftmost as is different.
+// Paths with different MEDs are not considered ECMP.
+//
+TEST_F(BgpRouteTest, PathCompareMed6) {
+    boost::system::error_code ec;
+    server_.global_config()->set_always_compare_med(true);
+    BgpAttrDB *db = server_.attr_db();
+
+    BgpAttrSpec spec1;
+    BgpAttrMultiExitDisc med_spec1(100);
+    spec1.push_back(&med_spec1);
+    AsPathSpec aspath_spec1;
+    AsPathSpec::PathSegment *ps1 = new AsPathSpec::PathSegment;
+    ps1->path_segment_type = AsPathSpec::PathSegment::AS_SEQUENCE;
+    ps1->path_segment.push_back(64514);
+    ps1->path_segment.push_back(64513);
+    aspath_spec1.path_segments.push_back(ps1);
+    spec1.push_back(&aspath_spec1);
+    BgpAttrPtr attr1 = db->Locate(spec1);
+    PeerMock peer1(BgpProto::EBGP, Ip4Address::from_string("10.1.1.2", ec));
+    BgpPath path1(&peer1, BgpPath::BGP_XMPP, attr1, 0, 0);
+
+    BgpAttrSpec spec2;
+    BgpAttrMultiExitDisc med_spec2(200);
+    spec2.push_back(&med_spec2);
+    AsPathSpec aspath_spec2;
+    AsPathSpec::PathSegment *ps2 = new AsPathSpec::PathSegment;
+    ps2->path_segment_type = AsPathSpec::PathSegment::AS_SEQUENCE;
+    ps2->path_segment.push_back(64512);
+    ps2->path_segment.push_back(64513);
+    aspath_spec2.path_segments.push_back(ps2);
+    spec2.push_back(&aspath_spec2);
+    BgpAttrPtr attr2 = db->Locate(spec2);
+    PeerMock peer2(BgpProto::EBGP, Ip4Address::from_string("10.1.1.1", ec));
+    BgpPath path2(&peer2, BgpPath::BGP_XMPP, attr2, 0, 0);
+
+    EXPECT_EQ(-1, path1.PathCompare(path2, false));
+    EXPECT_EQ(1, path2.PathCompare(path1, false));
+    EXPECT_EQ(-1, path1.PathCompare(path2, true));
+    EXPECT_EQ(1, path2.PathCompare(path1, true));
+}
+
 TEST_F(BgpRouteTest, CheckPrimaryPathsCountInTable1) {
     boost::system::error_code ec;
     BgpAttrDB *db = server_.attr_db();

diff --git a/src/bgp/testdata/config_test_46a.xml b/src/bgp/testdata/config_test_46a.xml
@@ -0,0 +1,13 @@
+<?xml version="1.0" encoding="utf-8"?>
+<config>
+    <routing-instance name='default-domain:default-project:ip-fabric:__default__'>
+      <bgp-router name='local'>
+          <address>127.0.0.1</address>
+          <autonomous-system>1</autonomous-system>
+      </bgp-router>
+      <bgp-router name='remote'>
+          <address>127.0.0.100</address>
+          <autonomous-system>1</autonomous-system>
+      </bgp-router>
+    </routing-instance>
+</config>
diff --git a/src/bgp/testdata/config_test_46b.xml b/src/bgp/testdata/config_test_46b.xml
@@ -0,0 +1,16 @@
+<?xml version="1.0" encoding="utf-8"?>
+<config>
+    <global-system-config>
+        <bgp-always-compare-med>true</bgp-always-compare-med>
+    </global-system-config>
+    <routing-instance name='default-domain:default-project:ip-fabric:__default__'>
+      <bgp-router name='local'>
+          <address>127.0.0.1</address>
+          <autonomous-system>1</autonomous-system>
+      </bgp-router>
+      <bgp-router name='remote'>
+          <address>127.0.0.100</address>
+          <autonomous-system>1</autonomous-system>
+      </bgp-router>
+    </routing-instance>
+</config>
diff --git a/src/bgp/testdata/config_test_46c.xml b/src/bgp/testdata/config_test_46c.xml
@@ -0,0 +1,16 @@
+<?xml version="1.0" encoding="utf-8"?>
+<config>
+    <global-system-config>
+        <bgp-always-compare-med>false</bgp-always-compare-med>
+    </global-system-config>
+    <routing-instance name='default-domain:default-project:ip-fabric:__default__'>
+      <bgp-router name='local'>
+          <address>127.0.0.1</address>
+          <autonomous-system>1</autonomous-system>
+      </bgp-router>
+      <bgp-router name='remote'>
+          <address>127.0.0.100</address>
+          <autonomous-system>1</autonomous-system>
+      </bgp-router>
+    </routing-instance>
+</config>
diff --git a/src/schema/vnc_cfg.xsd b/src/schema/vnc_cfg.xsd
@@ -929,6 +929,11 @@ targetNamespace="http://www.contrailsystems.com/2012/VNC-CONFIG/0">
      Property('ibgp-auto-mesh', 'global-system-config', 'optional', 'CRUD',
               'When true, system will automatically create BGP peering mesh with all control-nodes that have same BGP AS number as global AS number.') -->
 
+<xsd:element name="bgp-always-compare-med" type="xsd:boolean"/>
+<!--#IFMAP-SEMANTICS-IDL
+     Property('bgp-always-compare-med', 'global-system-config', 'optional', 'CRUD',
+              'Always compare MED even if paths are received from different ASes.') -->
+
 <xsd:element name="ip-fabric-subnets" type="SubnetListType"/>
 <!--#IFMAP-SEMANTICS-IDL
      Property('ip-fabric-subnets', 'global-system-config', 'optional', 'CRUD',