Raft-safe ClickHouseKeeperInstallation rescale - #2041
Conversation
… tags map SetExclude()/SetLowPriority() delegated to Tags.Set on the ReconcileAttributes tags map, which NewReconcileAttributes never initialized. Tags.Set is a no-op on a nil map, so both setters silently did nothing everywhere they were used — including the CHI controller's exclude-host-during-restart and low-priority mechanisms. Lazily initialize the map before setting; regression test added. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
Adds pkg/model/chk/keeper: a small client to observe ClickHouse Keeper's committed Raft state. ParseClusterConfig decodes the /keeper/config znode payload (server.N=host:port;participant|learner;priority); GetCommittedMembership reads it over the ZK protocol via the existing pkg/model/zookeeper client; and the four-letter-word helpers (srvr/mntr/rqld) report role, synced-follower count, and request leadership. Stdlib + testify only; unit-tested against a fake 4LW server. No wiring yet — consumed by later commits. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
The CHK controller ran with no leader election, so two operator replicas would double-reconcile and race the shared ConfigMap. Enable controller-runtime Lease leader election for the CHK manager and grant coordination.k8s.io/leases in the RBAC (source template + regenerated install manifests + helm ClusterRole/Role). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
On scale-up the operator published a raft_configuration containing every desired server at once. Two freshly-started Keeper nodes sharing that config can form a quorum between themselves while the original node still runs as a single-node cluster — a split-brain window. (One fresh node alone cannot: it needs an existing member's vote, which a healthy member with a live leader refuses.) Stage newly-added hosts out of the shared raft config via the reconcile TagExclude attribute and let the existing per-host include path publish them one at a time, so the published config never contains more than one not-yet- committed server. Emit <start_as_follower> for members joining an established cluster (extra guard: a joiner won't start an election until it hears a heartbeat); never on fresh bootstrap, which NuRaft forbids for all servers. Join-vs-bootstrap is classified from CRHasEstablishedCluster (the ancestor already had hosts) — a reconcile-stable signal, not per-host statuses that mutate during the host loop. A host is staged only when its StatefulSet is confirmed absent (IsNotFound), so an interrupted reconcile never re-excludes and rolls back an already-committed member. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
…cale-down Replaces the fixed 7s post-create sleep with real verification against Keeper's committed membership. See worker-raft-membership.go for the design rationale. Scale-up: after each staged member is published and its pod started, wait until it appears in committed /keeper/config on every published member (membership barrier) and the leader reports it synced (mntr). CHK reaches Completed only once committed membership equals the desired set. Scale-down: transfer leadership (rqld) off departing members before publishing the shrunk config, then gate StatefulSet/PVC purge on confirmed committed removal (keyed off the purge set's contents, not the transient action plan). Whitelists rqld in the shipped four-letter-word list. Fail-safe: any barrier timeout / quorum loss returns an error and requeues — nothing is deleted or mutated on an unconfirmed observation. Stopped CRs short-circuit the barriers (pods are scaled to 0, nothing to observe). Secure- only Keepers, which the operator cannot observe over the closed plaintext port (no client TLS material, and Keeper serves no 4LW over the secure port), degrade to pre-feature behavior with a Warning event instead of looping. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
Extends test_020005 (1->3->1) to check committed /keeper/config membership and a single leader after each transition, and adds test_020005_1: a scale-down that cannot commit removals (no quorum) must not delete any StatefulSet — the fail-safe purge barrier holds. Drops the stale test_020005* flaky xfail marker. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Mikhail Filimonov <mfilimonov@altinity.com>
35124a3 to
4578a13
Compare
| // degradation is observable regardless of that config flag. | ||
| func (w *worker) warnRaftGatesSkippedSecureOnly(cr api.ICustomResource, gate string) { | ||
| msg := fmt.Sprintf( | ||
| "secure-only Keeper: skipping Raft %s — the operator cannot read /keeper/config or 4LW over the closed plaintext port and has no TLS client material to dial the secure port; rescale-safety is NOT verified (pre-branch behavior). See docs/chk-rescale-raft-safety-v3.md.", |
There was a problem hiding this comment.
This user-facing Warning event (visible in kubectl describe) references docs/chk-rescale-raft-safety-v3.md, but the PR doesn't add that file — the link points nowhere. Same dangling reference in code comments in worker.go, interface.go, generator.go, event-emitter.go. Please either include the doc in the PR or drop the references (at minimum from the event text).
| removed := removedHosts(cr) | ||
| if len(removed) == 0 { | ||
| return | ||
| } |
There was a problem hiding this comment.
On a multi-host scale-down (e.g. 3→1) the shrunk XML is published with all removals at once, and Keeper's dispatcher serializes them internally (verified against Keeper source: clusterUpdateWithReconfigDisabledThread pops one action and waits for commit before the next) — so this is safe. But Keeper logs LOG_WARNING "strictly not recommended" whenever the config diff contains more than one change, and the original design removes servers one at a time. The one-at-a-time staging in this PR covers additions only. Consider either staging removals one at a time too, or explicitly documenting the decision to rely on Keeper's internal serialization for removals (and expect the Keeper-side warning in logs).
| // Secure-only Keeper: the barrier cannot connect (plaintext port closed, | ||
| // no operator TLS client material). Blocking would orphan the STS/PVC | ||
| // forever; degrade to pre-branch behavior (allow the purge, unverified). | ||
| w.warnRaftGatesSkippedSecureOnly(cr, "purge barrier") |
There was a problem hiding this comment.
The secure-only degradation (skip all barriers, pre-branch behavior) is a pragmatic call and it's honestly surfaced via the Warning event — but it means TLS-only Keeper clusters get none of this PR's safety guarantees. The comment calls real TLS observation "a documented follow-up"; please open a tracking issue so it doesn't get lost.
| also use the default operator probes. | ||
| --> | ||
| <four_letter_word_white_list>conf,cons,crst,envi,ruok,srst,srvr,stat,wchs,dirs,mntr,isro</four_letter_word_white_list> | ||
| <four_letter_word_white_list>conf,cons,crst,envi,ruok,srst,srvr,stat,wchs,dirs,mntr,isro,rqld</four_letter_word_white_list> |
There was a problem hiding this comment.
rqld is added to the default whitelist only — a user who overrides four_letter_word_white_list (the option documented a few lines above) will silently lose proactive leadership transfer on scale-down. That's acceptable since rqld is best-effort and the purge barrier is the real fence, but worth a note in the option's doc comment so users know to include rqld.
|
|
||
| with When("Two of three keepers are down (no quorum for removal commit)"): | ||
| with When("keepers 1 and 2 are deleted"): | ||
| kubectl.launch(f"delete pod {keeper_pod_name(chk, 1)} {keeper_pod_name(chk, 2)} --wait=false") |
There was a problem hiding this comment.
Deleted pods are immediately recreated by the StatefulSet controller, so quorum can recover within the 60s window — at which point the operator may legitimately complete the scale-down and the sts_count == 3 assertion below fails precisely because the system worked correctly and fast. To make the no-quorum condition hold for the whole observation window, keep quorum broken deterministically instead of a one-shot delete pod — e.g. scale the two STSs to 0 replicas (or block raft traffic with a NetworkPolicy) before applying the scale-down manifest, and restore in the cleanup.
|
I suggest we do the following:
Let's see if the problem persists after that. |
Agreed, but a deterministic test needs artificial conditions — synthetic delays or manual network partitioning — because the bug is a race with a self-healing window. I'll re-add the "unhappy-path" test. It has to break into the cluster with
It's a correct step, but it is not sufficient on its own. The problem isn't the role we assign to a new member — it's that we don't track whether Raft actually reacted to each change before we make the next one. Raft membership changes are slow. Before touching the quorum again we have to confirm, for each change, that:
You can't mutate the quorum fire-and-forget. That's the root cause of the current bug. We publish the new config and start both extra nodes at once, without waiting. If the third node joins before the second has been fully committed to the quorum, the second and third form their own majority (2 of 3) and elect a leader without the original node. And that's correct, expected Raft behavior — it's indistinguishable from "only 2 of 3 nodes are online." So the key change this PR introduces is not |
4 participants
Make CHK scale-up/scale-down Raft-safe by replacing the fixed post-create sleep and the all-at-once XML publish with staged, verified membership changes.
Problem: on rescale the operator published a raft_configuration with every desired server at once and only waited a fixed 7s, never confirming committed Raft membership. Two freshly-started Keeper nodes could form a quorum between themselves while the original node still believed it was the whole cluster — a split-brain window. Scale-down deleted StatefulSets/PVCs with no barrier confirming the member had actually left committed membership.
Approach (hardened staged XML; source-verified against Keeper 23.8..master — versioned
reconfigCAS does not exist in any version, and the XML-diff mode is the reliable control plane):/keeper/configznode plus a leadermntrsynced-followers barrier. Never more than one not-yet-committed server in the published XML (split-brain guard). Emit<start_as_follower>for members joining an established cluster.Completedon committed membership == desired.rqld) off departing members, then gate StatefulSet/PVC purge on confirmed committed removal.Adds a keeper observe client (
pkg/model/chk/keeper), the controller barriers (pkg/controller/chk/worker-raft-membership.go), and e2e coverage including a deterministic split-brain demonstration (RED on the pre-fix operator, GREEN on this branch, across Keeper 23.8/24.8/25.8).It contains 6 commits to make review simpler. Check one commit after another.