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

chore(openshift): use cached, lazy init client instances #884

Merged
merged 6 commits into from
Apr 12, 2022

Conversation

andrewazores
Copy link
Member

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 the OpenShiftClient 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.

@andrewazores andrewazores added the chore Refactor, rename, cleanup, etc. label Apr 8, 2022
@andrewazores andrewazores requested a review from ebaron April 8, 2022 19:29
@andrewazores andrewazores changed the title Cache openshift client chore(openshift): use cached, lazy init client instances Apr 8, 2022
Copy link
Member

@ebaron ebaron left a 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.

@andrewazores
Copy link
Member Author

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

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. TargetConnectionManager

Perhaps we should just switch upstream to 2.x as well since it's still being supported alongside 3.x.

Sure, I think that's fine too.

@ebaron
Copy link
Member

ebaron commented Apr 12, 2022

@andrewazores Is there a limit on the number of entries in the cache?

@andrewazores
Copy link
Member Author

No, the cache has unlimited size. The only eviction policy is the 5 minute inactivity timeout.

@ebaron
Copy link
Member

ebaron commented Apr 12, 2022

That should be fine. I doubt there would be many concurrent users in a single namespace in 5 minutes.

Copy link
Member

@ebaron ebaron left a 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

@andrewazores andrewazores merged commit 68c19ff into cryostatio:main Apr 12, 2022
@andrewazores andrewazores deleted the cache-openshift-client branch April 12, 2022 19:48
mergify bot pushed a commit that referenced this pull request Apr 12, 2022
* 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)
ebaron pushed a commit that referenced this pull request Apr 12, 2022
* 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 <aazores@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport chore Refactor, rename, cleanup, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use cached client instances when communicating with OpenShift API server
2 participants