clickhouse 3.0.0 — deploy into an existing GVC - #509
Open
jacobecox wants to merge 4 commits into
Open
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The chart no longer declares `kind: gvc`. It deploys into the GVC you install into; `gvc.name` is gone and `gvc.locations` becomes the top-level `locations`. For clickhouse that list is the TOPOLOGY, not placement: its length selects the mode, a location's position in it is that shard's number and its `<macros><shard>` identity, and the first three entries are the Keeper Raft group. With the GVC no longer under the chart's control the list and reality can disagree in both directions, and in 2.8.0 both were silent. Three layers close that, copied from pgedge 2.0.0: - `defaultOptions.minScale/maxScale: 0` on both workloads, with `localOptions` supplying the real per-location counts. An undeclared GVC location gets `desiredScale: None` and starts nothing -- the phantom-shard case is closed by construction, not by detection. - `SHARD_INDEX` sentinel is now 0, and 0 is fatal. 2.8.0 let a location that never matched fall out of the loop at len(locations)+1: a shard number in no `<remote_servers>` entry, which joined Keeper, registered ReplicatedMergeTree paths under that phantom shard, and was invisible to every distributed query. The guard fires before config.xml is written and before the server launches. - A boot-time GVC read (`view` on the one install GVC, via a new `policy-gvc.yaml`) closes the other direction, which the platform does not validate at all. Hard-fails a FRESH data directory, warns an initialised one, and skips entirely if the read fails -- a control-plane hiccup must never stop a live cluster. Two clickhouse-specific fixes the audit demanded: - Keeper quorum is decided arithmetically before Keeper starts. Below a majority of the first-3 locations the container exits naming the count instead of forming an election that can never complete. `CURRENT_SERVER_ID == 0` is now fatal too; 2.8.0 wrote Raft server_id 0 and started anyway. - The `Keeper not ready yet, waiting...` loop was infinite. It is now bounded at 300s, skipped entirely once the database exists, and on expiry prints the configured Keeper members and the GVC's real locations before taking the server down with it. Other changes: - `internal_access` -> `internalAccess`, and `workloads` is finally wired into `inboundAllowWorkload`. It was hardcoded to `[]`, so choosing `type: workload-list` silently blocked all internal traffic. - Keeper `localOptions` render only for the first three locations. That removes the chart's only use of `localOptions[].suspend`, which permanently breaks a workload's inbound reachability from other locations. - `replicas: 0` was an undocumented special case that suspended a location while still consuming a shard index and a Raft slot. Refused at render instead. - New render-time validation: duplicate locations, empty list, and `clusterName` as a bare identifier (it is an XML element name AND unquoted `ON CLUSTER` DDL, so `my-cluster` produced DDL that does not parse). - Validators consolidated into one `clickhouse.validate`, invoked from both `identity.yaml` and `workload-server.yaml` -- Helm renders templates in reverse-lexical order, so wiring it only to identity.yaml let a malformed `locations` crash with a Go-template error before the friendly message fired. - Default `locations` is now a single location x 1 replica. A multi-location default is N simultaneous guesses about someone else's GVC, and every wrong guess is a silent no-op. - README gains Migrating from 2.x, a Prerequisites section covering the GVC requirement and its diagnostics, and a Connecting table using fully-qualified service DNS. The old example used the bare workload name, which is not reliable on this platform. Neither image ships curl, so the GVC read uses only the wget options GNU (server, ubuntu) and BusyBox (keeper, alpine) both accept. Verified byte-identical output in both images, with 403 and DNS-failure negative controls. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…0 minutes Found by the conventions review. The guard's own contract says "a control-plane hiccup must never be the reason a database refuses to start", and it did not hold: `-T 10` is a per-operation timeout, not a retry cap. GNU wget on the server image defaults to --tries=20 and retries connect and DNS failures, so a single call to an unreachable API can burn ~200s, and the three-attempt loop can reach ~600s -- past CLAUDE.md's five-minute readiness gate, in precisely the transient failure this code exists to tolerate. The spec deliberately excluded --tries because BusyBox wget (the keeper image) has no such option, so a portable bound has to come from outside wget. `timeout` is present in both images. Measured with controls, in the real images: server timeout 15 -> rc=124 at 15s (vs 33s for only THREE retries) keeper stops at its own -T 10 positive control: a fast connection-refused still returns immediately, so the bound does not penalise the healthy path Worst case is now 45s across the loop. Verified every rendered GVC read carries the prefix -- 2 occurrences in a 3-location render, 0 unbounded. The review's only warning; nothing here changes a path any functional row exercises, so the in-flight test round's completed rows stay valid. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same defect class etcd-ml hit in this batch, found independently. The internal firewall list governs intra-release traffic too -- Keeper's Raft on 9234, the servers' calls to Keeper on 9181, and server-to-server replication and distributed queries -- so a list naming only clients silently destroys the cluster. Measured by following the README: rbac_access_denied_matched_policy[none] ... "10.0.84.230:9234" Code: 279 ... Connection reset by peer ...while get-deployments reported every replica `ready=true restarts=0`. Control proving the mechanism: adding the keeper's own workload link restored `zk_followers 2 / zk_synced_followers 2`. Both workloads now carry the release's own server and keeper links. Both are included on both rather than the minimum each strictly needs: the over-grant is one release's workloads reaching each other inside one GVC, while the under-grant is a silently destroyed cluster, and getting the direction wrong is exactly how this shipped. Rendered across four controls -- unchanged `[]` on the same-gvc default; both own links plus the client in cluster mode on both workloads; no keeper link in single-node, where no Keeper exists; and no duplicates when the user names them too. Also gates the policy's reveal on the Keeper startup secret behind cluster mode. Single-node renders no Keeper, so 2.8.0 and the untouched 3.0.0 both granted reveal on a secret that is never created -- inert, but a dangling grant. Four README corrections the round measured: - the all-absent symptom is `desiredScale: 0`, not `None` (users grep for it) - a 3-shard rolling upgrade is ~83 s of total unavailability; a Keeper setting change ~60 s - renaming `clusterName` orphans existing Distributed tables (`Code: 701`) - with one replica per shard, losing a shard fails EVERY distributed query Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review diff:
git diff 559013c..HEAD -- clickhouse/versions/3.0.0/(
559013cis the verbatimcp -Rof 2.8.0; everything real is in the three commits after it.)Why a major version
The chart no longer creates a GVC — it deploys into the user's existing one. That is breaking, and the upgrade across the boundary is destructive: a chart that stops declaring a GVC makes
helm upgradeprune it, and deleting a GVC takes every workload, volumeset and identity inside.3.0.0 ships a render-time
failon the legacygvcvalues key. Tested against a live 2.8.0 release in its own chart-created GVC: three upgrade attempts, including one with the real shipped2.8.0/values.yaml, all refused at render (exit=1). Afterwards the GVC was stillversion=1, workloadv2, volumesetv6,ready: true, release still revision 1, stored specs identical — nothing was touched. Migration prose is in the README and the briefing.The three-layer defence, each with controls
minScale/maxScale: 0+localOptionsdesiredScale: 0,"deactivated because maxScale is set to 0"minScale/maxScale: 1applied and read back): both undeclared locations exit1, andGenerated ClickHouse configappears only from the declared location — so the exit precedes any config write or replicated-path registrationtargetKind: gvcpolicy)The read is also bounded against a slow failure, not a fast one: a black-holed
CPLN_ENDPOINTcosts ~39 s (1.1 s control vs 40 s),restarts=0, workload ready. An NXDOMAIN control would have returned instantly and proved nothing about the timeout.A pre-existing defect found while testing
internalAccess.type: workload-listsilently destroyed the cluster, reached by following the README. The list governs intra-release traffic too — Keeper's Raft on 9234, the servers' calls to Keeper on 9181, and server-to-server replication and distributed queries:...while
get-deploymentsreported every replicaready=true restarts=0. Both workloads now render this release's own server and keeper links ahead of the user's list, deduped.Re-tested with the exact command shape that destroyed it. Keeper leader with
zk_synced_followers 2; per-shard counts 1/100, 2/200, 3/300;SELECT count()returning 600, including when issued from the west shard;CREATE TABLE … ON CLUSTERstatus 0 on all three hosts.The enforcement control is the important half — an identical unlisted client exchanging real protocol bytes gets
Code: 210 … Connection reset by peer … :9000, while the listed client returns 600. So the fix widened the list by two entries rather than loosening the firewall. The split settled at 123 s.Both are included on both workloads rather than the minimum each strictly needs: the over-grant is one release's workloads reaching each other inside one GVC, while the under-grant is a silently destroyed cluster.
Present in 2.x and unrelated to the conversion.
Also
revealon the Keeper startup secret that is never created. Now gated; storedtargetLinksare exactly three, and the single-node arm still boots in 12 s and reads back its rows.desiredScale: 0(notNone); a 3-shard rolling upgrade is ~83 s of total unavailability and a Keeper setting change ~60 s; renamingclusterNameorphans existingDistributedtables (Code: 701); with one replica per shard, losing a shard fails every distributed query.Testing
45 PASS · 0 FAIL across two rounds. Functional equivalence proven in all three shapes — single-node (500 rows, objects in S3), single-shard 1×2 (bidirectional replication,
absolute_delay=0), multi-shard 3×1 (distributed query returning 600 from two different shards). Drift gate clean in every shape, 10/10 resourcesUnchangedwith byte-identical stored specs.Worth noting for future drift work: the API stores
inboundAllowWorkloadfully qualified and alphabetically re-sorted versus what the chart renders. It causes no churn here, and etcd's round found the same.Reported as untested rather than inferred: T31's Keeper "1 follower" sub-claim was never observable at 15-20 s sampling.
Briefing updated in the same PR.
🤖 Generated with Claude Code