Introduce cached client behind feature flag #1037
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
djwhatle
commented
Mar 29, 2021
|
I used this PR and I would just like to post the results here.
|
djwhatle
force-pushed
the
safe-cache
branch
2 times, most recently
from
28fb419
to
4613a9b
Compare
djwhatle
changed the title
djwhatle
changed the title
djwhatle
changed the title
djwhatle
commented
May 20, 2021
djwhatle
requested review from
pranavgaikwad,
shawn-hurley,
alaypatel07 and
sseago
pranavgaikwad
requested changes
May 20, 2021
|
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. |
sseago
approved these changes
Jun 3, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
ENABLE_CACHED_CLIENTCreateandUpdate, 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
managerdue 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
updatecalls 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
createorupdate(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)