Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: expanded postings can cache wrong data when queries are issued "in the future" #6562

Merged
merged 7 commits into from
Jan 29, 2025

Conversation

alanprot
Copy link
Member

@alanprot alanprot commented Jan 28, 2025

What this PR does:

See: #6556

This PR includes:

  • Added unit test to catch the issue
  • Improve fuzz test to add GetSeries call and run queries at a "future" timestamp
  • Increase the runs from 100 to 300 on the fuzz test (even though 100 was pretty consistent to catch the issue)
  • Bypass caching logic if the query is "in the future"

Additionally, we improved the prom to include all the matchers types: cortexproject/promqlsmith#142

Which issue(s) this PR fixes:
Fixes #6556

Checklist

  • Tests updated
  • [NA] Documentation added
  • [NA] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@alanprot alanprot force-pushed the fix/6556 branch 2 times, most recently from 5bc90ec to dba615b Compare January 28, 2025 19:16
@alanprot alanprot changed the title improve fuzz test for expanded postings cache Fix #6556 Jan 28, 2025
…ter the last sample added in the head

Signed-off-by: alanprot <[email protected]>
Signed-off-by: alanprot <[email protected]>
@alanprot alanprot requested a review from yeya24 January 29, 2025 00:36
@alanprot alanprot marked this pull request as ready for review January 29, 2025 00:36
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improve fuzz test to add GetSeries call and run queries at a "future" timestamp

I don't see we added this to fuzz test?

pkg/storage/tsdb/expanded_postings_cache.go Show resolved Hide resolved
@alanprot
Copy link
Member Author

alanprot commented Jan 29, 2025

Improve fuzz test to add GetSeries call and run queries at a "future" timestamp

I don't see we added this to fuzz test?

My bad.. i added to the unit test.. lemme add some on the fuzz test as well.

We do not have expanded postings for get series for now but is good to add in case we add cache there in the future.

@alanprot
Copy link
Member Author

@yeya24 Added the get series fuzzy testing.

@alanprot alanprot changed the title Fix #6556 Fix: expanded postings can cache wrong data when queries are issued "in the future" Jan 29, 2025
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@alanprot alanprot merged commit 4b32f29 into cortexproject:master Jan 29, 2025
17 checks passed
@alanprot alanprot deleted the fix/6556 branch January 29, 2025 17:23
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.

Expanded Postings Cache can cache wrong results on some edge cases:
2 participants