From b8be6ee525da2b9b588ffe821b93f9dc24de1c56 Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Thu, 8 Jun 2017 14:02:58 -0700 Subject: [PATCH] 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 | 4 +- src/bgp/bgp_config.h | 11 +- src/bgp/bgp_config_ifmap.cc | 5 + src/bgp/bgp_config_ifmap.h | 2 + src/bgp/bgp_config_parser.cc | 12 ++ src/bgp/bgp_path.cc | 12 +- src/bgp/bgp_server.cc | 8 ++ src/bgp/bgp_server.h | 3 + src/bgp/test/bgp_config_test.cc | 28 +++++ src/bgp/test/bgp_route_test.cc | 171 ++++++++++++++++++++------- src/bgp/testdata/config_test_46a.xml | 13 ++ src/bgp/testdata/config_test_46b.xml | 16 +++ src/bgp/testdata/config_test_46c.xml | 16 +++ src/schema/vnc_cfg.xsd | 5 + 14 files changed, 256 insertions(+), 50 deletions(-) create mode 100644 src/bgp/testdata/config_test_46a.xml create mode 100644 src/bgp/testdata/config_test_46b.xml create mode 100644 src/bgp/testdata/config_test_46c.xml diff --git a/src/bgp/bgp_attr.h b/src/bgp/bgp_attr.h index de9424e06ea..009e5473f2f 100644 --- a/src/bgp/bgp_attr.h +++ b/src/bgp/bgp_attr.h @@ -852,7 +852,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; @@ -946,6 +947,7 @@ class BgpAttrDB : public BgpPathAttributeDBbgp_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 index 7e2daf00743..d84197d2566 100644 --- a/src/bgp/bgp_config_ifmap.h +++ b/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_; }; diff --git a/src/bgp/bgp_config_parser.cc b/src/bgp/bgp_config_parser.cc index ab233b5f6ea..3cc1083c864 100644 --- a/src/bgp/bgp_config_parser.cc +++ b/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 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 index 79d081e5187..1d4eaad8899 100644 --- a/src/bgp/bgp_path.cc +++ b/src/bgp/bgp_path.cc @@ -6,8 +6,9 @@ #include -#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) diff --git a/src/bgp/bgp_server.cc b/src/bgp/bgp_server.cc index add265e33b4..b656517d6dc 100644 --- a/src/bgp/bgp_server.cc +++ b/src/bgp/bgp_server.cc @@ -86,6 +86,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 index 565da3acc98..303e9453e19 100644 --- a/src/bgp/bgp_server.h +++ b/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) { diff --git a/src/bgp/test/bgp_config_test.cc b/src/bgp/test/bgp_config_test.cc index a542d67e84e..db82ab9589c 100644 --- a/src/bgp/test/bgp_config_test.cc +++ b/src/bgp/test/bgp_config_test.cc @@ -3741,6 +3741,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, "", ""); + boost::replace_all(content_c, "", ""); + 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 index a36a4b01529..1ac149badfc 100644 --- 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/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(); diff --git a/src/bgp/testdata/config_test_46a.xml b/src/bgp/testdata/config_test_46a.xml new file mode 100644 index 00000000000..8e694ab0fce --- /dev/null +++ b/src/bgp/testdata/config_test_46a.xml @@ -0,0 +1,13 @@ + + + + +
127.0.0.1
+ 1 + + +
127.0.0.100
+ 1 + + + diff --git a/src/bgp/testdata/config_test_46b.xml b/src/bgp/testdata/config_test_46b.xml new file mode 100644 index 00000000000..a776c82f17a --- /dev/null +++ b/src/bgp/testdata/config_test_46b.xml @@ -0,0 +1,16 @@ + + + + true + + + +
127.0.0.1
+ 1 + + +
127.0.0.100
+ 1 + + + diff --git a/src/bgp/testdata/config_test_46c.xml b/src/bgp/testdata/config_test_46c.xml new file mode 100644 index 00000000000..e9dc792d190 --- /dev/null +++ b/src/bgp/testdata/config_test_46c.xml @@ -0,0 +1,16 @@ + + + + false + + + +
127.0.0.1
+ 1 + + +
127.0.0.100
+ 1 + + + diff --git a/src/schema/vnc_cfg.xsd b/src/schema/vnc_cfg.xsd index 21d22144dea..34ebd3cc720 100644 --- a/src/schema/vnc_cfg.xsd +++ b/src/schema/vnc_cfg.xsd @@ -948,6 +948,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.') --> + + +