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

src/bgp/bgp_attr.h

@@ -857,7 +857,8 @@ class BgpAttr {
     LabelBlockPtr label_block() const { return label_block_; }
     BgpOListPtr olist() const { return olist_; }
     BgpOListPtr leaf_olist() const { return leaf_olist_; }
-    BgpAttrDB *attr_db() const { return attr_db_; }
+    BgpAttrDB *attr_db() { return attr_db_; }
+    const BgpAttrDB *attr_db() const { return attr_db_; }
     uint32_t sequence_number() const;
     bool evpn_sticky_mac() const;
     bool etree_leaf() const;
@@ -953,6 +954,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_;

src/bgp/bgp_config.h

@@ -558,14 +558,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() { }
 
@@ -583,6 +584,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_;
@@ -592,6 +597,8 @@ class BgpGlobalSystemConfig {
     bool gr_enable_;
     bool gr_bgp_helper_;
     bool gr_xmpp_helper_;
+    bool always_compare_med_;
+
     DISALLOW_COPY_AND_ASSIGN(BgpGlobalSystemConfig);
 };
 

src/bgp/bgp_config_ifmap.cc

@@ -1864,6 +1864,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;
 }
 

src/bgp/bgp_config_ifmap.h

@@ -334,6 +334,7 @@ class BgpIfmapGlobalSystemConfig {
     const BgpGlobalSystemConfig *config() const { return &data_; }
     bool Update(BgpIfmapConfigManager *manager,
                 const autogen::GlobalSystemConfig *system);
+
 private:
     BgpGlobalSystemConfig data_;
 };
@@ -343,6 +344,7 @@ class BgpIfmapGlobalQosConfig {
     const BgpGlobalQosConfig *config() const { return &data_; }
     bool Update(BgpIfmapConfigManager *manager,
                 const autogen::GlobalQosConfig *qos);
+
 private:
     BgpGlobalQosConfig data_;
 };

src/bgp/bgp_config_parser.cc

@@ -668,6 +668,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;
 }

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;
@@ -90,9 +91,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)

src/bgp/bgp_server.cc

@@ -85,6 +85,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();

src/bgp/bgp_server.h

@@ -254,6 +254,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();
+    }
     BgpGlobalQosConfig *global_qos() { return global_qos_.get(); }
     bool gr_helper_disable() const { return gr_helper_disable_; }
     void set_gr_helper_disable(bool gr_helper_disable) {

src/bgp/test/bgp_config_test.cc

@@ -3755,6 +3755,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();

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/extended-community/etree.h"
@@ -615,6 +616,132 @@ TEST_F(BgpRouteTest, PathCompareMed3) {
     EXPECT_EQ(0, path2.PathCompare(path1, true));
 }
 
+//
+// Paths with 0 neighbor as are not compared w.r.t med.
+// Path with higher med happens to win since it has lower router id.
+// First segment in both paths is an AS_SET.
+// Paths with different MEDs are considered ECMP as leftmost AS is 0.
+//
+TEST_F(BgpRouteTest, PathCompareMed4) {
+    boost::system::error_code ec;
+    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_SET;
+    ps1->path_segment.push_back(64512);
+    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_SET;
+    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(0, path1.PathCompare(path2, true));
+    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));
+}
+
 //
 // Paths with sticky bit is chosen
 // Test with path having same seq no and different sticky bit
@@ -836,50 +963,6 @@ TEST_F(BgpRouteTest, PathCompareETree1) {
     EXPECT_EQ(0, path2.PathCompare(path1, true));
 }
 
-//
-// Paths with 0 neighbor as are not compared w.r.t med.
-// Path with higher med happens to win since it has lower router id.
-// First segment in both paths is an AS_SET.
-// Paths with different MEDs are considered ECMP as leftmost AS is 0.
-//
-TEST_F(BgpRouteTest, PathCompareMed4) {
-    boost::system::error_code ec;
-    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_SET;
-    ps1->path_segment.push_back(64512);
-    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_SET;
-    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(0, path1.PathCompare(path2, true));
-    EXPECT_EQ(0, path2.PathCompare(path1, true));
-}
-
 TEST_F(BgpRouteTest, CheckPrimaryPathsCountInTable1) {
     boost::system::error_code ec;
     BgpAttrDB *db = server_.attr_db();

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>