Concurrency safe client factory #4950
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
This was accessing the client field without locking the mutex and was potentially missing intialization. The dynamic client is actually exposing its REST client, so that can be used instead. Signed-off-by: Tom Wieczorek <[email protected]>
Keep the old CRD specific versions for backwards compatibility. Signed-off-by: Tom Wieczorek <[email protected]>
This allows the FakeClientFactory's GetRESTConfig method to panic without breaking tests. Code constructed with an empty rest.Config is not backed by a fake client at all, so it's better to have tests fail up front than to have strange errors when such constructed clients are actually used. Signed-off-by: Tom Wieczorek <[email protected]>
There's the possiblity that the REST config is not yet loaded when GetRESTConfig gets called. Returning a nil pointer along with a nil error is a problem. Signed-off-by: Tom Wieczorek <[email protected]>
Previously, the GetRESTClient method was not synchronized and not concurrency safe. Change this by adding mutexes and making it use lazy initialization like all other getters. Signed-off-by: Tom Wieczorek <[email protected]>
This way, the way the REST config is loaded is up to the client factory users. The special "admin" settings are now part of the controller command, freeing the client factory from any particular way of loading and setting up the config. Signed-off-by: Tom Wieczorek <[email protected]>
twz123
force-pushed
the
concurrency-safe-clientfactory
branch
from
77f5512 to
ed0bc7b
Compare
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.
Description
Remove
GetRESTClientThis was accessing the client field without locking the mutex and was potentially missing initialization. The dynamic client is actually exposing its REST client, so that can be used instead.
Use k0s's clientset in the k0s client factory
Keep the old CRD specific versions for backwards compatibility.
Lazy initialization for metadata client in Autopilot update checks
This allows
FakeClientFactory.GetRESTConfigmethod to panic without breaking tests. Code constructed with an empty rest.Config is not backed by a fake client at all, so it's better to have tests fail up front than to have strange errors when such constructed clients are actually used.Lazy initialization in client factory's
GetRESTClientPreviously, the
GetRESTClientmethod was not synchronized and not concurrency safe. Callers could get a nil pointer under certain circumstances. Change this by adding mutexes and making it use lazy initialization like all other getters. This means that this method needs to be fallible.Introduce
LoadRESTConfigfunction pointerThis way, the way the REST config is loaded is up to the client factory users. The special "admin" settings are now part of the controller command, freeing the client factory from any particular way of loading and setting up the config.
Type of change
How Has This Been Tested?
Checklist: