receive: Fixed leak on receive and querier proxying Store API Series, which was leaking on errors. by bwplotka · Pull Request #2866 · thanos-io/thanos

bwplotka · 2020-07-08T20:16:45Z

TestTenantSeriesSetServert_NotLeakingIfNotExhausted was showing leaks:

    TestTenantSeriesSetServert_NotLeakingIfNotExhausted/cancelled,_not_exhausted_StoreSet: leaktest.go:132: leaktest: timed out checking goroutines
    TestTenantSeriesSetServert_NotLeakingIfNotExhausted/cancelled,_not_exhausted_StoreSet: leaktest.go:150: leaktest: leaked goroutine: goroutine 84 [chan send]:
        github.com/thanos-io/thanos/pkg/store.(*tenantSeriesSetServer).Send(0xc000708360, 0xc0003104c0, 0x0, 0x0)
        	/home/bwplotka/Repos/thanos/pkg/store/multitsdb.go:141 +0x13e
        github.com/thanos-io/thanos/pkg/store.(*mockedStoreServer).Series(0xc0004e6330, 0xc0007083c0, 0x20ac2c0, 0xc000708360, 0x5116a0, 0x0)
        	/home/bwplotka/Repos/thanos/pkg/store/multitsdb_test.go:173 +0x76
        github.com/thanos-io/thanos/pkg/store.(*tenantSeriesSetServer).Series.func1(0x2097760, 0xc00003c940)
        	/home/bwplotka/Repos/thanos/pkg/store/multitsdb.go:121 +0x56
        github.com/thanos-io/thanos/pkg/tracing.DoInSpan(0x2097760, 0xc00003c940, 0x1c8bace, 0x17, 0xc000173760, 0x0, 0x0, 0x0)
        	/home/bwplotka/Repos/thanos/pkg/tracing/tracing.go:72 +0xcc
        github.com/thanos-io/thanos/pkg/store.(*tenantSeriesSetServer).Series(0xc000708360, 0x20983e0, 0xc0004e6330, 0xc0007083c0)
        	/home/bwplotka/Repos/thanos/pkg/store/multitsdb.go:120 +0xfa
        github.com/thanos-io/thanos/pkg/store.TestTenantSeriesSetServert_NotLeakingIfNotExhausted.func2.1(0xc000708360, 0xc0004e6330)
        	/home/bwplotka/Repos/thanos/pkg/store/multitsdb_test.go:225 +0x62
        created by github.com/thanos-io/thanos/pkg/store.TestTenantSeriesSetServert_NotLeakingIfNotExhausted.func2
        	/home/bwplotka/Repos/thanos/pkg/store/multitsdb_test.go:224 +0x618
    --- FAIL: TestTenantSeriesSetServert_NotLeakingIfNotExhausted/cancelled,_not_exhausted_StoreSet (10.03s)
FAIL

Process finished with exit code 1

TestMultiTSDBStore_NotLeakingOnSendError was showing:

TestMultiTSDBStore_NotLeakingOnSendError: leaktest.go:150: leaktest: leaked goroutine: goroutine 84 [chan send]:
        github.com/thanos-io/thanos/pkg/store.ctxRespSender.send(...)
        	/home/bwplotka/Repos/thanos/pkg/store/proxy.go:198
        github.com/thanos-io/thanos/pkg/store.(*MultiTSDBStore).Series.func1(0x0, 0x0)
        	/home/bwplotka/Repos/thanos/pkg/store/multitsdb.go:214 +0x5cf
        golang.org/x/sync/errgroup.(*Group).Go.func1(0xc0002708d0, 0xc000416380)
        	/home/bwplotka/Repos/thanosgopath/pkg/mod/golang.org/x/sync@v0.0.0-20200317015054-43a5402ce75a/errgroup/errgroup.go:57 +0x59
        created by golang.org/x/sync/errgroup.(*Group).Go
        	/home/bwplotka/Repos/thanosgopath/pkg/mod/golang.org/x/sync@v0.0.0-20200317015054-43a5402ce75a/errgroup/errgroup.go:54 +0x66
--- FAIL: TestMultiTSDBStore_NotLeakingOnSendError (10.02s)

Signed-off-by: Bartlomiej Plotka bwplotka@gmail.com

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

Changes

Verification

bwplotka · 2020-07-08T20:22:27Z

It's kind of late, so will fix tomorrow. I am actually considering implementing #2864 as a fix 🙈 But maybe it will be smarter to have something quicker for now. cc @brancz

The truth is that fixing and making sure all edge cases are ok, feels like duplication of work, if we have really exactly same struct available.

bwplotka · 2020-07-09T07:42:38Z

