Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deploy Infinispan with maxCount=10000 and num_owners=1 #818

Merged

Conversation

ryanemerson
Copy link
Contributor

@ryanemerson ryanemerson commented May 16, 2024

Resolves #805

@ahus1 ahus1 self-requested a review May 16, 2024 12:55
@ahus1 ahus1 changed the title Deploy Infinispan with maxCount=10000 and num_owners=1. Resolves #805 Deploy Infinispan with maxCount=10000 and num_owners=1 May 16, 2024
ahus1
ahus1 previously requested changes May 16, 2024
Copy link
Contributor

@ahus1 ahus1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ryanemerson - see below for my comments. One overall requirement I'd see: Those changes should only apply when persistent sessions are enabled.

Unfortunately a remedy to the bug we're handling in keycloak/keycloak#29592 might be not to apply this change in the external Infinispan.

With that recent development, I'd suggest to put this issue on hold until the other issue resolved. We would then revisit it again once the remote-store only changes from #28745 are in. But even then I'm not sure how an cache with maxCount and amnd xsite setup can maintain entries that are not outdated, as a remove in the primary site for a key that exists only in the secondary site will never reach the secondary site.

cc: @pruivo @mhajas

@@ -37,19 +37,23 @@
</encoding>
<memory max-count="10000"/>
</local-cache>
<distributed-cache name="sessions" owners="2">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes about owners and max-count should be active only when persistent sessions are enabled. At the moment Keycloak does those settings automatically in the CacheManagerFactory. Although this would prints an INFO message, I'd rather have the message in the log than maintaining two different configurations. So I'd ask you to revert the changes in this file.

@ahus1
Copy link
Contributor

ahus1 commented May 17, 2024

@ryanemerson - update to my previous comment: The PR keycloak/keycloak#29597 might still allow for an external Infinispan with maxCount, let's continue the discussion next week.

@ahus1 ahus1 self-assigned this May 17, 2024
@mhajas
Copy link
Contributor

mhajas commented May 17, 2024

@ryanemerson Is ISPN operator able to update existing caches with maxCount and owners? I remember that some time ago when I tried to change something in CR Operator refused to change it.

@ryanemerson
Copy link
Contributor Author

ryanemerson commented May 20, 2024

@ryanemerson Is ISPN operator able to update existing caches with maxCount and owners? I remember that some time ago when I tried to change something in CR Operator refused to change it.

Yes this is possible when the default strategy of spec.updates.strategy: retain is used on the Cache CR. The Operator will attempt to update the cache configuration on the server to equal that of the CR, however the operation will fail if the old and new configuration are incompatible for a runtime update.

https://infinispan.org/docs/infinispan-operator/main/operator.html#updating_cache_creating-caches

@ahus1 ahus1 dismissed their stale review May 27, 2024 08:21

Other PR is now merged, we are now safe to proceed as long as we do this only for persistent sessions enabled.

@ahus1
Copy link
Contributor

ahus1 commented May 27, 2024

@ryanemerson - the issues to handle the expiry of sessions is now resolved as the other PR is merged.

Could you please now attend to the other issue: Your changes should only apply when persistent sessions are enabled, as limiting the number of entries is otherwise not supported.

Thanks!

@ryanemerson ryanemerson force-pushed the update_caches_for_persistent_sessions branch from bb85891 to 8fbe24e Compare May 27, 2024 10:08
@ryanemerson
Copy link
Contributor Author

Suggestions added

@ryanemerson ryanemerson force-pushed the update_caches_for_persistent_sessions branch from 8fbe24e to 98127b9 Compare May 29, 2024 16:43
Closes keycloak#805

Signed-off-by: Ryan Emerson <remerson@redhat.com>
@ryanemerson ryanemerson force-pushed the update_caches_for_persistent_sessions branch from 98127b9 to 8ed29da Compare May 29, 2024 17:03
@ryanemerson
Copy link
Contributor Author

Your changes should only apply when persistent sessions are enabled, as limiting the number of entries is otherwise not supported.

It should be good now. If KC_PERSISTENT_SESSIONS is true when provisioning the Infinispan cluster then cache configs are updated accordingly.

@kami619
Copy link
Contributor

kami619 commented May 30, 2024

merging it, as its approved by @pruivo

@kami619 kami619 merged commit b4154d8 into keycloak:main May 30, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

For persistent sessions, deploy Infinispan with maxCount memory setting in user/client sessions cache
5 participants