|
| 1 | +# Remove the legacy cluster scheduling and staged reconfiguration paths |
| 2 | + |
| 3 | +- Associated (controller work, merged): #37214, #37452, #37671, #37767 |
| 4 | +- Associated (config owns builtin replicas, merged): #37929 |
| 5 | + |
| 6 | +## The Problem |
| 7 | + |
| 8 | +The cluster controller is merged, default-on since v26.29, and owns the |
| 9 | +replica set of every *user* managed cluster in production. But it still lives |
| 10 | +behind the break-glass dyncfg `ENABLE_CLUSTER_CONTROLLER`, and system/builtin |
| 11 | +clusters are excluded from its ownership (`ManagedClusterIds` filters |
| 12 | +`is_user()`). Those two facts keep three legacy code paths alive: |
| 13 | + |
| 14 | +1. **The legacy REFRESH scheduler** (`src/adapter/src/coord/cluster_scheduling.rs`), |
| 15 | + driven by a coordinator timer, superseded by the controller's |
| 16 | + `OnRefreshStrategy`. Both of its entry points return before doing any work |
| 17 | + while the gate is on, so it is a strict no-op in production. Only the gate |
| 18 | + keeps it reachable. |
| 19 | +2. **The legacy staged reconfiguration machine**: the `WaitForHydrated` and |
| 20 | + `Finalize` cluster stages, the `-pending` overlap replicas, and their |
| 21 | + connection-lifecycle cleanup, superseded by the controller's durable |
| 22 | + reconfiguration record plus the `AwaitReconfiguration` wait-shim. With the |
| 23 | + gate on it is reachable through exactly one door: a config-shape |
| 24 | + `ALTER CLUSTER <system cluster> ... WITH (WAIT ...)`. |
| 25 | +3. **The legacy direct replica create/drop path** in |
| 26 | + `sequence_alter_cluster_managed_to_managed`: synchronous whole-set |
| 27 | + recreation on a size change, direct create/drop on a replication-factor |
| 28 | + change. With the gate on it runs only for system clusters, which is the |
| 29 | + system-cluster exception in action: the sequencer must materialize their |
| 30 | + replicas itself because the controller will not. This path does not go |
| 31 | + away entirely, it shrinks to a single explicitly requested cut-over |
| 32 | + branch (see Part 3). |
| 33 | + |
| 34 | +The system-cluster exception was originally load-bearing: the boot-time |
| 35 | +builtin replica migration and the controller would have been two conflicting |
| 36 | +writers of one replica set. #37929 removed that conflict. |
| 37 | +`reconcile_builtin_cluster_replicas` now converges a builtin cluster's |
| 38 | +replica set on the cluster's own managed config at catalog open, the same |
| 39 | +config the controller derives its targets from, so the two converge on the |
| 40 | +same set by construction. The exception no longer prevents a bug. It only |
| 41 | +forces the sequencer to keep a second replica-materialization implementation |
| 42 | +alive for system clusters. |
| 43 | + |
| 44 | +## Success Criteria |
| 45 | + |
| 46 | +- The cluster controller owns the replica set of *all* managed clusters, user |
| 47 | + and system alike. The `is_user()` ownership conjuncts are gone. |
| 48 | +- `ENABLE_CLUSTER_CONTROLLER` is removed. The controller runs unconditionally |
| 49 | + (it still quiesces while the deployment is read-only). |
| 50 | +- The legacy REFRESH scheduler and the legacy staged reconfiguration machine |
| 51 | + are deleted, not fenced off. The direct create/drop path shrinks to a |
| 52 | + single whole-set cut-over branch, kept as an explicitly requested escape |
| 53 | + hatch (see Part 3). |
| 54 | +- Boot ordering and 0dt read-only behavior are preserved: system clusters have |
| 55 | + their replicas up and hydrated before the serve loop starts and before a 0dt |
| 56 | + cutover, provided by `reconcile_builtin_cluster_replicas` at catalog open, |
| 57 | + which stays. |
| 58 | + |
| 59 | +## Out of Scope |
| 60 | + |
| 61 | +- Removing the controller sub-behavior kill-switches |
| 62 | + `ENABLE_BACKGROUND_ALTER_CLUSTER` and `ENABLE_HYDRATION_BURST`. They gate |
| 63 | + controller behaviors, not legacy paths, and retire separately after burn-in. |
| 64 | +- Removing the feature-acceptance flags `enable_cluster_schedule_refresh` and |
| 65 | + `enable_auto_scaling_strategy`. Those gate user-facing SQL surface under a |
| 66 | + staged rollout. |
| 67 | +- Removing the durable `pending` field on `ReplicaLocation::Managed` and |
| 68 | + `remove_pending_cluster_replicas_migration`. The field becomes vestigial |
| 69 | + (always false) once the staged machine is gone, but the migration is the |
| 70 | + only remaining cleaner for `pending` replicas stranded by a crash on a |
| 71 | + pre-deletion version, so both go together in a later release (see Part 3). |
| 72 | + |
| 73 | +## Solution Proposal |
| 74 | + |
| 75 | +Three parts. Parts 1 and 2 each just widen `controller_owns` (drop one |
| 76 | +conjunct each), Part 3 deletes everything that is dead once `controller_owns` |
| 77 | +is constant-true. 1 and 2 are separable and safe in either order, 3 needs |
| 78 | +both. |
| 79 | + |
| 80 | +### Part 1: remove `ENABLE_CLUSTER_CONTROLLER` and the legacy scheduler |
| 81 | + |
| 82 | +Delete the dyncfg (definition, registration, and its five read sites, plus the |
| 83 | +sqllogictest binary's force-on block) and everything only it kept alive: |
| 84 | + |
| 85 | +- `cluster_scheduling.rs` wholesale: `check_scheduling_policies`, |
| 86 | + `check_refresh_policy`, `handle_scheduling_decisions`, and the |
| 87 | + `SchedulingDecision` / `RefreshDecision` types. |
| 88 | +- The plumbing: `Message::CheckSchedulingPolicies` and |
| 89 | + `Message::SchedulingDecisions` with their handlers, the coordinator timer |
| 90 | + and select-loop tick, and the `cluster_scheduling_decisions` coordinator |
| 91 | + state. |
| 92 | +- The two metrics `check_scheduling_policies_seconds` and |
| 93 | + `handle_scheduling_decisions_seconds`. |
| 94 | +- The system var `cluster_check_scheduling_policies_interval` (its sole |
| 95 | + consumer is the timer). |
| 96 | +- `ReplicaCreateDropReason::ClusterScheduling`, constructed only by the |
| 97 | + scheduler. |
| 98 | + |
| 99 | +The persisted audit vocabulary stays: `SchedulingDecisionsWithReasonsV2` and |
| 100 | +the audit-log types are written by the controller's `OnRefresh` path too |
| 101 | +(`refresh_window_decision_to_audit_log`), and old events must remain |
| 102 | +decodable regardless. |
| 103 | + |
| 104 | +### Part 2: controller owns system clusters |
| 105 | + |
| 106 | +Drop the `is_user()` conjunct from `ManagedClusterIds` and from the two |
| 107 | +`controller_owns` computations in the sequencer. Runtime ALTERs of system |
| 108 | +clusters then flow through the controller like any other managed cluster: |
| 109 | +config-shape changes reshape into a durable reconfiguration record, factor |
| 110 | +changes update the config and the controller converges the replica set. |
| 111 | + |
| 112 | +Ownership and the boot migration compose rather than conflict, in both |
| 113 | +directions: |
| 114 | + |
| 115 | +- The controller matches replicas by shape and count, never by name, so the |
| 116 | + migration-created `r1..rN` replicas satisfy its baseline and a steady |
| 117 | + system cluster reconciles to no decisions. |
| 118 | +- The migration converges by canonical name, so a boot after a reshape (or |
| 119 | + with a reconfiguration in flight) renames or re-creates replicas the |
| 120 | + controller had materialized under generator names. That is harmless churn: |
| 121 | + every replica is a cold process at boot anyway, so the cost is replica-id |
| 122 | + and audit-log noise, and an in-flight record is durable, so the controller |
| 123 | + picks the reconfiguration back up on its first tick. |
| 124 | + |
| 125 | +Points to settle in the implementation, none of them blockers: |
| 126 | + |
| 127 | +- `validate_reconfiguration_resource_limits` early-returns for non-user |
| 128 | + clusters with the comment "a system cluster never reshapes into a record", |
| 129 | + which this part makes false. The early return itself stays, since system |
| 130 | + clusters are exempt from `max_replicas_per_cluster` and credit accounting |
| 131 | + everywhere else, but the comment must state the exemption instead. |
| 132 | +- Audit attribution: boot-time creates by the migration keep the `System` |
| 133 | + reason, runtime creates by the controller audit `Manual` (the tag for |
| 134 | + replicas the cluster's own config calls for), the same as user clusters. |
| 135 | +- The controller's create path validates sizes via the cluster owner's |
| 136 | + allowed sizes. Builtin clusters owned by `mz_system` bypass |
| 137 | + `allowed_cluster_replica_sizes` (`get_role_allowed_cluster_sizes`), so no |
| 138 | + new failure mode there. `mz_support` / `mz_analytics` are owned by their |
| 139 | + own roles and see the same restriction their owners' ALTERs already see. |
| 140 | + |
| 141 | +### Part 3: delete the staged machine and slim the direct path to an escape hatch |
| 142 | + |
| 143 | +With `controller_owns` constant-true, `NeedsFinalization::Yes` has no |
| 144 | +producer. Delete: |
| 145 | + |
| 146 | +- `ClusterStage::WaitForHydrated` and `ClusterStage::Finalize`, their structs, |
| 147 | + dispatch arms, and handlers. |
| 148 | +- `NeedsFinalization` and `PENDING_REPLICA_SUFFIX`. |
| 149 | +- The `pending_cluster_alters` connection state and its retire paths: |
| 150 | + `drop_reconfiguration_replicas`, |
| 151 | + `retire_cluster_reconfigurations_for_conn`, |
| 152 | + `cancel_cluster_reconfigurations_for_conn`, and their call sites in |
| 153 | + connection cleanup and cancellation. |
| 154 | +- The `AlterClusterWhilePendingReplicas` error (its raise site goes with the |
| 155 | + machine, its catch site went with the scheduler). |
| 156 | +- In `sequence_alter_cluster_managed_to_managed` (single caller left once the |
| 157 | + scheduler is gone): the pending-replica arm and the separate scale-up/down |
| 158 | + branches. Factor-only changes are config-only writes the controller |
| 159 | + converges, like user clusters today. |
| 160 | + |
| 161 | +The whole-set recreate branch is deliberately **kept**, as the direct |
| 162 | +cut-over path: retire any carried reconfiguration record as cancelled, write |
| 163 | +the target config, and synchronously drop the observed owned replica set and |
| 164 | +recreate it at the target shape and factor, all in one catalog transaction |
| 165 | +with no record and no controller involvement. It is routed to by a |
| 166 | +zero-timeout commit strategy (`WITH (WAIT FOR '0s')`, or |
| 167 | +`WAIT UNTIL READY (TIMEOUT '0s', ON TIMEOUT 'COMMIT')`) instead of by |
| 168 | +`AlterClusterPlanStrategy::None` as today. Two reasons: |
| 169 | + |
| 170 | +- **Escape hatch.** Once the gate is gone there is no break-glass flag, and |
| 171 | + every other reshape depends on the controller ticking and applying. The |
| 172 | + direct path is the one reshape that still works when the controller itself |
| 173 | + is the problem, and it simultaneously unsticks a wedged reconfiguration by |
| 174 | + retiring the record. Requesting it stays safe under a live controller: the |
| 175 | + config write invalidates any in-flight tick's compare-and-append witness, |
| 176 | + so a stale controller batch is rejected, the same as any user DDL landing |
| 177 | + mid-tick. |
| 178 | +- **Honest semantics.** A zero timeout with commit already means "cut over |
| 179 | + now, hydrated or not". Doing it synchronously in the ALTER instead of one |
| 180 | + controller tick later is the same outcome, minus the tick. |
| 181 | + |
| 182 | +This branch keeps its supporting plumbing alive: the replica-id |
| 183 | +pre-allocation, the replica eval contexts for created replicas, and |
| 184 | +`cancel_carried_reconfiguration` (whose other callers, the `Finalize` handler |
| 185 | +and the scheduler, are deleted). The branch already drops the owned set by |
| 186 | +observed id rather than by canonical name, so it works after controller name |
| 187 | +drift and with overlap replicas present. |
| 188 | + |
| 189 | +What stays, and why: |
| 190 | + |
| 191 | +- `AlterClusterPlanStrategy` and `ClusterStage::AwaitReconfiguration`: the |
| 192 | + controller reshape path and its foreground wait-shim consume them, as does |
| 193 | + `cluster_alter_check_ready_interval`. |
| 194 | +- `remove_pending_cluster_replicas_migration`: an upgrade can come from a |
| 195 | + version whose staged machine crashed between the pending-create commit and |
| 196 | + finalize. Those replicas are durably `pending: true`, excluded from the |
| 197 | + controller's ownership test, and after this deletion no runtime path would |
| 198 | + ever clean them, so the catalog-open migration is the only remaining |
| 199 | + cleaner. It is removed together with the durable `pending` field once no |
| 200 | + supported upgrade source can still write pending replicas. |
| 201 | +- `reconcile_builtin_cluster_replicas`: still load-bearing in the two windows |
| 202 | + the controller cannot cover. `Coordinator::bootstrap` brings up only |
| 203 | + replicas already durable and runs before the controller task is spawned, |
| 204 | + and the controller is inactive in 0dt read-only mode, where the migration |
| 205 | + (running against the savepoint catalog) is what gets system clusters up and |
| 206 | + hydrated before cutover. |
| 207 | + |
| 208 | +## Behavior changes |
| 209 | + |
| 210 | +- **System-cluster shape ALTERs become controller-driven.** |
| 211 | + `ALTER CLUSTER mz_system SET (SIZE ...)` changes from a synchronous |
| 212 | + whole-set recreation to a background graceful reconfiguration (default |
| 213 | + deadline 24h, `ROLLBACK` on timeout). This is strictly more capable: the |
| 214 | + graceful path is available when wanted (previously it did not exist for |
| 215 | + system clusters), and `WITH (WAIT FOR '0s')` requests an immediate |
| 216 | + cut-over for operators who cannot afford the overlap set, for example when |
| 217 | + resizing on a deployment with no headroom. |
| 218 | +- **Factor changes on system clusters converge asynchronously.** |
| 219 | + `ALTER CLUSTER mz_support SET (REPLICATION FACTOR 1)` updates the config |
| 220 | + and returns, the controller materializes the replica within a tick. Same |
| 221 | + as user clusters today. |
| 222 | +- **A zero-timeout commit `WAIT` becomes synchronous.** Today |
| 223 | + `WITH (WAIT FOR '0s')` writes a record the controller commits on its next |
| 224 | + tick. It now takes the direct path and returns with the cut-over already |
| 225 | + transacted. Same outcome, one tick sooner, and it works even when the |
| 226 | + controller does not. |
| 227 | +- **No more break-glass flag.** After Part 1, reverting to the legacy paths |
| 228 | + requires a binary rollback. The gate has been default-on since v26.29, so |
| 229 | + the controller paths have several releases of burn-in. The direct path is |
| 230 | + the remaining operational escape hatch for reshapes. |
| 231 | + |
| 232 | +## Test and rollout surface |
| 233 | + |
| 234 | +- `test/sqllogictest/system-cluster.slt`: the blocks asserting a synchronous |
| 235 | + replica count right after `ALTER CLUSTER mz_system SET (SIZE ...)` and |
| 236 | + after factor flips need rework, since convergence is now asynchronous and |
| 237 | + slt does not retry. Move them to testdrive (which retries) or assert on |
| 238 | + the config instead of the replica set. |
| 239 | +- `test/testdrive/cluster-controller.td`: the gate-off sections go (the |
| 240 | + unmanaged-conversion refusal block and the `cc_handoff*` legacy-handoff |
| 241 | + scenarios). The file's freeze-the-controller technique (flipping the gate |
| 242 | + off to hold an in-flight state still) is replaced by cranking |
| 243 | + `cluster_controller_tick_interval` up. |
| 244 | +- `test/pg-cdc/cluster-graceful-reconfiguration.td`: the explicit legacy |
| 245 | + foreground section goes, the controller section stays. |
| 246 | +- Zero-timeout `WAIT` tests (`cluster-controller.td` and friends) now |
| 247 | + exercise the direct path, so assertions that expect a reconfiguration |
| 248 | + record for the `'0s'` case need adjusting. |
| 249 | +- `test/sqllogictest/mz_cluster_schedules.slt`: the |
| 250 | + `cluster_check_scheduling_policies_interval` validation block goes with the |
| 251 | + var. |
| 252 | +- `test/cluster/resources/resource-limits.td` exercises factor flips on |
| 253 | + `mz_analytics` with retrying queries, so it survives the switch to |
| 254 | + asynchronous convergence and pins the system-cluster limit exemption. |
| 255 | +- Flag-name references: `misc/python/materialize/mzcompose/__init__.py` pins |
| 256 | + `enable_cluster_controller` per version for mixed-version runs and needs an |
| 257 | + upper version bound once the removal version is known (older binaries keep |
| 258 | + the flag, newer ones warn on the unknown default). |
| 259 | + `misc/python/materialize/parallel_workload/action.py` drops it from the |
| 260 | + do-not-flip list, and the launchdarkly-flag-consistency allowlist entries |
| 261 | + (listed twice) surface as prunable. |
| 262 | + |
| 263 | +## Alternatives |
| 264 | + |
| 265 | +- **Delete the direct path entirely and let `WAIT FOR '0s'` ride the |
| 266 | + controller record.** Maximal deletion. Rejected: once the gate is gone, |
| 267 | + every reshape would depend on the controller ticking and applying, so a |
| 268 | + controller bug would leave no way to reshape any cluster, and no flag to |
| 269 | + fall back on. The kept branch is small, exercised by an explicit statement, |
| 270 | + and doubles as the cleanup tool for a wedged reconfiguration. |
| 271 | +- **Keep the system-cluster exception, reject `WITH (WAIT ...)` on system |
| 272 | + clusters, delete only the scheduler and the staged machine.** This closes |
| 273 | + the staged machine's last door with a new error instead of ownership. |
| 274 | + Rejected: it must keep the full legacy direct create/drop path alive for |
| 275 | + system clusters forever, so the sequencer retains a second complete |
| 276 | + replica-materialization implementation, and it removes a currently-working |
| 277 | + statement while this design makes the same statement do the right thing. |
| 278 | + Ownership deletes more code and adds a capability instead of an error. |
| 279 | +- **Accept and ignore `WAIT` on system clusters.** Rejected: an instant |
| 280 | + success that waited for nothing is the same silent lie we fixed for |
| 281 | + unmanaged clusters (see the v26 release notes entry on `WAIT` being |
| 282 | + silently ignored). Moot under this design, where the `WAIT` is honored. |
0 commit comments