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
| <operation_timeout_ms>10000</operation_timeout_ms> | ||
| <raft_logs_level>information</raft_logs_level> | ||
| <session_timeout_ms>100000</session_timeout_ms> | ||
| <use_xid_64>1</use_xid_64> |
| <!-- | ||
| async_replication and use_xid_64 are intentionally NOT set here. | ||
| They are version-gated coordination settings: older ClickHouse Keeper | ||
| releases reject them with UNKNOWN_SETTING and CrashLoop at startup |
There was a problem hiding this comment.
i think we will allow support 26.3 keeper for this, old clickhouse-keeper versions actually useless from my point of view without this settings
| // 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.
| util.Iline(raft, i, " <hostname>%s</hostname>", c.namer.Name(interfaces.NameInstanceHostname, host)) | ||
| util.Iline(raft, i, " <port>%d</port>", host.RaftPort.Value()) | ||
| if isJoiningEstablishedCluster(c.cr, host) { | ||
| util.Iline(raft, i, " <start_as_follower>true</start_as_follower>") |
There was a problem hiding this comment.
Nit: <start_as_follower> stays in the published ConfigMap until the next reconcile rewrites the XML (the joiner keeps ObjectStatusRequested only during the current pass, but the last-published XML persists). Harmless — the flag only affects boot, and the NuRaft all-followers restriction can't be hit since bootstrap never emits it — but worth a one-line comment so a future reader doesn't treat the lingering flag in the ConfigMap as a bug.
2 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.