[Receive] Fix race condition when adding multiple new tenants at once #7941

jnyi · 2024-11-25T18:07:45Z

Update: actually fix the issue instead of reverting the old one, memorize the TSDB client list is valuable to avoid creating thousands of slices in memory, see a6fbb9f

This reverted PR #7782 and fixed Issue #7892

Reproducible by newly added unit tests TestMultiTSDBAddNewTenant:

After this fix, unit test would pass

=== RUN   TestMultiTSDBAddNewTenant
=== PAUSE TestMultiTSDBAddNewTenant
=== CONT  TestMultiTSDBAddNewTenant
--- PASS: TestMultiTSDBAddNewTenant (13.82s)
=== RUN   TestMultiTSDBAddNewTenant/iteration-0
    --- PASS: TestMultiTSDBAddNewTenant/iteration-0 (1.39s)
=== RUN   TestMultiTSDBAddNewTenant/iteration-1
    --- PASS: TestMultiTSDBAddNewTenant/iteration-1 (1.36s)
=== RUN   TestMultiTSDBAddNewTenant/iteration-2
    --- PASS: TestMultiTSDBAddNewTenant/iteration-2 (1.39s)
=== RUN   TestMultiTSDBAddNewTenant/iteration-3
    --- PASS: TestMultiTSDBAddNewTenant/iteration-3 (1.40s)
=== RUN   TestMultiTSDBAddNewTenant/iteration-4
    --- PASS: TestMultiTSDBAddNewTenant/iteration-4 (1.40s)
=== RUN   TestMultiTSDBAddNewTenant/iteration-5
    --- PASS: TestMultiTSDBAddNewTenant/iteration-5 (1.36s)
=== RUN   TestMultiTSDBAddNewTenant/iteration-6
    --- PASS: TestMultiTSDBAddNewTenant/iteration-6 (1.37s)
=== RUN   TestMultiTSDBAddNewTenant/iteration-7
    --- PASS: TestMultiTSDBAddNewTenant/iteration-7 (1.38s)
=== RUN   TestMultiTSDBAddNewTenant/iteration-8
    --- PASS: TestMultiTSDBAddNewTenant/iteration-8 (1.41s)
=== RUN   TestMultiTSDBAddNewTenant/iteration-9
    --- PASS: TestMultiTSDBAddNewTenant/iteration-9 (1.36s)
PASS

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Verification

GiedriusS

So it's not a fix really but a revert. Thanks for the test which reproduces the bug, it will be much easier to fix. I think we should fix this instead of reverting because the cost of creating this slice is not trivial when you have thousands of selects per second.

jnyi · 2024-11-27T01:17:54Z

hi @GiedriusS

I got your point, this client list should be relatively stable most of the time (wasteful of memory to create slices), I spent some time to actually fix it, appreciate another review

pkg/receive/multitsdb.go

jnyi · 2024-11-27T04:00:05Z

I think the e2e test failure is tranisent, but i don't have permission to rerun

pkg/receive/multitsdb.go

Signed-off-by: Yi Jin <[email protected]>

pkg/receive/multitsdb.go

GiedriusS

🚀 thanks a lot for this and sorry for the problems

Signed-off-by: Yi Jin <[email protected]>

jnyi · 2024-12-02T22:06:15Z

🚀 thanks a lot for this and sorry for the problems

no worries, I've tried to fix the tests, might help merge it since all checks pass

GiedriusS

Please don't use force pushes in the future as it makes it hard to follow what changes between reviews, thanks!

…thanos-io#7941) * [Receive] fix race condition Signed-off-by: Yi Jin <[email protected]> * add a change log Signed-off-by: Yi Jin <[email protected]> * memorize tsdb local clients without race condition Signed-off-by: Yi Jin <[email protected]> * fix data race in testing with some concurrent safe helper functions Signed-off-by: Yi Jin <[email protected]> * address comments Signed-off-by: Yi Jin <[email protected]> --------- Signed-off-by: Yi Jin <[email protected]>

