Repository navigation
YARN-11984. Federation mock sub-cluster and router do not stop cleanly on SIGTERM - #8681
Merged
Merged
Conversation
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
slfan1989
requested changes
Aug 15, 2026
slfan1989
left a comment
slfan1989
left a comment
Contributor
There was a problem hiding this comment.
Thanks for fixing the shutdown lifecycle of the federation test processes. The service shutdown hooks, their ordering, and the extended timeout for the MiniYARNCluster look reasonable.
I found one remaining lifecycle issue in TestMockRouter: Router startup already initializes a FederationStateStore, but the code subsequently creates a second store and replaces the facade reference without closing the first one. The newly added hook therefore does not close every ZooKeeper-backed store created by the Router process. Please address the inline comment.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
The tests in the I ran
Same code, different results each run, always among The cause is a race in the test:
The suite this JIRA fixes, @slfan1989 could you take another look when you have time? |
joseluisll
force-pushed
the
YARN-11984
branch
from
September 3, 2026 21:46
0ef4a45 to
9ff0cc6
Compare
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
@pan3793 @slfan1989 may you review and give way to this as the latest psrt of the cluster leak testing improvement? |
Contributor
Author
|
@pan3793 @slfan1989 This PR is Green and ready to merge!. With this, the cluster leak improvement is finished. |
joseluisll
force-pushed
the
YARN-11984
branch
from
September 18, 2026 16:26
9ff0cc6 to
fc8ac07
Compare
|
💔 -1 overall
This message was automatically generated. |
joseluisll
force-pushed
the
YARN-11984
branch
from
September 25, 2026 01:26
fc8ac07 to
b2f3860
Compare
|
🎊 +1 overall
This message was automatically generated. |
joseluisll
force-pushed
the
YARN-11984
branch
from
September 26, 2026 10:11
b2f3860 to
f622780
Compare
|
🎊 +1 overall
This message was automatically generated. |
joseluisll
changed the title
Contributor
Author
|
@slfan1989 @pan3793 @Hexiaoqiao This PR is ready for review. Let's improve testing of Hadoop!. Thanks in advance. |
5 of 6 tasks
This was referenced Sep 27, 2026
joseluisll
changed the title
joseluisll
changed the title
Contributor
Author
|
I have narrowed the scope of the PR to just the SIGTERM fix, the fixes of flaky router tests is moved to specific new JIRA's |
3 of 6 tasks
joseluisll
commented
Oct 3, 2026
joseluisll
left a comment
joseluisll
left a comment
Contributor
Author
There was a problem hiding this comment.
@slfan1989 thanks for the review (review). The second-store issue is addressed: TestMockRouter no longer creates its own FederationStateStore.
Router#serviceInit already creates and initializes the facade's store (via SubClusterCleaner, which calls FederationStateStoreFacade#getInstance(Configuration)). So after router.init(conf) the mock now takes that same store with FederationStateStoreFacade.getInstance(conf).getStateStore() and registers the close hook on it. The Router process opens one ZooKeeper-backed store and the hook closes that one, with nothing orphaned by reinitialize(). The hook still runs at Router.SHUTDOWN_HOOK_PRIORITY - 5, after the Router's own hook, and is registered before router.start() so a failed start still closes the store.
Code: TestMockRouter.java#L86-L113
I've also narrowed this PR to the SIGTERM fix only: one commit, test code only. The Router flaky-test fixes and the two Router serviceStop fixes are moving to their own JIRAs. On Linux, TestYarnFederationWithCapacityScheduler passes 38/38, and all three child JVMs run their shutdown hooks and close their ZooKeeper sessions. Could you take another look?
Closed
3 of 6 tasks
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
This was referenced Oct 4, 2026
Merged
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
@slfan1989 CI and Yetus green, this PR is ready for review, after splitting and merging all YARN test fixes and spotbugs, only one part remain to finish the job: properly shutdown federation subcluster. Thank you in advance for your time. |
slfan1989
requested changes
Oct 7, 2026
3 of 6 tasks
slfan1989
requested changes
Oct 10, 2026
slfan1989
left a comment
slfan1989
left a comment
Contributor
There was a problem hiding this comment.
@joseluisll Thanks for your patience! #8808 and #8785 have now been merged. Could you please rebase this PR onto the latest trunk and push the updated branch to trigger fresh CI runs? We can then continue the review and move this PR toward merging.
slfan1989
changed the title
…y on SIGTERM. TestMockSubCluster and TestMockRouter are not tests - they have no @test methods. TestFederationSubCluster launches them as separate JVMs through JavaProcess and stops them with Process#destroy: SIGTERM on Linux and macOS, which runs shutdown hooks. Neither registered one, so the JVM died with its ResourceManager, NodeManagers and Router services still running: no service stop, and the ZooKeeper session was left to expire on its own. On Windows destroy maps to TerminateProcess, which runs no hooks, so nothing changes there. Both now register a CompositeServiceShutdownHook before init/start, so a failure part-way through startup still tears down whatever came up. Router#main already does exactly this; TestMockRouter starts the Router directly and so skipped it. Each uses the shutdown hook priority of the daemon it actually runs - ResourceManager's for the sub-cluster, Router's for the router - which every YARN daemon defines as 30. TestMockRouter additionally closes the ZooKeeper-backed FederationStateStore, which the facade never closes; that hook runs at a lower priority than the Router's - ShutdownHookManager runs the highest priority first - so the Router is stopped before its store goes away. It is registered once router.init() has returned and before router.start(), so a failed start still closes the store. It closes the store the facade already holds rather than building a second one: Router#serviceInit creates a SubClusterCleaner whose constructor initializes the facade, and reinitialize() swaps the reference without closing what it replaces, so a second store would orphan the first. The Router process opens one ZooKeeper session and closes it, rather than opening two and closing one. The sub-cluster hook gets an explicit 90s budget. Its three NodeManagers stop one after another, each waiting up to NM_PROCESS_KILL_WAIT_MS plus 1s (6s) for its applications to finish and then a hardcoded 10s in DeletionService#serviceStop: about 48s in all, which ShutdownHookManager's 30s default would abandon part-way through. The NodeManagers SIGKILL containers straight away instead of SIGTERM then SIGKILL, which these long-lived mock clusters never need. NM_PROCESS_KILL_WAIT_MS keeps its default: it also bounds how long a NodeManager waits for a container's pid file before giving up on killing it, so zeroing it could leak container processes. TestFederationSubCluster#stop signals all three child JVMs before waiting for any of them, so their shutdown hooks run concurrently rather than one after another; JavaProcess gains destroy(), which signals without waiting. Not a mini-cluster leak (see YARN-11983): these clusters are meant to outlive the method that starts them. The defect was unclean termination. Test-scope only; no production code is touched. Verified on Linux with TestYarnFederationWithCapacityScheduler: 38/38 pass, all three child JVMs run their hooks to completion and close their ZooKeeper session, and no hook hits its timeout. The suite costs 49s on trunk, 96s with the hooks. Contains content generated by Claude Code. Generated-by: Claude Code (Opus 5) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
joseluisll
force-pushed
the
YARN-11984
branch
from
October 10, 2026 07:40
6fc79bf to
da61fa9
Compare
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
Done! |
slfan1989
approved these changes
Oct 11, 2026
slfan1989
left a comment
slfan1989
left a comment
Contributor
There was a problem hiding this comment.
Thanks for rebasing! The previous lifecycle concerns have been addressed, and the latest branch includes both Router shutdown fixes. The latest GitHub Actions and Yetus checks are green. LGTM, ready to merge.
Contributor
|
@joseluisll Thanks for the contribution! Merged into trunk. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on (has to be merged first):
clears a CI failure: it edits TestDFSClientRetries.java:1309-1328, where TestDFSClientRetries fails at line 1326 in this PR's precommit, still red in the latest run
clears a CI failure: it changes McpHttpServlet, which carries the trunk spotbugs warning SE_BAD_FIELD that turns this PR red, still red in the latest run
clears a CI failure: it edits TestRouterWebServicesREST.java:694-717, where TestFederationWebApp fails at line 712 in this PR's precommit, still red in the latest run
Description of PR
JIRA: YARN-11984. Federation mock sub-cluster and router do not stop cleanly on SIGTERM.
TestMockSubClusterandTestMockRouterare not tests: they have no@Testmethods. They are process entry points.
TestFederationSubClusterstarts eachone as its own JVM through
JavaProcessand stops it withProcess#destroy.On Linux and macOS that sends SIGTERM, which runs shutdown hooks. Neither class
registered a hook, so each JVM exited with its services still running: no
serviceStopran for the sub-clusters' ResourceManager and NodeManagers or forthe Router, and the ZooKeeper sessions were left to expire on their own.
This PR:
CompositeServiceShutdownHookin both entry points, beforeinit/start, so a failure part-way through startup still tears downwhatever came up.
Router#mainalready does this.TestMockRouterstartsthe Router directly, so it skipped the hook.
TestMockRouter, closes the ZooKeeper-backedFederationStateStore, whichFederationStateStoreFacadenever closes. It closes the store the facadealready holds instead of creating a second one, which would have left the
first store open. This hook runs at a lower priority than the Router's, so
the Router stops before its store is closed.
after another, about 16s each, which doesn't fit in the 30s default. Sets
NM_SLEEP_DELAY_BEFORE_SIGKILL_MSto 0 so these long-lived mock clusterskill their containers immediately.
TestFederationSubCluster#stopsignal all three child JVMs beforewaiting for any of them, so their hooks run at the same time.
JavaProcessgainsdestroy(), which signals without waiting.Test code only; no production code changes.
How was this patch tested?
TestYarnFederationWithCapacityScheduler: 38/38 pass.Router) run their shutdown hooks to completion and close their ZooKeeper
sessions. No hook hits its timeout.
because the sub-clusters now stop their services instead of being killed.
For code changes:
declared according to the connector-specific documentation? Note: Automated CI
testing doesn't cover all cases so manual testing with cloud storage is still
required.
LICENSE,LICENSE-binary,NOTICE-binaryfiles?AI Tooling
If an AI tool was used:
where is the name of the AI tool used.
https://www.apache.org/legal/generative-tooling.html
Contains content generated by Claude Code.