✨ Make MaxConcurrentReconciles configurable
#138
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
lubedacht
reviewed
May 29, 2024
Keeps the `api` package clean. To scope the lock to a specific ICC, we make use of the keyed locks here again, even though we don't need their ability to react on canceled contexts. We therefore pass `context.Background` and ignore the error, as none can occur. In contrast to the previous approach, we lose the differentiation between read-only and read-write locks, but even for writing the locks are very short-lived, so it doesn't matter.
(If we only had unit tests for the controllers...) Locking `CurrentRequestByDatacenter` in the cluster scope is better than doing in the machine scope. I had that first, but the code looked strange and unrelated when suddenly patching the cluster after doing things in the machine scope. That's why the cluster scope contains a locker now. However, that scope is used from the machine controller. It cannot (and should not) access the cluster controller, so when building the cluster scope, the machine controller needs to pass its own locker. At the same time, the cluster controller also creates a cluster scope and is now required to pass a locker, even though it doesn't actually need one for its own work. An alternative could be having a controller package-wide locker (global variable)
Same for the `SetupWithManager` methods. I'm not convinced about this, but it's in-line with other providers.
lubedacht
reviewed
May 31, 2024
|
lubedacht
approved these changes
Jun 5, 2024
Mattes83
pushed a commit
to Mattes83/cluster-api-provider-ionoscloud
that referenced
this pull request
Jun 7, 2024
Improve performance by having more worker goroutines per controller. Besides the actual flags for specifying the number of concurrent reconciles per controller, this change adds locks as needed for thread safety. This applies to 3 kinds of operations: - Creating, updating and deleting LANs, as they are shared across machines. - IP block creation and deletion for machine failover IPs, as they are shared across machines. - Accessing the `IonosCloudCluster.Status.CurrentRequestByDatacenter` map, as it is shared across machines. For locking, a new `locker` package was added.
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.
What is the purpose of this pull request/Why do we need it?
Improve performance by having more worker goroutines per controller.
Description of changes:
Besides the actual flags for specifying the number of concurrent reconciles per controller, this change adds locks as needed for thread safety.
This applies to 3 kinds of operations:
IonosCloudCluster.Status.CurrentRequestByDatacentermap, as it is shared across machines.For locking, a new
lockerpackage was added.Special notes for your reviewer:
Reviewing commit by commit should be the easiest way.
Checklist: