Repository navigation
Shorter lifespan for offline session cache entries in memory - #26811
Conversation
|
@martin-kanis Is it basically about providing a server-wide setting to avoid setting the lifespan on per realm-basis? |
|
@pedroigor I would assume this is to have some way to evict data from the cache, especially for offline sessions that have no max session length. Since we have lazy loading now, it is definitely an improvement. |
Yeah, but does it make sense to include yet another server-level setting instead of just setting a lifespan to offline sessions in your realm? The ultimate goal is the same, no? Also, as you mentioned, why not just set a limit to the corresponding caches in the cache configuration? |
|
@pedroigor There is a difference from the offline session settings int he realm: offline sessions that are evicted from the cache might still be valid and will be lazy-loaded from the database into the cache. |
|
@sschu We picked the lifespan override as safer variant that will still safe some memory but removes a risk that you set fixed size for a cache, then you have many parallel offline token refreshes and a session got evicted from the cache in the middle of a request, just after it got imported from the DB. Infinispan uses tiny LFU algorithm (modification of least frequently used) for eviction and we are not fully sure how it would behave. We still might add and evaluate the cache size approach for future. We hope users will start experimenting with this lifespan override option and provide a feedback how it works or doesn't work for them. |
|
@martin-kanis OK, I think I get the point. It is probably the safer approach. But I would say the problem you described isn't very likely to happen as long as you have reasonable cache sizes > 10.000? You need to have really high load to have problems with parallel requests evicting each others' entries. Couldn't we also just document that we recommend a certain minimum size depending on the expected load? |
|
I'm really happy to see this eventually landing in main :) As far as I understand the implementation the new lifespan overrides for in memory items only affect cache items that are created (loaded-into memory) by Keycloak instances with that setting. So perhaps it would make sense to add a small note that a cluster restart might be needed to apply the new lifespans to all (offline)) cache entries. Another option would be to adjust the timestamps on the fly when an offline session is accessed and an "larger" lifespan is detected. |
|
@thomasdarimont This is a KC24 feature. When you roll out KC24, won't you restart all nodes anyways? |
|
@sschu it depends on whether an external infinispan is used and whether cached sessions from (<KC24) are compatible with KC24. Some folks might cycle their Keycloak cluster but perhaps not the infinispan cluster. |
|
You are right. I didn't check the implementation. I thought the expiration was added on a cache level on the embedded cache, but it looks like it is added when adding individual session entries... |
|
Would it be possible to backport this to 23.0.x / 22.0.x? |
@sschu - Infinispan uses "least frequently used" to expire entries, which I find hard to reason about in this scenario. Therefore, we'll stick with the expiry option. |
We're considering a backport. We had backporting in mind when we planned for this to keep the changes as minimal as possible. |
Oh, OK. I quite naturally assumed this to be least-recently-used. |
|
Another note on people using remote caches: There is Therefore, for those who are using this in an environment with a remote store, I recommend to set the lifespan for all distributed caches in their Infinispan XML file to a value that matches their environment. I'll open a follow-up issue for this to document this in the HA guide. We'll still need to analyze where that loading is really necessary (we know it is necessary for online user sessions). #26941 |
Closes keycloak#26810 Co-authored-by: Thomas Darimont <thomas.darimont@googlemail.com> Co-authored-by: Martin Kanis <mkanis@redhat.com> Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com> Signed-off-by: Martin Kanis <mkanis@redhat.com>
There was a problem hiding this comment.
Thank you for this change, @thomasdarimont, and @martin-kanis for documenting and testing it!
1 flaky test on run #10984 ↗︎
Details:
|
|||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| Partial realm export > Exports the realm |
Screenshots
|
|
Closes #26810
This is a new PR instead of #24302 where I added a model test and rebased the code. I also added documentation.