joseluisll commented on PR #8688:
URL: https://github.com/apache/hadoop/pull/8688#issuecomment-6602405119
Also there is a race that needs to be solved:
**The race**
After `awaitCacheInitialized(...)`, `loadFromZKCache()` applies a snapshot
of every cached node on the thread calling `startThreads()`. By then the cache
listener is already applying newer events on Curator's `SafeNotifyService`
thread. Nothing orders the two, so the starting thread can apply an older
snapshot of a token after the listener has applied a newer state:
```
starting thread listener thread
reads cached DT_n (valid)
peer cancels DT_n
NODE_DELETED ->
currentTokens.remove(DT_n)
processTokenAddOrUpdate(old bytes)
-> currentTokens.put(DT_n) (nothing removes it again)
```
Effects on the instance that is starting:
- A token cancelled by a peer during startup goes back into memory, and
`verifyToken()` **accepts it** until it is renewed or reaches its renew date.
- A token renewed by a peer during startup gets its **old renew date** back
in memory, so this instance's expiry scan can later delete a token that is
still valid in ZooKeeper.
Trunk does not have this race. There, `loadFromZKCache()` reads the cache
before it is filled (the bug this PR fixes), so the starting thread never
applies a token and the listener applies everything in order.
**Evidence**
The same test runs unchanged on trunk and on this PR. A peer cancels 300
tokens while another instance starts; afterwards, none of them may be in the
restarted instance's memory or pass `verifyToken()`. The only timing aid is a 2
ms sleep whenever the starting thread applies a token itself, which does
nothing on trunk.
| Version | Runs | Failed | Tokens applied by the starting thread |
Cancelled tokens still accepted |
|---|---|---|---|---|
| trunk `57f78d2` | 13 | 0 | 0 | 0 |
| this PR `ccf5b7f` | 13 | 6 | 170–209 | 1–3 per failed run |
Two deterministic tests reproduce the cancel and the renew cases one at a
time. They pause the starting thread before it applies a token, let a real peer
change it, and use a `runSafe()` no-op as a barrier until the listener has
handled the change. In Curator 5.2.0, listener calls and `runSafe()` tasks run
in order on one thread. Tests and write-up:
https://github.com/joseluisll/hadoop/blob/zkdtsm-race-repro/ZKDTSM-RACES-REPORT.md
(section 5)
**Suggested fix (tested)**
Drop `loadFromZKCache()` and let the listener apply the initial nodes too:
no `.afterInitialized()`, and after the await only `syncTokenOwnerStats()` for
the token cache. Curator delivers the initial nodes, then later changes, then
`initialized()`, all in order on one thread. So when the latch opens, memory is
complete and no older snapshot can land afterwards. With this change all 13
tests in `TestZKDelegationTokenSecretManager` pass, including
`testNodesLoadedAfterRestart` with its immediate assertion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
