cluster-id length equality for multipath

A fat tree topology running IBGP gets into two issues with anycast address
routing. Consider the following topology:

        R9   R10
          x x
  R3   R4     R7   R8
     x           x
  R1   R2     R5   R6
  |    |      |    |
 10/8 10/8  10/8   S

Let's remind ourselves of BGP decision process steps:

1. Highest Local Preference
2. Shortest AS Path Length
3. Lowest Origin Type
4. Lowest MED (Multi-Exit Discriminator)
5. Prefer External to Internal
6. Closest Egress (Lowest IGP Distance)
7. Tie Breaking (Lowest-Router-ID)
8. Tie Breaking (Lowest-cluster-list length)
9. Tie Breaking (Lowest-neighbor-address)

Without any policies, steps 1-6 will almost always evaluate identically for
all paths received on any router in the above topology. Let's assume that
the router-ids follow the following inequality: R1 < R2 < R5 < R6. Owing to
the 7th step above, all routers will now choose R1's path as the best. This
is undesirable. As an example, traffic from S to 10/8 will follow the path
S -> R6 -> R7 -> R9 -> R4 -> R2 -> 10/8 instead of S -> R6 -> R7 -> R5 -> 10/8.
Furthermore, once R7 (& R8) chooses R1's path as the best, it would withdraw
its path learned through (R5, R6) from (R9, R10). This leads to inefficient
load balancing - e.g. R9 can't do ECMP across all available egresses -
(R1, R2, R5).

The patch addresses these issues by noting that that cluster list is always
carried along with the routes and its length is a good indicator of IBGP
hops. It thus makes sense to compare that as an extension to metric after
step 6. That automatically ensures correct multipath computation.

Unfortunately a partial deployment of this in a generic topology (note:
fat-tree/clos topologies work fine) may lead to potential loops. It needs
to be looked into.

Signed-off-by: Pradosh Mohapatra <pmohapat@cumulusnetworks.com>
Reviewed-by: Dinesh G Dutt <ddutt@cumulusnetworks.com>

diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h
index cb401e71181765d2aa3cedb95e51cf53305b2048..7edf684021245af9755602629958c19f285133f6 100644 (file)
--- a/bgpd/bgp_attr.h
+++ b/bgpd/bgp_attr.h
@@ -132,6 +132,10 @@ struct transit
 
 #define ATTR_FLAG_BIT(X)  (1 << ((X) - 1))
 
