Skip to content

Allowlist StorageKafka2 Broker transport failure upgrade-check shutdown noise#106516

Merged
alexey-milovidov merged 1 commit into
ClickHouse:masterfrom
groeneai:groeneai/upgrade-check-kafka2-broker-transport
Jun 6, 2026
Merged

Allowlist StorageKafka2 Broker transport failure upgrade-check shutdown noise#106516
alexey-milovidov merged 1 commit into
ClickHouse:masterfrom
groeneai:groeneai/upgrade-check-kafka2-broker-transport

Conversation

@groeneai

@groeneai groeneai commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

The Upgrade check (amd_release) post-restart <Error> scrub catches a chronic shutdown-time log line emitted by StorageKafka2's background KafkaConsumer2::getAllTopicPartitionOffsets poll loop when the Redpanda broker is in transition during the upgrade restart sequence:

<Error> StorageKafka2 (test_X.`03923_kafka2_keeper_offsets_test_X_kafka` (uuid)): Exception during get topic partitions from Kafka: Local: Broker transport failure

The line is produced by LOG_ERROR at KafkaConsumer2.cpp:317 from the cppkafka::HandleException catch branch; the only call site is the background poll path in KeeperHandlingConsumer.cpp:174. The existing [rdk:FAIL]-prefix allowlist filter only matches raw librdkafka tagged log lines and does not cover this wrapper-exception variant.

CIDB confirms this is widespread chronic environmental noise, not a code regression: 38 hits across 38 distinct PRs in the last 14 days (8 / 16 / 14 on 2026-06-02 / 06-03 / 06-04), 0 master hits, 100% on Upgrade check (amd_release), 100% involve 03923_kafka2_keeper_offsets. The pattern has been visible in CIDB since 2026-03-05 (the test was added on 2026-02-15). Reported on PR #99647 (recent spike since 2026-06-02).

The added regex requires BOTH the StorageKafka2 engine context AND the Broker transport failure symptom together, mirroring the narrowing approach used by the existing rdk:FAIL.*Connect to.*failed: Connection refused filter. Real StorageKafka2 regressions (authentication failures, timeouts, protocol errors, other broker errors raised on this same code path) still surface.

Related: #99647
Related: #101576

Changelog category (leave one):

  • CI Fix or Improvement (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

...

Documentation entry for user-facing changes

  • Documentation is unchanged.

@groeneai @claude
Allowlist StorageKafka2 Broker transport failure upgrade-check shut…
cdd64b4 
…down noise

The upgrade-check post-restart `<Error>` scrub catches a chronic shutdown-time
log line emitted by `StorageKafka2`'s background `KafkaConsumer2::getAllTopicPartitionOffsets`
poll loop when the Redpanda broker is in transition during the upgrade restart
sequence:

```
<Error> StorageKafka2 (test_X.`03923_kafka2_keeper_offsets_test_X_kafka` (uuid)): Exception during get topic partitions from Kafka: Local: Broker transport failure
```

The line is produced by `LOG_ERROR` at `KafkaConsumer2.cpp:317` from the
`cppkafka::HandleException` catch branch; the only call site is the background
poll path in `KeeperHandlingConsumer.cpp:174`. The existing `[rdk:FAIL]`-prefix
allowlist filter only matches raw librdkafka tagged log lines and does not cover
this wrapper-exception variant.

CIDB confirms this is widespread chronic noise, not a code regression: 38 hits
across 38 distinct PRs in the last 14 days (8 / 16 / 14 on 2026-06-02 / 06-03 / 06-04),
0 master, 100% on `Upgrade check (amd_release)`, 100% involve
`03923_kafka2_keeper_offsets`. The pattern has been visible since 2026-03-05
(test added 2026-02-15). Reported by @ alexey-milovidov on PR ClickHouse#99647 (recent
spike since 2026-06-02).

The added regex requires BOTH the `StorageKafka2` engine context AND the
`Broker transport failure` symptom together, mirroring the narrowing approach
used by the existing `rdk:FAIL.*Connect to.*failed: Connection refused` filter.
Real `StorageKafka2` regressions (authentication failures, timeouts, protocol
errors, other broker errors raised in this same code path) still surface.

Related: ClickHouse#99647
Related: ClickHouse#101576

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

groeneai commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

Pre-PR validation gate

