Fix upgrade check to respect no-{sanitizer} tags#95652
Closed
nikitamikhaylov wants to merge 1 commit intomasterfrom
Closed
Fix upgrade check to respect no-{sanitizer} tags#95652nikitamikhaylov wants to merge 1 commit intomasterfrom
nikitamikhaylov wants to merge 1 commit intomasterfrom
Conversation
The upgrade check was failing with false positives when the new binary was built with sanitizers (e.g., MSAN) but the old version created tables with engines that are not supported in sanitizer builds (e.g., `DeltaLakeLocal`). The issue was that: 1. Tag filtering (`no-msan`, `no-asan`, etc.) was skipped during stress tests 2. Build flags were detected from the running (old) server, not the new binary This fix: - Adds `--extra-build-flags` argument to `clickhouse-test` to pass explicit sanitizer flags - Modifies tag filtering to apply during upgrade checks even though they are stress tests - Detects sanitizers from the new packages in `upgrade_runner.sh` before running tests on the old version - Adds `no-msan` tag to `03784_bad_base_backup.sh` which uses `DeltaLakeLocal` Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
|
Workflow [PR], commit [61b0e4f] Summary: ❌
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes the upgrade check to properly respect test skip tags (like no-msan, no-asan) when running with sanitizer builds. The upgrade check previously failed when old release versions created tables with engines not supported in sanitizer builds, causing false positive test failures.
Changes:
- Detects the sanitizer type from new ClickHouse packages before installing old ones
- Passes the detected sanitizer flags to clickhouse-test via a new
--extra-build-flagsparameter - Updates test filtering logic to apply no-{sanitizer} tags during upgrade checks
- Adds
no-msantag to a test that uses DeltaLake (unsupported in MSAN builds)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/docker_scripts/upgrade_runner.sh | Adds sanitizer detection logic by extracting and querying new packages, then passes detected sanitizer to test runner |
| tests/clickhouse-test | Adds --extra-build-flags argument and merges these flags into build_flags; updates test filtering to apply no-{sanitizer} tags during upgrade checks |
| tests/queries/0_stateless/03784_bad_base_backup.sh | Adds no-msan tag with comment explaining DeltaLakeLocal is unsupported in MSAN builds |
azat
reviewed
Feb 1, 2026
| # Detect sanitizer from new packages before installing old ones. | ||
| # This is needed to skip tests with no-{sanitizer} tags during upgrade check, | ||
| # because tests may create tables with engines that are not supported in sanitizer builds. | ||
| echo "Detecting sanitizer from new packages..." |
Member
There was a problem hiding this comment.
This is likely not fast, we can detect it from the check name
| @@ -1,4 +1,6 @@ | |||
| #!/usr/bin/env bash | |||
| # Tags: no-msan | |||
| # because tests may create tables with engines that are not supported in sanitizer builds. | ||
| echo "Detecting sanitizer from new packages..." | ||
| NEW_BINARY_SANITIZER="" | ||
| dpkg -x $PACKAGES_DIR/clickhouse-common-static_*.deb /tmp/new_clickhouse_extract |
Member
There was a problem hiding this comment.
Also there is a better way to extract one binary, I remember I had this oneliner - ar p *.deb data.tar.gz | tar -xzf- ./usr/bin/clickhouse, and you can even strip this prefix
Member
|
This seems like a real upgrade issue, but I can't reproduce it locally yet. I don't understand why sanitizers are related since the test don't have such tag. We should use a debug build for this check instead if it's actually related. The goal of upgrade check is to find upgrade bugs, not memory errors |
Member
|
BTW I have a general question, why do we even need upgrade check with MSan, we have stress tests for this. Let's simply leave only upgrade check with release? |
Member
|
Something like this #95884 |
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
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.
Upgrade check failure example: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=95611&sha=254a0211695d3925b5c924650b098045831546e9&name_0=PR&name_1=Upgrade%20check%20%28amd_msan%29&name_1=Upgrade%20check%20%28amd_msan%29
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Respect the skip tags (
no-msan,no-asan) for the Upgrade check with a respective sanitizer. Otherwise, it leads to a false positive failure when the previous version (release build) creates a table that is simply not supported.Documentation entry for user-facing changes