[
https://issues.apache.org/jira/browse/YUNIKORN-3370?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18112303#comment-18112303
]
Dale Richardson edited comment on YUNIKORN-3370 at 9/7/26 11:31 AM:
--------------------------------------------------------------------
[~wilfreds] You are right on both counts, and I have re-checked the whole
description against master rather than just those two lines.
* {{{}stopped{}}}: the CAS pair in {{Stop()}} /
{{StartServiceWithPublisher()}} landed with YUNIKORN-3336 (#1111) on 29 July.
This JIRA was written against the pre-3336 code and filed two weeks after that
fix merged, so both statements about the event system were wrong when filed. A
start after a stop does not leak a handler.
* The callback: agreed, a registration is not a goroutine and {{reloadConfig}}
exits once the change is processed. Deregistering it would break turning the
event system on and off via config. Dropped.
I also re-ran the experiment the description rests on, on current k8shim master
({{{}edef7f7e{}}}, core {{{}2577453{}}}), adding {{coreContext.StopAll()}} to
{{{}MockScheduler.stop(){}}}:
* {{{}TestAssumePodError -count=15{}}}: 15/15 pass with and without the call
(it was ~25% failing when this was filed; the test has since been rewritten by
YUNIKORN-3355 and the retry loop by YUNIKORN-3369, and the original cause was
never pinned down).
* Full {{pkg/shim -count=2}} with the call: pass.
* With the call in place, all 17 core-service {{IgnoreTopFunction}} entries in
the shim's leakcheck (#1061) can be deleted and {{pkg/shim}} still passes under
goleak.
Core's own {{pkg/scheduler/tests}} mock scheduler already does
{{StartAllServices()}} / {{StopAll()}} per test inside one binary, which is the
in-process restart this JIRA said does not work.
Two linked issues follow from this:
* YUNIKORN-3363 was closed as a duplicate of this one on the strength of the
callback proposal. With that withdrawn it duplicates nothing; I will change its
resolution to Not A Problem unless you object.
* YUNIKORN-3416 left two items "with 3370": the {{UserGroupCache}} stopped flag
never being cleared, and the getter racing the nil write in {{Stop()}}. The
first was also wrong when written, the flag has been cleared on re-creation
since YUNIKORN-2233. The second did not show under {{-race}} in two full
{{pkg/shim}} runs with {{StopAll()}}. Neither is being carried here.
YUNIKORN-3412 is unaffected and stays related; routine {{StopAll()}} in tests
will exercise that path more, not less.
So the "not restartable in-process" claim is withdrawn. What remains is what
YUNIKORN-3366 described before I closed it as a duplicate of this one: tests in
both repos start core services and never stop them. I have rewritten the
description accordingly; the fix is {{defer Stop()}} / {{StopAll()}} in the
tests, then deleting the exemptions in #1061 and #1124. If you would rather
keep this under the 3366 number, I can reopen that and close this instead
flagging since it changes the scope of what you picked up.
was (Author: tigerquoll):
[~wilfreds] You are right on both counts, and I have re-checked the whole
description against master rather than just those two lines.
* {{{}stopped{}}}: the CAS pair in {{Stop()}} /
{{StartServiceWithPublisher()}} landed with YUNIKORN-3336 (#1111) on 29 July.
This JIRA was written against the pre-3336 code and filed two weeks after that
fix merged, so both statements about the event system were wrong when filed. A
start after a stop does not leak a handler.
* The callback: agreed, a registration is not a goroutine and {{reloadConfig}}
exits once the change is processed. Deregistering it would break turning the
event system on and off via config. Dropped.
I also re-ran the experiment the description rests on, on current k8shim master
({{{}edef7f7e{}}}, core {{{}2577453{}}}), adding {{coreContext.StopAll()}} to
{{{}MockScheduler.stop(){}}}:
* {{{}TestAssumePodError -count=15{}}}: 15/15 pass with and without the call
(it was ~25% failing when this was filed; the test has since been rewritten by
YUNIKORN-3355 and the retry loop by YUNIKORN-3369, and the original cause was
never pinned down).
* Full {{pkg/shim -count=2}} with the call: pass.
* With the call in place, all 17 core-service {{IgnoreTopFunction}} entries in
the shim's leakcheck (#1061) can be deleted and {{pkg/shim}} still passes under
goleak.
Core's own {{pkg/scheduler/tests}} mock scheduler already does
{{StartAllServices()}} / {{StopAll()}} per test inside one binary, which is the
in-process restart this JIRA said does not work.
So the "not restartable in-process" claim is withdrawn. What remains is what
YUNIKORN-3366 described before I closed it as a duplicate of this one: tests in
both repos start core services and never stop them. I have rewritten the
description accordingly; the fix is {{defer Stop()}} / {{StopAll()}} in the
tests, then deleting the exemptions in #1061 and #1124. If you would rather
keep this under the 3366 number, I can reopen that and close this instead
flagging since it changes the scope of what you picked up.
> yunikorn-core services are not restartable in-process, so the shim cannot
> stop the core it starts without intermittent failures
> -------------------------------------------------------------------------------------------------------------------------------
>
> Key: YUNIKORN-3370
> URL: https://issues.apache.org/jira/browse/YUNIKORN-3370
> Project: Apache YuniKorn
> Issue Type: Test
> Components: shim - kubernetes
> Reporter: Dale Richardson
> Assignee: weichen lai
> Priority: Minor
> Labels: newbie
>
> Follow-up to YUNIKORN-3357. Supersedes YUNIKORN-3366 (closed as a duplicate
> of this issue) and replaces the original description of this issue, which
> claimed the core services could not be restarted in-process. That claim was
> wrong: {{EventSystemImpl.Stop()}} / {{StartServiceWithPublisher()}} have been
> CAS-gated since YUNIKORN-3336, {{UserGroupCache.Stop()}} resets its
> singleton, and core's own {{pkg/scheduler/tests}} mock scheduler already
> starts and stops the full service set per test in one binary. The A/B in the
> shim (adding {{coreContext.StopAll()}} to {{{}MockScheduler.stop(){}}}) that
> motivated the original description no longer reproduces on master (0/15
> failures either way).
> What is actually wrong is test hygiene: tests in both repos start core
> services and never stop them, so their goroutines outlive the test binary and
> the goleak check has to exempt them.
> Core (#1124), 5 exemptions:
> * {{events.(*EventSystemImpl).StartServiceWithPublisher.func1}} and
> {{{}events.(*eventPublisher).start.func1{}}}: tests call {{StartService()}}
> without {{Stop()}}
> * {{scheduler.(*partitionManager).cleanRoot}} and {{{}cleanExpiredApps{}}}:
> tests build a {{ClusterContext}} / partition without {{Stop()}}
> * {{{}common/security.(*UserGroupCache).run{}}}: tests resolve users without
> stopping the cache
> Shim (#1061), 17 exemptions, all core-service goroutines:
> {{MockScheduler.stop()}} stops the shim but not the core it started. Adding
> {{fc.coreContext.StopAll()}} lets all 17 be deleted; verified on the #1061
> branch with the exemption list emptied.
> Repro: delete the exemption in {{pkg/common/leakcheck/leakcheck.go}} and run
> the package; goleak's {{VerifyTestMain}} fails the binary with the leaked
> stack.
> Fix: {{defer <service>.Stop()}} / {{StopAll()}} in the affected tests, then
> delete the exemptions. Can be split per package (events / partition cleaners
> / UserGroupCache / shim mock).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
