[
https://issues.apache.org/jira/browse/YUNIKORN-3363?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Dale Richardson updated YUNIKORN-3363:
--------------------------------------
Description:
Follow-up to YUNIKORN-3357 (goleak adoption, PR #1124); burns down leakcheck
exemptions events.(*EventSystemImpl).StartServiceWithPublisher.func1 and
events.(*eventPublisher).start.func1.
{{events.Init()}} registers a configmap callback ({{{}event_system.go{}}}) that
{{EventSystemImpl.Stop()}} never removes via
{{configs.RemoveConfigMapCallback}} — compare {{{}health_checker.go{}}}, which
deregisters correctly. Any configuration reload after {{Stop()}} runs
{{reloadConfig() -> restart()}} and resurrects an event system that nobody
holds a reference to; its handler ({{{}StartServiceWithPublisher.func1{}}}) and
publisher ({{{}eventPublisher.start.func1{}}}) goroutines then run for the life
of the process.
Aggravator: {{eventSystemId}} is {{{}fmt.Sprintf("event-system-%d",
time.Now().Unix()){}}}, so repeated {{Init()}} calls collide on the callback
key within a second and accumulate across seconds.
update: the mechanism (Stop() doesn't deregister the callback) is real, but
harmless while Stop() only ever coincides with process exit; the resurrection
impact is latent, appears only if core becomes restartable in-process.
{{events.Init()}} registers a configmap callback ({{{}event_system.go{}}}) that
{{EventSystemImpl.Stop()}} never removes via
{{configs.RemoveConfigMapCallback}} — unlike {{{}health_checker.go{}}}, which
deregisters correctly. The registration outlives {{{}Stop(){}}}.
Impact is latent, not active under the current lifecycle. As noted on PR #1124,
{{Stop()}} today only ever runs as the process exits, so no config reload fires
the callback afterwards and nothing is resurrected — there is no leak in normal
operation. The defect bites only if the event system is stopped and later
re-started within a live process, i.e. once core is restartable in-process
(YUNIKORN-3370): a post-{{{}Stop(){}}} config reload would then run
{{reloadConfig() -> restart()}} and resurrect a system nobody holds a reference
to, leaking its handler and publisher goroutines.
Aggravator for that case: {{eventSystemId}} is
{{{}fmt.Sprintf("event-system-%d", time.Now().Unix()){}}}, so repeated
{{Init()}} calls collide on the callback key within a second.
Proposed fix as part of, YUNIKORN-3370:
* deregister the callback in {{Stop()}} (or re-register in
{{{}StartServiceWithPublisher{}}})
* use a unique id (uuid) instead of a unix-seconds timestamp
* delete the events exemptions in {{pkg/common/leakcheck/leakcheck.go}} in
core (#1124) and shim (#1061)
was:
Follow-up to YUNIKORN-3357 (goleak adoption, PR #1124); burns down leakcheck
exemptions events.(*EventSystemImpl).StartServiceWithPublisher.func1 and
events.(*eventPublisher).start.func1.
{{events.Init()}} registers a configmap callback ({{{}event_system.go{}}}) that
{{EventSystemImpl.Stop()}} never removes via
{{configs.RemoveConfigMapCallback}} — compare {{{}health_checker.go{}}}, which
deregisters correctly. Any configuration reload after {{Stop()}} runs
{{reloadConfig() -> restart()}} and resurrects an event system that nobody
holds a reference to; its handler ({{{}StartServiceWithPublisher.func1{}}}) and
publisher ({{{}eventPublisher.start.func1{}}}) goroutines then run for the life
of the process.
Aggravator: {{eventSystemId}} is {{{}fmt.Sprintf("event-system-%d",
time.Now().Unix()){}}}, so repeated {{Init()}} calls collide on the callback
key within a second and accumulate across seconds.
Proposed fix:
* deregister the callback in {{Stop()}} (or re-register in
{{{}StartServiceWithPublisher{}}})
* use a unique id (uuid) instead of a unix-seconds timestamp
* delete the two events exemptions in {{pkg/common/leakcheck/leakcheck.go}}
update: the mechanism (Stop() doesn't deregister the callback) is real, but
harmless while Stop() only ever coincides with process exit; the resurrection
impact is latent, appears only if core becomes restartable in-process.
> Event system restarts after Stop(): configmap callback never deregistered,
> leaking handler and publisher goroutines
> -------------------------------------------------------------------------------------------------------------------
>
> Key: YUNIKORN-3363
> URL: https://issues.apache.org/jira/browse/YUNIKORN-3363
> Project: Apache YuniKorn
> Issue Type: Bug
> Components: core - scheduler
> Reporter: Dale Richardson
> Assignee: weichen lai
> Priority: Minor
> Labels: newbie
>
> Follow-up to YUNIKORN-3357 (goleak adoption, PR #1124); burns down leakcheck
> exemptions events.(*EventSystemImpl).StartServiceWithPublisher.func1 and
> events.(*eventPublisher).start.func1.
> {{events.Init()}} registers a configmap callback ({{{}event_system.go{}}})
> that {{EventSystemImpl.Stop()}} never removes via
> {{configs.RemoveConfigMapCallback}} — compare {{{}health_checker.go{}}},
> which deregisters correctly. Any configuration reload after {{Stop()}} runs
> {{reloadConfig() -> restart()}} and resurrects an event system that nobody
> holds a reference to; its handler ({{{}StartServiceWithPublisher.func1{}}})
> and publisher ({{{}eventPublisher.start.func1{}}}) goroutines then run for
> the life of the process.
> Aggravator: {{eventSystemId}} is {{{}fmt.Sprintf("event-system-%d",
> time.Now().Unix()){}}}, so repeated {{Init()}} calls collide on the callback
> key within a second and accumulate across seconds.
> update: the mechanism (Stop() doesn't deregister the callback) is real, but
> harmless while Stop() only ever coincides with process exit; the resurrection
> impact is latent, appears only if core becomes restartable in-process.
> {{events.Init()}} registers a configmap callback ({{{}event_system.go{}}})
> that {{EventSystemImpl.Stop()}} never removes via
> {{configs.RemoveConfigMapCallback}} — unlike {{{}health_checker.go{}}}, which
> deregisters correctly. The registration outlives {{{}Stop(){}}}.
> Impact is latent, not active under the current lifecycle. As noted on PR
> #1124, {{Stop()}} today only ever runs as the process exits, so no config
> reload fires the callback afterwards and nothing is resurrected — there is no
> leak in normal operation. The defect bites only if the event system is
> stopped and later re-started within a live process, i.e. once core is
> restartable in-process (YUNIKORN-3370): a post-{{{}Stop(){}}} config reload
> would then run {{reloadConfig() -> restart()}} and resurrect a system nobody
> holds a reference to, leaking its handler and publisher goroutines.
> Aggravator for that case: {{eventSystemId}} is
> {{{}fmt.Sprintf("event-system-%d", time.Now().Unix()){}}}, so repeated
> {{Init()}} calls collide on the callback key within a second.
> Proposed fix as part of, YUNIKORN-3370:
> * deregister the callback in {{Stop()}} (or re-register in
> {{{}StartServiceWithPublisher{}}})
> * use a unique id (uuid) instead of a unix-seconds timestamp
> * delete the events exemptions in {{pkg/common/leakcheck/leakcheck.go}} in
> core (#1124) and shim (#1061)
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
