[WIP] : Add goleak check to plugin/storage/integration #5468

varshith257 · 2024-05-21T02:46:16Z

Which problem is this PR solving?

Description of the changes

Added goleak checks for to plugin/storage/integration test files

How was this change tested?

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

Signed-off-by: Vamshi Maskuri <[email protected]>

varshith257 · 2024-05-21T03:12:44Z

@yurishkuro Found some unexpected goroutines leak from go test ./plugin/storage/integration after adding check for goleak:

goleak: Errors on successful test run: found unexpected goroutines:
[Goroutine 6 in state select, with github.com/golang/glog.(*fileSink).flushDaemon on top of the stack:
github.com/golang/glog.(*fileSink).flushDaemon(0x2b76918)
        /home/vamshi/go/pkg/mod/github.com/golang/[email protected]/glog_file.go:351 +0xb9
created by github.com/golang/glog.init.1 in goroutine 1
        /home/vamshi/go/pkg/mod/github.com/golang/[email protected]/glog_file.go:166 +0x126
]
FAIL    github.com/jaegertracing/jaeger/plugin/storage/integration      0.467s

Do this leaks need to be fixed or ignored?

varshith257 · 2024-05-21T03:18:12Z

It seems the goroutine leak is created by the glog package. It is part of glog's internal mechanism to periodically flush log data to files. It is in a select state, waiting for a condition to either flush the logs or terminate.

yurishkuro · 2024-05-21T03:19:38Z

I suggest you start with getting make lint green

Signed-off-by: Vamshi Maskuri <[email protected]>

…go-leak

Signed-off-by: Vamshi Maskuri <[email protected]>

varshith257 · 2024-05-21T03:42:02Z

I suggest you start with getting make lint green

Done!

(zulip-py3-venv) vamshi@LAPTOP-L0PR20E0:~/jaeger$ go test ./plugin/storage/integration
ok      github.com/jaegertracing/jaeger/plugin/storage/integration      0.016s

varshith257 · 2024-05-21T04:19:11Z

It seems like the test failure is due to the detection of unexpected goroutines, particularly those related to the net/http package and the olivere/elastic package.

Signed-off-by: Vamshi Maskuri <[email protected]>

yurishkuro · 2024-05-21T05:05:46Z

pkg/testutils/leakcheck.go

+// be stopped when the test finishes, leading to a detected but expected leak.
+func IgnoreElasticHealthCheckerLeak() goleak.Option {
+	return goleak.IgnoreTopFunction("github.com/olivere/elastic.(*Client).healthchecker")
+}


no, we don't want to do that. We ignore the other ones because those libs always leak goroutines with no ability to shut them down. In contrast, this ES client leaking is our fault, the unit test is kinda unstable and doesn't properly close all clients.

Can we look at a way of using the defer Close() method?
But I am not sure, there are already cleanup functions.
But yes, the tests look flaky as there are no client close methods used anywhere except the cleanup methods.

@yurishkuro Any suggestions on this?

no, we don't want to do that. We ignore the other ones because those libs always leak goroutines with no ability to shut them down. In contrast, this ES client leaking is our fault, the unit test is kinda unstable and doesn't properly close all clients.

@yurishkuro Here's our conversation for it

Yes, we need to track down the leaks and ensure Close are called as needed.

@yuriksho The builds are failing due to this:

