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

Concurrency safe client factory #4950

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

twz123
Copy link
Member

@twz123 twz123 commented Sep 10, 2024

Description

  • Remove GetRESTClient
    This 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.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.

  • Lazy initialization in client factory's GetRESTClient
    Previously, the GetRESTClient method 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 LoadRESTConfig function pointer
    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.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

  • Manual test
  • Auto test added

Checklist:

  • My code follows the style guidelines of this project
  • My commit messages are signed-off
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

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 twz123 changed the title git st Concurrency safe client factory Sep 10, 2024
@twz123 twz123 added the chore label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant