[
https://issues.apache.org/jira/browse/YUNIKORN-3357?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Dale Richardson updated YUNIKORN-3357:
--------------------------------------
Description:
Yunikorn tests currently leak goroutines silently: components started by a test
(event system, partition manager cleaners, user group cache) outlive the test
binary with nobody noticing, and one intermittent shutdown race makes a leaked
goroutine appear in ~43% of full pkg/scheduler/tests runs.
Proposal: wire [uber-go/goleak|https://github.com/uber-go/goleak] into every
test package in core/k8shim repos via a shared {{pkg/common/leakcheck}} helper.
* One {{TestMain}} per test package
* All exemptions live in a single (per repo) documented list in
{{{}leakcheck.options(){}}}; {{VerifyTestMain}} accepts per-package extra
options so the shared list never needs widening
* No existing test or production code is modified; goleak is already a
transitive dependency and becomes a direct test-only one ({{{}go.sum{}}}
unchanged)
Switching detection on in Yunikorn-core surfaced seven pre-existing
leaked-goroutine shapes. Each gets a documented baseline exemption so this
change lands green and blocks new kinds of leaks immediately; each exemption is
to be deleted 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|
Items 2, 3(b) and 7 are production shutdown gaps rather than test hygiene and
will be filed as separate JIRAs (linked).
Known limitation, stated in the code: 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.
was:
Yunikorn tests currently leak goroutines silently: components started by a test
(event system, partition manager cleaners, user group cache) outlive the test
binary with nobody noticing, and one intermittent shutdown race makes a leaked
goroutine appear in ~43% of full pkg/scheduler/tests runs.
Proposal: wire [uber-go/goleak|https://github.com/uber-go/goleak] into every
test package one act repo via a shared {{pkg/common/leakcheck}} helper.
* One {{TestMain}} per test package
* All exemptions live in a single (per repo) documented list in
{{{}leakcheck.options(){}}}; {{VerifyTestMain}} accepts per-package extra
options so the shared list never needs widening
* No existing test or production code is modified; goleak is already a
transitive dependency and becomes a direct test-only one ({{{}go.sum{}}}
unchanged)
Switching detection on in Yunikorn-core surfaced seven pre-existing
leaked-goroutine shapes. Each gets a documented baseline exemption so this
change lands green and blocks new kinds of leaks immediately; each exemption is
to be deleted 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|
Items 2, 3(b) and 7 are production shutdown gaps rather than test hygiene and
will be filed as separate JIRAs (linked).
Known limitation, stated in the code: 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.
> Add goroutine leak detection to core tests via uber-go/goleak
> -------------------------------------------------------------
>
> Key: YUNIKORN-3357
> URL: https://issues.apache.org/jira/browse/YUNIKORN-3357
> Project: Apache YuniKorn
> Issue Type: Improvement
> Components: core - common, core - scheduler
> Reporter: Dale Richardson
> Priority: Major
>
> Yunikorn tests currently leak goroutines silently: components started by a
> test (event system, partition manager cleaners, user group cache) outlive the
> test binary with nobody noticing, and one intermittent shutdown race makes a
> leaked goroutine appear in ~43% of full pkg/scheduler/tests runs.
> Proposal: wire [uber-go/goleak|https://github.com/uber-go/goleak] into every
> test package in core/k8shim repos via a shared {{pkg/common/leakcheck}}
> helper.
> * One {{TestMain}} per test package
> * All exemptions live in a single (per repo) documented list in
> {{{}leakcheck.options(){}}}; {{VerifyTestMain}} accepts per-package extra
> options so the shared list never needs widening
> * No existing test or production code is modified; goleak is already a
> transitive dependency and becomes a direct test-only one ({{{}go.sum{}}}
> unchanged)
> Switching detection on in Yunikorn-core surfaced seven pre-existing
> leaked-goroutine shapes. Each gets a documented baseline exemption so this
> change lands green and blocks new kinds of leaks immediately; each exemption
> is to be deleted 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|
> Items 2, 3(b) and 7 are production shutdown gaps rather than test hygiene and
> will be filed as separate JIRAs (linked).
> Known limitation, stated in the code: 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.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
