fix(integration): skip or in cross-version query fuzz comparisons - #7805
Open
CharlieTLe wants to merge 1 commit into
Open
fix(integration): skip or in cross-version query fuzz comparisons#7805CharlieTLe wants to merge 1 commit into
or in cross-version query fuzz comparisons#7805CharlieTLe wants to merge 1 commit into
Conversation
TestExpandedPostingsCacheFuzz compares the latest released Cortex image (cortex-1, resolved from the VERSION file, currently v1.21.1 -> Prometheus 3.8.1) against the current build (cortex-2 -> Prometheus 3.9.1 since cortexproject#7535). The two embedded Prometheus versions disagree about what to do when a query result contains series that become identical after __name__ removal. Prometheus <= 3.8 failed the whole query in cleanupMetricLabels: if mat.ContainsSameLabelset() { ev.errorf("vector cannot contain metrics with the same labelset") } Prometheus 3.9 replaced that with mergeSeriesWithSameLabelset, which merges the colliding series when their timestamps do not overlap and only errors when they do. Its doc comment names the exact shape: "operations like OR combine series that originally had different names but end up with the same labelset after dropping the name". So the fuzzer generating something like -( label_replace(rate({__name__="test_series_6"}[4m]), "__promqlsmith_dst_label__", "$1", "__name__", "(.*)") or {__name__="test_series_6",test_label="test_label_value_2"} ) gets `execution: vector cannot contain metrics with the same labelset` from cortex-1 and no error at all from cortex-2, which the test reports as an error mismatch. sameErrorClass (cortexproject#7550) cannot reconcile it because one side has no error. No release tag contains cortexproject#7535 yet, so master and the newest released image are guaranteed to disagree here until the next release. The divergence follows the image, not the expanded-postings-cache flag: running the failing query against the release image *with* the cache enabled still errors, and against HEAD *without* the cache still succeeds. isValidQuery(expr, skipBackwardIncompat=true) already drops queries whose semantics changed across the embedded Prometheus versions (stddev, stdvar, quantile, predict_linear, atan2). Add `or` to that set, since `or` is the only operator that can union series carrying different __name__s into one result - the precondition for the collision. `and` and `unless` only ever return series from their left hand side and are left alone. Whether a given `or` actually collides can only be known by evaluating it, so the filter is syntactic; it is an AST walk for parser.LOR rather than a strings.Contains, so a label *value* containing "or" cannot accidentally drop a query. skipBackwardIncompat=true is only passed by the cross-version tests, so `or` remains fully covered by the fuzz tests that compare two instances of the same build (TestVerticalShardingFuzz, TestProtobufCodecFuzz, TestParquetFuzz, ...). Verified against the seed from the failing CI run, plus four other seeds: CORTEX_IMAGE=<local build> FUZZ_SEED=1787335629 go test -v \ -tags "integration,requires_docker,integration_query_fuzz" \ -timeout 2400s -count=1 ./integration/ \ -run '^TestExpandedPostingsCacheFuzz$' reproduces `case 453 error mismatch` before the change and passes after it. Fixes cortexproject#7803 Signed-off-by: Charlie Le <charlie_le@apple.com>
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.
What this PR does
TestExpandedPostingsCacheFuzzcompares the latest released Cortex image (cortex-1, resolved from theVERSIONfile — currentlyv1.21.1, Prometheus 3.8.1) against the current build (cortex-2, Prometheus 3.9.1 since #7535). The two embedded Prometheus versions disagree about what to do when a query result contains series that become identical after__name__removal.Prometheus ≤ 3.8 failed the whole query in
cleanupMetricLabels:Prometheus 3.9 replaced that with
mergeSeriesWithSameLabelset, which merges the colliding series when their timestamps do not overlap and only errors when they do. Its doc comment names the exact shape: "operations like OR combine series that originally had different names but end up with the same labelset after dropping the name."So when the fuzzer generates something like
cortex-1returnsexecution: vector cannot contain metrics with the same labelsetandcortex-2returns no error.sameErrorClass(#7550) cannot reconcile that, because one side has no error at all.git tag --contains 52a8537e2ais empty, somasterand the newest released image are guaranteed to disagree on this shape until the next release.The expanded postings cache is not the variable. The test flips both the image and the cache flag at once, so I crossed them over: the release image with the cache enabled still errors, and HEAD without the cache still succeeds. Details in #7803.
isValidQuery(expr, skipBackwardIncompat=true)already drops queries whose semantics changed across the embedded Prometheus versions (stddev,stdvar,quantile,predict_linear,atan2). This PR addsorto that set, sinceoris the only operator that can union series carrying different__name__s into one result — the precondition for the collision.andandunlessonly ever return series from their left-hand side and are left alone.Whether a given
oractually collides can only be known by evaluating it, so the filter is syntactic. It is implemented as an AST walk forparser.LORrather than astrings.Containson the rendered query, so a label value containingorcannot accidentally drop a query.skipBackwardIncompat=trueis only passed by the cross-version tests, soorremains fully covered by the fuzz tests that compare two instances of the same build (TestVerticalShardingFuzz,TestProtobufCodecFuzz,TestParquetFuzz, …).Verification
Compile check:
Reproduced the CI failure locally at the seed CI logged, against a locally built image (
linux/arm64, macOS/Docker Desktop) — same case index, same query, same errors as CI:With this change, the same seed passes, and so do four other seeds (so this is not a one-seed special case):
To avoid racing another build sharing this Docker daemon, the image was built under a private tag rather than via
make ./cmd/cortex/.uptodate(which retags:latest).Related
The other failure in the same CI job —
TestVerticalShardingFuzzreturning thevector()fallback instead of the LHS oforwhen vertical sharding is enabled — is a genuine product bug, not a test artifact. It is tracked in #7804 and fixed separately in the query-frontend sharding analyzer; deliberately not papered over by filtering the fuzz corpus.CHANGELOG
None: test-only change, matching a7e4c78 ("Fix flaky pkg/compactor tests", #7796).
Fixes #7803