tigerquoll opened a new pull request, #1124: URL: https://github.com/apache/yunikorn-core/pull/1124
### What is this PR for? Adds goroutine leak detection to the test suite using [uber-go/goleak](https://github.com/uber-go/goleak), wired into **every test package** (20 of them) via a shared `pkg/common/leakcheck` helper: - One `TestMain` per package; all shared exemptions live in a single documented list in `leakcheck.options()`; `VerifyTestMain` accepts per-package extra options so the shared list never needs widening - `pkg/rmproxy` is the one deliberate exception: it has no test functions, so a hook there would guard nothing (documented in the package doc) No existing test or production code is modified. `goleak` was already a transitive dependency (`go.sum` unchanged); it is promoted to a direct test-only dependency. ### The baseline exemption list is a burn-down list Switching detection on surfaced seven pre-existing leaked-goroutine shapes. Each has a documented exemption so this change lands green and blocks new *kinds* of leaks immediately; each exemption is to be removed by its own follow-up fix: | # | Goroutine | Cause | |---|---|---| | 1 | `EventSystemImpl.StartServiceWithPublisher.func1` | tests start the event system without `Stop()` | | 2 | `eventPublisher.start.func1` | `Init()` registers a configmap callback that `Stop()` never removes — a config update after shutdown restarts the event system | | 3 | `EventStreaming.CreateEventStream.func1` | (a) test never calls `RemoveStream`; (b) forwarder blocks on a bare `consumer <- event` send, so slow-consumer eviction cannot release it | | 4 | `partitionManager.cleanRoot` | tests build a `ClusterContext` without `Stop()` | | 5 | `partitionManager.cleanExpiredApps` | same as 4 | | 6 | `security.UserGroupCache.run` | same as 4 — `ClusterContext.Stop()` already stops it | | 7 | `ClusterContext.notifyRMNewAllocation` | shutdown race: `StopAll` stops the scheduler before the RM proxy; a queued allocation event can win the select over the closed stop channel, then the notify blocks forever on its unbuffered reply because `handleRMEvents` exits without draining (reproduced ~43% of full `pkg/scheduler/tests` runs before the exemption) | Items 2, 3(b) and 7 are production shutdown gaps rather than test hygiene; they will be filed as separate JIRAs. Known limitation, stated in the code: the exemptions match on top stack frame, so the baseline is a ratchet against new leak *kinds*, not an instance count; three entries key on positional `.funcN` closure names that the follow-up fixes should replace by hoisting the goroutine bodies to named methods. ### What type of PR is it? * [ ] - Bug Fix * [x] - Improvement * [ ] - Feature * [ ] - Documentation * [ ] - Hot Fix * [ ] - Refactoring ### What is the Jira issue? https://issues.apache.org/jira/browse/YUNIKORN-3357 ### How should this be tested? - `go test -count=1 ./pkg/scheduler/tests/` ×5: all green (this package's leak is the intermittent #7 above — before its exemption it failed ~43% of full runs) - `go test -count=3` on the other instrumented packages: no goleak failures - `go test -race ./pkg/...` and the CI-style `-race -tags deadlock` run with deadlock detection enabled: all 20 packages ok - `make lint`, `make license-check`, `go vet ./pkg/...`: clean Note: four packages (`common/security`, `entrypoint`, `metrics`, `scheduler/objects`) fail under `go test -count=3` on unmodified master as well — pre-existing non-idempotent tests over process-global state, unrelated to this change and not goleak failures. Generated by the Author with assistance from Claude Code -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
