Allowlist rdb_test_ DDLWorker UUID-collision upgrade-check restart noise#107126
Allowlist rdb_test_ DDLWorker UUID-collision upgrade-check restart noise#107126groeneai wants to merge 1 commit into
Conversation
Pre-PR validation gate
Session id: cron:clickhouse-worker-slot-24:20260611-012600 |
|
cc @alexey-milovidov — could you review this? Same class as #106616 and #106516: a targeted |
|
Workflow [PR], commit [b97afb4] Summary: ✅ AI ReviewSummaryThis PR adds a new Findings❌ Blockers
Final VerdictStatus:
|
CI summary, covered HEAD
|
9a74820 to
7fc5706
Compare
…noise The upgrade-check post-restart `<Error>` scrub catches a benign, self-healing error from the `--replicated-database` test wrapper: ``` <Error> DDLWorker(rdb_test_mur4ipro_2): Error on initialization of rdb_test_mur4ipro_2: Code: 57. DB::Exception: Mapping for table with UUID=69047a76-6be0-4eef-8696-576514388289 already exists. It happened due to UUID collision, most likely because some not random UUIDs were manually specified in CREATE queries. (TABLE_ALREADY_EXISTS), Stack trace ... ``` `clickhouse-test --replicated-database` creates each test's database as `ENGINE=Replicated(...)`, named `rdb_test_<rnd>_<shard>`. On the upgrade restart that database's DDLWorker runs `DatabaseReplicatedDDLWorker::initializeReplication` -> `recoverLostReplica`, which re-creates tables from the ZooKeeper metadata snapshot. If a stale local table still owns a table's UUID (a leftover `_tmp_replace_*` from `CREATE OR REPLACE`, or a table not yet finally dropped), `DatabaseCatalog::addUUIDMapping` reports the collision as a non-fatal `TABLE_ALREADY_EXISTS` (code 57), not a `LOGICAL_ERROR`. The DDLWorker main loop (`initializeMainThread`) catches it, logs this `<Error>` line, waits 5s and retries; recovery self-heals (after `max_retries_before_automatic_recovery` retries it forces a digest reset). The server stays up, so every other upgrade-check sub-test passes (including "Server successfully started"); only the post-restart `<Error>` scrub trips. This is chronic CI noise across many unrelated PRs, not a code regression. The two existing UUID-mapping masks do not cover this: one hardcodes a single UUID (useless, UUIDs are random), the other matches only `_repl_01111_` databases. The new regex requires ALL of `Error on initialization of` (logged at exactly one site, the DDLWorker recovery retry), the `rdb_test_` test-DB prefix, the UUID mapping message, AND the `TABLE_ALREADY_EXISTS` code together. A real `LOGICAL_ERROR` UUID-mapping crash, the same collision on a non-test database, a different init failure on an `rdb_test_` DB, and unrelated `TABLE_ALREADY_EXISTS` errors all still surface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
7fc5706 to
b97afb4
Compare
| -e "Exception during get topic partitions from Kafka: Local: Broker transport failure" \ | ||
| /test_output/clickhouse-server.upgrade.log \ | ||
| | grep -av -e "_repl_01111_.*Mapping for table with UUID" \ | ||
| | grep -av -e "Error on initialization of rdb_test_.*Mapping for table with UUID=.*already exists.*TABLE_ALREADY_EXISTS" \ |
There was a problem hiding this comment.
DatabaseReplicatedDDLWorker::initializeMainThread logs this exact Error on initialization of ... line for every failed recovery attempt and then just waits 5s and retries. In this job we only prove that the server process starts and answers a trivial query; we never touch the leftover rdb_test_* databases again, and clickhouse-test --upgrade-check runs with --fake-drop, so those replicated databases are intentionally left behind for the post-upgrade phase.
That means this new grep -av no longer only suppresses transient noise: it also hides a real recoverLostReplica regression if an rdb_test_* database gets stuck in the same UUID-collision retry loop after upgrade. As long as the server answers SELECT 1, the upgrade check will now pass with no remaining signal that replicated-database recovery is broken. We need an extra post-restart assertion that the rdb_test_* databases actually finish recovery before masking this line.
There was a problem hiding this comment.
Verified against source. Your two facts check out:
Error on initialization of {db}is emitted at one site,src/Databases/DatabaseReplicatedWorker.cpp:131, in thewhile (!stop_flag)catch that waits 5s and retries, sogrep -avdrops every occurrence, not just transient ones.- The
rdb_test_*DBs are intentionally left by--fake-drop(tests/clickhouse-test:6736) and hitinitializeReplication->recoverLostReplicaon restart.
The problem with a post-restart assertion is timing: upgrade_runner.sh stops the server at line 322 and only scrubs clickhouse-server.upgrade.log at line 454, so there is no live server left to query system.databases. The only remaining signal is log-string correlation (require Finishing replica initialization, line 264, for every errored rdb_test_ DB). That is racy: recovery retries on a 5s cadence and stop_server can land between an error and the next successful attempt on a fully benign run, which would fail the upgrade check for every PR. A new cross-PR flake in shared CI is worse than one masked benign line.
Residual risk acknowledged: after max_retries_before_automatic_recovery (default 10, DatabaseReplicatedSettings.cpp:23) the loop force-resets the digest and re-recovers from the ZK snapshot; a regression that still fails past that point would keep this line masked. I am weighing that against the flake cost above, and against the 4-token regex still surfacing a LOGICAL_ERROR UUID crash, the same collision on a non-rdb_test_ DB, a different init failure on an rdb_test_ DB, or an unrelated TABLE_ALREADY_EXISTS. This matches the merged precedent for benign scrub noise (#106616, #106516).
Keeping the line-level allowlist. If a maintainer prefers a stricter guard, a count threshold (fail if the collision repeats more than K times) is the least-fragile option, but the flake-vs-signal call is theirs.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
...
Description
Allowlists a benign, self-healing error from the
--replicated-databasetest wrapper that trips the upgrade-check post-restart<Error>scrub. Chronic CI noise onUpgrade check (amd_release), not a code regression.The failing line (repeated every ~5s as the DDLWorker retries):
clickhouse-test --replicated-databasenames each test DBrdb_test_<rnd>_<shard>withENGINE=Replicated(...). On the upgrade restart its DDLWorker runsDatabaseReplicatedDDLWorker::initializeReplication->recoverLostReplica, re-creating tables from the ZooKeeper metadata snapshot. If a stale local table still owns a table's UUID (a leftover_tmp_replace_*fromCREATE OR REPLACE, or a table not yet finally dropped),DatabaseCatalog::addUUIDMappingreports the collision as a non-fatalTABLE_ALREADY_EXISTS(code 57), not aLOGICAL_ERROR.initializeMainThreadcatches it, logs this line, waits 5s and retries; recovery self-heals (aftermax_retries_before_automatic_recoveryretries it forces a digest reset). The server stays up - every other upgrade-check sub-test passes, including "Server successfully started"; only the<Error>scrub trips.The two existing UUID-mapping masks do not cover this case: one hardcodes a single UUID (useless, UUIDs are random), the other matches only
_repl_01111_databases. The added regex requires ALL ofError on initialization of(logged at exactly one site, the DDLWorker recovery retry), therdb_test_test-DB prefix, the UUID mapping message, AND theTABLE_ALREADY_EXISTScode together. So a realLOGICAL_ERRORUUID-mapping crash, the same collision on a non-test database, a different init failure on anrdb_test_DB, and unrelatedTABLE_ALREADY_EXISTSerrors all still surface. This mirrors the narrowing approach of the existing multi-substring filters in the same scrub.Provenance:
Upgrade check (amd_release)on PR #106107, commit 942a72f, sub-test "Error message in clickhouse-server.log".Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=106107&sha=942a72f5e14987e3d86f7011d1b688ed4a11e9ea&name_0=PR&name_1=Upgrade%20check%20%28amd_release%29
Chronic across many unrelated PRs in the last 30 days (e.g. #101757, #103952, #101033, #106230).