Skip to content

fix(integration): skip or in cross-version query fuzz comparisons - #7805

Open
CharlieTLe wants to merge 1 commit into
cortexproject:masterfrom
CharlieTLe:flaky-expanded-postings-cache-fuzz
Open

fix(integration): skip or in cross-version query fuzz comparisons#7805
CharlieTLe wants to merge 1 commit into
cortexproject:masterfrom
CharlieTLe:flaky-expanded-postings-cache-fuzz

Conversation

@CharlieTLe

Copy link
Copy Markdown
Member

What this PR does

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 #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 when the fuzzer generates 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"}
)

cortex-1 returns execution: vector cannot contain metrics with the same labelset and cortex-2 returns no error. sameErrorClass (#7550) cannot reconcile that, because one side has no error at all. git tag --contains 52a8537e2a is empty, so master and 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 adds 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 implemented as an AST walk for parser.LOR rather than a strings.Contains on the rendered query, 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, …).

Verification

Compile check:

$ go vet -tags "integration,requires_docker,integration_query_fuzz" ./integration/...
(no output)

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:

$ CORTEX_IMAGE=<local build of master> FUZZ_SEED=1787335629 go test -v \
    -tags "integration,requires_docker,integration_query_fuzz" -timeout 2400s \
    -count=1 ./integration/ -run '^TestExpandedPostingsCacheFuzz$'

    query_fuzz_test.go:2270: integration fuzz random seed: overridden to 1787335629 via FUZZ_SEED
    query_fuzz_test.go:649: case 453 error mismatch.
        range query: -(
            label_replace(
              rate({__name__="test_series_6"}[4m]),
              "__promqlsmith_dst_label__",
              "$1",
              "__name__",
              "(.*)"
            )
          or
            {__name__="test_series_6",test_label="test_label_value_2"}
        )
        err1: execution: vector cannot contain metrics with the same labelset
        err2: <nil>
--- FAIL: TestExpandedPostingsCacheFuzz (9.65s)

With this change, the same seed passes, and so do four other seeds (so this is not a one-seed special case):

FUZZ_SEED=1787335629 --- PASS: TestExpandedPostingsCacheFuzz (22.54s)
FUZZ_SEED=1          --- PASS: TestExpandedPostingsCacheFuzz (19.55s)
FUZZ_SEED=424242     --- PASS: TestExpandedPostingsCacheFuzz (19.38s)
FUZZ_SEED=987654321  --- PASS: TestExpandedPostingsCacheFuzz (19.14s)
FUZZ_SEED=1787335630 --- PASS: TestExpandedPostingsCacheFuzz (18.81s)

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 — TestVerticalShardingFuzz returning the vector() fallback instead of the LHS of or when 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

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky test: TestExpandedPostingsCacheFuzz — Prometheus 3.9 merges same-labelset series after __name__ removal, latest release image errors

1 participant