fix: don't drop a non-empty Atomic default database during Replicated conversion#255
Merged
GrigoryPervakov merged 2 commits intoJun 30, 2026
Merged
Conversation
…cated conversion
ensureReplicaDefaultDatabaseEngine converts the `default` database from
Atomic to Replicated when enableDatabaseSync is on. When `default` already
existed as a non-Replicated engine *and contained tables*, the count > 0
branch logged "data loss is possible" but then fell through and ran
`DROP DATABASE default SYNC` anyway, destroying those tables before
recreating `default` as Replicated.
This races any client that seeds `default` while it is still Atomic on a
fresh cluster (e.g. an OTel collector that auto-creates tables on first
export): if the seed lands before the operator's first reconcileDatabaseSync
pass, the operator drops the seeded schema and it never comes back.
Make the behavior match the documented intent ("never drops tables or
recreates the default database as Replicated if any tables have been
created"): when `default` is non-empty and not Replicated, leave it as-is
and skip the conversion. The engine is still switched automatically on an
empty `default`, so fresh clusters keep getting the Replicated engine.
Skip (return nil) rather than error so an otherwise-healthy cluster running
on an Atomic `default` is not held with SchemaInSync=False forever.
Add an integration test asserting a seeded, non-empty Atomic `default`
survives EnsureDefaultDatabaseEngine with its engine and rows intact.
GrigoryPervakov
changed the title
wrn14897
marked this pull request as ready for review
June 30, 2026 17:54
… default Per review: rather than silently skipping (return nil), return an error when `default` is non-empty and not Replicated so the operator sets the SchemaInSync condition to false and surfaces the unconvertible database instead of hiding it. The database is still left intact (never dropped). Update the integration test to assert EnsureDefaultDatabaseEngine reports failure while the seeded table and Atomic engine are preserved.
GrigoryPervakov
approved these changes
Jun 30, 2026
GrigoryPervakov
enabled auto-merge (squash)
June 30, 2026 18:28
GrigoryPervakov
deleted the
warren/fix-default-db-drop-on-nonempty-atomic
branch
June 30, 2026 19:01
wrn14897
added a commit
to ClickHouse/ClickStack-helm-charts
that referenced
this pull request
Jul 20, 2026
## Summary Bumps `clickhouse-operator-helm` from v0.0.2 to **v0.0.7** and fixes a resulting full-stack integration-test failure caused by a new operator resource default that OOMKilled the single-replica ClickHouse deployment. Supersedes the upstream bump in [#224](#224). ## Changes ### Operator bump - `clickhouse-operator-helm` `~0.0.2` → `~0.0.7` (`Chart.yaml`, `Chart.lock`) - `clickstack-operators` chart `1.0.0` → **`1.1.0`** — required so chart-releaser (`skip_existing: true`) actually publishes the updated dependency; minor (not patch) because operator behavior changed and values keys were renamed since 1.0.0. - `values.yaml`: rename `enable` → `enabled` for `webhook` / `certManager` / `crd` — **required**, since v0.0.6+ renamed these keys. The old `enable` keys are silently ignored and would leave `webhook`/`certManager` enabled, breaking the install (no cert-manager present). - `integration-tests/full-stack/assert.sh`: scope `kubectl wait` with `--field-selector=status.phase!=Succeeded`. **Required by the bump:** v0.0.4+ introduces a version-probe Job (absent in v0.0.2) that leaves a `Completed` pod, which `kubectl wait --for=condition=Ready pods --all` can never satisfy → wait times out → suite fails. ### ClickHouse hardening (new) **Explicit container resources (`containerTemplate.resources`: 2Gi memory, 500m CPU request)** The chart previously set no `resources`, relying on the operator default. v0.0.6 changed that default (operator [PR #206](ClickHouse/clickhouse-operator#206): *"use the same req/limit for memory in default"*) to **512Mi, request == limit**. At 512Mi, ClickHouse computes `max_server_memory_usage` ≈ 460 MiB; the full ClickStack schema plus ingestion and background merges exceeds it → **OOMKilled (exit 137)**, crash-loop. The chart now sets explicit resources, overridable per environment via `clickhouse.cluster.spec`. ### Why v0.0.7 (and not v0.0.6) v0.0.6 defaults `enableDatabaseSync: true`, converting the `default` database to the **Replicated (DatabaseReplicated)** engine. In this single-replica deployment, a transient Keeper exception during ClickHouse startup (`Cannot use any of provided ZooKeeper nodes`, exit 231) desynced the Replicated database and **silently dropped every seeded table** (`otel_traces`, `otel_logs`, …) — the collector then loops on `Table default.otel_traces does not exist` and the smoke test times out. This branch initially carried an `enableDatabaseSync: false` workaround for that. Operator **v0.0.7** fixes the root cause upstream ([clickhouse-operator#255](ClickHouse/clickhouse-operator#255): *don't drop a non-empty Atomic default database during Replicated conversion*), so the workaround was reverted and we ship v0.0.7 with operator defaults. The upstream chart values are byte-identical between 0.0.6 and 0.0.7, so no further values changes were needed. ## Verification - **Resources (local kind, fresh full-stack cluster):** repro'd ClickHouse OOMKill at 512Mi; after fix CH sees 2.0 GiB, `max_server_memory_usage` 1.80 GiB, stable. - **databaseSync (on v0.0.6):** repro'd — `default` engine `Replicated`, all tables dropped after a startup Keeper hiccup. v0.0.7 carries the upstream fix; CI full-stack suite exercises it end-to-end. - `helm unittest charts/clickstack`: **179/179 pass**. - Both charts and both example values files (`alb-ingress`, `api-only`) render cleanly. ## Note 2Gi raises the chart's baseline ClickHouse memory footprint vs. the operator's 512Mi default. Overridable per environment via `clickhouse.cluster.spec`. ## Ref #224 #226 https://github.com/ClickHouse/clickhouse-operator/releases/tag/v0.0.7 --------- Co-authored-by: Pervakov Grigorii <pervakov.grigory@gmail.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
2 participants
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.
Summary
ensureReplicaDefaultDatabaseEngineconverts thedefaultdatabase from Atomic to Replicated whenenableDatabaseSyncis enabled. Whendefaultalready existed with a non-Replicated engine and contained tables, thecount > 0branch logged"data loss is possible"but then fell through and ranDROP DATABASE default SYNCanyway, destroying those tables before recreatingdefaultas Replicated.This makes the code match its documented intent — "It never drops tables or recreates the default database as Replicated if any tables have been created" — by refusing to convert when
defaultis non-empty and not Replicated, and returning an error so the operator reports the problem.The race this fixes
On a fresh cluster,
defaultstarts as Atomic and empty. The operator only converts it to Replicated on a reconcile pass after the replica isReady(reconcileDatabaseSyncoperates onreadyReplicas). Any client that connects and seedsdefaultin that window — e.g. an OTel collector that auto-creates tables on first export — creates tables in the still-Atomicdefault. If that seed lands before the operator's first conversion pass, the operator finds a non-empty Atomicdefault, warns, and drops it. The seeded schema never comes back; downstream consumers loop onTable default.<x> does not exist.This was hit in the ClickStack helm-charts on the
clickhouse-operator-helmv0.0.6 bump (single-replica deployment): allotel_*tables were dropped on first boot. Ref: ClickHouse/ClickStack-helm-charts#240.Behavior change
defaultstateReplicatedSchemaInSync=false)Fresh/empty clusters still get the Replicated engine automatically; only a populated
defaultis now preserved.Why return an error (not skip)
When
defaultis non-empty and not Replicated, the operator cannot perform the requested conversion without losing data. Returning an error propagates throughEnsureDefaultDatabaseEngineso the operator setsClickHouseConditionTypeSchemaInSync = false, surfacing the unconvertible database rather than silently leaving the cluster in a state that does not match the requested topology. The database itself is never dropped. (Per review feedback.)Test
Added an integration spec (
internal/controller/clickhouse/commands_test.go): seed a table with rows into the Atomicdefault, runEnsureDefaultDatabaseEngine, and assert it reports failure (false) while the engine is stillAtomicand the rows survived. The existing Atomic→Replicated conversion spec (emptydefault) continues to pass.Full
commandersuite passes locally (testcontainers).Notes
enableDatabaseSyncsemantics are unchanged.