Include AS path length check in ECMP decision

Note that this changes ecmp behavior from older releases, but this is
deemed to be fine since the new behavior is what most users will expect
by default.

A new knob/option to ignore AS path length can be added if/when needed.

Change-Id: I394f0a683d1ef1fa3db5bd983829acf4aab2c684
Closes-Bug: 1582452

src/bgp/bgp_path.cc

@@ -75,12 +75,12 @@ int BgpPath::PathCompare(const BgpPath &rhs, bool allow_ecmp) const {
 
     KEY_COMPARE(llgr_stale, rllgr_stale);
 
-    // For ECMP paths, above checks should suffice
+    KEY_COMPARE(attr_->as_path_count(), rattr->as_path_count());
+
+    // For ECMP paths, above checks should suffice.
     if (allow_ecmp)
         return 0;
 
-    KEY_COMPARE(attr_->as_path_count(), rattr->as_path_count());
-
     KEY_COMPARE(attr_->origin(), rattr->origin());
 
     // Compare med if both paths are learnt from the same neighbor as.

src/bgp/test/bgp_route_test.cc

@@ -124,6 +124,48 @@ TEST_F(BgpRouteTest, Paths) {
     route.RemovePath(&peer);
 }
 
+//
+// Path with shorter AS Path is better - even when building ECMP nexthops.
+//
+TEST_F(BgpRouteTest, PathCompareAsPathLength) {
+    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);
+    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.
 //