-
Notifications
You must be signed in to change notification settings - Fork 31
chore(openshift): use cached, lazy init client instances #884
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
chore(openshift): use cached, lazy init client instances #884
Conversation
bb75cc3
to
df2f1b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes look good to me. I'll give this a try in OpenShift.
Do you know if we're using any Caffeine 3.x API here? We still downgrade this to 2.x for our downstream builds to sync with Quarkus. Perhaps we should just switch upstream to 2.x as well since it's still being supported alongside 3.x.
src/main/java/io/cryostat/net/openshift/OpenShiftAuthManager.java
Outdated
Show resolved
Hide resolved
I think it's all 2.x here still, the stuff being used here is the same as or even a smaller feature subset than what is already being used elsewhere ex.
Sure, I think that's fine too. |
@andrewazores Is there a limit on the number of entries in the cache? |
No, the cache has unlimited size. The only eviction policy is the 5 minute inactivity timeout. |
b75b354
to
8c3ed1c
Compare
That should be fine. I doubt there would be many concurrent users in a single namespace in 5 minutes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to work fine in OpenShift
* chore(openshift): extract OpenShiftNetworkModule for auth manager * chore(openshift): lazy-load namespace and account token from fs * feat(openshift): lazy-load openshift client instance * apply spotless formatting * feat(openshift): cache per-user-token client instances * remove unnecessary warning suppressions (cherry picked from commit 68c19ff)
* chore(openshift): extract OpenShiftNetworkModule for auth manager * chore(openshift): lazy-load namespace and account token from fs * feat(openshift): lazy-load openshift client instance * apply spotless formatting * feat(openshift): cache per-user-token client instances * remove unnecessary warning suppressions (cherry picked from commit 68c19ff) Co-authored-by: Andrew Azores <[email protected]>
Fixes #883
Refactors the
OpenShiftAuthManager
, moving it into its own subpackage with its own DI module for encapsulation. The mounted namespace and account token files are only read from filesystem once, and theOpenShiftClient
instance using the service account token is created once with lazy initialization and then that instance reused for the lifetime of the Cryostat container.OpenShiftClient
instances using a user's token are stored in a cache, expiring (closing the client instance) after 5 minutes of inactivity on the client instance, so that we can reuse the client and its HTTP connection(s) while a user is actively making API requests through Cryostat. This reduces the number of memory allocations we're doing and should allow for some HTTP connection reuse, too. The time-based cache eviction ensures that the cache does not grow unbounded for long-lived Cryostat deployments which may have many different users over time.