add generated upgrade test harness - #477
Draft
aagbsn wants to merge 14 commits into
Draft
Conversation
aagbsn
marked this pull request as draft
August 11, 2026 05:16
Member
|
We should also read the changelog for any potential tricky breaking changes and ideally run the target version with the API + data pipeline to make sure that it's able to work without any problems. Scrolling through the changelog for "backward incompatible change" here are some highlights worth being careful of:
So we need to be extra careful once we go past 26.3 (maybe we should stop 1 LTS behind until we have done thorough testing). Especially tricky are ones where they say things like:
Which means that we need to make sure that we aren't using these functions, otherwise they will crash at runtime.
Might apply to us in the data pipeline It might also be worth passing the whole changelog history into an llm and give it our codebase to hunt for potential breakages. In any case I would suggest for sure upgrading to strictly less than |
Cross-checked sql/001_schema.sql against every ClickHouse fixture in
ooni/backend (ooniapi/services/{oonimeasurements,ooniprobe,oonirun,
testlists}) column by column.
- event_detector_changepoints was accidentally built from backend's
oonimeasurements column set (`*_current_state` enums) while credited
to devops's schema.sql, which defines a different, incompatible set
of columns (last_ts, *_obs_w_sum, *_w_sum, current_mean). Corrected
to match devops's schema.sql, since that's the source of truth for
what's actually deployed on oonidata_cluster.
- Added event_detector_cusums (present in devops's schema.sql, missed
in the original copy) and url_priorities (absent from devops's
schema.sql entirely, despite being referenced by the ansible
clickhouse_custom_grants for oonitestlists; ported from backend's
CollapsingMergeTree definition as ReplicatedCollapsingMergeTree).
- Documented, rather than silently picking a side on, four places
where devops and backend disagree with each other: fastpath column
types/indexes, analysis_web_measurement PARTITION BY/ORDER BY,
jsonl's extra date/source/update_time columns, and faulty_measurements'
async_insert settings. Followed devops in all four; see the header
comment in sql/001_schema.sql for specifics.
- Flagged eleven tables that exist in backend's legacy ooniprobe/
oonirun/testlists fixtures but nowhere in devops's schema.sql
(test_groups, accounts, session_expunge, counters_test_list,
counters_asn_test_list, msmt_feedback, fingerprints_dns,
fingerprints_http, asnmeta, incidents, oonirun) as an open question
rather than adding them speculatively.
- Fixed harness/scenarios.py's SQL statement loader: it split the
schema file on bare ";" characters, which broke once the new
documentation comments above used semicolons as normal punctuation.
Now strips full-line "--" comments before splitting.
Generated with Claude Sonnet 5 (Claude Code / Cowork).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…truth Add .github/workflows/clickhouse_upgrade_test.yml with two jobs: the recommended staged (LTS-hop) upgrade path and the naive direct-jump path (continue-on-error, since it's diagnostic, not a merge gate). Each node-upgrade and ON CLUSTER DDL check is its own workflow step with its own pass/fail checkmark, timing, and log, rather than one opaque job -- built on a new ci_step.py CLI (setup/upgrade-node/ verify-ddl/report/teardown) where each subcommand is a fresh process that recovers cluster state via `docker inspect` instead of requiring shared state between steps. Refactored harness/scenarios.py so the new granular step functions (setup_step, upgrade_node_step, verify_ddl_step, step_ok) are what both ci_step.py and the existing local-use scenario_staged_lts()/ scenario_direct_jump() call -- the CI path and `make test` can no longer silently diverge. Added harness/compose.py:current_env() and report.py:render_ci_steps_report() to support this. Added a docker-compose mem_limit (CI runners are more memory-constrained than a dev laptop) and a .gitignore for __pycache__/generated results/. Also fixed sql/001_schema.sql's obs_web table using a real `SHOW CREATE TABLE` dump run against production: added the missing probe_id column, all three minmax indexes, and the PARTITION BY clause derived from bucket_date. Confirmed fastpath already matched prod exactly. Generated with Claude Sonnet 5 (Claude Code / Cowork). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Cross-checked citizenlab, citizenlab_flip, jsonl,
analysis_web_measurement, event_detector_changepoints,
event_detector_cusums, and faulty_measurements against `SHOW CREATE
TABLE` run directly on production.
- jsonl, faulty_measurements: exact matches, no changes.
- citizenlab / citizenlab_flip: their ZK paths are swapped relative
to their table names (citizenlab's data lives under
.../citizenlab_flip/{shard} and vice versa -- a swap-pair pattern,
not a bug to tidy up). Also added citizenlab_flip, which had been
missing from this file entirely.
- analysis_web_measurement: added 4 missing columns
(top_dns_rule_id/top_tcp_rule_id/top_tls_rule_id/probe_id) and the
same 3-index minmax trio obs_web has.
- event_detector_changepoints and event_detector_cusums: rewritten
from scratch. devops' own schema.sql -- not just backend's copies
of it -- turned out to be stale/inaccurate for both: production
uses plain, non-replicated ReplacingMergeTree for both tables (no
ON CLUSTER coordination between replicas), no PARTITION BY on
either, and entirely different column sets than schema.sql
describes. Worth flagging to whoever maintains
scripts/cluster-migration/schema.sql separately from this test.
Every table in sql/001_schema.sql has now been checked against a live
SHOW CREATE TABLE, not just against devops/backend's copies of it.
Generated with Claude Sonnet 5 (Claude Code / Cowork).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
aagbsn
added a commit
that referenced
this pull request
Aug 17, 2026
Patch 1 — fix-false-positive-replication-errors.patch (touches harness/validate.py, harness/scenarios.py, harness/report.py): Fix false-positive replication-error detection that failed real CI run PR #477's staged-upgrade job failed at "Hop 1/4: upgrade ch2" (run 32041884883) with CANNOT_READ_ALL_DATA logged on ch1/ch3. Root cause, confirmed from the raw job log: forcibly recreating ch2's container (how this harness simulates an in-place upgrade) drops the other nodes' live connections to it, which ClickHouse logs as a NETWORK/CANNOT_READ_ALL_DATA error regardless of version -- a harmless, self-healing side effect of the container bounce, not a compatibility problem. The write-then-read-back probe and row-count convergence checks in that same step had already confirmed replication was fine. harness/validate.py now snapshots system.errors counters before each node bounce and diffs after, classifying new errors as "transient" (expected from any container recreate: NETWORK/CANNOT_READ_ALL_DATA/REPLICA-session/ etc, non-gating) vs "hard" (CHECKSUM/UNKNOWN_FORMAT/TOO_OLD/CORRUPTED -- only these fail a hop). This also fixes a second, related bug: the old `last_error_time > now() - INTERVAL 30 MINUTE` window re-flagged errors from earlier hops in every later step of the same CI job. Verified against the exact values from the failing run (see harness/scenarios.py's _hop_ok): the real hop1-ch2 step now evaluates to PASS. Also hardened harness/scenarios.py:upgrade_node_step() with a try/except (matching the existing pattern in setup_step()) so a docker/compose-level failure produces a clean, diagnosable failed-step result instead of an uncaught traceback -- a no-regrets fix, not the actual root cause here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Patch 2 — stop-before-26.3-and-audit-findings.patch (touches harness/versions.py, README.md): Recommend stopping upgrade at 25.8.29.51 for now; record audit findings Addresses the remaining points from hellais's review on PR #477: - harness/versions.py now exposes RECOMMENDED_NOW = "25.8.29.51" (last LTS strictly before 26.3) separately from LTS_HOPS, since 26.3 ships a backward-incompatible nested-data-type serialization change that can make downgrading lossy -- per the review comment. LTS_HOPS still walks the full ladder to 26.7.3.19 so this harness keeps validating that leg; it's just not yet the production recommendation. - README documents a point-by-point response to the review comment, including two new confirmed findings from grepping ooni/backend and ooni/data: searchAny/searchAll/hasAnyTokens/hasAllTokens are absent from both codebases (clean), and the TRUNCATE-on-replicated-table pattern flagged by the reviewer exists in *two* places doing the identical citizenlab_flip truncate+insert+exchange sequence (ooni/data's oonipipeline AND ooni/backend's analysis service -- not yet confirmed which is actually deployed), plus in two backend test fixtures (oonirun, ooniprobe) that truncate replicated tables in CI. - Still open and called out explicitly rather than silently dropped: full changelog sweep across every 24.9-26.7 release, running the target version against ooni/backend's API and oonipipeline's own test suite, and confirming which ClickHouse version introduced the truncate-replicated restriction. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Patch 1 — fix-false-positive-replication-errors.patch (touches harness/validate.py, harness/scenarios.py, harness/report.py): Fix false-positive replication-error detection that failed real CI run PR #477's staged-upgrade job failed at "Hop 1/4: upgrade ch2" (run 32041884883) with CANNOT_READ_ALL_DATA logged on ch1/ch3. Root cause, confirmed from the raw job log: forcibly recreating ch2's container (how this harness simulates an in-place upgrade) drops the other nodes' live connections to it, which ClickHouse logs as a NETWORK/CANNOT_READ_ALL_DATA error regardless of version -- a harmless, self-healing side effect of the container bounce, not a compatibility problem. The write-then-read-back probe and row-count convergence checks in that same step had already confirmed replication was fine. harness/validate.py now snapshots system.errors counters before each node bounce and diffs after, classifying new errors as "transient" (expected from any container recreate: NETWORK/CANNOT_READ_ALL_DATA/REPLICA-session/ etc, non-gating) vs "hard" (CHECKSUM/UNKNOWN_FORMAT/TOO_OLD/CORRUPTED -- only these fail a hop). This also fixes a second, related bug: the old `last_error_time > now() - INTERVAL 30 MINUTE` window re-flagged errors from earlier hops in every later step of the same CI job. Verified against the exact values from the failing run (see harness/scenarios.py's _hop_ok): the real hop1-ch2 step now evaluates to PASS. Also hardened harness/scenarios.py:upgrade_node_step() with a try/except (matching the existing pattern in setup_step()) so a docker/compose-level failure produces a clean, diagnosable failed-step result instead of an uncaught traceback -- a no-regrets fix, not the actual root cause here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addresses the remaining points from hellais's review on PR #477: - harness/versions.py now exposes RECOMMENDED_NOW = "25.8.29.51" (last LTS strictly before 26.3) separately from LTS_HOPS, since 26.3 ships a backward-incompatible nested-data-type serialization change that can make downgrading lossy -- per the review comment. LTS_HOPS still walks the full ladder to 26.7.3.19 so this harness keeps validating that leg; it's just not yet the production recommendation. - README documents a point-by-point response to the review comment, including two new confirmed findings from grepping ooni/backend and ooni/data: searchAny/searchAll/hasAnyTokens/hasAllTokens are absent from both codebases (clean), and the TRUNCATE-on-replicated-table pattern flagged by the reviewer exists in *two* places doing the identical citizenlab_flip truncate+insert+exchange sequence (ooni/data's oonipipeline AND ooni/backend's analysis service -- not yet confirmed which is actually deployed), plus in two backend test fixtures (oonirun, ooniprobe) that truncate replicated tables in CI. - Still open and called out explicitly rather than silently dropped: full changelog sweep across every 24.9-26.7 release, running the target version against ooni/backend's API and oonipipeline's own test suite, and confirming which ClickHouse version introduced the truncate-replicated restriction. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
aagbsn
force-pushed
the
add_437_clickhouse_upgrade_test
branch
from
August 17, 2026 16:10
4dd3e8c to
52df4e9
Compare
Run 32044578317 on PR #477 confirmed the false-positive fix works (the full 24.8.6.70 -> 25.3.14.14 hop passed clean, including further transient non-gating blips) and then hit a real, hard failure during 25.3.14.14 -> 25.8.29.51: once ch1+ch2 were on 25.8.29.51, a merge on either produced a part whose mark file ch3 (still on 25.3.14.14) could not parse at all (`Code: 79. Unknown mark file extension: '4'. INCORRECT_FILE_NAME`), permanently stuck its replication queue, and failed the write-then-read-back probe for the first time in the run. Tried extensively (8+ attempts across clickhouse.com's combined changelog pages, version anchors, GitHub's raw CHANGELOG.md, per-release GitHub pages, and the docs repo's raw markdown) to find the exact changelog entry for this and came up empty -- every source either 404s/robots-blocks or truncates to only the most recent 1-2 months regardless of prompting. Confirmed the exact intermediate version numbers via endoflife.date's API instead. harness/versions.py's LTS_HOPS now walks 25.4.13.22, 25.5.11.15, 25.6.13.41, and 25.7.8.71 between the two LTS releases (mirrored in .github/workflows/clickhouse_upgrade_test.yml as 8 hops instead of 4, kept in sync by the existing sanity-check assertion) so the next CI run localizes which specific monthly release introduces the incompatible mark format instead of only knowing it's somewhere in a 5-month span. RECOMMENDED_NOW is pulled back to 25.3.14.14 -- the only hop confirmed clean in real CI -- until that lands. README's TL;DR and PR #477 review-response sections updated to match; job timeout bumped 60->90min for the added steps. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ings Run 32047534149 (staged-upgrade, with the bisection hops from the previous patch) came back conclusive: 24.8.6.70 -> 25.3.14.14 -> 25.4.13.22 -> 25.5.11.15 -> 25.6.13.41 -> 25.7.8.71 all upgrade cleanly, node by node, zero hard errors -- only the expected transient bounce noise. The failure reappears exactly and only at 25.7.8.71 -> 25.8.29.51, same failure family as run 32044578317 but a different specific manifestation this time (Code: 226 NO_FILE_IN_DATA_PART, missing columns_substreams.txt, new .cmrk4 mark-file extension). This rules out a gradual drift across the 25.3-25.8 span: it's one version boundary, 25.8.29.51, changing the on-disk compact-part format in a way no earlier binary in this range can read. Ties this to a changelog note found earlier (v25.12: "Enable advanced shared data for JSON by default... after that change downgrade to versions before 25.8 will be not possible, because these versions won't be able to read new data parts with JSON column") -- scoped to JSON and to downgrading, but naming 25.8 as where the underlying substream-based part format landed. Our failing table (citizenlab) has no JSON column, so this is likely that same infrastructure applying more broadly than advertised. Flagged as corroborating, not confirmed -- still haven't gotten the actual changelog text to read directly despite repeated attempts. harness/versions.py's module docstring is rewritten to record this as a confirmed result rather than an open investigation. RECOMMENDED_NOW stays at 25.3.14.14 rather than bumping to 25.7.8.71: the newly-confirmed-clean releases are all non-LTS with ~1 month of support each, so "safe to pass through" isn't the same claim as "worth resting on". README's "Real CI findings", "What was and wasn't verified" sections updated with the run 3 results and both run URLs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… hop Adds continue-on-error to every upgrade-node/verify-ddl step so a hard failure on one node (e.g. the confirmed 25.7.8.71 -> 25.8.29.51 mark-file incompatibility) doesn't stop the remaining nodes in that hop from upgrading too. The job's pass/fail signal moves to `ci_step.py report`, which now returns 1 if any recorded step failed, since individual steps no longer do. This lets the next CI run reveal whether upgrading the lagging node to the same version also clears its stuck replication queue, or whether the divergence is permanent.
Run 32122682392 (continue-on-error added last patch) completed the full 8-hop staged-upgrade ladder: both the 25.8.29.51 mark-file incompatibility and the 26.3.17.110 nested-type serialization change (flagged in PR #477 review) turn out to be transient, self-healing mixed-version friction -- the lagging node's stuck replication queue clears as soon as its own upgrade finishes, in both cases. Neither is a structural block. Promotes RECOMMENDED_NOW to 26.7.3.19 and adds PRODUCTION_HOPS, a 4-hop runbook (skipping the monthly bisection releases, which existed only to localize the incompatibility in CI) with an operational caveat: upgrade all three nodes back-to-back for the two hops that hit a real incompatibility, rather than spacing them out like every other hop. Also fixes a reporting gap in validate.py: NO_FILE_IN_DATA_PART wasn't in HARD_ERROR_NAME_PATTERNS, so hop6-ch2's report said "no incompatibility errors" even though it failed from exactly one (caught only via the separate replication_queue_problems() check, not system.errors).
Extends the harness with a second, additional CI job that verifies the actual OONI data pipeline -- not just synthetic seed data -- survives the production upgrade path. Design per direct instruction: download real data exactly once per run, re-verify integrity + query correctness at each hop, and walk PRODUCTION_HOPS (the real 4-hop runbook) rather than every diagnostic bisection waypoint. - harness/real_data.py: new module. Snapshots real-data tables (row count + order-independent cityHash64 checksum) across all 3 nodes, loads real OONI measurements once via ooni/data#160's downloader/fastpath containers, takes a golden snapshot, then per hop: upgrades all 3 nodes (reusing scenarios.upgrade_node_step unmodified), diffs against the golden snapshot, and re-runs ooni/data's own pytest suite against the real api-oonimeasurements service. - docker-compose.real-data.yml: new overlay adapting ooni/data#160's tests/integration stack onto this project's existing 3-node replicated cluster instead of a single throwaway node. Pinned to the add_end_to_end_tests branch since #160 is still unmerged -- update the ref once it merges. - sql/001_schema.sql: adds fingerprints_dns, fingerprints_http, obs_web_ctrl, obs_http_middlebox, obs_openvpn -- the tables oonipipeline's observations workflow and fastpath actually write, with per-table provenance notes and manual signedness fixes where oonipipeline's own DDL generator is known-wrong (Int32/Int8 vs the already-verified obs_web table's UInt32/UInt8 convention). - harness/compose.py: multi-compose-file support (files= param throughout) plus inspect_exit_code()/run_oneoff() for polling one-shot containers and running a fresh `verify` pytest pass per hop. - harness/scenarios.py: extracted apply_schema() from load_schema_and_seed() so the real-data scenario can reuse the schema load without the synthetic seed rows; generalized step_ok() to cover every non-upgrade-node step shape via a single top-level "ok" fallback. - harness/report.py: render_ci_step() now handles the real-data scenario's step shapes (setup-real-data, load-real-data, golden-snapshot, verify-e2e, real-data-hop). - ci_step.py: 6 new subcommands (setup-real-data, load-real-data, golden-snapshot, verify-e2e, real-data-hop, teardown-real-data) wiring harness/real_data.py into discrete, individually pass/fail-able CI steps, matching the existing setup/upgrade-node/verify-ddl pattern. - .github/workflows/clickhouse_upgrade_test.yml: new real-data-upgrade job (checkout ooni/data at add_end_to_end_tests, sanity-check PRODUCTION_HOPS alignment, setup/load/snapshot/4-hop-loop/report/ teardown). Deliberately not run on pull_request -- depends on an unmerged external branch and real network downloads -- only on push to main and workflow_dispatch (scenario: real-data or all). - README.md: documents the new job's design, the load-once/verify-per-hop rationale, the external-branch caveat, and marks PR #477 review point 4 ("run the target version against the real API + data pipeline") as addressed. Verified offline (no Docker daemon available in this sandbox, consistent with every prior patch in this project): all Python modules compile, all YAML/compose files parse and interpolate, SQL is structurally sound (balanced parens/backticks, unique table names), and every new step shape round-trips correctly through step_ok()/render_ci_step(). Real validation deferred to an actual GitHub Actions run.
a820098 extracted apply_schema() out of load_schema_and_seed() so the real-data scenario could reuse the schema load without the synthetic seed rows. That extraction left load_schema_and_seed()'s seed-loading loop still calling entry.execute(...), but entry was only ever assigned inside apply_schema()'s own scope now -- broke setup for both staged-upgrade and direct-jump-upgrade (NameError: name 'entry' is not defined), confirmed in CI run 32132374150.
Wires harness/real_data.py's real-data scenario (and the ci_step.py subcommands added in a820098) into an actual CI job -- these existed but weren't invoked anywhere yet. Adds the real-data-upgrade job (checkout ooni/data, sanity-check PRODUCTION_HOPS, setup/load/ snapshot/4-hop-loop/report/teardown) plus the real-data/all workflow_dispatch scenario options. See a820098's commit message and README.md's "Real-data end-to-end scenario" section for the design.
…y.py) Existing scenarios only compare data-at-rest between checkpoints; this adds a continuous read/write canary that runs for the entire PRODUCTION_HOPS rollout and directly proves the three claims that matter for a live-cluster go/no-go: no full-cluster downtime, no blocked writes, and no lost or corrupted data, not just "data matched at the start and end."
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.
This implements a test harness to verify clickhouse upgrades for our database schema. #437