HADOOP-19966. ZKDelegationTokenSecretManager may fail to load tokens and keys from ZooKeeper on startup - #8688
HADOOP-19966. ZKDelegationTokenSecretManager may fail to load tokens and keys from ZooKeeper on startup#8688pan3793 wants to merge 3 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.
| 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(); |
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. |
|
@slfan1989, mind taking another look? still see such testing failure occasionly https://github.com/apache/hadoop/actions/runs/32465896350/job/96725649556 |
@pan3793 Thank you very much for your contribution! I will take a closer look at this PR today. |
slfan1989
left a comment
slfan1989
left a comment
There was a problem hiding this comment.
Thanks for working on this. The overall direction looks correct, and both Yetus and the targeted test suite pass.
However, I have two concerns before approving:
-
The fixed 10-second cache initialization timeout may cause startup failures for large token/key trees or slower ZooKeeper environments.
-
The added eventual wait in
testNodesLoadedAfterRestartweakens the regression test, because the previous asynchronous implementation could also pass once the listener catches up. Since the production change guarantees initialization beforeinit()returns, the test should retain an immediate assertion.
I also suggest cleaning up partially started caches and counters when initialization fails. For now, I think these points should be addressed before the PR is merged.
| private void awaitCacheInitialized(CountDownLatch initialized, | ||
| String cacheName) throws IOException { | ||
| try { | ||
| if (!initialized.await(CACHE_INITIALIZED_TIMEOUT_SECONDS, TimeUnit.SECONDS)) { |
There was a problem hiding this comment.
initialized() is emitted only after all initial nodes have been loaded into the cache, so the required time depends on the size of the token/key trees and ZooKeeper latency. A fixed 10-second timeout can make an otherwise healthy service fail to start when the cache is large or the configured ZooKeeper timeout is longer.
Could this timeout be configurable, or derived from an existing ZooKeeper timeout setting, with an appropriate default?
There was a problem hiding this comment.
Made configurable via zk-dt-secret-manager.zkCacheInitTimeout (ms) in f35a83a. Default 0 waits indefinitely; the tests set 10s.
| // The good token should be loaded on startup, and removed after expiry. | ||
| id = smNew.decodeTokenIdentifier(token); | ||
| final AbstractDelegationTokenIdentifier idGood = id; | ||
| GenericTestUtils.waitFor(new Supplier<Boolean>() { |
There was a problem hiding this comment.
The production change is intended to guarantee that the cache is fully loaded before tm.init() returns. Adding an eventual 5-second wait here weakens that regression guarantee: the old asynchronous implementation could also pass once its listener catches up.
Could we keep the immediate assertion after tm.init() ? Ideally, the test should fail without the awaitCacheInitialized() production change and pass with it.
There was a problem hiding this comment.
Restored the immediate assertion after tm.init() in f35a83a. The trunk failure linked above fails on exactly this assertion without the production change.
| .build(); | ||
| keyCache.listenable().addListener(keyCacheListener); | ||
| keyCache.start(); | ||
| awaitCacheInitialized(keyCacheInitialized, "key"); |
There was a problem hiding this comment.
When cache initialization times out or is interrupted, startThreads() throws after the cache and shared counters have already been started. The caller does not automatically invoke stopThreads() after a failed initialization, so these resources may remain active. Could the partially started resources be closed before propagating the failure?
There was a problem hiding this comment.
startThreads() now calls stopThreads() before rethrowing on any failure, with an isRunning() guard so the cleanup never stops a running instance, in f35a83a.
| .build(); | ||
| tokenCache.listenable().addListener(tokenCacheListener); | ||
| tokenCache.start(); | ||
| awaitCacheInitialized(tokenCacheInitialized, "token"); |
…ces on failed start Add zk-dt-secret-manager.zkCacheInitTimeout (ms, default 0 waits indefinitely), call stopThreads() when startThreads() fails, and restore the immediate assertion in testNodesLoadedAfterRestart. Assisted-by: Claude Fable 5.1
|
🎊 +1 overall
This message was automatically generated. |
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. The root container znodes also carry Curator's default data (the
local address), and since HADOOP-17835 (3.4.0) CuratorCache includes the root
node itself, so that data was fed to the key/token parsers.
This change:
when processing cache events and when streaming the cache, since it is a
container rather than a key/token.
Curator's default (the local address). Existing roots keep their old
payload, so the root skip above is the actual fix.
cache initialization; default 0 waits indefinitely, the tests set 10s.
leak the caches, counters, or the Curator client.
use setCurator.
Contains content generated by deepseek-v4-pro and Claude Fable 5.1
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