Added same for ProxyStore to ensure that works.

bwplotka · 2020-07-09T08:18:12Z

Apparently proxy also leaks, but less ;p 🤦

   TestProxyStore_NotLeakingOnPrematureFinish: leaktest.go:132: leaktest: timed out checking goroutines
    TestProxyStore_NotLeakingOnPrematureFinish: leaktest.go:150: leaktest: leaked goroutine: goroutine 83 [chan send]:
        github.com/thanos-io/thanos/pkg/store.ctxRespSender.send(...)
        	/home/bwplotka/Repos/thanos/pkg/store/proxy.go:198
        github.com/thanos-io/thanos/pkg/store.(*ProxyStore).Series.func1(0x0, 0x0)
        	/home/bwplotka/Repos/thanos/pkg/store/proxy.go:305 +0x611
        golang.org/x/sync/errgroup.(*Group).Go.func1(0xc000712cf0, 0xc0000a4280)
        	/home/bwplotka/Repos/thanosgopath/pkg/mod/golang.org/x/sync@v0.0.0-20200317015054-43a5402ce75a/errgroup/errgroup.go:57 +0x59
        created by golang.org/x/sync/errgroup.(*Group).Go
        	/home/bwplotka/Repos/thanosgopath/pkg/mod/golang.org/x/sync@v0.0.0-20200317015054-43a5402ce75a/errgroup/errgroup.go:54 +0x66
    --- PASS: TestProxyStore_NotLeakingOnPrematureFinish/failling_send (0.00s)
FAIL

Process finished with exit code 1

bwplotka · 2020-07-09T09:20:56Z

Fixed all in separate commit.

… which was leaking on errors. Fixes: #2823 TestTenantSeriesSetServert_NotLeakingIfNotExhausted was showing leaks: ``` TestTenantSeriesSetServert_NotLeakingIfNotExhausted/cancelled,_not_exhausted_StoreSet: leaktest.go:132: leaktest: timed out checking goroutines TestTenantSeriesSetServert_NotLeakingIfNotExhausted/cancelled,_not_exhausted_StoreSet: leaktest.go:150: leaktest: leaked goroutine: goroutine 84 [chan send]: github.com/thanos-io/thanos/pkg/store.(*tenantSeriesSetServer).Send(0xc000708360, 0xc0003104c0, 0x0, 0x0) /home/bwplotka/Repos/thanos/pkg/store/multitsdb.go:141 +0x13e github.com/thanos-io/thanos/pkg/store.(*mockedStoreServer).Series(0xc0004e6330, 0xc0007083c0, 0x20ac2c0, 0xc000708360, 0x5116a0, 0x0) /home/bwplotka/Repos/thanos/pkg/store/multitsdb_test.go:173 +0x76 github.com/thanos-io/thanos/pkg/store.(*tenantSeriesSetServer).Series.func1(0x2097760, 0xc00003c940) /home/bwplotka/Repos/thanos/pkg/store/multitsdb.go:121 +0x56 github.com/thanos-io/thanos/pkg/tracing.DoInSpan(0x2097760, 0xc00003c940, 0x1c8bace, 0x17, 0xc000173760, 0x0, 0x0, 0x0) /home/bwplotka/Repos/thanos/pkg/tracing/tracing.go:72 +0xcc github.com/thanos-io/thanos/pkg/store.(*tenantSeriesSetServer).Series(0xc000708360, 0x20983e0, 0xc0004e6330, 0xc0007083c0) /home/bwplotka/Repos/thanos/pkg/store/multitsdb.go:120 +0xfa github.com/thanos-io/thanos/pkg/store.TestTenantSeriesSetServert_NotLeakingIfNotExhausted.func2.1(0xc000708360, 0xc0004e6330) /home/bwplotka/Repos/thanos/pkg/store/multitsdb_test.go:225 +0x62 created by github.com/thanos-io/thanos/pkg/store.TestTenantSeriesSetServert_NotLeakingIfNotExhausted.func2 /home/bwplotka/Repos/thanos/pkg/store/multitsdb_test.go:224 +0x618 --- FAIL: TestTenantSeriesSetServert_NotLeakingIfNotExhausted/cancelled,_not_exhausted_StoreSet (10.03s) FAIL Process finished with exit code 1 ``` TestMultiTSDBStore_NotLeakingOnPrematureFinish was showing: ``` TestMultiTSDBStore_NotLeakingOnSendError: leaktest.go:150: leaktest: leaked goroutine: goroutine 84 [chan send]: github.com/thanos-io/thanos/pkg/store.ctxRespSender.send(...) /home/bwplotka/Repos/thanos/pkg/store/proxy.go:198 github.com/thanos-io/thanos/pkg/store.(*MultiTSDBStore).Series.func1(0x0, 0x0) /home/bwplotka/Repos/thanos/pkg/store/multitsdb.go:214 +0x5cf golang.org/x/sync/errgroup.(*Group).Go.func1(0xc0002708d0, 0xc000416380) /home/bwplotka/Repos/thanosgopath/pkg/mod/golang.org/x/sync@v0.0.0-20200317015054-43a5402ce75a/errgroup/errgroup.go:57 +0x59 created by golang.org/x/sync/errgroup.(*Group).Go /home/bwplotka/Repos/thanosgopath/pkg/mod/golang.org/x/sync@v0.0.0-20200317015054-43a5402ce75a/errgroup/errgroup.go:54 +0x66 --- FAIL: TestMultiTSDBStore_NotLeakingOnSendError (10.02s) ``` Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

