refactor broker overlay for topology flexibility #4474
Conversation
|
This pull request introduces 1 alert when merging 4b18b51 into 92921b3 - view on LGTM.com new alerts:
|
|
Pushed fixups to various ci failures... |
I don't necessarily have a problem with this change, but I wondered if you considered adding an attribute to track the current number of broker tbon children and keeping |
Now that you raise the question, the change does seem unjustified since this is likely to only affect the system instance. I just didn't know what to set it to when the tree shape is arbitrary. Maybe we should just leave it unset if the tree is not a complete k-ary. I didn't have a particular use case for an attribute containing the number of direct peers, so I'd vote we add that later if needed (it would be easy). |
That seems reasonable to me.
That was my main thought. If there isn't a use case, keep |
|
Ok, pushed that change, as well as a fix for a ci failure in the topology test that I missed in the last round. |
|
oops, I realized I need to update the flux-broker-attributes(7) man page and fix a couple commit message errors. Force pushing again - hope that doesn't mess up any reviews in progress. |
| @@ -406,7 +406,7 @@ static void update_torpid_children (struct overlay *ov) | |||
| } | |||
| } | |||
|
|
|||
| /* N.B. overlay_child_status_update() ensures child_lookup() only | |||
| /* N.B. overlay_child_status_update() ensures child_lookup_online() only | |||
There was a problem hiding this comment.
typo in commit message: "Problen:"
|
Fixed that typo and rebased on current master - i'll set mwp. |
Problem: kary.h is included by broker.c but that class is not used there. Drop include directive.
Problem: overlay function comment refers to itself using the name of another function, potentially creating confusion. Correct comment.
Problem: the kary class used by the overlay network to calculate routes and peers is limited to complete k-ary trees, but soon non-complete tree topologies will be required. Add a new topology class which represents arbitrary tree topologies. The default topology is flat. A complete k-ary topology may be set with topology_set_kary(). Methods are added to support the queries needed by the broker overlay network. Add unit test.
Problem: the broker overlay code uses the kary class to calculate peers and routes, but this will not work once the topology is allowed to have a non-complete tree shape. Switch to the topology class, which supports any tree shape. The topology is created in the bootstrap methods and passed to the overlay using overlay_set_topology(), which replaces overlay_set_geometry(). Use the topology_set_kary() method to enact a complete topology as before, but never call the kary_*() functions directly and remove all assuptions about complete trees from the code. Note that formerly the fact that child peers had contiguous ranks was used to directly index into the array of children when selecting a route. To avoid slowing down that critical path, use the topology aux container to store a pointer to the child peer struct. The container is accessed in O(1), and its items in O(N) where N is the item's position in the container. Update the overlay unit test.
Problem: strtol() and strtoul() are not being checked properly in the broker attribute code. Fix the code to conform to recommendations in strtoul(3), which involves clearing errno before the call and testing it afterwards.
Problem: code for fetching a u32 attribute will be duplicated in boot_pmi.c and boot_config.c. Add attr_get_uint32() to attrs.[ch].
Problem: in the future, the broker bootstrap methods (config file and PMI) will handle the tbon.fanout broker attribute differently, but the attribute is currently managed in shared code in overlay.c. Move the handling of the attribute to the bootstrap methods where the code can be allowed to diverge. Drop tbon.fanout test from overlay unit test, since this is no longer a feature that belongs to the overlay abstraction. Allow a fanout value of 0 to indicate "flat", regardless of the instance size, since the topology class supports that. Update t0001-basic.t sharness test to allow that value to succeed. Update flux-broker-attributes(7) to say what a value of 0 means.
Problem: the systemd unit file has to specify a large value for the tbon.fanout attribute to make the default system instance use a flat TBON topology. Make the default TBON fanout 0 (flat) when Flux is booted from config files. Drop the tbon.fanout=256 setting in the systemd unit file.
Codecov Report
@@ Coverage Diff @@
## master #4474 +/- ##
==========================================
+ Coverage 83.35% 83.36% +0.01%
==========================================
Files 400 401 +1
Lines 67342 67501 +159
==========================================
+ Hits 56132 56273 +141
- Misses 11210 11228 +18
|
Problem: the broker overlay network code assumes a "complete" k-ary tree topology.
This PR prepares the broker to support a custom configured tree topology as discussed in #3799 by removing assumptions about the topology from the overlay code and pushing the details into a new class. Later, we can add support to that class for setting up trees that are wired any way we like, for example with all interior nodes (overlay message routers) mapped to cluster service nodes that are less likely to be destabilized by running resource intensive user applications.
The most user-visible change here is that the
tbon.fanoutbroker attribute has changed:In addition, the default topology is now flat for an instance bootstrapped from configuration (normally the system instance).