From fb7f2ea2e65591ec76b7d098946cbd025c42e66c Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Tue, 26 Jul 2016 17:07:26 -0700 Subject: [PATCH] Ignore AS path length when comparing service chain paths for ECMP Change-Id: I53226017cac9a7ddfd24f693a6161faca14cc8b4 Closes-Bug: 1606737 --- src/bgp/bgp_path.cc | 11 +++- src/bgp/test/bgp_route_test.cc | 101 ++++++++++++++++++++++++++++++++- 2 files changed, 110 insertions(+), 2 deletions(-) diff --git a/src/bgp/bgp_path.cc b/src/bgp/bgp_path.cc index 08d5762b3e6..e2a96c056f2 100644 --- a/src/bgp/bgp_path.cc +++ b/src/bgp/bgp_path.cc @@ -75,7 +75,11 @@ int BgpPath::PathCompare(const BgpPath &rhs, bool allow_ecmp) const { KEY_COMPARE(llgr_stale, rllgr_stale); - KEY_COMPARE(attr_->as_path_count(), rattr->as_path_count()); + // Do not compare as path length for service chain paths at this point. + // We want to treat service chain paths as ECMP irrespective of as path + // length. + if (!attr_->origin_vn_path() || !rattr->origin_vn_path()) + KEY_COMPARE(attr_->as_path_count(), rattr->as_path_count()); KEY_COMPARE(attr_->origin(), rattr->origin()); @@ -87,6 +91,11 @@ int BgpPath::PathCompare(const BgpPath &rhs, bool allow_ecmp) const { if (allow_ecmp) return 0; + // Compare as path length for service chain paths since we bypassed the + // check previously. + if (attr_->origin_vn_path() && rattr->origin_vn_path()) + KEY_COMPARE(attr_->as_path_count(), rattr->as_path_count()); + // Prefer locally generated routes over bgp and xmpp routes. BOOL_COMPARE(peer_ == NULL, rhs.peer_ == NULL); diff --git a/src/bgp/test/bgp_route_test.cc b/src/bgp/test/bgp_route_test.cc index 40e91f05e0c..b36cb1130ad 100644 --- a/src/bgp/test/bgp_route_test.cc +++ b/src/bgp/test/bgp_route_test.cc @@ -7,6 +7,7 @@ #include "bgp/bgp_log.h" #include "bgp/bgp_server.h" #include "bgp/inet/inet_route.h" +#include "bgp/origin-vn/origin_vn.h" #include "control-node/control_node.h" #include "net/community_type.h" @@ -129,7 +130,7 @@ TEST_F(BgpRouteTest, Paths) { // // Path with shorter AS Path is better - even when building ECMP nexthops. // -TEST_F(BgpRouteTest, PathCompareAsPathLength) { +TEST_F(BgpRouteTest, PathCompareAsPathLength1) { boost::system::error_code ec; BgpAttrDB *db = server_.attr_db(); @@ -168,6 +169,104 @@ TEST_F(BgpRouteTest, PathCompareAsPathLength) { EXPECT_EQ(1, path2.PathCompare(path1, true)); } +// +// AS Path length is ignored for service chain paths - only when building ECMP +// nexthops. +// +TEST_F(BgpRouteTest, PathCompareAsPathLength2) { + 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_SEQUENCE; + ps1->path_segment.push_back(64512); + ps1->path_segment.push_back(64512); + aspath_spec1.path_segments.push_back(ps1); + spec1.push_back(&aspath_spec1); + OriginVnPathSpec ovn_spec1; + OriginVn origin_vn1(64512, 999); + ovn_spec1.origin_vns.push_back(origin_vn1.GetExtCommunityValue()); + spec1.push_back(&ovn_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(100); + 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(64512); + ps2->path_segment.push_back(64512); + aspath_spec2.path_segments.push_back(ps2); + spec2.push_back(&aspath_spec2); + OriginVnPathSpec ovn_spec2; + OriginVn origin_vn2(64512, 999); + ovn_spec2.origin_vns.push_back(origin_vn2.GetExtCommunityValue()); + spec2.push_back(&ovn_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)); +} + +// +// AS Path length is not ignored when comparing a service chain path and a +// regular path - even when building ECMP nexthops. +// +TEST_F(BgpRouteTest, PathCompareAsPathLength3) { + 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_SEQUENCE; + ps1->path_segment.push_back(64512); + ps1->path_segment.push_back(64512); + aspath_spec1.path_segments.push_back(ps1); + spec1.push_back(&aspath_spec1); + OriginVnPathSpec ovn_spec1; + OriginVn origin_vn1(64512, 999); + ovn_spec1.origin_vns.push_back(origin_vn1.GetExtCommunityValue()); + spec1.push_back(&ovn_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(100); + 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(64512); + ps2->path_segment.push_back(64512); + 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 same router id are considered are equal. //