Sync from open-cluster-management-io/governance-policy-propagator: #130 #459
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)
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)
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)
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)
Signed-off-by: Justin Kulikauskas <[email protected]> (cherry picked from commit dfde80f)
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)
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)
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)
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)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
It calls them "Implicit memory aliasing in for loop" Signed-off-by: Justin Kulikauskas <[email protected]>
Signed-off-by: Justin Kulikauskas <[email protected]>
|
It looks like this will need additional fixes which we'll have to push back up to ocm-io somehow. |
|
Kudos, SonarCloud Quality Gate passed! |
mprahl
approved these changes
Oct 11, 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 |
|
/cherry-pick release-2.9 |
|
@JustinKuli: #459 failed to apply on top of branch "release-2.9": In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick release-2.9 |
|
@JustinKuli: #459 failed to apply on top of branch "release-2.9": In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
Syncing open-cluster-management-io/governance-policy-propagator#130.
Closes #455