ACM-7397: Reduce the reconciles caused from policy set updates

[JeffeyL]

Currently, a reconcile request is made for every single policy in the policy set on update. A custom event handler was added to only reconcile the differing policies between the new and old policy sets. policySetPredicateFuncs was also removed, and the logic was added to the policySetMapper function.

Ref: https://issues.redhat.com/browse/ACM-7397

[mprahl]

This is policyset specific. Could you please rename it to indicate that?

[mprahl]

If I'm reading this correctly, diffPolicies only contains policies that were previously in the PolicySet where as it should contain policies that are no longer in the old PolicySet and the new policies in the new PolicySet.

[mprahl]

This isn't needed since Go can range over an empty slice.

[mprahl]

Is this required? It seems to me that the event handler could just directly enqueue the policies rather create a fake PolicySet for the policySetMapper.

[JeffeyL]

I wasn't able to figure out how to directly enqueue the policies, as they are of type NonEmptyString and I couldn't figure out how to convert them to policyv1.Policy type.

[JustinKuli]

I think this still needs some changes from Matt's comments, but also just let me know if you need any help rebasing this on top of the recent changes. I re-organized all the mappers and things, which will probably get in your way :/

[JeffeyL]

Reopen after syncing changes

[mprahl]

I think it'd be cleaner if you did something like this (untested):

diff --git a/controllers/common/policyset_handler.go b/controllers/common/policyset_handler.go
index bf4f676..e6ccbf4 100644
--- a/controllers/common/policyset_handler.go
+++ b/controllers/common/policyset_handler.go
@@ -7,41 +7,55 @@ import (
        "context"
 
        // "k8s.io/apimachinery/pkg/api/equality"
+
+       "k8s.io/apimachinery/pkg/types"
        "k8s.io/client-go/util/workqueue"
        "sigs.k8s.io/controller-runtime/pkg/client"
        "sigs.k8s.io/controller-runtime/pkg/event"
-       "sigs.k8s.io/controller-runtime/pkg/handler"
+       "sigs.k8s.io/controller-runtime/pkg/reconcile"
 
        policiesv1beta1 "open-cluster-management.io/governance-policy-propagator/api/v1beta1"
 )
 
