HADOOP-19966. ZKDelegationTokenSecretManager may fail to load tokens and keys from ZooKeeper on startup - #8688
Open
pan3793 wants to merge 2 commits into
Open
HADOOP-19966. ZKDelegationTokenSecretManager may fail to load tokens and keys from ZooKeeper on startup#8688pan3793 wants to merge 2 commits into
pan3793 wants to merge 2 commits into
Conversation
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a ZooKeeper-backed delegation token startup issue in ZKDelegationTokenSecretManager where tokens/keys might not be loaded because CuratorCache initializes asynchronously, and where root container znodes created without explicit data can contain unexpected default payloads under Curator 5.2.0+.
Changes:
- Wait for
CuratorCacheinitialization before loading tokens/keys from the cache. - Skip processing the root container znodes when consuming cache events and when streaming the cache contents.
- Create root znodes with explicit empty data, and adjust tests to better isolate/cleanup curator usage and reduce restart flakiness.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java | Ensures cache initialization completes before loading and avoids parsing container-root nodes; creates roots with explicit empty data. |
| hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/token/delegation/TestZKDelegationTokenSecretManager.java | Improves test reliability around restart loading and curator lifecycle handling. |
Suppressed comments (1)
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/token/delegation/TestZKDelegationTokenSecretManager.java:649
ExecutorServiceshutdown andtm1/tm2.destroy()are inside the main try-block, so an exception frominvokeAll()/future.get()can leak threads and running secret managers. Ensure the executor is always shut down and both token managers are destroyed via afinally.
try {
// When the init method is called,
// the ZKDelegationTokenSecretManager#startThread method will be called,
// and the creatingParentContainersIfNeeded will be called to create the nameSpace.
ExecutorService executorService = Executors.newFixedThreadPool(2);
Callable<Boolean> tm1Callable = () -> {
tm1.init();
return true;
};
Callable<Boolean> tm2Callable = () -> {
tm2.init();
return true;
};
List<Future<Boolean>> futures = executorService.invokeAll(
Arrays.asList(tm1Callable, tm2Callable));
for (Future<Boolean> future : futures) {
assertTrue(future.get());
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+561
to
+567
| try { | ||
| DelegationTokenManager tm1 = new DelegationTokenManager(conf, new Text("foo")); | ||
|
|
||
| // When the init method is called, | ||
| // the ZKDelegationTokenSecretManager#startThread method will be called, | ||
| // and the creatingParentContainersIfNeeded will be called to create the nameSpace. | ||
| tm1.init(); | ||
| // When the init method is called, | ||
| // the ZKDelegationTokenSecretManager#startThread method will be called, | ||
| // and the creatingParentContainersIfNeeded will be called to create the nameSpace. | ||
| tm1.init(); |
Member
Author
There was a problem hiding this comment.
Fixed: destroy() and executor shutdown/await moved into finally in both tests.
…and keys from ZooKeeper on startup Wait for CuratorCache initialization before loading each cache, skip the root container znode when processing cache events, and create the container znodes with explicit empty data. Assisted-by: deepseek-v4-pro
Move token manager destruction and executor shutdown into finally blocks in testCreatingParentContainersIfNeeded and testMultipleInit. Assisted-by: deepseek-v4-pro
|
🎊 +1 overall
This message was automatically generated. |
Member
Author
|
@slfan1989, mind taking another look? still see such testing failure occasionly https://github.com/apache/hadoop/actions/runs/32465896350/job/96725649556 |
Contributor
@pan3793 Thank you very much for your contribution! I will take a closer look at this PR today. |
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
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.
Description of PR
ZKDelegationTokenSecretManager reads the CuratorCache immediately after
starting it, but CuratorCache populates asynchronously, so the cache can
still be empty and existing tokens/keys are silently not loaded into memory
on startup. It also created the root container znodes data-less, which
Curator 5.2.0+ defaults to the local address, so the container znode data
could not be parsed.
This change:
when processing cache events and when streaming the cache, since it is a
container rather than a key/token.
use setCurator, and waits for the good token to load in
testNodesLoadedAfterRestart.
Contains content generated by deepseek-v4-pro
How was this patch tested?
mvn -pl hadoop-common-project/hadoop-common -am
-Dtest=TestZKDelegationTokenSecretManager test
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