Repository navigation
Replace operation set wrong lifespan in remote infinispan database an… - #15619
Conversation
…d leads to session eviction
|
@mposolda Hi, do know why we use -1 for a lifespan when we add an entry to the remote cache for the first time instead of actual lifespan which is calculated for the entry? This seems to be causing problems with |
|
@daviddelannoy Would you have a chance to verify the fix please? I think we don't have a easy way to test it in our testsuite as it involves a remote Infinispan with a database. |
|
I will try to see if we still have some test environment to test it, but not quite soon (not this month) As other few people were interested in a fix for this issue I will try to test it, thanks for the PR 😉👍 |
|
@martin-kanis thank you very much for your efforts! Your code change seems to do the trick. I just gave this a quick spin with my example setup: https://github.com/thomasdarimont/keycloak-project-example/tree/main/deployments/local/clusterx#run-keycloakx-cluster-with-database-backed-user-sessions Here is an example for the However as @daviddelannoy mentioned, I still need to apply my fix for Tables generated by
Empty ispn_sessions: ispn_sessions after login with user1 to client1 ispn_sessions after user1 accesses client2 ispn_clientSessions with 2 active client sessions ispn_sessions after login with user2 to client1 ispn_clientSessions with 3 active client sessions for 2 users ispn_sessions after user1 logout ispn_clientSessions after user1 logout |
There was a problem hiding this comment.
@martin-kanis @thomasdarimont Thanks for the fix and for the testing with the fix applied.
TBH I don't recall the reasoning for using -1 here. I don't see a problem with your change and hence I am approving this PR.
We can run cross-dc to doublecheck if there is a regression, but I don't know if it is possible to run cross-dc tests against main branch with the latest changes (removed Wildfly etc).
I am approving and leaving to you whether you want to run some more tests or whether you want also someone else approval for this PR.
|
@thomasdarimont Thank you for the verification! |
Thank you Marek for the review. Yeah, it is not possible to run cross-dc test with current |
At the end, GHA keycloak build was failing because of a node js issue so I run cross-dc locally on Keycloak 19 + this patch and tests passed. |
Okay, good to know. One idea I had about this was, that instead of using a flag like I did (for testing ;-)), we could allow writes to cachestore if a cache is explicitly configured with persistence or a remote-store. |
|
@martin-kanis Thanks for doublecheck with the tests. I am merging the PR then. |
|
@martin-kanis another note: When using a database for cache storage, then we currently have to explicitly configure either a jdbc connection-pool or a datasource. It would be great if we could just reference the quarkus datasource somehow. Perhaps this could be implemented with a custom In my spring-boot-keycloak-server-example I use the later approach for exposing the Spring Boot datasource with a "interal" JNDI lookup to the Keycloak infrastructure. I think the same would work for quarkus keycloak without any modifications to infinispan. |
…d leads to session eviction (keycloak#15619) Closes keycloak#10755
Backport of fix in PR keycloak#15619.
…d leads to session eviction
Fixes #10755