…thanos-io#7941) * [Receive] fix race condition Signed-off-by: Yi Jin <[email protected]> * add a change log Signed-off-by: Yi Jin <[email protected]> * memorize tsdb local clients without race condition Signed-off-by: Yi Jin <[email protected]> * fix data race in testing with some concurrent safe helper functions Signed-off-by: Yi Jin <[email protected]> * address comments Signed-off-by: Yi Jin <[email protected]> --------- Signed-off-by: Yi Jin <[email protected]> Signed-off-by: Saswata Mukherjee <[email protected]>

* Merge pull request #7674 from didukh86/query_frontend_tls_redis_fix Query-frontend: Fix connection to Redis cluster with TLS. Signed-off-by: Saswata Mukherjee <[email protected]> * Capnp: Use segment from existing message (#7945) * Capnp: Use segment from existing message Signed-off-by: Filip Petkovski <[email protected]> * Downgrade capnproto Signed-off-by: Filip Petkovski <[email protected]> --------- Signed-off-by: Filip Petkovski <[email protected]> Signed-off-by: Saswata Mukherjee <[email protected]> * [Receive] Fix race condition when adding multiple new tenants at once (#7941) * [Receive] fix race condition Signed-off-by: Yi Jin <[email protected]> * add a change log Signed-off-by: Yi Jin <[email protected]> * memorize tsdb local clients without race condition Signed-off-by: Yi Jin <[email protected]> * fix data race in testing with some concurrent safe helper functions Signed-off-by: Yi Jin <[email protected]> * address comments Signed-off-by: Yi Jin <[email protected]> --------- Signed-off-by: Yi Jin <[email protected]> Signed-off-by: Saswata Mukherjee <[email protected]> * Cut patch release v0.37.1 Signed-off-by: Saswata Mukherjee <[email protected]> * Update promql-engine for subquery fix (#7953) Signed-off-by: Saswata Mukherjee <[email protected]> * Sidecar: Ensure limit param is positive for compatibility with older Prometheus (#7954) Signed-off-by: Saswata Mukherjee <[email protected]> * Update changelog Signed-off-by: Saswata Mukherjee <[email protected]> * Fix changelog Signed-off-by: Saswata Mukherjee <[email protected]> --------- Signed-off-by: Saswata Mukherjee <[email protected]> Signed-off-by: Filip Petkovski <[email protected]> Signed-off-by: Yi Jin <[email protected]> Co-authored-by: Filip Petkovski <[email protected]> Co-authored-by: Yi Jin <[email protected]>

…thanos-io#7941) * [Receive] fix race condition Signed-off-by: Yi Jin <[email protected]> * add a change log Signed-off-by: Yi Jin <[email protected]> * memorize tsdb local clients without race condition Signed-off-by: Yi Jin <[email protected]> * fix data race in testing with some concurrent safe helper functions Signed-off-by: Yi Jin <[email protected]> * address comments Signed-off-by: Yi Jin <[email protected]> --------- Signed-off-by: Yi Jin <[email protected]>

pull-request-size bot added the size/L label Nov 25, 2024

jnyi mentioned this pull request Nov 25, 2024

receive: memoize exemplar/TSDB clients #7782

Merged

jnyi changed the title ~~[Receive] Fix issue-7892 race condition~~ [Receive] Fix race condition when adding multiple new tenants at once Nov 25, 2024

This was referenced Nov 25, 2024

[Receive] New Tenant is not queryable by receiver #7892

Open

[ES-1314123] race condition for memorize tsdb client databricks/thanos#108

Merged

GiedriusS reviewed Nov 26, 2024

View reviewed changes

jnyi requested a review from GiedriusS November 27, 2024 01:18

jnyi force-pushed the issue-7892 branch from a6fbb9f to 06e6aa5 Compare November 27, 2024 01:22

jnyi commented Nov 27, 2024

View reviewed changes

pkg/receive/multitsdb.go Outdated Show resolved Hide resolved

