Limit blueprint changes due to clickhouse (#9100)

Do not create a new blueprint if only the clickhouse cluster keeper log
index changes.

Fixes #9098

Author: andrewjstone

nexus/reconfigurator/planning/src/blueprint_builder/clickhouse.rs

@@ -147,17 +147,8 @@ impl ClickhouseAllocator {
             return bump_gen_if_necessary(new_config);
         };
 
-        // Save the log index for next time if it's been incremented
-        let inventory_updated = if inventory_membership
-            .leader_committed_log_index
-            > self.parent_config.highest_seen_keeper_leader_committed_log_index
-        {
-            new_config.highest_seen_keeper_leader_committed_log_index =
-                inventory_membership.leader_committed_log_index;
-            true
-        } else {
-            false
-        };
+        let inventory_updated = inventory_membership.leader_committed_log_index
+            > self.parent_config.highest_seen_keeper_leader_committed_log_index;
 
         if current_keepers != inventory_membership.raft_config {
             // We know that there is a reconfiguration in progress. If there has
@@ -167,6 +158,12 @@ impl ClickhouseAllocator {
                 return bump_gen_if_necessary(new_config);
             }
 
+            // Save the log index from inventory because we are going to
+            // need to use it to see if inventory has changed next time
+            // through the planner.
+            new_config.highest_seen_keeper_leader_committed_log_index =
+                inventory_membership.leader_committed_log_index;
+
             // We're still trying to reach our desired state. We want to ensure,
             // however, that if we are currently trying to add a node, that we
             // have not expunged the zone of the keeper that we are trying to
@@ -247,18 +244,36 @@ impl ClickhouseAllocator {
                 // Remove the keeper for the first expunged zone we see.
                 // Remember, we only do one keeper membership change at time.
                 new_config.keepers.remove(zone_id);
+
+                if inventory_updated {
+                    // Save the log index from inventory because we are going to
+                    // need to use it to see if inventory has changed next time
+                    // through the planner.
+                    new_config.highest_seen_keeper_leader_committed_log_index =
+                        inventory_membership.leader_committed_log_index;
+                }
+
                 return bump_gen_if_necessary(new_config);
             }
         }
 
-        // Do we need to add any nodes to in service zones that don't have them
+        // Do we need to add any nodes to in-service zones that don't have them
         for zone_id in &active_clickhouse_zones.keepers {
             if !new_config.keepers.contains_key(zone_id) {
                 // Allocate a new `KeeperId` and map it to the keeper zone
                 new_config.max_used_keeper_id += 1.into();
                 new_config
                     .keepers
                     .insert(*zone_id, new_config.max_used_keeper_id);
+
+                if inventory_updated {
+                    // Save the log index from inventory because we are going to
+                    // need to use it to see if inventory has changed next time
+                    // through the planner.
+                    new_config.highest_seen_keeper_leader_committed_log_index =
+                        inventory_membership.leader_committed_log_index;
+                }
+
                 return bump_gen_if_necessary(new_config);
             }
         }

nexus/reconfigurator/planning/src/planner.rs

@@ -5224,9 +5224,10 @@ pub(crate) mod test {
         );
         assert_eq!(bp4_config.keepers, bp3_config.keepers);
         assert_eq!(bp4_config.servers, bp3_config.servers);
+        // The blueprint shouldn't update solely because the log index changed
         assert_eq!(
             bp4_config.highest_seen_keeper_leader_committed_log_index,
-            1
+            0
         );
 
         // Let's bump the clickhouse target to 5 via policy so that we can add
@@ -5388,9 +5389,10 @@ pub(crate) mod test {
         );
         assert_eq!(bp8_config.keepers, bp7_config.keepers);
         assert_eq!(bp7_config.keepers.len(), target_keepers as usize);
+        // The blueprint should not change solely due to the log index in inventory changing
         assert_eq!(
             bp8_config.highest_seen_keeper_leader_committed_log_index,
-            3
+            2
         );
 
         logctx.cleanup_successful();

nexus/reconfigurator/planning/tests/output/planner_deploy_all_keeper_nodes_3_4.txt

@@ -21,7 +21,7 @@ to:   blueprint 92fa943c-7dd4-48c3-9447-c9d0665744b6
     max used keeper id:::::::::::::::::::::::::::::   3 (unchanged)
     cluster name:::::::::::::::::::::::::::::::::::   oximeter_cluster (unchanged)
     cluster secret:::::::::::::::::::::::::::::::::   6a984e2e-20be-4ed9-b572-d7ef063c67f7 (unchanged)
-*   highest seen keeper leader committed log index:   0 -> 1
+    highest seen keeper leader committed log index:   0 (unchanged)
 
     clickhouse keepers at generation 2:
     ------------------------------------------------

nexus/reconfigurator/planning/tests/output/planner_deploy_all_keeper_nodes_4_5.txt

@@ -235,7 +235,7 @@ to:   blueprint 2886dab5-61a2-46b4-87af-bc7aeb44cccb
 *   max used keeper id:::::::::::::::::::::::::::::   3 -> 4
     cluster name:::::::::::::::::::::::::::::::::::   oximeter_cluster (unchanged)
     cluster secret:::::::::::::::::::::::::::::::::   6a984e2e-20be-4ed9-b572-d7ef063c67f7 (unchanged)
-    highest seen keeper leader committed log index:   1 (unchanged)
+*   highest seen keeper leader committed log index:   0 -> 1
 
     clickhouse keepers generation 2 -> 3:
     ------------------------------------------------