+#define BGP_CLUSTER_LIST_LENGTH(attr)                          \
+  (((attr)->flag & ATTR_FLAG_BIT(BGP_ATTR_CLUSTER_LIST)) ?     \
+   (attr)->extra->cluster->length : 0)
+
 typedef enum {
  BGP_ATTR_PARSE_PROCEED = 0,
  BGP_ATTR_PARSE_ERROR = -1,

diff --git a/bgpd/bgp_mpath.c b/bgpd/bgp_mpath.c
index 7999d16b6636b5c100d0de4e09e2f607e8d222a6..5590b0513bc3a0c5f0c9ea79edfe8da98758700e 100644 (file)
--- a/bgpd/bgp_mpath.c
+++ b/bgpd/bgp_mpath.c
@@ -46,7 +46,7 @@
  */
 int
 bgp_maximum_paths_set (struct bgp *bgp, afi_t afi, safi_t safi,
-                       int peertype, u_int16_t maxpaths)
+                       int peertype, u_int16_t maxpaths, u_int16_t options)
 {
   if (!bgp || (afi >= AFI_MAX) || (safi >= SAFI_MAX))
     return -1;
@@ -55,6 +55,7 @@ bgp_maximum_paths_set (struct bgp *bgp, afi_t afi, safi_t safi,
     {
     case BGP_PEER_IBGP:
       bgp->maxpaths[afi][safi].maxpaths_ibgp = maxpaths;
+      bgp->maxpaths[afi][safi].ibgp_flags |= options;
       break;
     case BGP_PEER_EBGP:
       bgp->maxpaths[afi][safi].maxpaths_ebgp = maxpaths;
@@ -82,6 +83,7 @@ bgp_maximum_paths_unset (struct bgp *bgp, afi_t afi, safi_t safi,
     {
     case BGP_PEER_IBGP:
       bgp->maxpaths[afi][safi].maxpaths_ibgp = BGP_DEFAULT_MAXPATHS;
+      bgp->maxpaths[afi][safi].ibgp_flags = 0;
       break;
     case BGP_PEER_EBGP:
       bgp->maxpaths[afi][safi].maxpaths_ebgp = BGP_DEFAULT_MAXPATHS;

diff --git a/bgpd/bgp_mpath.h b/bgpd/bgp_mpath.h
index 37b9ac8b7e217fc50673a661a9a3e1c6722af080..0645e6c2763575ee102bbd8694c3ec89e7af8c3f 100644 (file)
--- a/bgpd/bgp_mpath.h
+++ b/bgpd/bgp_mpath.h
@@ -49,7 +49,8 @@ struct bgp_info_mpath
 };
 
 /* Functions to support maximum-paths configuration */
-extern int bgp_maximum_paths_set (struct bgp *, afi_t, safi_t, int, u_int16_t);
+extern int bgp_maximum_paths_set (struct bgp *, afi_t, safi_t, int, u_int16_t,
+                                 u_int16_t);
 extern int bgp_maximum_paths_unset (struct bgp *, afi_t, safi_t, int);
 
 /* Functions used by bgp_best_selection to record current

diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index 04cbb8ab8dfa41d3713290f7e4cdb175a33fb770..6397c550903aa7792e3484c5ae3c5d0ccbaa0f54 100644 (file)
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -324,7 +324,7 @@ bgp_med_value (struct attr *attr, struct bgp *bgp)
 /* Compare two bgp route entity.  br is preferable then return 1. */
 static int
 bgp_info_cmp (struct bgp *bgp, struct bgp_info *new, struct bgp_info *exist,
-             int *paths_eq)
+             int *paths_eq, struct bgp_maxpaths_cfg *mpath_cfg)
 {
   struct attr *newattr, *existattr;
   struct attr_extra *newattre, *existattre;
@@ -477,6 +477,26 @@ bgp_info_cmp (struct bgp *bgp, struct bgp_info *new, struct bgp_info *exist,
   if (newm > existm)
     ret = 0;
 
+  /* 8.1. Same IGP metric. Compare the cluster list length as
+     representative of IGP hops metric. Rewrite the metric value
+     pair (newm, existm) with the cluster list length. Prefer the
+     path with smaller cluster list length.                       */
+  if (newm == existm)
+    {
+      if (peer_sort (new->peer) == BGP_PEER_IBGP
+         && peer_sort (exist->peer) == BGP_PEER_IBGP
+         && CHECK_FLAG (mpath_cfg->ibgp_flags,
+                        BGP_FLAG_IBGP_MULTIPATH_SAME_CLUSTERLEN))
+       {
+         newm = BGP_CLUSTER_LIST_LENGTH(new->attr);
+         existm = BGP_CLUSTER_LIST_LENGTH(exist->attr);
+         if (newm < existm)
+           ret = 1;
+         if (newm > existm)
+           ret = 0;
+       }
+    }
+
   /* 9. Maximum path check. */
   if (newm == existm)
     {
@@ -540,12 +560,8 @@ bgp_info_cmp (struct bgp *bgp, struct bgp_info *new, struct bgp_info *exist,
     return 0;
 
   /* 12. Cluster length comparision. */
-  new_cluster = exist_cluster = 0;
-
-  if (newattr->flag & ATTR_FLAG_BIT(BGP_ATTR_CLUSTER_LIST))
-    new_cluster = newattre->cluster->length;
-  if (existattr->flag & ATTR_FLAG_BIT(BGP_ATTR_CLUSTER_LIST))
-    exist_cluster = existattre->cluster->length;
+  new_cluster = BGP_CLUSTER_LIST_LENGTH(new->attr);
+  exist_cluster = BGP_CLUSTER_LIST_LENGTH(exist->attr);
 
   if (new_cluster < exist_cluster)
     return 1;
@@ -1352,7 +1368,8 @@ bgp_best_selection (struct bgp *bgp, struct bgp_node *rn,
                {
                  if (CHECK_FLAG (ri2->flags, BGP_INFO_SELECTED))
                    old_select = ri2;
-                 if (bgp_info_cmp (bgp, ri2, new_select, &paths_eq))
+                 if (bgp_info_cmp (bgp, ri2, new_select, &paths_eq,
+                                   mpath_cfg))
                    {
                      bgp_info_unset_flag (rn, new_select, BGP_INFO_DMED_SELECTED);
                      new_select = ri2;
@@ -1405,7 +1422,7 @@ bgp_best_selection (struct bgp *bgp, struct bgp_node *rn,
       bgp_info_unset_flag (rn, ri, BGP_INFO_DMED_CHECK);
       bgp_info_unset_flag (rn, ri, BGP_INFO_DMED_SELECTED);
 
-      if (bgp_info_cmp (bgp, ri, new_select, &paths_eq))
+      if (bgp_info_cmp (bgp, ri, new_select, &paths_eq, mpath_cfg))
        {
          if (do_mpath && bgp_flag_check (bgp, BGP_FLAG_DETERMINISTIC_MED))
            bgp_mp_dmed_deselect (new_select);

diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c
index 22d5c64075666700d625cd656d1813225f1c5148..6ca595c294acd52b9b4163578be23a988d8b11a3 100644 (file)
--- a/bgpd/bgp_vty.c
+++ b/bgpd/bgp_vty.c
@@ -651,34 +651,58 @@ DEFUN (no_bgp_confederation_peers,
   return CMD_SUCCESS;
 }
 
-/* Maximum-paths configuration */
-DEFUN (bgp_maxpaths,
-       bgp_maxpaths_cmd,
-       "maximum-paths <1-255>",
-       "Forward packets over multiple paths\n"
-       "Number of paths\n")
+/**
+ * Central routine for maximum-paths configuration.
+ * @peer_type: BGP_PEER_EBGP or BGP_PEER_IBGP
+ * @set: 1 for setting values, 0 for removing the max-paths config.
+ */
+int
+bgp_maxpaths_config_vty (struct vty *vty, int peer_type, char *mpaths,
+                        u_int16_t options, int set)
 {
   struct bgp *bgp;
   u_int16_t maxpaths;
   int ret;
+  afi_t afi;
+  safi_t safi;
 
   bgp = vty->index;
+  afi = bgp_node_afi (vty);
+  safi = bgp_node_safi (vty);
 
-  VTY_GET_INTEGER_RANGE ("maximum-paths", maxpaths, argv[0], 1, 255);
+  if (set)
+    {
+      VTY_GET_INTEGER_RANGE ("maximum-paths", maxpaths, mpaths, 1, 255);
+      ret = bgp_maximum_paths_set (bgp, afi, safi, peer_type, maxpaths,
+                                  options);
+    }
+  else
+    ret = bgp_maximum_paths_unset (bgp, afi, safi, peer_type);
 
-  ret = bgp_maximum_paths_set (bgp, bgp_node_afi (vty), bgp_node_safi(vty),
-                              BGP_PEER_EBGP, maxpaths);
   if (ret < 0)
     {
       vty_out (vty,
-              "%% Failed to set maximum-paths %u for afi %u, safi %u%s",
-              maxpaths, bgp_node_afi (vty), bgp_node_safi(vty), VTY_NEWLINE);
+              "%% Failed to %sset maximum-paths %s %u for afi %u, safi %u%s",
+              (set == 1) ? "" : "un",
+              (peer_type == BGP_PEER_EBGP) ? "ebgp" : "ibgp",
+              maxpaths, afi, safi, VTY_NEWLINE);
       return CMD_WARNING;
     }
 
   return CMD_SUCCESS;
 }
 
+
+/* Maximum-paths configuration */
+DEFUN (bgp_maxpaths,
+       bgp_maxpaths_cmd,
+       "maximum-paths <1-255>",
+       "Forward packets over multiple paths\n"
+       "Number of paths\n")
+{
+  return bgp_maxpaths_config_vty(vty, BGP_PEER_EBGP, argv[0], 0, 1);
+}
+
 DEFUN (bgp_maxpaths_ibgp,
        bgp_maxpaths_ibgp_cmd,
        "maximum-paths ibgp <1-255>",
@@ -686,25 +710,19 @@ DEFUN (bgp_maxpaths_ibgp,
        "iBGP-multipath\n"
        "Number of paths\n")
 {
-  struct bgp *bgp;
-  u_int16_t maxpaths;
-  int ret;
-
-  bgp = vty->index;
-
-  VTY_GET_INTEGER_RANGE ("maximum-paths", maxpaths, argv[0], 1, 255);
-
-  ret = bgp_maximum_paths_set (bgp, bgp_node_afi (vty), bgp_node_safi(vty),
-                              BGP_PEER_IBGP, maxpaths);
-  if (ret < 0)
-    {
-      vty_out (vty,
-              "%% Failed to set maximum-paths ibgp %u for afi %u, safi %u%s",
-              maxpaths, bgp_node_afi (vty), bgp_node_safi(vty), VTY_NEWLINE);
-      return CMD_WARNING;
-    }
+  return bgp_maxpaths_config_vty(vty, BGP_PEER_IBGP, argv[0], 0, 1);
+}
 
-  return CMD_SUCCESS;
+DEFUN (bgp_maxpaths_ibgp_cluster,
+       bgp_maxpaths_ibgp_cluster_cmd,
+       "maximum-paths ibgp <1-255> equal-cluster-length",
+       "Forward packets over multiple paths\n"
+       "iBGP-multipath\n"
+       "Number of paths\n"
+       "Match the cluster length\n")
+{
+  return bgp_maxpaths_config_vty(vty, BGP_PEER_IBGP, argv[0],
+                                BGP_FLAG_IBGP_MULTIPATH_SAME_CLUSTERLEN, 1);
 }
 
 DEFUN (no_bgp_maxpaths,
@@ -714,22 +732,7 @@ DEFUN (no_bgp_maxpaths,
        "Forward packets over multiple paths\n"
        "Number of paths\n")
 {
-  struct bgp *bgp;
-  int ret;
-
-  bgp = vty->index;
-
-  ret = bgp_maximum_paths_unset (bgp, bgp_node_afi (vty), bgp_node_safi(vty),
-                                BGP_PEER_EBGP);
-  if (ret < 0)
-    {
-      vty_out (vty,
-              "%% Failed to unset maximum-paths for afi %u, safi %u%s",
-              bgp_node_afi (vty), bgp_node_safi(vty), VTY_NEWLINE);
-      return CMD_WARNING;
-    }
-
-  return CMD_SUCCESS;
+  return bgp_maxpaths_config_vty(vty, BGP_PEER_EBGP, NULL, 0, 0);
 }
 
 ALIAS (no_bgp_maxpaths,
@@ -747,22 +750,7 @@ DEFUN (no_bgp_maxpaths_ibgp,
        "iBGP-multipath\n"
        "Number of paths\n")
 {
-  struct bgp *bgp;
-  int ret;
-
-  bgp = vty->index;
-
-  ret = bgp_maximum_paths_unset (bgp, bgp_node_afi (vty), bgp_node_safi(vty),
-                                BGP_PEER_IBGP);
-  if (ret < 0)
-    {
-      vty_out (vty,
-              "%% Failed to unset maximum-paths ibgp for afi %u, safi %u%s",
-              bgp_node_afi (vty), bgp_node_safi(vty), VTY_NEWLINE);
-      return CMD_WARNING;
-    }
-
-  return CMD_SUCCESS;
+  return bgp_maxpaths_config_vty(vty, BGP_PEER_IBGP, NULL, 0, 0);
 }
 
 ALIAS (no_bgp_maxpaths_ibgp,
@@ -773,6 +761,15 @@ ALIAS (no_bgp_maxpaths_ibgp,
        "iBGP-multipath\n"
        "Number of paths\n")
 
+ALIAS (no_bgp_maxpaths_ibgp,
+       no_bgp_maxpaths_ibgp_cluster_cmd,
+       "no maximum-paths ibgp <1-255> equal-cluster-length",
+       NO_STR
+       "Forward packets over multiple paths\n"
+       "iBGP-multipath\n"
+       "Number of paths\n"
+       "Match the cluster length\n")
+
 int
 bgp_config_write_maxpaths (struct vty *vty, struct bgp *bgp, afi_t afi,
                           safi_t safi, int *write)
@@ -787,8 +784,12 @@ bgp_config_write_maxpaths (struct vty *vty, struct bgp *bgp, afi_t afi,
   if (bgp->maxpaths[afi][safi].maxpaths_ibgp != BGP_DEFAULT_MAXPATHS)
     {
       bgp_config_write_family_header (vty, afi, safi, write);
-      vty_out (vty, " maximum-paths ibgp %d%s",
-              bgp->maxpaths[afi][safi].maxpaths_ibgp, VTY_NEWLINE);
+      vty_out (vty, " maximum-paths ibgp %d",
+              bgp->maxpaths[afi][safi].maxpaths_ibgp);
+      if (CHECK_FLAG (bgp->maxpaths[afi][safi].ibgp_flags,
+                     BGP_FLAG_IBGP_MULTIPATH_SAME_CLUSTERLEN))
+       vty_out (vty, " equal-cluster-length");
+      vty_out (vty, "%s", VTY_NEWLINE);
     }
 
   return 0;
@@ -9179,14 +9180,20 @@ bgp_vty_init (void)
   install_element (BGP_IPV6_NODE, &no_bgp_maxpaths_cmd);
   install_element (BGP_IPV6_NODE, &no_bgp_maxpaths_arg_cmd);
   install_element (BGP_NODE, &bgp_maxpaths_ibgp_cmd);
+  install_element(BGP_NODE, &bgp_maxpaths_ibgp_cluster_cmd);
   install_element (BGP_NODE, &no_bgp_maxpaths_ibgp_cmd);
   install_element (BGP_NODE, &no_bgp_maxpaths_ibgp_arg_cmd);
+  install_element (BGP_NODE, &no_bgp_maxpaths_ibgp_cluster_cmd);
   install_element (BGP_IPV4_NODE, &bgp_maxpaths_ibgp_cmd);
+  install_element(BGP_IPV4_NODE, &bgp_maxpaths_ibgp_cluster_cmd);
   install_element (BGP_IPV4_NODE, &no_bgp_maxpaths_ibgp_cmd);
+  install_element (BGP_IPV4_NODE, &no_bgp_maxpaths_ibgp_cluster_cmd);
   install_element (BGP_IPV4_NODE, &no_bgp_maxpaths_ibgp_arg_cmd);
   install_element (BGP_IPV6_NODE, &bgp_maxpaths_ibgp_cmd);
+  install_element(BGP_IPV6_NODE, &bgp_maxpaths_ibgp_cluster_cmd);
   install_element (BGP_IPV6_NODE, &no_bgp_maxpaths_ibgp_cmd);
   install_element (BGP_IPV6_NODE, &no_bgp_maxpaths_ibgp_arg_cmd);
+  install_element (BGP_IPV6_NODE, &no_bgp_maxpaths_ibgp_cluster_cmd);
 
   /* "timers bgp" commands. */
   install_element (BGP_NODE, &bgp_timers_cmd);

diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
index eae803de1435f72f6ef4134df64147e04d9fa1e4..322cc7ed61915401f581d060b2b4c6b296e2fadd 100644 (file)
--- a/bgpd/bgpd.h
+++ b/bgpd/bgpd.h
@@ -169,6 +169,8 @@ struct bgp
   struct bgp_maxpaths_cfg {
     u_int16_t maxpaths_ebgp;
     u_int16_t maxpaths_ibgp;
+    u_int16_t ibgp_flags;
+#define BGP_FLAG_IBGP_MULTIPATH_SAME_CLUSTERLEN (1 << 0)
   } maxpaths[AFI_MAX][SAFI_MAX];
 };
 

diff --git a/tests/bgp_mpath_test.c b/tests/bgp_mpath_test.c
index 3d0ecb78bff2dd05cd450971b1cabebe0ceec6ea..a6ca9c53716a0e43dfb3b6b9c60c07fdabe89cc9 100644 (file)
--- a/tests/bgp_mpath_test.c
+++ b/tests/bgp_mpath_test.c
@@ -157,9 +157,9 @@ run_bgp_cfg_maximum_paths (testcase_t *t)
     for (safi = SAFI_UNICAST; safi < SAFI_MAX; safi++)
       {
         /* test bgp_maximum_paths_set */
-        api_result = bgp_maximum_paths_set (bgp, afi, safi, BGP_PEER_EBGP, 10);
+        api_result = bgp_maximum_paths_set (bgp, afi, safi, BGP_PEER_EBGP, 10, 0);
         EXPECT_TRUE (api_result == 0, test_result);
-        api_result = bgp_maximum_paths_set (bgp, afi, safi, BGP_PEER_IBGP, 10);
+        api_result = bgp_maximum_paths_set (bgp, afi, safi, BGP_PEER_IBGP, 10, 0);
         EXPECT_TRUE (api_result == 0, test_result);
         EXPECT_TRUE (bgp->maxpaths[afi][safi].maxpaths_ebgp == 10, test_result);
         EXPECT_TRUE (bgp->maxpaths[afi][safi].maxpaths_ibgp == 10, test_result);