jnyi force-pushed the issue-7892 branch from 1050a23 to af00aae Compare November 27, 2024 03:23

pedro-stanaka approved these changes Nov 27, 2024

View reviewed changes

GiedriusS reviewed Nov 27, 2024

View reviewed changes

pkg/receive/multitsdb.go Show resolved Hide resolved

jnyi requested a review from GiedriusS November 27, 2024 18:21

jnyi force-pushed the issue-7892 branch 2 times, most recently from 519b937 to ea3c2a0 Compare November 27, 2024 18:50

jnyi added 4 commits November 28, 2024 12:04

[Receive] fix race condition

ef01290

Signed-off-by: Yi Jin <[email protected]>

add a change log

8dbd7e4

Signed-off-by: Yi Jin <[email protected]>

memorize tsdb local clients without race condition

7193180

Signed-off-by: Yi Jin <[email protected]>

fix data race in testing with some concurrent safe helper functions

ca6c149

Signed-off-by: Yi Jin <[email protected]>

jnyi force-pushed the issue-7892 branch from ea3c2a0 to ca6c149 Compare November 28, 2024 20:05

GiedriusS reviewed Dec 2, 2024

View reviewed changes

pkg/receive/multitsdb.go Outdated Show resolved Hide resolved

GiedriusS reviewed Dec 2, 2024

View reviewed changes

pkg/receive/multitsdb.go Outdated Show resolved Hide resolved

jnyi requested a review from GiedriusS December 2, 2024 18:51

jnyi force-pushed the issue-7892 branch 4 times, most recently from 8582bfc to 982408e Compare December 2, 2024 20:36

GiedriusS previously approved these changes Dec 2, 2024

View reviewed changes

GiedriusS enabled auto-merge (squash) December 2, 2024 20:39

auto-merge was automatically disabled December 2, 2024 21:04
Head branch was pushed to by a user without write access

jnyi dismissed GiedriusS’s stale review via 28580fd December 2, 2024 21:04

jnyi force-pushed the issue-7892 branch 2 times, most recently from 28580fd to 83b09f5 Compare December 2, 2024 21:30

address comments

0b25115

Signed-off-by: Yi Jin <[email protected]>

jnyi force-pushed the issue-7892 branch from 83b09f5 to 0b25115 Compare December 2, 2024 21:49

jnyi requested a review from GiedriusS December 2, 2024 22:06

GiedriusS approved these changes Dec 3, 2024

View reviewed changes

GiedriusS merged commit 1ea4e69 into thanos-io:main Dec 3, 2024
22 checks passed

BrewTestBot mentioned this pull request Dec 4, 2024

thanos 0.37.1 Homebrew/homebrew-core#200044

Merged

jnyi mentioned this pull request Dec 13, 2024

cherry pick upstream fix to memorize tsdb client databricks/thanos#112

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Receive] Fix race condition when adding multiple new tenants at once #7941

[Receive] Fix race condition when adding multiple new tenants at once #7941

jnyi commented Nov 25, 2024 •

edited

Loading

GiedriusS left a comment

jnyi commented Nov 27, 2024 •

edited

Loading

jnyi commented Nov 27, 2024

GiedriusS left a comment

jnyi commented Dec 2, 2024

GiedriusS left a comment

[Receive] Fix race condition when adding multiple new tenants at once #7941

[Receive] Fix race condition when adding multiple new tenants at once #7941

Conversation

jnyi commented Nov 25, 2024 • edited Loading

Changes

Verification

GiedriusS left a comment

Choose a reason for hiding this comment

jnyi commented Nov 27, 2024 • edited Loading

jnyi commented Nov 27, 2024

GiedriusS left a comment

Choose a reason for hiding this comment

jnyi commented Dec 2, 2024

GiedriusS left a comment

Choose a reason for hiding this comment

jnyi commented Nov 25, 2024 •

edited

Loading

jnyi commented Nov 27, 2024 •

edited

Loading