Enable and enforce goroutine leak checks in tests

[yurishkuro]

Enforce the use of https://github.com/uber-go/goleak across the repo:

• a linter similar to scripts/check-test-files.sh that verifies that all packages with tests have goleak in TestMain
  • first deploy the script in audit mode where it only prints offending packages
  • then make the script fail the CI once the next item is addressed
  • Done in Feature: add goleak to check goroutine leak in tests #5010
• add goleak to all packages via TestMain. Some packages may pass the validation, some may fail and would need to be fixed. It's best to break this into several PRs, first to add goleak to packages that do not fail, and then potentially separate PRs for each package where goleak does fail with the corresponding fixes to the tests.
  • Run make goleak to see the list of packages missing the linter
  • In packages with multiple *_test.go files add the check to a new file package_test.go
    • Remember to add copyright header - run make fmt after committing the new files
  • Examples:
    • Add goleak check to cmd/agent/app/configmanager #5015
    • Add goleak check to packages with empty tests #5017
• Fix leaks in the integration tests (see Add go leak checks to ES/OS tests #6339)
• Change linter script to fail CI on errors going forward

State on March 8 2024:

$ make goleak
Verifying that all packages with tests have goleak in their TestMain
🔴 Error(check-goleak): no goleak check in package ./cmd/collector/app/
🔴 Error(check-goleak): no goleak check in package ./cmd/internal/printconfig/
🔴 Error(check-goleak): no goleak check in package ./cmd/query/app/
🔴 Error(check-goleak): no goleak check in package ./pkg/cassandra/gocql/testutils/
🔴 Error(check-goleak): no goleak check in package ./pkg/testutils/
🔴 Error(check-goleak): no goleak check in package ./plugin/metrics/prometheus/metricsstore/
🔴 Error(check-goleak): no goleak check in package ./plugin/sampling/strategystore/adaptive/
🔴 Error(check-goleak): no goleak check in package ./plugin/storage/integration/
Error(check-goleak): no goleak check in 8 package(s).

[Freedisch]

@yurishkuro, I will look into this

[akagami-harsh]

@yurishkuro, i would also like to work on this, if @Freedisch dosen't mind

[yurishkuro]

Latest: 33 packages remaining

Error(check-goleak): no goleak check in 33 package(s).

[Pushkarm029]

working on /cmd/collector/app/zipkin/

[limistah]

I have implemented this into the package folders in this PR

[WillSewell]

I had a look into ./plugin/sampling/strategystore/adaptive/. One challenge is the high default Processor.followerRefreshInterval (20s) which causes the runUpdateProbabilitiesLoop to hang long enough that either Close() needs to block a long time, or the test times out. In processor_test.go the default value can be overridden before Processor.Start is called. However in factory_test.go this is not currently possible because the Factory wraps both the initialising and starting of the Processor in the CreateStrategyStore method.

I think there's a couple of options:

1. Add the followerRefreshInterval field to the Factory which could be set in the test code, and then this could be set on the Processor in the CreateStrategyStore method.
2. Expose --sampling.follower-refresh-interval as an option

For now I'll go for 1. but I thought I'd see whether there is any appetite for 2. because it would have the side benefit of making the test code a bit cleaner.

[WillSewell]

Solved in a different way: dfe7adc

[Manik2708]

#5468 (comment) I started with this like by adding adding goleak in a single test. From there I got errors and by reffering to this I got to know that we have to use a seperate grpc clients for each test instead of using as a single (initialized in the e2eInitialize), then I placed the TestMain for checking all go leaks and got the tests passed for cassandra. @yurishkuro What should be the correct way? Should we be bothered for failing tests when checked seperately or way encorprated in #5468 is correct?

[yurishkuro]

@Manik2708 if you are able to get clean run by adding goleak check into individual tests, feel free to create PRs for those - it will narrow down the scope of remaining work (once the whole package is clean it's easy to remove those in favor of a package-level check, although we may just want to keep them per-test too, as a debugging tool).

I will need more information about what specific problem you found. I don't believe it should matter if some gRPC clients are alive for the duration of the overall test vs. individual tests as long as they are closed at the end of the parent test.

[Manik2708]

This was the error which I got when I used goleak.VerifyNone(t) in a test of getOperations for Cassandra. This error was not present when goleak was ran for the whole package. The gRPC error is same as described in my above comment.

    integration.go:357: 2024-12-08 22:24:42.04  Writing trace with 5 spans
    integration.go:261: Retrieved operations: [{example-operation-1 unspecified} {example-operation-3 server} {example-operation-4 client}]
    e2e_integration.go:270: Purging storage via http://0.0.0.0:9231/purge
    integration.go:269: found unexpected goroutines:
        [Goroutine 52 in state select, with google.golang.org/grpc/internal/grpcsync.(*CallbackSerializer).run on top of the stack:
        google.golang.org/grpc/internal/grpcsync.(*CallbackSerializer).run(0xc000216c20, {0x2b146e8, 0xc0004fe230})
        	/home/manik/go/pkg/mod/google.golang.org/[email protected]/internal/grpcsync/callback_serializer.go:88 +0x1eb
        created by google.golang.org/grpc/internal/grpcsync.NewCallbackSerializer in goroutine 22
        	/home/manik/go/pkg/mod/google.golang.org/[email protected]/internal/grpcsync/callback_serializer.go:52 +0x207
         Goroutine 53 in state select, with google.golang.org/grpc/internal/grpcsync.(*CallbackSerializer).run on top of the stack:
        google.golang.org/grpc/internal/grpcsync.(*CallbackSerializer).run(0xc000216c50, {0x2b146e8, 0xc0004fe280})
        	/home/manik/go/pkg/mod/google.golang.org/[email protected]/internal/grpcsync/callback_serializer.go:88 +0x1eb
        created by google.golang.org/grpc/internal/grpcsync.NewCallbackSerializer in goroutine 22
        	/home/manik/go/pkg/mod/google.golang.org/[email protected]/internal/grpcsync/callback_serializer.go:52 +0x207
         Goroutine 50 in state IO wait, with internal/poll.runtime_pollWait on top of the stack:
        internal/poll.runtime_pollWait(0x740c6f938df0, 0x72)
        	/usr/local/go/src/runtime/netpoll.go:351 +0x85
        internal/poll.(*pollDesc).wait(0xc0004c9020, 0x72, 0x0)
        	/usr/local/go/src/internal/poll/fd_poll_runtime.go:84 +0xb1
        internal/poll.(*pollDesc).waitRead(...)
        	/usr/local/go/src/internal/poll/fd_poll_runtime.go:89
        internal/poll.(*FD).Read(0xc0004c9000, {0xc00032f000, 0x1000, 0x1000})
        	/usr/local/go/src/internal/poll/fd_unix.go:165 +0x466
        net.(*netFD).Read(0xc0004c9000, {0xc00032f000, 0x1000, 0x1000})
        	/usr/local/go/src/net/fd_posix.go:55 +0x4b
        net.(*conn).Read(0xc0004f6050, {0xc00032f000, 0x1000, 0x1000})
        	/usr/local/go/src/net/net.go:189 +0xad
        net/http.(*persistConn).Read(0xc0005807e0, {0xc00032f000, 0x1000, 0x1000})
        	/usr/local/go/src/net/http/transport.go:2052 +0xf9
        bufio.(*Reader).fill(0xc0004f2120)
        	/usr/local/go/src/bufio/bufio.go:110 +0x29a
        bufio.(*Reader).Peek(0xc0004f2120, 0x1)
        	/usr/local/go/src/bufio/bufio.go:148 +0xc5
        net/http.(*persistConn).readLoop(0xc0005807e0)
        	/usr/local/go/src/net/http/transport.go:2205 +0x354
        created by net/http.(*Transport).dialConn in goroutine 24
        	/usr/local/go/src/net/http/transport.go:1874 +0x29b4
         Goroutine 51 in state select, with net/http.(*persistConn).writeLoop on top of the stack:
        net/http.(*persistConn).writeLoop(0xc0005807e0)
        	/usr/local/go/src/net/http/transport.go:2519 +0x1c5
        created by net/http.(*Transport).dialConn in goroutine 24
        	/usr/local/go/src/net/http/transport.go:1875 +0x2a3f
         Goroutine 54 in state select, with google.golang.org/grpc/internal/grpcsync.(*CallbackSerializer).run on top of the stack:
        google.golang.org/grpc/internal/grpcsync.(*CallbackSerializer).run(0xc000216c80, {0x2b146e8, 0xc0004fe2d0})
        	/home/manik/go/pkg/mod/google.golang.org/[email protected]/internal/grpcsync/callback_serializer.go:88 +0x1eb
        created by google.golang.org/grpc/internal/grpcsync.NewCallbackSerializer in goroutine 22
        	/home/manik/go/pkg/mod/google.golang.org/[email protected]/internal/grpcsync/callback_serializer.go:52 +0x207
         Goroutine 58 in state select, with google.golang.org/grpc/internal/resolver/dns.(*dnsResolver).watcher on top of the stack:
        google.golang.org/grpc/internal/resolver/dns.(*dnsResolver).watcher(0xc00011e000)
        	/home/manik/go/pkg/mod/google.golang.org/[email protected]/internal/resolver/dns/dns_resolver.go:220 +0x3ee
        created by google.golang.org/grpc/internal/resolver/dns.(*dnsBuilder).Build in goroutine 7
        	/home/manik/go/pkg/mod/google.golang.org/[email protected]/internal/resolver/dns/dns_resolver.go:149 +0x645
         Goroutine 6 in state select, with google.golang.org/grpc/internal/grpcsync.(*CallbackSerializer).run on top of the stack:
        google.golang.org/grpc/internal/grpcsync.(*CallbackSerializer).run(0xc00025a380, {0x2b146e8, 0xc0001a43c0})
        	/home/manik/go/pkg/mod/google.golang.org/[email protected]/internal/grpcsync/callback_serializer.go:88 +0x1eb
        created by google.golang.org/grpc/internal/grpcsync.NewCallbackSerializer in goroutine 22
        	/home/manik/go/pkg/mod/google.golang.org/[email protected]/internal/grpcsync/callback_serializer.go:52 +0x207
         Goroutine 7 in state select, with google.golang.org/grpc/internal/grpcsync.(*CallbackSerializer).run on top of the stack:
        google.golang.org/grpc/internal/grpcsync.(*CallbackSerializer).run(0xc00025a3b0, {0x2b146e8, 0xc0001a4410})
        	/home/manik/go/pkg/mod/google.golang.org/[email protected]/internal/grpcsync/callback_serializer.go:88 +0x1eb
        created by google.golang.org/grpc/internal/grpcsync.NewCallbackSerializer in goroutine 22
        	/home/manik/go/pkg/mod/google.golang.org/[email protected]/internal/grpcsync/callback_serializer.go:52 +0x207
         Goroutine 8 in state select, with google.golang.org/grpc/internal/grpcsync.(*CallbackSerializer).run on top of the stack:
        google.golang.org/grpc/internal/grpcsync.(*CallbackSerializer).run(0xc00025a3e0, {0x2b146e8, 0xc0001a4460})
        	/home/manik/go/pkg/mod/google.golang.org/[email protected]/internal/grpcsync/callback_serializer.go:88 +0x1eb
        created by google.golang.org/grpc/internal/grpcsync.NewCallbackSerializer in goroutine 22
        	/home/manik/go/pkg/mod/google.golang.org/[email protected]/internal/grpcsync/callback_serializer.go:52 +0x207
         Goroutine 56 in state IO wait, with internal/poll.runtime_pollWait on top of the stack:
        internal/poll.runtime_pollWait(0x740c6f938cd8, 0x72)
        	/usr/local/go/src/runtime/netpoll.go:351 +0x85
        internal/poll.(*pollDesc).wait(0xc0004c91a0, 0x72, 0x0)
        	/usr/local/go/src/internal/poll/fd_poll_runtime.go:84 +0xb1

[yurishkuro]

@Manik2708 first, you mentioned that adding goleak.VerifyNone(t) to some test did not result in an issue - can you create PRs for those?

second, are you talking about v1 or v2 integration tests? For v1 there should be no gRPC in the picture. If you are talking about v2, then again gRPC there is not specific to Cassandra, but to the overall test setup, so I would expect the same issue to occur for another storage backend that might be easier to run locally than the one with Cassandra.

Lastly, I think I would be only putting goleak.VerifyNone(t) to the top-level tests, not individual sub-tests.

[yurishkuro]

looking at the dump output, it mentions e2e_integration.go, which is v2 tests. But we already have package-level testutils.VerifyGoLeaks(m) there, so I am not sure how you are getting this failure.

[Manik2708]

@Manik2708 first, you mentioned that adding goleak.VerifyNone(t) to some test did not result in an issue - can you create PRs for those?

second, are you talking about v1 or v2 integration tests? For v1 there should be no gRPC in the picture. If you are talking about v2, then again gRPC there is not specific to Cassandra, but to the overall test setup, so I would expect the same issue to occur for another storage backend that might be easier to run locally than the one with Cassandra.

Lastly, I think I would be only putting goleak.VerifyNone(t) to the top-level tests, not individual sub-tests.

I think there is some mis-understanding! I said that tests are not passing for individual tests but they are passing for package. Yes, this output is for v2. But thanks for the clarification as I was putting the goleak test in individual test not in top level test. I think now these errors are justified.

[yurishkuro]

I don't think we need anything extra for v2 tests since we already have package-level check. The last package missing checks is plugin/storage/integration (which is also used by v2 e2e tests, so a bit puzzling why it fails)