Skip to content

Commit

Permalink
Ignore AS path length when comparing service chain paths for ECMP
Browse files Browse the repository at this point in the history 
Change-Id: I53226017cac9a7ddfd24f693a6161faca14cc8b4
Closes-Bug: 1606737
  • Loading branch information
Nischal Sheth committed Jul 27, 2016
1 parent 306d40e commit 893a82c
Show file tree
Hide file tree
Showing 2 changed files with 110 additions and 2 deletions.
11 changes: 10 additions & 1 deletion src/bgp/bgp_path.cc
View file
Expand Up @@ -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());

Expand All @@ -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);

Expand Down
101 changes: 100 additions & 1 deletion src/bgp/test/bgp_route_test.cc
View file
Expand Up @@ -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"

Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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.
//
Expand Down

0 comments on commit 893a82c

Please sign in to comment.