Repository navigation
Fix CHI ZK root-path ensure logic - #2073
Conversation
|
Reviewed this against the reported symptom in depth. Most of the mechanics here are right — the ctx plumbing into 1. The 0.27.3 behaviour change is not in the ZK code
A cluster whose ZK is down has permanently NotReady pods, so Worth flagging because the gate in this PR treats a symptom of that at the ZK surface only — the K8s API and ClickHouse SQL calls in the same reconcile still fire every 60s. 2. BLOCKER — the gate can permanently prevent root-path creation
if ok, err := p.Connection.Exists(ctx, subPath); !ok {
if err != nil {
if ctx.Err() != nil { return ctx.Err() }
log.Warning("zk path ensure Exists failed for %s after retries", subPath)
} // <- falls through, and the function ends with `return nil`So for the common case — ensemble unreachable, reconcile not cancelled — the chain is:
Before this PR that self-healed on the next pass. Same gate, second scenario: wipe the ZK PVCs and rebuild the ensemble behind the same DNS name — the chroot is gone, the generation is unchanged, and the root path is never recreated. Two ways out: return a non-nil error from 3. The PR does not fix the permanent probing
The orphaned That is the 4. Log volume increases in exactly the reported scenarioThe three new Suggest 5. DNS now shares the 10s connect budget
6. Informational, not reachable today
Happy to help land whichever direction you prefer. Heads-up on sequencing: I have a branch addressing (3) plus the ctx plumbing and routing the library's dial logging through the operator logger, and it touches the same four files with incompatible signatures ( |
|
Correction to my earlier review — I got the severity of finding #2 wrong, and I want to fix that on the record before it costs you time. The gate is not a blockerI wrote that the gate could leave "replicated DDL failing and nothing ever retrying it," and called it a BLOCKER. That overstates it. The root-path precreation is a convenience, not a prerequisite — the operator already says so itself, in the branch immediately above the one I was reviewing: // When the resolved ZK ensemble is TLS-only, the operator's
// plaintext go-zookeeper dial cannot reach it. The bundled client has no
// TLS-aware path-ensure today, so skip the precreation step and let the
// ClickHouse server itself create the root path on its first DDL — that
// dial happens inside ClickHouse over its (TLS-aware) Keeper client.So the operator deliberately skips the ensure entirely for TLS-only ensembles and relies on ClickHouse to create the root. That is exactly the state the gate can leave a cluster in. The mechanism I described still holds — an ensure that fails against an unreachable ensemble returns nil, the reconcile completes, the ancestor generation advances, and the gate then skips the ensure for that generation. But the consequence is only that the operator stops precreating the path. ClickHouse creates it on first DDL, as it already does on the TLS path. Downgrading from BLOCKER to LOW/informational. I found this out the hard way: I tried making Sorry for the noise on that one. Findings #4 (per-attempt log volume) and #5 (DNS sharing the connect budget) stand as written, and #3 (the One thing worth acting on, in shared codeWhile reworking this I hit a latent bug in // vendor/github.com/go-zookeeper/zk/conn.go:1165-1173
if err == ErrConnectionClosed { return false, nil, err }
exists := true
if err == ErrNoNode { exists = false; err = nil }
return exists, &res.Stat, errSo Not introduced by you — it is the same shape at |
Summary
ctxinto ZK path ensure for interruptible retriestest_010010_1to check different scenarios.Test plan
go test ./pkg/model/zookeeper/ ./pkg/controller/chi/ -vet=off -count=1test_010010_1