created by github.com/olivere/elastic.DialContext in goroutine 166 /home/runner/go/pkg/mod/github.com/olivere/[email protected]+incompatible/client.go:404 +0xcd7 Goroutine 239 in state select, with github.com/olivere/elastic.(*Client).healthchecker on top of the stack: github.com/olivere/elastic.(*Client).healthchecker(0xc000525a00) and Goroutine 90 in state select, with net/http.(*persistConn).readLoop on top of the stack: net/http.(*persistConn).readLoop(0xc002b32b40) /opt/hostedtoolcache/go/1.22.3/x64/src/net/http/transport.go:2261 +0x154c created by net/http.(*Transport).dialConn in goroutine 102 /opt/hostedtoolcache/go/1.22.3/x64/src/net/http/transport.go:1799 +0x2774 Goroutine 26 in state IO wait, with internal/poll.runtime_pollWait on top of the stack: internal/poll.runtime_pollWait(0x7fc30f538b58, 0x72) /opt/hostedtoolcache/go/1.22.3/x64/src/runtime/netpoll.go:345 +0x85 internal/poll.(*pollDesc).wait(0xc000114520, 0x72, 0x0) /opt/hostedtoolcache/go/1.22.3/x64/src/internal/poll/fd_poll_runtime.go:84 +0xb1 internal/poll.(*pollDesc).waitRead(...) /opt/hostedtoolcache/go/1.22.3/x64/src/internal/poll/fd_poll_runtime.go:89 internal/poll.(*FD).Read(0xc000114500, {0xc00051c000, 0x1000, 0x1000}) /opt/hostedtoolcache/go/1.22.3/x64/src/internal/poll/fd_unix.go:164 +0x466 net.(*netFD).Read(0xc000114500, {0xc00051c000, 0x1000, 0x1000}) /opt/hostedtoolcache/go/1.22.3/x64/src/net/fd_posix.go:55 +0x4b net.(*conn).Read(0xc00010e138, {0xc00051c000, 0x1000, 0x1000}) /opt/hostedtoolcache/go/1.22.3/x64/src/net/net.go:179 +0xad net/http.(*persistConn).Read(0xc000518240, {0xc00051c000, 0x1000, 0x1000}) /opt/hostedtoolcache/go/1.22.3/x64/src/net/http/transport.go:1977 +0xf9 bufio.(*Reader).fill(0xc00010baa0) /opt/hostedtoolcache/go/1.22.3/x64/src/bufio/bufio.go:110 +0x2b0 bufio.(*Reader).Peek(0xc00010baa0, 0x1) /opt/hostedtoolcache/go/1.22.3/x64/src/bufio/bufio.go:148 +0xc7 net/http.(*persistConn).readLoop(0xc000518240) /opt/hostedtoolcache/go/1.22.3/x64/src/net/http/transport.go:2141 +0x354 created by net/http.(*Transport).dialConn in goroutine 24 /opt/hostedtoolcache/go/1.22.3/x64/src/net/http/transport.go:1799 +0x2[774](https://github.com/jaegertracing/jaeger/actions/runs/9208742224/job/25331628794?pr=5468#step:9:775) Goroutine 27 in state select, with net/http.(*persistConn).writeLoop on top of the stack: net/http.(*persistConn).writeLoop(0xc000518240) /opt/hostedtoolcache/go/1.22.3/x64/src/net/http/transport.go:2444 +0x1c5 created by net/http.(*Transport).dialConn in goroutine 24 /opt/hostedtoolcache/go/1.22.3/x64/src/net/http/transport.go:1800 +0x27ff

I have tried different methods of using close methods but they didn't turn around to work on. Any suggestions/alter approaches to try on

I have tried different methods of using close methods

trying random things is not how to solve this problem. We need to identify which specific test is causing the leak, and understand what causes the ES client to leak.

plugin/storage/integration/package_test.go

Signed-off-by: Vamshi Maskuri <[email protected]>

…go-leak

Signed-off-by: Vamshi Maskuri <[email protected]>

…go-leak

varshith257 · 2024-05-23T00:37:05Z

how is it done?

The \ has been added unexpectedly 😅

Signed-off-by: Vamshi Maskuri <[email protected]>

Bumps the otel-collector group with 30 updates: Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Vamshi Maskuri <[email protected]>

Signed-off-by: Vamshi Maskuri <[email protected]>

…o-leak

Signed-off-by: Vamshi Maskuri <[email protected]>

varshith257 · 2024-05-23T13:30:10Z

@yurishkuro It's done now!

yurishkuro · 2024-05-23T14:36:16Z

The CI is still broken

varshith257 · 2024-05-23T14:44:37Z

The CI is still broken

#5468 (comment)

I have tried different methods of using close methods but they didn't turn around to work on.

…o-leak

Signed-off-by: Vamshi Maskuri <[email protected]>

…go-leak

Signed-off-by: Vamshi Maskuri <[email protected]>

varshith257 · 2024-06-11T03:42:17Z

@yurishkuro Once have a look at goroutine leaks of Kafka. Multiple parts of Kafka making the leaks. Can we have a close method for each component of it to better overcome this or any alternatives?

yurishkuro · 2024-06-11T16:41:51Z

plugin/storage/integration/kafka_test.go

@@ -133,5 +133,6 @@ func TestKafkaStorage(t *testing.T) {
 	SkipUnlessEnv(t, "kafka")
 	s := &KafkaIntegrationTestSuite{}
 	s.initialize(t)
+	defer s.CleanUp(t)


yurishkuro · 2024-06-11T16:42:38Z

plugin/storage/es/factory_test.go

@@ -418,6 +418,10 @@ func testPasswordFromFile(t *testing.T, f *Factory, getClient func() es.Client,
 	require.NoError(t, os.WriteFile(newPwdFile, []byte(pwd2), 0o600))
 	require.NoError(t, os.Rename(newPwdFile, pwdFile))

+	// Close the old client


this is not related to the PR title

varshith257 · 2024-06-12T19:05:53Z

Holding this PR for now, there are a lot of leaks in storage dir. Will be further investigate and make progress on this

yurishkuro · 2024-06-12T20:47:04Z

The tactic I would recommend is:

add goleak check to individual tests
verify that they are clean, or fix if they are not
submit a PR
repeat for other tests as independent PRs

Once all tests are clean with individual checks, add a final PR to remove individual checks and only add a package level check (and fix the script to no longer having relaxed requirement for integration pkg)

varshith257 added 2 commits May 21, 2024 08:01

added go leak test to plugin/storage/integration

d2c0010

Signed-off-by: Vamshi Maskuri <[email protected]>

fix fmt

b5cac56

Signed-off-by: Vamshi Maskuri <[email protected]>

varshith257 requested a review from a team as a code owner May 21, 2024 02:46

varshith257 requested a review from albertteoh May 21, 2024 02:46

varshith257 marked this pull request as draft May 21, 2024 02:48

removed duplicates

a5dce47

Signed-off-by: Vamshi Maskuri <[email protected]>

varshith257 marked this pull request as ready for review May 21, 2024 03:11

Merge branch 'main' into go-leak

fd4dc2f

varshith257 added 3 commits May 21, 2024 08:56

fix lint

ce71dc1

Signed-off-by: Vamshi Maskuri <[email protected]>

Merge branch 'go-leak' of https://github.com/varshith257/jaeger into …

b62696b

…go-leak

fix lint fail

28f7e70

Signed-off-by: Vamshi Maskuri <[email protected]>

varshith257 added 3 commits May 21, 2024 09:50

add close method

131670a

Signed-off-by: Vamshi Maskuri <[email protected]>

rever close

fa99c9c

Signed-off-by: Vamshi Maskuri <[email protected]>

update leakcheck to ignore leaks from oliver package

a9e4bd8

Signed-off-by: Vamshi Maskuri <[email protected]>

yurishkuro reviewed May 21, 2024

View reviewed changes

plugin/storage/integration/package_test.go Outdated Show resolved Hide resolved

varshith257 and others added 9 commits May 21, 2024 10:43

fix copyright

9de3a85

Signed-off-by: Vamshi Maskuri <[email protected]>

Merge branch 'main' into go-leak

268efd6

fix close

4da4483

Signed-off-by: Vamshi Maskuri <[email protected]>

Merge branch 'go-leak' of https://github.com/varshith257/jaeger into …

ef66bb6

…go-leak

Update leakcheck.go

489d417

Signed-off-by: Vamshi Maskuri <[email protected]>

Update elasticsearch_test.go

89bd4c0

Signed-off-by: Vamshi Maskuri <[email protected]>

fix

c3c678d

Signed-off-by: Vamshi Maskuri <[email protected]>

Merge branch 'go-leak' of https://github.com/varshith257/jaeger into …

86b77b5

…go-leak

Merge branch 'main' into go-leak

fbe3bba

varshith257 and others added 3 commits May 23, 2024 18:30

fix lint

aa14af7

Signed-off-by: Vamshi Maskuri <[email protected]>

fix lint

713cf0a

Signed-off-by: Vamshi Maskuri <[email protected]>

varshith257 force-pushed the go-leak branch from 58d66d0 to 713cf0a Compare May 23, 2024 13:00

varshith257 and others added 5 commits May 23, 2024 18:31

Merge branch 'main' of https://github.com/jaegertracing/jaeger into g…

9560ff9

…o-leak

remove other commits

6973742

Signed-off-by: Vamshi Maskuri <[email protected]>

remove go.sum

2e65af8

Signed-off-by: Vamshi Maskuri <[email protected]>

Update go.sum

836770e

Signed-off-by: Vamshi Maskuri <[email protected]>

remove go.mod

3219b38

Signed-off-by: Vamshi Maskuri <[email protected]>

varshith257 marked this pull request as draft May 23, 2024 18:38

varshith257 and others added 2 commits May 24, 2024 23:11

Merge branch 'main' of https://github.com/jaegertracing/jaeger into g…

7abf15b

…o-leak

Merge branch 'main' into go-leak

6ddcef5

varshith257 changed the title ~~Add goleak check to plugin/storage/integration~~ [WIP] : Add goleak check to plugin/storage/integration Jun 5, 2024

varshith257 and others added 5 commits June 10, 2024 13:48

Merge branch 'main' into go-leak

5b11aa4

Merge branch 'main' of https://github.com/jaegertracing/jaeger into g…

cf934b4

…o-leak

close client 1

5d9926b

Signed-off-by: Vamshi Maskuri <[email protected]>

Merge branch 'go-leak' of https://github.com/varshith257/jaeger into …

12288b0

…go-leak

fix kafka leaks

4fc9bd0

Signed-off-by: Vamshi Maskuri <[email protected]>

yurishkuro reviewed Jun 11, 2024

View reviewed changes

varshith257 closed this Jun 13, 2024

This was referenced Oct 21, 2024

Enable and enforce goroutine leak checks in tests #5006

Closed

feat: add goleak check #6105

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] : Add goleak check to plugin/storage/integration #5468

[WIP] : Add goleak check to plugin/storage/integration #5468

varshith257 commented May 21, 2024 •

edited

Loading

varshith257 commented May 21, 2024 •

edited

Loading

varshith257 commented May 21, 2024 •

edited

Loading

yurishkuro commented May 21, 2024

varshith257 commented May 21, 2024

varshith257 commented May 21, 2024

yurishkuro May 21, 2024

varshith257 May 21, 2024 •

edited

Loading

varshith257 May 23, 2024

yurishkuro May 23, 2024

varshith257 May 23, 2024

varshith257 May 23, 2024

yurishkuro May 23, 2024

varshith257 May 23, 2024

varshith257 commented May 23, 2024

varshith257 commented May 23, 2024

yurishkuro commented May 23, 2024

varshith257 commented May 23, 2024

varshith257 commented Jun 11, 2024

yurishkuro Jun 11, 2024

yurishkuro Jun 11, 2024

varshith257 commented Jun 12, 2024

yurishkuro commented Jun 12, 2024

[WIP] : Add goleak check to plugin/storage/integration #5468

[WIP] : Add goleak check to plugin/storage/integration #5468

Conversation

varshith257 commented May 21, 2024 • edited Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

varshith257 commented May 21, 2024 • edited Loading

varshith257 commented May 21, 2024 • edited Loading

yurishkuro commented May 21, 2024

varshith257 commented May 21, 2024

varshith257 commented May 21, 2024

Choose a reason for hiding this comment

varshith257 May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varshith257 commented May 23, 2024

varshith257 commented May 23, 2024

yurishkuro commented May 23, 2024

varshith257 commented May 23, 2024

varshith257 commented Jun 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varshith257 commented Jun 12, 2024

yurishkuro commented Jun 12, 2024

varshith257 commented May 21, 2024 •

edited

Loading

varshith257 commented May 21, 2024 •

edited

Loading

varshith257 commented May 21, 2024 •

edited

Loading

varshith257 May 21, 2024 •

edited

Loading