cluster-controller: graceful reconfiguration strategy + ALTER reshape + wait-shim#37452
cluster-controller: graceful reconfiguration strategy + ALTER reshape + wait-shim#37452aljoscha wants to merge 6 commits into
Conversation
0396524 to
0ddd9cc
Compare
| /// rewritten in place for bookkeeping (stamping or resetting the linger | ||
| /// clock), which is not a lifecycle transition and declares no audit. | ||
| #[derive(Clone, Copy, Debug, PartialEq, Eq)] | ||
| pub enum BurstAudit { |
There was a problem hiding this comment.
this is in here, rather than the "burst" PR because we roll that into the state/plumbing changes, I suppose.
There was a problem hiding this comment.
Yes, exactly. The durable base commit lands the full audit shape surface in one place (one proto migration, one hashes/snapshot update), and BurstAudit is the in-memory mirror of the ClusterHydrationBurstV1 shapes that live in that same migration. The burst PR only adds the writer that declares these intents; it adds no new durable or type surface.
| /// to the realized config immediately. The controller converges the replica | ||
| /// set onto the target and cuts the realized shape over at hydration. | ||
| /// | ||
| /// **Fold semantics.** When a record is already in flight, the target is an |
There was a problem hiding this comment.
We might have to reconsider this whole bit about folding in even non-shape changes. Why do we do that? Why is it not okay to just update workload_class, schedule, scaling strategies, even replication factor while a reconfiguration is in progress? You take a look, explore, and ponder, and we discuss in our chat, please. Not here on github.
| /// Returns only checks that are already storage-hydrated and known to the | ||
| /// compute controller. The compute receiver completes off the coordinator | ||
| /// loop. | ||
| fn start_hydration_checks( |
There was a problem hiding this comment.
please double check that this matches the legacy code paths, in terms of semantics, report your findings right here, please
There was a problem hiding this comment.
Compared against legacy (check_if_pending_replicas_hydrated_stage on main). The controller APIs used are the same (compute collections_hydrated_on_replicas + storage collections_hydrated_on_replicas, both skipping transient collections, exports/tables/webhooks always hydrated), but the semantics differ in three deliberate ways:
- Per-replica vs. any-of-set (stricter here). Legacy asks "is every collection hydrated on at least one pending replica", so with rf > 1 it could finalize while a pending replica was still cold. Here each target replica is probed individually and cut-over requires all shape-matching replicas hydrated (the HA argument in
target_hydrated's doc). - Replica-pinned MVs. Legacy passes an empty exclude set. Combined with
replicas_hosting(added by compute: fix hydration check for replica-targeted collections #35539 for the 0dt caught-up check), an MV pinned viaIN CLUSTER ... REPLICAto an old replica makes legacy's check return false forever: the ALTER polls until timeout, then rolls back or force-commits. The per-replica exclusion here (MVs pinned to a different replica, keyed byglobal_id_writes()) fixes that hang. The exclusion never existed in legacy. - Unknown replica / missing instance. Legacy converts the controller error into
AdapterError::internal("Failed to check hydration")and fails the whole ALTER. Here it's "not hydrated, re-probe next tick", which matches the shift from a session-driven stage machine to a background reconciler.
Also worth noting: legacy checks exactly the -pending replica set; here all observed replicas are probed and the strategy filters to shape-matching ones, which lets existing replicas legitimately count toward a target (e.g. an AZ-only or rf-composition case). The off-coordinator-loop compute await mirrors legacy's spawned oneshot; storage stays synchronous on the loop in both.
So: not a 1:1 match, it's stricter on cut-over, more forgiving on transient controller errors, and fixes the pinned-MV hang. Happy to align any of these back toward legacy if you disagree with a divergence.
| /// then. A steady cluster is never probed, keeping the seam pay-for-what-you- | ||
| /// use. The pull is per-cluster (the controller asks only about that cluster's | ||
| /// replicas). | ||
| async fn enrich_hydration( |
There was a problem hiding this comment.
feels a bit weird that we're pulling for the strategies, and looking into state for them. Shouldn't it be the strategy logic itself that requests this information? And then we also don't need to store it in cluster state, which has this awkward comment on this about it. If you agree, implement this as a change on top that slots into our stack of PRs.
There was a problem hiding this comment.
Agreed, and implemented as a new commit on top ("cluster-controller: strategies declare the live signals they need"). The strategy now declares what it needs via Strategy::signal_request, a pure function of the durable state, and the kernel unions the requests, fetches them through the ctx, and passes a LiveSignals argument to update_state/desired_replicas. ClusterState is back to exactly the witness material plus the observed replica set, and the awkward witness-exclusion comment is gone.
This also cleaned up the later PRs in the stack, which had been accreting kernel-side engagement predicates: enrich_refresh_window (on-refresh) and burst_arming_candidate/needs_hydration_signal (burst) are gone, each strategy declares its own needs now.
One consequence worth knowing: requests are pure over durable state, so the burst strategy cannot condition its hydration request on the object-existence result fetched in the same round. A policy-carrying cluster with zero hydratable objects now gets its replicas probed each tick (in-memory controller state), where the old kernel skipped that probe. The seam test pins the new contract and documents the trade.
| } | ||
|
|
||
| fn update_state(&self, state: &ClusterState, now: Timestamp) -> StateWrite { | ||
| let Some(record) = &state.reconfiguration else { |
There was a problem hiding this comment.
wait doesn't this make it so we always write an empty record, regardless of current state?
There was a problem hiding this comment.
No: StateWrite::default() is the all-None write, and the kernel drops it before anything happens. merge_state_writes filters .filter(|write| !write.is_empty()) and the phase-1 loop skips with if write.is_empty() { continue; }, so no decision is emitted at all. Also reconfiguration: None inside a StateWrite means "leave the record untouched", not "clear it" (clearing would be Some(ReconfigurationWrite { record: None, .. })). I'll add a sentence to the StateWrite docs making the empty-means-no-write contract explicit, since it evidently doesn't read that way.
|
|
||
| > DROP CLUSTER cc_graceful | ||
|
|
||
| # A logging (INTROSPECTION) change is also a config-shape change: the controller |
There was a problem hiding this comment.
double check that this was also the behavior of a legacy code, for graceful reconfig, and report here please, and put this and everything also in a summary in our chat
There was a problem hiding this comment.
Checked against legacy (upstream/main, sequence_alter_cluster_managed_to_managed): yes, this matches. Legacy's reshape trigger is exactly
let config_changed = new_size != &size
|| new_availability_zones != &availability_zones
|| new_logging != &logging;(cluster.rs:1306-1308 on main), so a logging-only change (INTROSPECTION INTERVAL and DEBUGGING both fold into logging at plan time) always meant full replica re-creation in legacy too. With a WAIT clause it took the pending-replica graceful path, without one it dropped and recreated all replicas in a single transaction. There was no in-place path for logging changes.
The audit trail differs only in vocabulary, not shape. For an RF=N reshape both legacy and this branch emit N creates + N drops, but legacy hardcoded ReplicaCreateDropReason::Manual for both the pending creates (call site at cluster.rs:288) and the finalization drops (cluster.rs:394), so everything read manual. The branch attributes creates as reconfiguration and cut-over drops as retired, which is strictly more informative.
One legit shape nuance: legacy renamed pending replicas back to the old names at finalization (r1 stays r1), while the controller advances to fresh names (r1 -> r2), which the td asserts a few lines above.
| > CREATE CLUSTER cc_altback (SIZE 'scale=1,workers=1', REPLICATION FACTOR 1) | ||
|
|
||
| > ALTER CLUSTER cc_altback SET (SIZE 'scale=1,workers=2') | ||
| > ALTER CLUSTER cc_altback SET (SIZE 'scale=1,workers=1') |
There was a problem hiding this comment.
doesn't this have the potential to be flaky?
There was a problem hiding this comment.
Yes, on a badly stalled host: the scenario relied on the statement gap being shorter than a replica boot plus hydration, so the first reconfiguration could in principle cut over before the re-target lands. Fixed in a new commit on top: the fallback tick cadence is suspended (huge interval) across the two ALTERs. Ticks then fire only on catalog changes (reconcile_now), and the tick the first ALTER triggers cannot observe a hydrated target yet (its replica has not booted), so the cut-over cannot precede the re-target regardless of scheduling. The later cc_cancel audit scenario in the stack has the same shape and gets the same treatment there.
| /// even where the resulting value doesn't change, so both still need to reach | ||
| /// the reshape path regardless of whether a reconfiguration is already in | ||
| /// progress. | ||
| fn alter_changes_replica_shape(options: &PlanClusterOption) -> bool { |
There was a problem hiding this comment.
should do an exhaustive destructure here
There was a problem hiding this comment.
Done — exhaustive destructure now, so a new PlanClusterOption field forces a decision here.
| ); | ||
| if new_config == config && !(cluster_controller_owns && reconfiguration_in_flight) { | ||
|
|
||
| // Replication factor is one of the four dimensions the controller's |
There was a problem hiding this comment.
I have a feeling we need to do a pass over all the comments/rustdoc introduced in this change and cut it back a bit/consolidate, per our standing regular guidelines.
There was a problem hiding this comment.
Agreed, did a pass: cut the routing/no-op-guard/reshape comments down to the non-obvious parts, merged the two-paragraph alter_changes_replica_shape doc into one, and trimmed the cancel_carried_reconfiguration call-site comment and the handle_scheduling_decisions comment. Also caught and fixed a real bug from a review pass in the same commit: that scheduling path bypasses the new replication-factor guard entirely (it calls sequence_alter_cluster_managed_to_managed directly, not through the staged sequencer), so it now defers itself while a reconfiguration is in flight instead of racing it.
ce1c713 to
f666926
Compare
f666926 to
ac80fb1
Compare
mtabebe
left a comment
There was a problem hiding this comment.
Still have tests and src/adapter/src/coord/sequencer/inner/cluster.rs to look at ... will keep going
| We introduce a **cluster controller** as a dedicated task inside `environmentd`. It is the single decision-maker for the replica set of every user managed cluster (system and builtin clusters keep their bootstrap-owned replicas). It operates as a reconciler: it reads desired cluster state from the durable catalog, observes the actual replica set and live status signals, computes a desired replica set per cluster by combining a set of **strategies**, and emits catalog-change commands to the Coordinator via a message channel. The Coordinator remains the sole writer of catalog state. | ||
|
|
||
| ALTER CLUSTER operations are reshaped so that the user's intent is written durably and immediately to the catalog — as an in-flight target record, leaving the cluster's realized config in place until the controller cuts over; the controller then reconciles from that intent. Graceful reconfiguration becomes a strategy within the controller framework, as does hydration burst. The existing `ON REFRESH` scheduling is lifted into the framework. | ||
| ALTER CLUSTER operations are reshaped so that the user's intent is written durably and immediately to the catalog as a reconfiguration record with `status = InProgress`, leaving the cluster's realized config in place until the controller cuts over. The controller then reconciles from that intent and retains the latest record with a terminal status when it resolves. Graceful reconfiguration becomes a strategy within the controller framework, as does hydration burst. The existing `ON REFRESH` scheduling is lifted into the framework. |
There was a problem hiding this comment.
Nice, 👍 thanks for keeping this doc up to date
| alter_strategy = AlterClusterPlanStrategy::try_from(alter_strategy_extracted)?; | ||
|
|
||
| // Only a replica config shape change has a hydrate-overlap | ||
| // to wait on. Reject a `WAIT` on anything else rather than |
| /// `ALTER` writes this record with [`ReconfigurationStatus::InProgress`] and | ||
| /// returns. The realized config (`cluster.size`, ...) is advanced by the | ||
| /// controller only at cut-over. When the reconfiguration settles, the controller | ||
| /// retains the record with a terminal status so readers can inspect the latest |
| crate::json_compatible!(v88::OnTimeoutAction with v89::OnTimeoutAction); | ||
|
|
||
| /// Adds `ReconfigurationState::status`, backfilling any in-flight | ||
| /// reconfiguration as `InProgress`. The remaining v88->v89 changes are |
There was a problem hiding this comment.
I'm kind of curious ... how does a reconfiguration happen across an upgrade? I thought we blue-green?
So is the situation that on the new "green" side we have a burst reconfiguration running for hydration and that will get marked as in progress?
That green side can't write to the durable catalog so it is all in its in memory catalog right?
There was a problem hiding this comment.
If so ... why is there a need for a migration?
Shouldn't they just see the correct state by default?
(I realize that in a non blue-green world, or a world where we can read mixed catalogs we would need to support this, I'm just a little confused)
There was a problem hiding this comment.
Talked about this "offline" , it's just that we are careful in migrating everything, and if there happens to be a reconfig record as we're upgrading, it get's rewritten, and the rest of the behavior "falls" out of that. Though, for real, we can't even have background reconfigurations in prod or anywhere right now, so ... 🤷♂️
|
|
||
| impl From<ReconfigurationStatus> for durable::ReconfigurationStatus { | ||
| fn from(status: ReconfigurationStatus) -> Self { | ||
| match status { |
There was a problem hiding this comment.
🤷 this is kind of annoying to have to continue to have this translation layer... why do you like to do this?
There was a problem hiding this comment.
Also talked about this one: I don't "like" it, but it's what we're currently doing. We might want to do a pass afterwards to remove the extra in-memory or whatnot types when we don't need them
| /// A write to the `reconfiguration` record, bundled with the audit intent | ||
| /// declaring which lifecycle transition the write represents. | ||
| /// | ||
| /// Bundling means a writer cannot move the record without deciding, at the same |
| let target_replicas: Vec<_> = state | ||
| .replicas | ||
| .iter() | ||
| .filter(|r| r.shape.matches(&target_shape)) |
There was a problem hiding this comment.
I am a little confused by two things here: (1) are we filtering to any replicas that have the same shape, or just the relevant replicas that also have the target shape, and (2) are we targetting ALL replicas or just enough replicas to satisfy the replication factor?
This might be a misreading in the long diff though...
There was a problem hiding this comment.
hey aj, can you double check this: I think the semantics we want is: we filter to target replica shape, we know our replication factor (rf), and so we need rf-many target replicas that are hydrated to go ahead. If there are extra replicas at the target shape (for whatever reason, probably doesn't happen in practice) that's fine, and we shouldn't block until all of them are hydrated. Please answer in here.
There was a problem hiding this comment.
Checked, and the code did the stricter thing: cut-over required all target-shape replicas hydrated (count >= rf AND every matching one hydrated), so a stray extra could block indefinitely. Changed to the semantics you describe: at least rf-many hydrated target-shape replicas suffice, extras neither count against nor block the cut-over. The kernel test that pinned the old behavior now pins the new one.
One nota bene: the post-cut-over reconcile retires surplus replicas hydration-blind (it cannot be hydration-aware, the hydration signal is only requested while a record is in progress), so in the pathological extra-replica case it may keep the cold one and retire a hydrated one. Extras cannot arise through the ALTER surface though (the record path requires a shape change, and replicas of a managed cluster are controller-managed), so that stays theoretical.
There was a problem hiding this comment.
As a follow-up, we might want to make retiring of replicas hydration aware? So when we know we need to cut back because we have too many replicas of a given shape, we first drop those that are not hydrated and keep hydrated ones?
| // | ||
| // The failed apply rolled back without changing durable state, | ||
| // so this tick's `expected` witness is still current, unless a | ||
| // concurrent user `ALTER` re-targeted the record, in which case |
There was a problem hiding this comment.
Is there a race with a cancellation? Or another alter? What do you mean left to converge?
There was a problem hiding this comment.
a cancellation or retarget (say an ALTER with a different shape or config), and if that happens to be applied before we arrive here, the apply fails and the controller re-tries whatever it needs to do on the next tick. And converging here means that the state or the provisioned replicas converge to whatever is implied by durable cluster state (when the next tick comes, and beyond that). Does that help?
| old_config: &ClusterConfig, | ||
| new_config: &ClusterConfig, | ||
| ) -> bool { | ||
| fn record_of( |
There was a problem hiding this comment.
This is inline here but duplicated about 20 lines later
There was a problem hiding this comment.
Done: hoisted into a shared reconfiguration_record_of helper used by both call sites.
| (ReconfigurationLifecycleV1::ResourceExhausted, None) | ||
| } | ||
| }; | ||
| let coherent = matches!( |
There was a problem hiding this comment.
I might have missed this, but somewhere we should have a table/diagram with the valid state transitions/combinations
There was a problem hiding this comment.
Added a transition table to the rustdoc of the in-memory ReconfigurationStatus (from/to/audited-as, plus the no-revival rule), and the coherence check's doc now points at it.
…gration v88->v89) **NOTE: This is part of PR3 in a stack of PRs, the full branch is at MaterializeInc#36738 All durable-catalog changes for controller-driven graceful reconfiguration, in one migration: - `ReconfigurationState` gains a `status` (in-progress, finalized, timed-out, cancelled, resource-exhausted). ALTER writes the record as in-progress and the controller retains it with a terminal status instead of deleting it, so the latest outcome stays inspectable from SQL without replaying the audit log. The v88->v89 migration backfills an in-flight record as in-progress. - The audit-log event shapes gain the fidelity the lifecycle needs: a resource-exhausted reconfiguration transition, a `forced` flag on finalized transitions (deadline commit vs. hydrated cut-over), and a `finish_cause` on hydration-burst finished transitions (linger-elapsed vs. no-longer-warranted). The events are emitted by the follow-up commit. This one only lands the shapes. - `parse_catalog_audit_log_details` learns the new enum variants, plus null passthrough for optional enum fields (`finish_cause`), keeping the `mz_audit_events` round-trip property intact. The in-memory and durable mirrors, proto conversions, snapshot, and hashes move in lockstep. Two adapter call sites get a mechanical `status` fixup. Behavior changes land in the follow-up commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ac80fb1 to
0978025
Compare
mtabebe
left a comment
There was a problem hiding this comment.
Ok, in general, it looks good to me. I left some claude comments on the tests. I'll keep iterating on this though.
| workload_class: new_config.workload_class.clone(), | ||
| timeout_time: Instant::now() + timeout.to_owned(), | ||
| on_timeout: on_timeout.to_owned(), | ||
| // The legacy foreground wait uses COMMIT as |
There was a problem hiding this comment.
Isn't rollback (what you have now) safer? Why was it commit before?
There was a problem hiding this comment.
It is safer yes, and I don't know why the default was COMMIT before. This will go away once we flip the feature flags (and then remove the legacy code), and even with the new foreground wait shim we'll use ROLLBACK as the default. I wanted to decouple the default change from the code change, and have it come from when we flip the flags.
There was a problem hiding this comment.
Ah, please double check me aj and reply here: even after all the flags of our in-flight stack are flipped on, we still default to COMMIT for the foreground wait shim path? What was our reasoning again?
There was a problem hiding this comment.
Checked: no, the COMMIT default does not survive the flag flip. The unwrap_or(Commit) here sits exclusively on the legacy pending-replica stage machine (ClusterStage::WaitForHydrated -> check_if_pending_replicas_hydrated_stage), which only runs when the controller does not own the cluster. It preserves legacy's implicit COMMIT for the flag-off world and gets deleted with the rest of the legacy code.
With the flags on, every shape-changing ALTER takes the record path, where the default is ROLLBACK (reshape_alter_cluster_managed resolves on_timeout.unwrap_or(Rollback), and a bare WAIT FOR also maps to ROLLBACK). The foreground wait shim has no timeout action of its own at all: it only polls until the record settles, so the semantics always come from the record. Blocking or background, flags on means ROLLBACK unless the user spells out ON TIMEOUT COMMIT.
There was a problem hiding this comment.
One correction to my reply above: bare WAIT FOR '<duration>' is sugar for ON TIMEOUT COMMIT, not ROLLBACK (deliberate legacy parity, documented at the desugar site in reshape_alter_cluster_managed). The rest stands: no WAIT at all resolves to ROLLBACK with the system-default timeout, and WAIT UNTIL READY without an explicit ON TIMEOUT resolves to ROLLBACK. So after the flag flip the defaults are ROLLBACK everywhere except the explicit WAIT FOR sugar, which keeps its legacy commit-at-deadline meaning.
| MAX_REPLICAS_PER_CLUSTER.name(), | ||
| )?; | ||
|
|
||
| // Global credit rate: the peak is `credit(realized) + credit(target)`. |
There was a problem hiding this comment.
Not a fan of this logic being in here... can it be extracted out?
There was a problem hiding this comment.
You mean extracted out into a method, or out of the sequencing logic? If the latter, where to?
| // | ||
| // NOTE: If the controller stops resolving a record while it is | ||
| // in progress, the shim waits indefinitely. Cancelling the session | ||
| // only stops waiting. It does not abort the durable reconfiguration. |
There was a problem hiding this comment.
Right this makes sense, but what can we do? Can we not move the record to cancelled and the reconfiguration see that and then cancel?
There was a problem hiding this comment.
We could, but right now the idea is that cancelling an in-flight reconfiguration has to be explicit. The current (legacy) implementation has the property that cancelling the session will cancel an in-flight reconfiguration, which is not good/surprising. As it is right now in this PR, the "blocking" waiting "optional" UX, if you will
There was a problem hiding this comment.
aj says:
Mechanically possible (that's exactly how cancel works), but deliberately not wired to session-cancel: the durable background lifecycle is decoupled from session lifetime by design, so a network
blip on a blocking ALTER doesn't abort a long reconfiguration. Explicit cancel exists (ALTER back).
so basically what I said but with more rationale... 😅
There was a problem hiding this comment.
[From claude 🤖 ]
The current cc_timeout block only proves that the three ON TIMEOUT spellings (default, explicit ROLLBACK, explicit COMMIT, and WAIT FOR desugaring) are accepted under the gate. Every scenario uses an empty cluster that hydrates promptly, so success precedence always wins and the deadline is never hit.
Concrete scenarios I'd want to see
- ON TIMEOUT ROLLBACK — deadline elapses un-hydrated. Force the target not to hydrate before the deadline. Assert:
- mz_clusters.size = pre-reconfig size.
- mz_cluster_replicas shows only the pre-reconfig replicas (all target replicas dropped, including any that hydrated).
- The retained record has status = TimedOut.
- An audit event of type ReconfigurationLifecycleV1::TimedOut exists with the correct deadline.
- The blocking ALTER (if enable_background_alter_cluster = false) returns AlterClusterTimeout. - ON TIMEOUT COMMIT — deadline elapses un-hydrated. Same forcing setup. Assert:
- mz_clusters.size = target size (forced cut-over).
- mz_cluster_replicas shows the target-shape replicas (possibly un-hydrated; that's the explicit tradeoff).
- The retained record has status = Finalized.
- An audit event of type ReconfigurationLifecycleV1::Finalized exists with forced: true (this is exactly the forced field the audit-log schema added for —
worth exercising).
- The blocking ALTER returns success, not timeout. - Implicit ROLLBACK default (ALTER with no WITH (WAIT ...)). Uses default_cluster_reconfiguration_timeout. Set that dyncfg small in the test setup, force
non-hydration, assert same shape as scenario 1. This is the release-note-flipped default; ideally it's the first thing asserted.
There was a problem hiding this comment.
Fair point that the deadline never actually fired in td. Added two deterministic end-to-end scenarios to this file (the hydration blocker turns out to be pure SQL, the same heavy indexed cross-join view the cloudtest uses, so no orchestration is needed):
- ROLLBACK at the deadline: heavy indexed view,
TIMEOUT '1s'. Asserts the audit trail is exactlystarted->timed-out, the realized config is untouched, and only the original replica remains. - COMMIT at the deadline: same setup. Asserts
started->finalizedwithforced: true(the one path that sets theforcedaudit field, previously unexercised end-to-end) and the realized config advanced to the target.
The race is one-sided: the view takes tens of seconds to hydrate against a 1s deadline, and a miracle host fails the test loudly with a successful cut-over rather than hanging. Skipped the third scenario (implicit default): it needs fiddling the default-timeout dyncfg to ~0 and back, and the default's resolution (ROLLBACK + the system default timeout) is pinned at the unit level. The full provision-then-teardown at the k8s level stays covered by the cloudtest.
There was a problem hiding this comment.
Small upgrade after checking for stronger precedent: the scenarios now block hydration with a mz_unsafe.mz_sleep materialized view (an hour of sleep per input row, argument derived from table data so it cannot be constant-folded) instead of the heavy cross-join. That makes the block wall-clock deterministic on any host rather than a CPU-speed bet, and it burns no CPU while the scenario runs. Assertions are unchanged.
There was a problem hiding this comment.
[From 🤖 ]
The ~12 .td files touched here all rely on testdrive's implicit > SELECT retry to wait for async reshape. That retry has no upper bound. A wedged controller means the query polls silently until the whole-test timeout fires, and the Buildkite log points at the suite rather than the wedged step.
test/cluster/mzcompose.py:8019-8033 already has the bounded pattern: 120 × 0.5s ceiling, explicit AssertionError naming the cluster and expected count. That's what a stuck reconcile should look like from a debugger's seat.
Can we somehow generalize this pattern to avoid the silent timeouts?
There was a problem hiding this comment.
AFAIK testdrive has a timeout that we also set in CI, and when the result doesn't settle on the expectation we'll print what we got and what was expected
8fc699a to
e42a11d
Compare
… + wait-shim **NOTE: This is PR3 in a stack of PRs, the full branch is at MaterializeInc#36738 Move graceful (zero-downtime) cluster reconfiguration into the cluster controller as a pure strategy driven by the durable `reconfiguration` record. Everything lands dark behind the `enable_cluster_controller` master gate. The legacy 3-stage machine still runs when the gate is off. What happens: - ALTER writes (or folds onto) the durable record as in-progress and leaves the realized config in place. The controller desires the target replicas on top of the realized set (the hydrate-overlap) and advances the realized config to the target once the target set is fully hydrated. Success takes precedence over the deadline. At the deadline un-hydrated, ON TIMEOUT COMMIT cuts over anyway and ROLLBACK abandons the reconfiguration without touching the realized config. - The record is retained with a terminal status (finalized, timed-out, cancelled, resource-exhausted) instead of deleted, so the latest outcome stays inspectable from SQL. - Strategies declare the audit intent together with the state write (`ReconfigurationWrite` bundles the record with its transition). The intent rides the controller decision into `Op::UpdateClusterConfig`, and the catalog builds the audit event from the written record, validates that the intent coheres with the record status, and commits state and event in one transaction. A forced cut-over (COMMIT at the deadline, target un-hydrated) is recorded as `forced` on the finalized event, which only the decision point knows. - A foreground wait-shim (`ClusterStage::AwaitReconfiguration`) polls the record until it settles, preserving today's blocking ALTER UX. With the new `enable_background_alter_cluster` dyncfg on, ALTER returns immediately. - ALTER validates the overlap's transient resource peak up front (both shapes at once, against `max_replicas_per_cluster` and `max_credit_consumption_rate`). A record that becomes unsatisfiable later (a limit shrank) is aborted and audited as resource-exhausted rather than parked in a retry loop. Points worth attention: - An ALTER back to the realized shape is a cancel: the sequencer writes the record as cancelled and declares the cancelled audit intent at the same decision point, and the controller tears the pending target replicas down. The cancel path also skips the resource checks, keeping the escape hatch usable at the limits. - The wait-shim deliberately has no timeout of its own: the controller owns the deadline, and a shim-side timeout could race a COMMIT-at-deadline cut-over and misreport it as a timeout. Session disconnect stops waiting but does not abort the reconfiguration. - Paths that omit ON TIMEOUT resolve their own default: controller-owned paths use ROLLBACK (never silently induces downtime by cutting over to an un-hydrated target), while the legacy foreground path keeps its historical implicit COMMIT. - Hydration is read through a new narrow ctx seam (`hydrated_replicas`), pulled on demand only while a reconfiguration is in flight and excluded from the compare-and-append witness. - The managed-to-managed ALTER path no longer preallocates replica ids when the controller owns the replica set. The controller allocates its own when it materializes the change. Tests: kernel and flow cases in mz-cluster-controller (cut-over, partial hydration, timeout-vs-hydrated precedence, COMMIT vs ROLLBACK at the deadline, ALTER-back, target folding, resource-exhaustion sheds), audit builder unit tests in mz-adapter, and an extended cluster-controller.td. Source-ingest and CDC testdrive files gain poll-for-convergence guards because replication-factor changes now reconcile asynchronously. Implements the graceful reconfiguration part of doc/developer/design/20260522_cluster_autoscaling.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… fixes) **NOTE: This is part of PR3 in a stack of PRs, the full branch is at MaterializeInc#36738 Review follow-ups to the graceful reconfiguration change. Kept separate for reviewability: - Refuse converting a cluster to unmanaged while a reconfiguration is in progress (new `AlterClusterUnmanagedWhileReconfiguring` error). The unmanaged variant has no reconfiguration field, so the conversion would silently drop the in-progress record with no terminal status and no audit event, and strand any overlap replicas the controller already created. The user can cancel (ALTER back to the realized size) or wait for the record to settle first. - Backstop the same class of bug in the catalog: an `Op::UpdateClusterConfig` that moves the reconfiguration lifecycle (a status change, a fresh record, or dropping an in-progress record) without declaring an audit intent now fails the transaction. - Legacy ALTER paths (controller gate off) retain a stale in-progress record as cancelled, declaring the matching audit intent, instead of carrying it forward unchanged. Nothing on those paths ever settles a record, and carrying one forward invites a bogus revival, up to a forced cut-over to an obsolete target, when the gate is turned back on. - Compute the credit peak by multiplication instead of repeated addition. - Comment fixes: correct the justification of the graceful strategy's past-deadline early return (it covers a deadline crossing between the two reconcile phases' clock reads, not a stale witness), drop a reference to a strategy that does not exist, reword a test whose cancel framing predated the immediate-cancel semantics, and a sweep of sentence-structure nits. Tests: kernel cases pinning that an rf=0 target cuts over on the first tick and that an extra un-hydrated same-shape replica blocks cut-over (deliberate conservatism), unit coverage for the new lifecycle-movement guard, and testdrive coverage for the reconfiguration lifecycle audit trail (`started`/`finalized` with `forced`) and the refused unmanaged conversion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
**NOTE: This is part of PR3 in a stack of PRs, the full branch is at MaterializeInc#36738 Move the "when does a strategy need hydration" knowledge out of the kernel and into the strategy. Previously the kernel's `enrich_hydration` re-stated the graceful strategy's engagement condition (an in-progress record) to decide when to probe, and stashed the result in `ClusterState::hydrated_replicas`, a live-signal field that needed an awkward exclusion from the compare-and-append witness. Now a strategy declares its needs via `Strategy::signal_request`, a pure function of the durable state. The kernel unions the requests across strategies, fetches them through the ctx (per cluster, only where requested, so a steady cluster is still never probed), and passes the result to `update_state` and `desired_replicas` as a separate `LiveSignals` argument. `ClusterState` is now exactly the witness material plus the observed replica set. Strategies stay pure and synchronous: they never touch the ctx, they only name what they need and compute over what was fetched. Later strategies with their own live signals (burst arming, refresh windows) extend `SignalRequest`/`LiveSignals` instead of adding kernel-side engagement predicates. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ng-robust **NOTE: This is part of PR3 in a stack of PRs, the full branch is at MaterializeInc#36738 The back-to-back ALTERs raced the controller's convergence: whether the second ALTER cancels the in-flight reconfiguration or the first has already cut over depends on timing, and the scenario asserted the cancel interleaving specifically. Assert only the convergent outcome (the cluster settles at the original size with a single replica), which holds under either interleaving. The cancel semantics themselves stay pinned in the controller's deterministic kernel unit tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ALTER **NOTE: This is part of PR3 in a stack of PRs, the full branch is at MaterializeInc#36738 An `ALTER` that changed no replica config shape dimension (`SIZE`, `AVAILABILITY ZONES`, either `INTROSPECTION` option) but did carry a `WAIT` clause, or that changed the replication factor while a reconfiguration was already in flight, used to be folded into the `reconfiguration` record anyway: the deadline and `on_timeout` action were unconditionally recomputed from this `ALTER`'s (possibly absent) `WAIT` clause, and the record's status was recomputed and could re-declare a spurious `started` audit event, all triggered by a change that had nothing to do with the reconfiguration. Tighten the surface instead: - A `WAIT` clause is now rejected at plan time unless the `ALTER` also sets a shape option. There is nothing to hydrate-overlap on otherwise, so a `WAIT` on such a statement was either a silent no-op or, worse, implied a wait the controller never performed. - Changing the replication factor while a reconfiguration is in flight is now a hard error (`AdapterError::AlterClusterReplicationFactorWhileReconfiguring`). Replication factor is one of the four dimensions the controller's cut-over sets atomically from the record's target, so changing it independently would either be silently clobbered at cut-over or, folded into the target instead, double-count an already-materialized overlap against the resource limits validated at `ALTER` time. - `reconfiguration_needs_record` (renamed `alter_changes_replica_shape`) is now a pure, statement-level check: does this `ALTER` set a shape option, full stop. The old second condition ("or a reconfiguration is already in flight") is gone, since it's now unreachable through the `ALTER` path: a replication-factor change while in flight is refused above, and every other field applies directly to the realized config regardless of an in-flight record, without ever touching it. - The legacy (`controller_owns == false`) config-write path still retains a stale in-progress record as cancelled when it writes a new realized config, since nothing converges an orphaned record left over from a gate flip. With the controller owning the cluster, a record still in flight at that point belongs to a live, converging reconfiguration the write has no business touching, so it is now carried through untouched instead. - `handle_scheduling_decisions` (the ON REFRESH auto on/off policy) flips the replication factor directly and bypasses the `ALTER` path's new guard entirely, so it now defers its own flip (skips this tick, same treatment as the pre-existing pending-replica case) whenever a reconfiguration is in progress, rather than racing the replication factor out from under the record's target. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
e42a11d to
1a01739
Compare
NOTE: This is PR3 in a stack of PRs, the full branch is at #36738
Move graceful (zero-downtime) cluster reconfiguration into the cluster
controller as a pure strategy driven by a durable
reconfigurationrecord. Everything lands dark behind the
enable_cluster_controllermaster gate. The legacy 3-stage machine still runs when the gate is off.
How it works:
ALTER CLUSTER(SIZE,AVAILABILITY ZONES,INTROSPECTION) writes the durable record as in-progress and leavesthe realized config in place. The controller desires the target
replicas on top of the realized set (the hydrate-overlap) and advances
the realized config to the target once the target set is fully
hydrated. At the deadline un-hydrated, ON TIMEOUT COMMIT cuts over
anyway (audited as forced) and ROLLBACK abandons the reconfiguration
without touching the realized config. Success takes precedence over
the deadline.
cancelled, resource-exhausted) instead of being deleted, inspectable
from SQL. All durable-catalog changes land in one migration
(v88->v89), including the audit-log event fidelity the lifecycle
needs (a
resource-exhaustedtransition, aforcedflag onfinalized events, a
finish_causeon hydration-burst finishedevents).
and the catalog validates coherence and commits state and event in
one transaction. Any config write that moves the reconfiguration
lifecycle without a declared intent fails the transaction.
(
Strategy::signal_request) as a pure function of durable state; thekernel unions the requests across strategies and fetches them once
per tick. Hydration is only pulled while a strategy is engaged.
ALTERback to the realized shape is an immediate, terminalcancel. Further shape
ALTERs while a record is in flight re-targetit (fold per-dimension onto the in-flight target).
WAITclause on anALTERthat changes no shape dimension is rejected at plan time,changing the replication factor while a reconfiguration is in flight
is a hard error, and everything else (
WORKLOAD CLASS,SCHEDULE,...) applies directly without touching the record. The ON REFRESH
scheduling policy defers its replication-factor flips while a
reconfiguration is in flight for the same reason. Converting to
unmanaged mid-reconfiguration is refused.
new
enable_background_alter_clusterdyncfg on, ALTER returnsimmediately.
record that becomes unsatisfiable later is aborted and audited as
resource-exhausted rather than parked in a retry loop.
Points worth attention:
owns the deadline, and a shim-side timeout could race a
COMMIT-at-deadline cut-over and misreport it as a timeout. Session
disconnect stops waiting but does not abort the reconfiguration.
paths use ROLLBACK (never silently induces downtime by cutting over to
an un-hydrated target), while the legacy foreground path keeps its
historical implicit COMMIT.
when the controller owns the replica set. The controller allocates its
own when it materializes the change.
in the controller's deterministic kernel unit tests; the end-to-end
tests assert convergent outcomes.
Tests: kernel and flow cases in mz-cluster-controller (cut-over, partial
hydration, timeout-vs-hydrated precedence, COMMIT vs ROLLBACK at the
deadline, ALTER-back, target folding, resource-exhaustion sheds, live
signals), audit builder and catalog-coherence unit tests in mz-adapter,
WAIT-surface regression tests in managed_cluster.slt, and an extended
cluster-controller.td. Source-ingest and CDC testdrive files gain
poll-for-convergence guards because replication-factor changes now
reconcile asynchronously.
Implements the graceful reconfiguration part of
doc/developer/design/20260522_cluster_autoscaling.md.
Implements SQL-338