NamespaceSelector 'reconciler' to help trigger evaluations #158
Conversation
pkg/common/namespace_selection.go
Outdated
| return sel.hasUpdate | ||
| } | ||
|
|
||
| return true // no selections populated, call Get to populate it |
There was a problem hiding this comment.
I think this should return false since wouldn't this make shouldEvaluatePolicy always return true for those without namespace selectors?
There was a problem hiding this comment.
Let me test it. I think it might still get populated on the first Get.
There was a problem hiding this comment.
Nevermind, I think you're right
There was a problem hiding this comment.
I addressed this in shouldEvaluatePolicy - when the selector wouldn't be used, it doesn't even check if the SelectorReconciler thinks here is an update (avoiding a read lock).
And I added a test for the behavior.
There was a problem hiding this comment.
I think the evaluationInterval CRD field description might need to change to indicate that an updated namespaceSelector result will cause the policy to be reevaluated.
There was a problem hiding this comment.
Also, HasUpdate is only called in shouldEvaluatePolicy. I think it'd be cleaner to just return false by default.
There was a problem hiding this comment.
Well, it's certainly cleaner, since if the map is missing the key, the empty value for the struct is returned, which then has false in the field this is looking for. So now it's a one-liner 😆
I've updated the CRD field description.
JustinKuli
force-pushed
the
6428-ns-evaluations-2
branch
from
569765a
to
19286cc
Compare
|
Still a draft, I want to add some more doc comments, but I think this is functionally very close to done. |
Previously, these settings were not being applied to the config used in 'hosted mode'. This commit also includes a typo fix, and adjusts some settings used by the tests. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
JustinKuli
force-pushed
the
6428-ns-evaluations-2
branch
from
19286cc
to
b5fbfcf
Compare
Previously, the evaluation interval could prevent policies from being applied on new namespaces when they appeared. Now, a new controller watches namespaces on the target cluster, and can signal to the config policy controller when a policy's selected namespaces have changed, so that it can be re-evaluated. A new controller-manager is used in hosted mode, in order to create a cache for the namespaces on the target cluster. This change required some setup adjustments in order to start both managers in this case. Refs: - https://issues.redhat.com/browse/ACM-6428 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
JustinKuli
force-pushed
the
6428-ns-evaluations-2
branch
from
b5fbfcf
to
e53c5fe
Compare
|
[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 |
openshift-merge-robot
merged commit 7e64817
into
open-cluster-management-io:main
Instead of a watch for each policy's namespace selector, this implementation uses a separate controller, watching all the namespaces on the target cluster. (Note: since the existing controller manager was based on the cluster the controller is running on, but this is not always the same as the target cluster, some additional setup was needed.)
Being able to list all of the namespaces on the target cluster is needed for policies that only use Include and Exclude in their NamespaceSelector. Since that list can be cached, it seems nicer to the API server to do the LabelSelector calculations locally, and just have the one watch.
This implementation addresses the limitations noted in #157, and has adjusted the tests accordingly.