bwplotka · 2020-07-09T09:25:54Z

Unchained, and put against master, should be ready for review.

brancz · 2020-07-09T09:56:49Z

looks like the e2e test failure might be legitimate

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

bwplotka · 2020-07-09T13:34:00Z

Fixed, looks like copying is needed for TSDB, let's dive deeper into this after leak fix (:

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

bwplotka requested a review from brancz July 8, 2020 20:16

bwplotka force-pushed the receive-tenant-leak branch 3 times, most recently from b3fc9f2 to 63921c1 Compare July 9, 2020 07:42

bwplotka force-pushed the receive-tenant-leak branch 3 times, most recently from f04c44a to b3f37a1 Compare July 9, 2020 08:16

bwplotka changed the title ~~[Chained] receive: Fixed leak on receive Store API Series, which was leaking on errors.~~ [Chained] receive: Fixed leak on receive and querier proxying Store API Series, which was leaking on errors. Jul 9, 2020

bwplotka force-pushed the receive-tenant-leak branch from 1aa69ed to f070bc1 Compare July 9, 2020 09:23

bwplotka changed the title ~~[Chained] receive: Fixed leak on receive and querier proxying Store API Series, which was leaking on errors.~~ receive: Fixed leak on receive and querier proxying Store API Series, which was leaking on errors. Jul 9, 2020

bwplotka changed the base branch from encode-120 to master July 9, 2020 09:24

bwplotka marked this pull request as ready for review July 9, 2020 09:24

Quick fix for leaks.

e3a0510

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

bwplotka force-pushed the receive-tenant-leak branch from f070bc1 to e3a0510 Compare July 9, 2020 09:25

bwplotka requested review from GiedriusS, kakkoyun, povilasv and squat July 9, 2020 09:26

bwplotka added 2 commits July 9, 2020 14:27

Fixed issues found by lint.

f51776a

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

Get back copying.

b902c37

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

bwplotka force-pushed the receive-tenant-leak branch from 025f62f to b902c37 Compare July 9, 2020 13:32

Lint

547003d

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

brancz approved these changes Jul 9, 2020

View reviewed changes

bwplotka merged commit 60ede4c into master Jul 9, 2020

bwplotka deleted the receive-tenant-leak branch July 9, 2020 16:22

bwplotka mentioned this pull request Jul 10, 2020

Thanos Receive - High memory usage #2810

Closed

BhartiAgrawalNvidia mentioned this pull request Oct 13, 2020

Receive: gets stuck at a certain point right after CompactHead and shipping. #3197

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

receive: Fixed leak on receive and querier proxying Store API Series, which was leaking on errors.#2866

receive: Fixed leak on receive and querier proxying Store API Series, which was leaking on errors.#2866
bwplotka merged 5 commits intomasterfrom
receive-tenant-leak

bwplotka commented Jul 8, 2020 •

edited

Loading

Uh oh!

bwplotka commented Jul 8, 2020 •

edited

Loading

Uh oh!

bwplotka commented Jul 9, 2020

Uh oh!

bwplotka commented Jul 9, 2020

Uh oh!

bwplotka commented Jul 9, 2020

Uh oh!

bwplotka commented Jul 9, 2020

Uh oh!

brancz commented Jul 9, 2020

Uh oh!

bwplotka commented Jul 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

bwplotka commented Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Verification

Uh oh!

bwplotka commented Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bwplotka commented Jul 9, 2020

Uh oh!

bwplotka commented Jul 9, 2020

Uh oh!

bwplotka commented Jul 9, 2020

Uh oh!

bwplotka commented Jul 9, 2020

Uh oh!

brancz commented Jul 9, 2020

Uh oh!

bwplotka commented Jul 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bwplotka commented Jul 8, 2020 •

edited

Loading

bwplotka commented Jul 8, 2020 •

edited

Loading