Repository navigation
More graceful leaving of distributed caches - #52477
Conversation
|
@pruivo - can you please have a look? While this terminate vs. view propagation sounds plausible to me, I'd like to have your opinion if it makes sense. And about the timeout: I know that there are a lot of Infinispan timeouts. Should other timeouts be aligned as well? Or should rather be the 10 seconds timeout be enlarged? Thanks! |
6d27cf5 to
ad15781
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new ISPN000208 suppression check is likely ineffective due to Infinispan message prefixes (and there is a small cleanup needed in the new shutdown-leave logic).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Improves embedded Infinispan rolling-restart behavior by proactively leaving distributed caches before cache-manager shutdown, tightening RPC timeouts for select DIST_SYNC caches, and suppressing a known noisy Infinispan error log during expected state-transfer scenarios.
Changes:
- Add an explicit per-cache leave sequence (plus a short propagation wait) before
EmbeddedCacheManager.stop()during shutdown. - Configure a 3s
remoteTimeoutforactionTokens,authenticationSessions, andloginFailureswhen clustered. - Suppress
ISPN000208logs viaKeycloakLogFilterto reduce expected rolling-restart noise.
File summaries
| File | Description |
|---|---|
| quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/logging/KeycloakLogFilter.java | Adds suppression for ISPN000208 to reduce false-alarm error logs during state transfer. |
| model/infinispan/src/main/java/org/keycloak/spi/infinispan/impl/embedded/CacheConfigurator.java | Applies a shorter remoteTimeout to specific DIST_SYNC caches to fit within retry budgets. |
| model/infinispan/src/main/java/org/keycloak/connections/infinispan/DefaultInfinispanConnectionProviderFactory.java | Leaves distributed caches before stopping the cache manager to avoid RPCs being routed to already-torn-down caches. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // is logged at ERROR and alarms users. With numOwners>=2 enforced by Keycloak, a single node | ||
| // departure always leaves at least one live owner, so this is expected during rolling restarts. | ||
| // https://github.com/keycloak/keycloak/issues/52088 | ||
| if (Objects.equals(record.getLevel(), Level.SEVERE) && record.getLoggerName().equals("org.infinispan.CLUSTER") && record.getMessage().startsWith("ISPN000208")) { |
| var dm = cache.getAdvancedCache().getDistributionManager(); | ||
| if (dm == null) { | ||
| continue; | ||
| } |
Note on Infinispan 16.2+ upgradeWhen Keycloak upgrades to Infinispan 16.2+, the The other two changes from this PR remain independently valuable after the upgrade:
|
Closes keycloak#52088 Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
ad15781 to
01c757d
Compare
Adding a check on startup to ensure that in all setups. Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
There was a problem hiding this comment.
🟡 Changes recommended
The two-node shutdown race remains, genuine data-loss diagnostics may be hidden, and the configured timeout contradicts the stated design.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/6 changed files
- Comments generated: 3
- Review effort level: Balanced
Closes #52088
Assumptions and design decisions
1. Per-cache leave before
cacheManager.stop()Infinispan's
DefaultCacheManager.stop()already sends a per-cacheCacheLeaveCommandviaStateTransferManagerImpl.stop(), but by that point local cache components are torn down immediately after — other nodes still have the old topology and route RPCs to the departing node's dead caches, producingISPN000427remote-timeout errors.The fix calls
LocalTopologyManager.leave()for each distributed cache beforecacheManager.stop(). This is safe because DIST_SYNC's synchronous backup ack guarantees that any in-flight write completes (or fails) before the leave takes effect — there is no window where a write is acknowledged to the caller but lost because the node departed.After leaving, the departing node is no longer a participant in the post-leave rebalance, so there is no need to wait for the rebalance to complete — only for the updated topology to propagate.
2. The 500ms fixed wait
After all leave commands are sent, a 500ms
Thread.sleep()gives the coordinator time to broadcast the updated topology to remaining members. This is a pragmatic choice — there is no Infinispan API to "wait until all members have installed the new topology." The wait is skipped when the cluster has only 2 members (the remaining node receives the topology update directly as the coordinator or the only peer) and when the node is alone (clusterSize <= 1).3. The 3-second
remoteTimeoutApplied to
actionTokens,authenticationSessions, andloginFailures— the three DIST_SYNC caches that don't already have a custom timeout. The value is derived from two constraints:Session caches (
userSessions,clientSessions,offlineSessions,offlineClientSessions) are not changed — with persistent sessions enabled, they usenumOwners=1and different access patterns.4. ISPN000208 log suppression
ISPN000208 "No live owners found for segments …"fires inStateConsumerImpl.findSources()during state transfer when the current node is the sole remaining owner after another node departs. The log level is ERROR, but the data is safe — this node has it. WithnumOwners >= 2enforced by Keycloak for these caches, a single-node departure always leaves at least one live owner, so ISPN000208 is expected noise during rolling restarts. It is suppressed using the existingKeycloakLogFiltermechanism (same as ISPN000312).5. Interaction with
ShutdownManagerShutdownManager(added in PR #44620) waits for any in-progress rebalance to settle before shutdown proceeds, but does not do a graceful per-cache leave. The newleaveCaches()runs afterShutdownManager.onShutdown()returns, so the sequence is: wait for stable topology → leave all caches → wait for topology propagation → stop cache manager. This ordering ensures we don't leave caches while a rebalance is still in progress.