-var _ handler.EventHandler = &enqueueRequestsFromPolicySetMapFunc{}
+// EnqueueRequestsFromPolicySet adds reconcile requests for every policy in the policy set, except on updates, it'll
+// only return the diff.
+type EnqueueRequestsFromPolicySet struct{}
+
+// mapPolicySetToPolicies maps a PolicySet to all the Policies in its policies list.
+func mapPolicySetToRequests(object client.Object) []reconcile.Request {
+       log := log.WithValues("policySetName", object.GetName(), "namespace", object.GetNamespace())
+       log.V(2).Info("Reconcile Request for PolicySet")
 
-// EnqueueRequestsFromMapFunc same as original EnqueueRequestsFromMapFunc
-// execept this only queues the diff between the new and old objects
-func EnqueueRequestsFromPolicySetMapFunc(fn handler.MapFunc) handler.EventHandler {
-       return &enqueueRequestsFromPolicySetMapFunc{
-               toRequests: fn,
+       var result []reconcile.Request
+
+       //nolint:forcetypeassert
+       policySet := object.(*policiesv1beta1.PolicySet)
+
+       for _, plc := range policySet.Spec.Policies {
+               log.V(2).Info("Found reconciliation request from a policyset", "policyName", string(plc))
+
+               request := reconcile.Request{NamespacedName: types.NamespacedName{
+                       Name:      string(plc),
+                       Namespace: object.GetNamespace(),
+               }}
+               result = append(result, request)
        }
-}
 
-// EnqueueRequestsFromMapFunc same as original EnqueueRequestsFromMapFunc
-// execept this doesn't queue old object for update
-type enqueueRequestsFromPolicySetMapFunc struct {
-       // Mapper transforms the argument into a slice of keys to be reconciled
-       toRequests handler.MapFunc
+       return result
 }
 
 // Create implements EventHandler
-func (e *enqueueRequestsFromPolicySetMapFunc) Create(ctx context.Context, evt event.CreateEvent,
+func (e *EnqueueRequestsFromPolicySet) Create(ctx context.Context, evt event.CreateEvent,
        q workqueue.RateLimitingInterface,
 ) {
-       e.mapAndEnqueue(ctx, q, evt.Object)
+       for _, policy := range mapPolicySetToRequests(evt.Object) {
+               q.Add(policy)
+       }
 }
 
 // Update implements EventHandler
 // Enqueues the diff between the new and old policy sets in the UpdateEvent
-func (e *enqueueRequestsFromPolicySetMapFunc) Update(ctx context.Context, evt event.UpdateEvent,
+func (e *EnqueueRequestsFromPolicySet) Update(ctx context.Context, evt event.UpdateEvent,
        q workqueue.RateLimitingInterface,
 ) {
        //nolint:forcetypeassert
@@ -73,33 +87,31 @@ func (e *enqueueRequestsFromPolicySetMapFunc) Update(ctx context.Context, evt ev
                }
        }
 
-       diffPolicySet := policiesv1beta1.PolicySet{Spec: policiesv1beta1.PolicySetSpec{
-               Policies: diffPolicies,
-       }}
+       for _, plc := range diffPolicies {
+               log.V(2).Info("Found reconciliation request from a policyset", "policyName", string(plc))
 
-       e.mapAndEnqueue(ctx, q, &diffPolicySet)
+               request := reconcile.Request{NamespacedName: types.NamespacedName{
+                       Name:      string(plc),
+                       Namespace: newPolicySet.GetNamespace(),
+               }}
+               q.Add(request)
+       }
 }
 
 // Delete implements EventHandler
-func (e *enqueueRequestsFromPolicySetMapFunc) Delete(ctx context.Context, evt event.DeleteEvent,
+func (e *EnqueueRequestsFromPolicySet) Delete(ctx context.Context, evt event.DeleteEvent,
        q workqueue.RateLimitingInterface,
 ) {
-       e.mapAndEnqueue(ctx, q, evt.Object)
+       for _, policy := range mapPolicySetToRequests(evt.Object) {
+               q.Add(policy)
+       }
 }
 
 // Generic implements EventHandler
-func (e *enqueueRequestsFromPolicySetMapFunc) Generic(ctx context.Context, evt event.GenericEvent,
+func (e *EnqueueRequestsFromPolicySet) Generic(ctx context.Context, evt event.GenericEvent,
        q workqueue.RateLimitingInterface,
 ) {
-       e.mapAndEnqueue(ctx, q, evt.Object)
-}
-
-func (e *enqueueRequestsFromPolicySetMapFunc) mapAndEnqueue(ctx context.Context, q workqueue.RateLimitingInterface,
-       object client.Object,
-) {
-       for _, req := range e.toRequests(ctx, object) {
-               q.Add(req)
+       for _, policy := range mapPolicySetToRequests(evt.Object) {
+               q.Add(policy)
        }
 }
-
-// var NeverEnqueue = predicate.NewPredicateFuncs(func(o client.Object) bool { return false })
diff --git a/controllers/propagator/rootpolicy_setup.go b/controllers/propagator/rootpolicy_setup.go
index 5081738..aadd7fe 100644
--- a/controllers/propagator/rootpolicy_setup.go
+++ b/controllers/propagator/rootpolicy_setup.go
@@ -7,7 +7,6 @@ import (
        "context"
 
        "k8s.io/apimachinery/pkg/api/equality"
-       "k8s.io/apimachinery/pkg/types"
        clusterv1beta1 "open-cluster-management.io/api/cluster/v1beta1"
        appsv1 "open-cluster-management.io/multicloud-operators-subscription/pkg/apis/apps/placementrule/v1"
        ctrl "sigs.k8s.io/controller-runtime"
@@ -44,8 +43,7 @@ func (r *RootPolicyReconciler) SetupWithManager(mgr ctrl.Manager, maxConcurrentR
                        builder.WithPredicates(rootPolicyNonStatusUpdates())).
                Watches(
                        &policiesv1beta1.PolicySet{},
-                       common.EnqueueRequestsFromPolicySetMapFunc(mapPolicySetToPolicies),
-                       builder.WithPredicates(policySetPolicyListChanged())).
+                       &common.EnqueueRequestsFromPolicySet{}).
                Watches(
                        &policiesv1.PlacementBinding{},
                        handler.EnqueueRequestsFromMapFunc(mapBindingToPolicies(mgr.GetClient())),
@@ -90,50 +88,6 @@ func rootPolicyNonStatusUpdates() predicate.Funcs {
        }
 }
 
-// mapPolicySetToPolicies maps a PolicySet to all the Policies in its policies list.
-func mapPolicySetToPolicies(_ context.Context, object client.Object) []reconcile.Request {
-       log := log.WithValues("policySetName", object.GetName(), "namespace", object.GetNamespace())
-       log.V(2).Info("Reconcile Request for PolicySet")
-
-       var result []reconcile.Request
-
-       //nolint:forcetypeassert
-       policySet := object.(*policiesv1beta1.PolicySet)
-
-       for _, plc := range policySet.Spec.Policies {
-               log.V(2).Info("Found reconciliation request from a policyset", "policyName", string(plc))
-
-               request := reconcile.Request{NamespacedName: types.NamespacedName{
-                       Name:      string(plc),
-                       Namespace: object.GetNamespace(),
-               }}
-               result = append(result, request)
-       }
-
-       return result
-}
-
-// policySetPolicyListChanged triggers reconciliation if the list of policies in the PolicySet has
-// changed, or if the PolicySet was just created or deleted.
-func policySetPolicyListChanged() predicate.Funcs {
-       return predicate.Funcs{
-               UpdateFunc: func(e event.UpdateEvent) bool {
-                       //nolint:forcetypeassert
-                       policySetObjNew := e.ObjectNew.(*policiesv1beta1.PolicySet)
-                       //nolint:forcetypeassert
-                       policySetObjOld := e.ObjectOld.(*policiesv1beta1.PolicySet)
-
-                       return !equality.Semantic.DeepEqual(policySetObjNew.Spec.Policies, policySetObjOld.Spec.Policies)
-               },
-               CreateFunc: func(e event.CreateEvent) bool {
-                       return true
-               },
-               DeleteFunc: func(e event.DeleteEvent) bool {
-                       return true
-               },
-       }
-}
-
 // mapBindingToPolicies maps a PlacementBinding to the Policies that are either directly in its
 // subjects list, or are in a PolicySet which is a subject of this PlacementBinding.
 func mapBindingToPolicies(c client.Client) handler.MapFunc {

[mprahl]

@JeffeyL, this looks great! Just a few comments to update/remove.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JeffeyL, 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:

• OWNERS [mprahl]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment