-
Notifications
You must be signed in to change notification settings - Fork 42
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
Introduce cached client behind feature flag #1037
Conversation
28fb419
to
4613a9b
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.
Overall looks good to me. I will wait for operator PR for the feature flag.
Need to rebase. Doing it now. |
Squashed to make the rebase easier. |
Save client It is alive Checkpoint Pre cleanup Log name of migcluster starting remote watch for Filter list of events with exact watch to allow for cache usage Start transitioning event logging to keying on UID clientMap access safety add timout on spinlock add consistency check for remote watch add all clients to the shared client map to avoid reconstruction change spin lock implementation improve cache consistency logic for clients Rename vars related to cached client Pre-refactor to separate client names Reorganize getClient Cleaning up helper code checking for cache population Fix event indexer Tested and working Add switch to enable / disable cached client Cleanup compat client, return errors from helpers Clean up redundant comment Remove stunnel.go from bad rebase Put all cached client logic inside feature gate in migcluster_types Fix log statement with mismatched kv pairs
Just got the rebase finished. Need to do a bit of testing to verify that everything is working as expected. |
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.
LGTM
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.
The manager stop changes look sane to me.
Summary
ENABLE_CACHED_CLIENT
Create
andUpdate
, blocks until the cache catches upComplement to operator PR: migtools/mig-operator#675
To test
Open (Potential) Problems:
Need to add safe map access to the stored list of clients.
Need to properly handle teardown and recreation of client in shared map when a migcluster credential set changes
manager
due to bad MigCluster creds (this may be handled by current logic)Need to properly destroy manager which holds the cache when a migcluster is removed.
Need to make sure that
update
calls are blocking until receipt of update is present in our cache (e.g. a GET shows the expected resourceVersion)Need to add a while polling for cache to populate after a
create
orupdate
(for a rare race condition I'm imagining where we create something but something else deletes, we wouldn't want to hang forever waiting, maybe 10 second max)