Protect CHK recreate with live Raft quorum safety

[alex-zaitsev]

Summary

Fixes #2069 (CHK Raft quorum safety during rolls) and #2035 (drop pointless same-size settle delay). Partial #2059 (propagate reconcile status persist errors).

Quorum-safe Keeper rolls (#2069)

On a multi-node CHK, the operator must not take a Ready replica down if that would leave the ensemble below Raft majority, and must not keep rolling the next replica while a previous one never rejoined.

Behavior:

• Before disrupting a host, classify the pass from live Ready counts (not CR ancestor): rolling when the ensemble already has quorum (or is single-node); bootstrap when it does not.
• Rolling waits for Ready; bootstrap waits for Started only so siblings can come up together. The rolling/bootstrap decision is frozen for the host pass so a force-restart (ReadyReplicas→0) cannot flip the pass into bootstrap mid-flight.
• If disrupting this host would break quorum, wait briefly for headroom (poll ~5s, up to ~2m). If still unsafe, defer that host ([RaftQuorumUnsafe]), continue other replicas, then soft-requeue in 5s (not error backoff).
• Prefer recovering not-Ready replicas before rolling Ready peers (helps interrupted rolls and tight ensembles).
• STS create/update wait failure aborts the reconcile instead of ignoring and walking to the next replica (ClickHouseKeeperInstallation controller can recreate every replica of an ensemble without keeping a Raft quorum running #2069 root cause).

Out of scope here: committed Raft membership / mntr barriers (#2041 — hook only), staged rescale (e.g. 3→2→1), and replacing blind downscale sleeps with per-step settle.

Settle delay and status (#2035 / partial #2059)

• Same-size reconciles no longer sleep 10s.
• Downscale still uses the existing 120s pre-settle + 60s post-purge pauses.
• Reconcile start/completion status update failures are returned to the controller; completion metrics fire only after Completed is persisted.

vs CHI shard safety (#1704)

	CHI	CHK (this PR)
Gate	≥1 healthy peer in the shard	Raft majority of Ready members
On refuse	Defer	Wait up to ~2m, then defer + 5s soft requeue
Snapshot before disrupt	No	Yes (survives force-restart Ready drop)
Recovery-first	Yes	Yes

Test plan

• Unit: go test ./pkg/controller/chk/
• Regression: test_020005 xfail removed
• E2e: test_020003 / test_020005 (keeper upgrade / scale)
• E2e: test_020003_3 — 3-node CHK, broken-image roll stops on one replica, peers stay Ready through operator restart, recovery to good image (CHI test_010083 analogue)
• Close fix(chk): let healthy keeper reconciles complete #2059 after merge (remaining items intentionally not included)

Follow-ups

• Staged Raft rescale (one peer per step)
• Per-step quorum settle instead of fixed 120s+60s on downscale
• Full Raft-safe ClickHouseKeeperInstallation rescale #2041 membership barrier after join

[sunsingerus]

Reviewed this in depth — the direction is right and test_020003_3 is a good analogue of the CHI test_010083 shape. But there are two issues I'd call blocking, plus a semantic conflict with current 0.27.4, so I'd like changes before merge.

BLOCKER 1 — a 2-node ensemble can never be rolled again

raftQuorumSize(2) = 2. Disrupting either Ready host leaves remaining = 1 < 2, so ensembleQuorumSafeAfterDisrupt is permanently false:

N=1  quorum=1  after disrupt=0  safe=False   (short-circuited by members<=1)
N=2  quorum=2  after disrupt=1  safe=False   <-- no escape
N=3  quorum=2  after disrupt=2  safe=True

The arithmetic is right — you genuinely cannot take a node out of a 2-node Raft ensemble and keep majority. The problem is the policy: every host defers, reconcileShardWithHosts returns ErrCRUDDeferred, the controller requeues in 5s, forever. Each pass burns the full ~2 min wait per host first.

This lands on existing 2-replica CHKs the moment the operator is upgraded — no spec change needed. The old code rolled through and converged. There's no force flag, no bounded give-up, no terminal state; the CR sits in InProgress with [RaftQuorumUnsafe] indefinitely.

Same shape for N=3 when one replica is permanently unrecoverable for an external reason (PV bound to a dead node, deleted PVC): the two survivors are pinned to the old spec forever.

Needs an escape hatch — a defer budget that converts to Abort, or treating readyCount == members with members <= 2 as disruptable.

BLOCKER 2 — the 2-minute wait cannot observe what it is waiting for

countReadyEnsembleMembers reads host.Runtime.CurStatefulSet and only falls back to a live Get when that is nil. fillCurSTS populates it for every host at reconcile start, so the fallback is dead code in production. refreshQuorumSnapshotCounts then re-Gets only the current host's STS.

So during the poll loop, peers' Ready state is frozen at reconcile-start values — the wait can never see a peer recover, which is the only thing that would make it proceed. It always burns the full budget and then defers.

TestEnsureQuorumSafeToDisruptHost ("waits until ready count increases") passes only because it injects countReadyEnsembleMembersFn and bypasses this path entirely.

Fix: re-Get peers inside refreshQuorumSnapshotCounts, or drop the cache preference there.

HIGH — members is the desired count, so downscale disables the gate

snapshotHostEnsemble uses cr.HostsCount() — the target count. Committed Raft membership is the ancestor set until clean() purges peers.

On a 3→1 downscale, members = 1, so ensureQuorumSafeToDisruptHost short-circuits on snap.members <= 1 and the gate turns off exactly when live Raft still needs 2 of 3. Same class for 3→2. That's a green-light, not a conservative approximation — the opposite direction from the rest of the design. Suggest max(desired, ancestorHostsCount).

HIGH — the gate silently disables itself on a transient API error

countReadyEnsembleMembers and refreshQuorumSnapshotCounts both do sts, _ = ...Get(...) and drop the error. One 500 or timeout undercounts Ready, which flips rolling to false — disabling both the quorum gate and SetWaitUntilReady. That is precisely the #2069 fan-out this PR exists to prevent, re-enabled by an apiserver blip. Note the base code routes STS Gets through common/kube.GetWithRetry; this path doesn't.

HIGH — the merge conflict is semantic, not mechanical

The PR is CONFLICTING against current 0.27.4. One file conflicts: pkg/controller/chk/worker.go, three hunks. 0.27.4 gained an aborted bool return on finalizeCR that deliberately skips ReconcileComplete()/SetAncestor when normalize aborted. This PR replaces finalizeCR with persistReconcileCompleted, which calls both unconditionally — so taking "theirs" silently reverts that fix and publishes Completed for a spec the operator refused. TestPersistReconcileCompleted currently pins the removed guard.

Relatedly, the branch is based well below current 0.27.4 and the diff deletes ~1,470 lines of existing test code (chi/worker-deleter_test.go, worker-reconcile-gate_test.go, clickhouse/connection_pool_test.go, zookeeper/path_manager_test.go, and others). I assume that's a rebase artifact rather than intent, but it needs rebasing onto 0.27.4 before the coverage picture can be judged.

Tests

The unit tests are mostly good and genuinely non-vacuous — TestSnapshotHostEnsemble, TestEnsembleQuorumSafeAfterDisrupt, TestHostDisruptionWouldBreakQuorum, TestMembershipSettleDelay all fail if the logic is reverted. Three gaps:

• TestRaftQuorumSize covers only 0, 1, 3, 5. An N=2 case would have caught BLOCKER 1 immediately. Please add even sizes.
• TestErrCRUDDeferredIsDistinctFromAbort is fully vacuous — ErrCRUDDeferred already exists at 0.27.4, and the test is a verbatim copy of the CHI one. It passes unchanged against pre-PR code.
• The frozen-decision property, the 5s soft requeue, the recovery-first wiring, and noteCRUDResult's deferred aggregation across concurrent workers have no coverage.

test_020003_3 is weaker than its CHI analogue in one load-bearing spot. test_010083 does force_chi_reconcile(chi, "force", "Aborted") — a state only reachable after the operator processed. This one uses force_chk_reconcile(chk, "force", "InProgress"), and since the CHK is already stuck InProgress from the defer loop, both waits are satisfied instantly. It can go green while the operator is a second away from violating the invariant. A soak — poll startTime for ~60s — would fix that. It would otherwise catch the regression: pre-PR all three replicas roll, so the assertions do fail on old code.

The test_020005 xfail is commented out rather than deleted, in the first commit, with no root-cause note. Both endpoints of its 1→3→1 scale take paths the gate doesn't engage. I'd keep the xfail until it's green over several CI runs.

Two design points

The 2-minute wait doesn't earn its cost. ReconcileCHKsThreadsNumber defaults to 1, so the blocking loop holds the operator's only CHK goroutine — per host. A 3-node ensemble with two gated hosts blocks ~4 min, then requeues in 5s and repeats, head-of-line blocking every other CHK in the cluster. The 5s requeue already provides the retry; CHI just defers immediately. I'd delete the wait.

Consider splitting. The quorum gating (#2069) is the risky part. The status-error propagation, the settle-delay extraction, and especially newChkStatefulSetFallback returning ErrCRUDAbort are independently reviewable and shippable. That fallback flip changes CHK behaviour on every STS wait failure, including bootstrap — bundling it under a quorum PR hides it.

Also worth lifting noteCRUDResult, isHostHealthyForReconcile and the recovery-first ordering into pkg/controller/common rather than re-implementing the CHI versions.

Verdict

Request changes. Top three: the N=2 wedge, the wait that can't see peer recovery, and the members denominator on downscale. Happy to help with any of them — and I'd take the rebase onto current 0.27.4 first, since that's needed regardless and resolves the finalizeCR conflict.