[release-2.9] 🥸 Sync from open-cluster-management-io/governance-policy-propagator: #130 #462
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 commit only moves some code between files, it does change any content. Signed-off-by: Justin Kulikauskas <[email protected]> (cherry picked from commit 81284e9) (cherry picked from commit 52484a7)
This adds some documentation, as well as renames the functions to be a bit more descriptive. Most code changes are only stylistic or to help them match other similar functions. Signed-off-by: Justin Kulikauskas <[email protected]> (cherry picked from commit 594df54) (cherry picked from commit 4762d9d)
The new Propagator type has (almost) all the methods that were on the PolicyReconciler - the only exception is `Reconcile`, which is on the new RootPolicyReconciler type. This should help organize things to make a new ReplicatedPolicyReconciler type. Signed-off-by: Justin Kulikauskas <[email protected]> (cherry picked from commit 6b98674) (cherry picked from commit fd24168)
This new reconciler currently handles when a replicated policy changes unexpectedly, and is triggered by events coming from the root policy reconciler. This removes the custom concurrency for replicated policy creation and deletion, and the concurrency here will be adjustable via the more usual `MaxConcurrentReconciles` controller option. Because the replicated policy reconciler changes the same resources that trigger its reconciliation, it often reconciles the same resource twice in a row. That needs to be addressed for this to be more performant than the previous implementation. When running the tests, it became apparent that the encryptionkeys controller triggers reconciliation on the replicated policies by adding an annotation to the root policy. This is another place for possible improvements, it can likely also trigger replicated policy reconciles directly. Refs: - https://issues.redhat.com/browse/ACM-7332 Signed-off-by: Justin Kulikauskas <[email protected]> (cherry picked from commit 3bd0d29) (cherry picked from commit 0496315)
Signed-off-by: Justin Kulikauskas <[email protected]> (cherry picked from commit dfde80f) (cherry picked from commit c3bfd4c)
This should help answer some performance questions in the propagator. These metrics are not as good as running a real performance tests, but they will provide some data, from tests we already run regularly. Signed-off-by: Justin Kulikauskas <[email protected]> (cherry picked from commit 35d5aa2) (cherry picked from commit 225880f)
Based on metrics from the e2e tests, this may reduce the number of replicated policy reconciles by 35%. Events from the dependency watcher are artificially delayed on the replicated policy reconcile queue - the watch there and a watch for the controller-runtime cache on ManagedCluster labels will race each other, and if the cache is old during the reconcile, the needed update will be missed. This is not the best way to resolve the race: it would be better to ensure that both use the same watch or cache somehow. Refs: - https://issues.redhat.com/browse/ACM-7332 Signed-off-by: Justin Kulikauskas <[email protected]> update resource version cache (cherry picked from commit 76f7ff6) (cherry picked from commit bf1edc0)
This increases the root policy concurrency from 1 to 2, and makes it configurable. It also adds concurrency to the replicated policy - the default is 10, up from the concurrency it replaces which defaulted to 5. Some refactoring was done to have all reconciler concurrencies be configured uniformly. Signed-off-by: Justin Kulikauskas <[email protected]> (cherry picked from commit aa643c0) (cherry picked from commit 3651c4f)
The bigger change is that the status is now updated before sending the events to the replicated policy reconciler - this should help prevent some requeues. Otherwise, these are largely just organizational changes. Signed-off-by: Justin Kulikauskas <[email protected]> (cherry picked from commit b2aa21d) (cherry picked from commit 56b9c23)
It calls them "Implicit memory aliasing in for loop" Signed-off-by: Justin Kulikauskas <[email protected]> (cherry picked from commit daa7599)
Signed-off-by: Justin Kulikauskas <[email protected]> (cherry picked from commit bbe0749)
|
Kudos, SonarCloud Quality Gate passed! |
mprahl
approved these changes
Oct 17, 2023
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JustinKuli, mprahl The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
This is a manual cherry-pick of #459