Merge pull request #6808 from garlick/3level

broker: add 'mincrit' topology

Author: mergify[bot]

doc/man7/flux-broker-attributes.rst

@@ -177,12 +177,30 @@ TREE BASED OVERLAY NETWORK
 ==========================
 
 tbon.topo [Updates: C]
-   URI describing the TBON tree topology such as ``kary:16``.  The ``kary``
-   scheme selects a complete, k-ary tree with fanout *k*, with ``kary:0``
-   meaning that rank 0 is the parent of all other ranks by convention.  The
-   ``binomial`` scheme selects a binomial tree topology of the minimum order
-   that fits the instance size.  Default: ``kary:32``, unless bootstrapping by
-   TOML configuration, then see :man5:`flux-config-bootstrap`.
+   A URI describing the TBON tree topology.  The following schemes are
+   available:
+
+   kary:k
+      A complete, k-ary tree with fanout *k*.  By convention, ``kary:0``
+      indicates that rank 0 is the parent of all other ranks.
+
+   mincrit[:k]
+      Minimize critical ranks.  The tree height is limited to three.
+      If *k* is specified, it sets the number of router (interior) ranks,
+      making the number of critical ranks *k* plus one.  The fanout from
+      routers to leaves is determined by the instance size.  If *k* is
+      unspecified, *k* is set to a value that avoids exceeding a fanout of
+      1024 at any level.  If that cannot be achieved in three levels,
+      then rank 0 is overloaded.
+
+   binomial
+      Binomial tree topology of the minimum order that fits the instance size.
+
+   custom
+      The topology is set by TOML configuration.
+      See :man5:`flux-config-bootstrap`.
+
+   The default value is ``kary:32``.
 
 tbon.descendants
    Number of descendants "below" this node of the tree based

doc/test/spell.en.pws

@@ -969,3 +969,4 @@ YQ
 composable
 orchestrator
 fsck
+mincrit

src/broker/test/topology.c

@@ -250,6 +250,24 @@ struct internal_ranks_test internal_ranks_tests[] = {
     { 4,  "binomial","0,2" },
     { 8,  "binomial","0,2,4,6" },
     { 16, "binomial","0,2,4,6,8,10,12,14" },
+    { 4096, "mincrit:0", "0" },
+    { 1, "mincrit:2", "" },
+    { 2, "mincrit:2", "0" },
+    { 3, "mincrit:2", "0" },
+    { 4, "mincrit:2", "0-1" },
+    { 5, "mincrit:2", "0-2" },
+    { 6, "mincrit:2", "0-2" },
+    { 7, "mincrit:2", "0-2" },
+    { 1024, "mincrit:2", "0-2" },
+    { 512, "mincrit", "0" },
+    // leader has 1024 children at size=1024+1=1025
+    { 1025, "mincrit", "0" },
+    { 1026, "mincrit", "0-2" },
+    // 2 routers have 1024 children each at size=1024+1024+3=2051
+    { 2051, "mincrit", "0-2" },
+    { 2052, "mincrit", "0-3" },
+    // a large el capitan run
+    { 10800, "mincrit", "0-11" },
     { -1, NULL, NULL  }
 };
 
@@ -637,6 +655,60 @@ void test_binomial5 (void)
     topology_decref (topo);
 }
 
+void test_mincrit5 (void)
+{
+    struct topology *topo;
+    flux_error_t error;
+
+    topo = topology_create ("mincrit:zz", 5, &error);
+    if (!topo)
+        diag ("%s", error.text);
+    ok (topo == NULL,
+        "mincrit topology fails with unknown path");
+
+    topo = topology_create ("mincrit:2", 5, &error);
+    ok (topo != NULL,
+        "mincrit:2 topology of size=5 works");
+
+    ok (topology_set_rank (topo, 1) == 0,
+        "set rank to 1");
+    ok (topology_get_parent (topo) == 0,
+        "rank 1 parent is 0");
+    ok (topology_get_child_ranks (topo, NULL, 0) == 1,
+        "rank 1 has one child");
+    ok (topology_get_level (topo) == 1,
+        "rank 1 level is 1");
+
+    ok (topology_set_rank (topo, 2) == 0,
+        "set rank to 2");
+    ok (topology_get_parent (topo) == 0,
+        "rank 2 parent is 0");
+    ok (topology_get_child_ranks (topo, NULL, 0) == 1,
+        "rank 2 has one child");
+    ok (topology_get_level (topo) == 1,
+        "rank 2 level is 1");
+
+    ok (topology_set_rank (topo, 3) == 0,
+        "set rank to 3");
+    ok (topology_get_parent (topo) == 1,
+        "rank 3 parent is 1");
+    ok (topology_get_child_ranks (topo, NULL, 0) == 0,
+        "rank 3 has no children");
+    ok (topology_get_level (topo) == 2,
+        "rank 3 level is 2");
+
+    ok (topology_set_rank (topo, 4) == 0,
+        "set rank to 4");
+    ok (topology_get_parent (topo) == 2,
+        "rank 4 parent is 2");
+    ok (topology_get_child_ranks (topo, NULL, 0) == 0,
+        "rank 4 has no children");
+    ok (topology_get_level (topo) == 2,
+        "rank 4 level is 2");
+
+    topology_decref (topo);
+}
+
 int main (int argc, char *argv[])
 {
     plan (NO_PLAN);
@@ -650,6 +722,7 @@ int main (int argc, char *argv[])
     test_custom ();
     test_rank_aux ();
     test_binomial5 ();
+    test_mincrit5 ();
 
     done_testing ();
 }

src/broker/topology.c

@@ -40,6 +40,9 @@ struct topology {
 static int kary_plugin_init (struct topology *topo,
                              const char *path,
                              flux_error_t *error);
+static int mincrit_plugin_init (struct topology *topo,
+                                const char *path,
+                                flux_error_t *error);
 static int binomial_plugin_init (struct topology *topo,
                                  const char *path,
                                  flux_error_t *error);
@@ -49,6 +52,7 @@ static int custom_plugin_init (struct topology *topo,
 
 static const struct topology_plugin builtin_plugins[] = {
     { .name = "kary",   .init = kary_plugin_init },
+    { .name = "mincrit", .init = mincrit_plugin_init },
     { .name = "binomial", .init = binomial_plugin_init },
     { .name = "custom", .init = custom_plugin_init },
 };
@@ -381,34 +385,33 @@ struct idset *topology_get_internal_ranks (struct topology *topo)
     return NULL;
 }
 
-/* kary plugin
- */
-static int parse_k (const char *s, int *result, flux_error_t *error)
+static int parse_k (const char *s, int *result)
 {
     char *endptr;
     unsigned long val;
 
     if (!s || strlen (s) == 0)
-        goto error;
+        return -1;
     errno = 0;
     val = strtoul (s, &endptr, 10);
     if (errno != 0 || *endptr != '\0')
-        goto error;
+        return -1;
     *result = val;
     return 0;
-error:
-    errprintf (error, "kary k value must be an integer >= 0");
-    return -1;
 }
 
+/* kary plugin
+ */
 static int kary_plugin_init (struct topology *topo,
                              const char *path,
                              flux_error_t *error)
 {
     int k;
 
-    if (parse_k (path, &k, error) < 0)
+    if (parse_k (path, &k) < 0) {
+        errprintf (error, "kary k value must be an integer >= 0");
         return -1;
+    }
     if (k > 0) {
         for (int i = 0; i < topo->size; i++) {
             topo->node[i].parent = kary_parentof (k, i);
@@ -419,6 +422,70 @@ static int kary_plugin_init (struct topology *topo,
     return 0;
 }
 
+/* Given size and k (number of routers), determine fanout from
+ * routers to leaves.
+ */
+static int mincrit_router_fanout (int size, int k)
+{
+    int crit = 1 + k;
+    int leaves = size - crit;
+    int fanout = leaves / k;
+    if (leaves % k > 0)
+        fanout++;
+    return fanout;
+}
+
+/* Choose a value for k that balances minimizing critical nodes and
+ * keeping the fanout from routers to leaves at or below a threshold.
+ * The height is always capped at 3, so max_fanout might be exceeded
+ * from leader to routers for large size or small max_fanout.
+ * Do choose k=0 (flat tree) if max_fanount can be met by the leader node.
+ * Don't choose k=1, since that just pushes some router work off
+ * to rank 1, without tree benefits.
+ */
+static int mincrit_choose_k (int size, int max_fanout)
+{
+    int k = 0;
+    if (size > max_fanout + 1) {
+        k = 2;
+        while (mincrit_router_fanout (size, k) > max_fanout)
+            k++;
+    }
+    return k;
+}
+
+/* mincrit plugin
+ * A k-ary tree "squashed" down to at most three levels.
+ * The value of k determines the fanout from leader to routers.
+ * The number of nodes determines the fanout from routers to leaves.
+ * The value of k may be 0, or be unspecifed (letting the system choose).
+ */
+static int mincrit_plugin_init (struct topology *topo,
+                                const char *path,
+                                flux_error_t *error)
+{
+    int k;
+
+    if (path && strlen (path) > 0) {
+        if (parse_k (path, &k) < 0) {
+            errprintf (error, "mincrit k value must be an integer >= 0");
+            return -1;
+        }
+    }
+    else {
+        k = mincrit_choose_k (topo->size, 1024);
+    }
+    /* N.B. topo is initialized with rank 0 as the parent of all other ranks
+     * before plugin init is called, therefore only the leaves need to have
+     * their parent set here.
+     */
+    if (k > 0) {
+        for (int i = k + 1; i < topo->size; i++)
+            topo->node[i].parent = (i - k - 1) % k + 1;
+    }
+    return 0;
+}
+
 /* binomial plugin
  */
 static int binomial_smallest_k (int size)

t/t0019-tbon-config.t

@@ -168,4 +168,23 @@ test_expect_success 'broker -Stbon.topo=binomial option works' '
 		flux getattr tbon.topo >topo_binomial.out &&
 	test_cmp topo_binomial.exp topo_binomial.out
 '
+test_expect_success 'broker -Stbon.topo=mincrit option works' '
+	echo mincrit >topo_mincrit.exp &&
+	flux start ${ARGS} -Stbon.topo=mincrit \
+		flux getattr tbon.topo >topo_mincrit.out &&
+	test_cmp topo_mincrit.exp topo_mincrit.out
+'
+test_expect_success 'broker -Stbon.topo=mincrit:2 option works' '
+	echo 2 >topo_crit2.exp &&
+	flux start -s5 ${ARGS} -Stbon.topo=mincrit:2 \
+		flux getattr tbon.maxlevel >topo_crit2.out &&
+	test_cmp topo_crit2.exp topo_crit2.out
+'
+test_expect_success 'broker -Stbon.topo=mincrit is flat for small size' '
+	echo 1 >topo_critsmall.exp &&
+	flux start -s5 ${ARGS} -Stbon.topo=mincrit \
+		flux getattr tbon.maxlevel >topo_critsmall.out &&
+	test_cmp topo_critsmall.exp topo_critsmall.out
+'
+
 test_done