# Question Answer
a Deterministic repro? Yes. The exact log line shape is fixed (sole producer is KafkaConsumer2.cpp:317 -> LOG_ERROR(log, "Exception during get topic partitions from Kafka: {}", e.what()) with cppkafka::HandleException e.what() = Local: Broker transport failure). The grep regex was sanity-checked against the actual log line from CIDB sample pull_request_number = 106505, commit 7c58770bd39d (filtered as expected) and against synthetic real-regression variants (timeout, auth-failure, other broker errors) which were correctly NOT filtered.
b Root cause explained? Yes. The KafkaConsumer2::getAllTopicPartitionOffsets background poll loop (called from KeeperHandlingConsumer::tryAcquireConsumer at KeeperHandlingConsumer.cpp:174 via the BgMBSchPool background pool) keeps polling Kafka metadata while the Redpanda broker is in transition during the upgrade-check restart sequence. cppkafka raises HandleException("Local: Broker transport failure"), the catch branch logs <Error>, and the line ends up in clickhouse-server.upgrade.log. The post-restart scrub in tests/docker_scripts/upgrade_runner.sh then surfaces it via Error message in clickhouse-server.log. The existing [rdk:FAIL]-prefix allowlist filter only matches raw librdkafka tagged log lines (different shape — they begin with [rdk:FAIL] [thrd:127.0.0.1:9092/...]: ...); this wrapper-exception variant has no [rdk:FAIL] prefix and was unfiltered.
c Fix matches root cause? Yes. Same pattern as the existing rdk:FAIL.*Connect to.*failed: Connection refused filter (also a known shutdown-time librdkafka noise filter): a narrow regex requiring BOTH the loading context (StorageKafka2) AND the symptom (Broker transport failure) together. Real StorageKafka2 regressions (timeouts, auth errors, protocol errors, other broker errors raised on the same code path) still surface because the regex does not match those shapes.
d Test intent preserved? / New tests added? N/A in the literal "regression test" sense — this is an upgrade-check error-allowlist tuning, the same class of change as Allow-list benign NuRaft shutdown rpc-header EOF in upgrade check (commit c3b940eec29), Allowlist 02555_davengers_rename_chain upgrade-check errors (commit 0ce4ea38e62), Allowlist Tuple element name 'null' is reserved (commit f62aefdf898), and Narrow rdk:FAIL filter (commit 51c835bd15c, my prior PR #101576). None of those followed up with a new test; the upgrade check itself is the relevant test surface, and the change reduces false positives without weakening real-regression detection (the regex is narrow — sibling StorageKafka2 errors with different symptoms still surface).
e Both directions demonstrated? Yes. Without filter: the actual CIDB log line (e.g. 2026.06.05 00:55:48.265039 [ 869353 ] {BgMBSchPool::deb758c7} <Error> StorageKafka2 (test_1.\03923_kafka2_keeper_offsets_test_1_kafka`): Exception during get topic partitions from Kafka: Local: Broker transport failure) is matched by the regex and would be filtered. With filter: synthetic regressions (StorageKafka2 ... Connection timeout while ..., StorageKafka2 ... Exception during get topic partitions from Kafka: Local: Authentication failed`) are NOT matched and would still surface. The CI run on this PR will validate the end-to-end upgrade-check behavior.
f Fix is general, not a narrow patch? Intentionally narrow, AND correctly so. The exact log shape is produced by exactly ONE source line (KafkaConsumer2.cpp:317); broader filtering would risk masking unrelated StorageKafka2 errors. Sibling code paths (KafkaConsumer1, the StorageKafka2 writer side, producer-side errors) emit DIFFERENT log shapes and are intentionally excluded — broker-transport-failure raised in those places would be a different signal that needs its own assessment, not auto-filtering. Per the @Algunenano "no speculative defensive changes" directive: this is NOT a defensive code change, it is a known-noise allowlist for an environment race we have CIDB-confirmed 38× over 14d (61× over 90d).

Session id: cron:clickhouse-ci-task-worker:20260605-001900

@groeneai groeneai mentioned this pull request Jun 5, 2026
Open
3 tasks
@groeneai

groeneai commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

cc @maxknv — could you review this? It adds a narrow allowlist entry in the upgrade-check log scrub for the chronic shutdown-time StorageKafka2 ... Local: Broker transport failure line emitted by the background KafkaConsumer2 poll loop while Redpanda is in transition during the upgrade restart sequence. Same shape as the existing rdk:FAIL.*Connect to.*failed: Connection refused filter; real StorageKafka2 regressions still surface.

@PedroTadim PedroTadim added the can be tested Allows running workflows for external contributors label Jun 5, 2026
@clickhouse-gh

clickhouse-gh Bot commented Jun 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [cdd64b4]

Summary: ✅

AI Review

Summary

This PR adds a targeted upgrade_runner.sh log allowlist for the StorageKafka2 metadata-poll Local: Broker transport failure line seen during upgrade-check broker transitions. I verified the filter shape against the changed script, the log producer in KafkaConsumer2::getAllTopicPartitionOffsets, the PR discussion, and the current PR diff; I did not find a code-review issue that warrants an inline comment.

Final Verdict

Status: ✅ Approve

@clickhouse-gh clickhouse-gh Bot added the pr-ci label Jun 5, 2026
@alexey-milovidov alexey-milovidov self-assigned this Jun 6, 2026
@alexey-milovidov
alexey-milovidov added this pull request to the merge queue Jun 6, 2026
Merged via the queue into ClickHouse:master with commit cfe1993 Jun 6, 2026
166 checks passed
@robot-ch-test-poll robot-ch-test-poll added the pr-synced-to-cloud The PR is synced to the cloud repo label Jun 6, 2026
This was referenced Jun 7, 2026
Merged
Merged
Open
@groeneai groeneai mentioned this pull request Jul 19, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexey-milovidov alexey-milovidov alexey-milovidov approved these changes

Assignees

@alexey-milovidov alexey-milovidov

Labels

can be tested Allows running workflows for external contributors pr-ci pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@groeneai @alexey-milovidov @PedroTadim @robot-ch-test-poll