-
[OPIK-6901] [BE] fix: harden traces cutover rollback tooling and runbook (#7645)
frostbyte_neo 发布于
2026-07-29 09:48:56 +00:00 - [OPIK-6901] [BE] fix: harden traces cutover rollback tooling and runbook
Follow-up to the merged buffered-cutover runbook (OPIK-6901), from rehearsing the whole
procedure — forward and every rollback stage — end-to-end on a local Opik and re-reviewing
the recovery machinery. Driver logic was sound; this fixes tooling/runbook gaps, one
recovery-path data-loss footgun, and a post-rollback Liquibase drift.Rollback safety:
- Park the rolled-back successor as
traces_post_rollback_backup(symmetric with
traces_pre_cutover_backup) instead of reusingtraces_local_v2.traces_local_v2had
double-duty as both the disposable pre-cutover shadow and the retained rollback backup —
topologically identical states — so a rolled-back estate was ambiguous and--stage A
(discard the shadow) could TRUNCATE the retained backup. The distinct_backupname makes
the estate self-describing and closes that footgun. Clean taxonomy:*_backup= retained
(finalize-only);traces_local_v2/traces_local= working/live successor. - Gate
--reverse-replay-onlyon the post-promote shape (traces = restored original with
traces_post_rollback_backuppresent), not just "non-Distributed". The guard-less reverse
replay pointed at the live successor (post-EXCHANGE, pre-rollback) would mask
deleted-then-recreated rows — silent data loss. - Correct the stage-A closing message (shadow discarded, nothing to finalize) — the shared
"parked backup / finalize" message applied only to stages B/C.
Finalize recycles the rollback backup into an empty traces_local_v2:
- A rollback + finalize used to DROP the parked successor, leaving no
traces_local_v2— but
migration 000101 (ALTERed by 000106/000107) created it, so Liquibase records it as present.
That drift blocked a retry (the runbook's "traces_local_v2 exists empty" prereq) and left
the DB inconsistent with the applied changesets. - The parked
traces_post_rollback_backupIS the 000101 object, renamed (a ReplicatedMergeTree
replica path is fixed at CREATE and survives renames). So finalize, in the rollback case, now
recycles it back into an emptytraces_local_v2(TRUNCATE + RENAME) instead of DROP: discards
the successor data but restores the exact 000101 shadow — schema, codecs and replica path —
so the estate matches the applied Liquibase state and a retry starts from a clean shadow.
TRUNCATE-then-RENAME (each atomic, ON CLUSTER; no single truncate-and-rename exists) so a
crash between them only ever leaves an empty backup that re-running finalize recovers. The
successful-cutover case still DROPstraces_pre_cutover_backup(unchanged). Retention is
unchanged: the backup is kept through the soak; the recycle only runs on finalize.sh --confirm.
Tooling / operability:
- Decouple
clickhouse-clientfrom any bundled ClickHouse container: it is an operator
prerequisite (native, or the official-image wrapperscripts/clickhouse-client-docker.sh)
that connects over the network via CLICKHOUSE_* env — the same mechanism in every
environment. The local-rehearsal README used a compose-containerdocker execthat neither
reached the driver scripts nor represents a real run. exchange_and_wrap.shdeferred-wrap hint and the runbook now show--confirm-daos-retargeted
(required for any wrap); the load-test EXCHANGE example shows the required gates.
Docs:
- Add a local "Rehearsing rollback" section; fix the stale
000005version-check comment
(last_updated_atis DateTime64(6) since migration 000019; the guard remains a backstop for
legacy pre-000019 parts).
Tests:
- Gate test updated (rollback reimplementation, post-rollback assertions, reset recovery), and
the stage-B test now re-runs the reverse replay after the rollback to pin its idempotence —
the contract--reverse-replay-onlyrests on (post-cutover delete stays masked, no live
survivor dropped).
Validated 10/10 on the gate test and end-to-end on local docker: the full forward procedure
plus every rollback stage (A discard-shadow, B swap-back, C promote-original), each followed by
finalize — the rollback case recycling to an emptytraces_local_v2whose SHOW CREATE is
byte-identical to the pristine 000101 table (incl. the replica path), a fresh
backfill+delta+verify then passing against it (retry-ready); the successful-cutover DROP path
still finalizing to ['traces']; and--reverse-replay-onlyallowed+idempotent post-rollback and
correctly rejected otherwise.- docs(cutover): clarify per-host vs cross-host ON CLUSTER atomicity in rollback/finalize
Baz flagged the stage-B rollback comment as claiming RENAME ... ON CLUSTER is globally
atomic. It doesn't (the "one atomic statement" point is per-host, contrasting the
two-statement EXCHANGE+RENAME window), but the wording read as an absolute cross-cluster
guarantee while the runbook is careful to say "gapless per node". Align the stage-B/C
rollback SQL comments and the finalize.sh recycle comment with the runbook: state the
per-host atomicity as the real guarantee, and name the cross-host behavior explicitly —
ON CLUSTER is eventually-consistent (DDL queue converges a lagging host, or the call fails
loudly), not globally atomic; the same accepted skew as any ON CLUSTER DDL and a non-issue
in the default single-node deployment. Comment-only; no behavior change.- docs(cutover): frame cross-replica ON CLUSTER skew for multi-replica production + maintenance note
The prior comment-precision pass described the cross-host ON CLUSTER case as "a non-issue in
the default single-node deployment", which downplays it: production is multi-replica. Correct
the framing in the stage-B/C rollback SQL, finalize.sh and the runbook, and add the operator
maintenance note.- Cross-replica: ON CLUSTER runs synchronously (the client blocks until every reachable replica
applies it, or throws naming a laggard that then converges via the DDL queue), so there is no
durable mixed topology — only a sub-second cross-replica skew as it propagates, the same
accepted skew as the wrap. Not globally atomic (no ClickHouse primitive for that); nil on a
single replica. - Rollback stages B/C rename the LIVE
traces, so reads hit the skew: rollback.sh now prints a
maintenance NOTE and the runbook says to run the rollback in a maintenance moment / with reads
quiesced on a multi-replica cluster. - finalize is EXEMPT: its TRUNCATE+RENAME touch only the parked backup / disposable shadow, never
the livetraces, so there is no live-read skew and no maintenance window is needed — stated
explicitly in finalize.sh and the runbook.
Comment/doc-only; no behavior change.
- fix(cutover): cluster-aware finalize detection + assert parked-backup contents in rollback tests
Addresses two later Baz review comments on PR #7645.
finalize.sh (severity high) — production is multi-replica, and finalize is the one
irreversible step, so resolve the parked-backup detection CLUSTER-WIDE via clusterAllReplicas
(mirroring exchange_and_wrap.sh's settle gate) instead of the connected node only: a name
present on some-but-not-all replicas means an ON CLUSTER DDL has not finished propagating, so
acting on one node's partial view could recycle/drop mid-transition — refuse loudly instead of
a silent "Nothing to finalize". Also guard the recycle destination: refuse (cluster-wide) if
traces_local_v2already exists before the TRUNCATE, since recycle renames the backup INTO
that name and RENAME will not overwrite — a stray shadow means a retry cutover started before
the rollback was finalized. Declined the broader schema/replica-path validation Baz suggested:
traces_post_rollback_backupis created only by the rollback RENAME (it IS the 000101 object
by construction), so validating engine/columns guards a manufactured state the code-style rule
says to avoid.TracesLocalV2CutoverTest (severity low) — the stage-B/C rollback tests asserted the parked
backup only EXISTS, so a rename that produced an empty or wrong table would pass while losing
the successor data the parking exists to preserve. Assert the backup actually holds the
successor data (survivors present, post-cutover delete stays masked) and carries the successor
(non-Nullable end_time) schema. 10/10 green.- docs(cutover): clarify verify.sh --from-week/--to-week are 0-based week offsets
Full forward E2E rehearsal finding: the runbook's post-EXCHANGE compare hint wrote
--to-week <last-full-week>, which reads like a date, but verify.sh takes a 0-based
integer week offset from the anchor Monday. Passing a date fails with "must be a
non-negative integer". Clarify both mentions (the sealed-weeks compare tip and the
feasibility bullet) so operators pass an integer offset (e.g. --to-week 3).Doc-only; found while validating the runbook against a live run.
- fix(cutover): make finalize.sh empty-row guard cluster-aware
Follow-up to the cluster-aware detection: the LIVE_ROWS/BACKUP_ROWS emptiness guard still
counted on the connected replica only, so the "refuse if live is empty but the backup holds
data" check reflected one node, not the cluster — inconsistent with the clusterAllReplicas
detection in the same script (Baz review). Add a max_rows() helper that takes the max row
count across replicas (clusterAllReplicas grouped by host; fail-loud on a down replica, like
classify) and use it for both counts.Behavior is unchanged on a single replica (max over one host = that host's count); on
multi-replica it now reflects the most-caught-up replica, so a lagging/empty connected node
can no longer skew the guard. The post-cutover DROP path's Distributedtracesalready
aggregates the cluster and the max-over-hosts still yields its true total. Guard still fails
safe either way. Validated live: max_rows returns the correct counts against the single-node
estate; bash -n clean.- docs(cutover): correct verify version-check + finalize crash-recovery comments
Two comment accuracy fixes from a colleague review (JetoPistola).
000005_verify_migration.sql — the version-collapse guard comment claimed it detects legacy
sub-microsecond version-selection on pre-000019 parts. It cannot: ClickHouse coerces
last_updated_at to its DECLARED DateTime64(6) at read time, so raw ns digits are unrecoverable
through the column and uniqExact(last_updated_at) == uniqExact(toDateTime64(last_updated_at,6))
always — collapse_keys is structurally 0 on a (6)-declared schema (empirically confirmed;
inert since #7572). Reword to say it is inert defense-in-depth that can only fire if the
column is ever re-declared at higher precision, and note that dropping the guard +
--allow-version-collapse as dead code is a reasonable follow-up (kept out of scope here).finalize.sh — the recycle comment said re-running finalize "recovers" a crash between the
ordered TRUNCATE and RENAME. That's only true single-host; under partial ON CLUSTER
propagation the re-run's cluster-wide classify sees a mixed state and refuses loudly (finish
the RENAME by hand), never silently corrupting. Reword to reflect both.Comment-only; no behavior change.
- refactor(cutover): remove the inert version-collapse guard from verify.sh
The version-collapse guard (version-check + version-check-drill blocks in 000005, the
--allow-version-collapse flag, and the exit-3 REVIEW-REQUIRED path in verify.sh) is dead
code: it reads last_updated_at through a DateTime64(6)-declared column, and ClickHouse
coerces to the declared type at read time, so uniqExact(last_updated_at) always equals
uniqExact(toDateTime64(last_updated_at, 6)) and collapse_keys is structurally 0 on the
post-000019 schema — the exit-3 path is unreachable. It has been inert since #7572.The team is accepting the (vanishingly small) risk of sub-microsecond version-selection
collision, and the product is standardizing on microsecond dates, so the guard has no future
either. Remove it rather than keep dead code with a misleading comment. The microsecond
fidelity fingerprint (count + normalized checksum, FINAL + mask-honored) is unchanged and
remains the verify gate; verify.sh now exits 0 (PASS) / 1 (fidelity mismatch) only.Validated live: backfill traces -> traces_local_v2 then verify PASSES all windows (exit 0)
with the simplified final line; the mismatch path still FAILS (exit 1); no version-collapse
output remains. bash -n clean; gate test unaffected (it does not reference the guard).
下载附件