Change policy set updates to use diff between new and old sets

Currently, a reconcile request is made for every single policy in the policy set on update. A custome event handler was added to only reconcile the differing policies between the new and old policy sets. ref: https://issues.redhat.com/browse/ACM-7397 Signed-off-by: Jeffrey Luo <[email protected]>
open-cluster-management-io · Oct 19, 2023 · db38835 · db38835
1 parent df22cc2
commit db38835
Show file tree

Hide file tree

Showing 2 changed files with 115 additions and 47 deletions.
diff --git a/controllers/common/policyset_handler.go b/controllers/common/policyset_handler.go
@@ -0,0 +1,114 @@
+// Copyright Contributors to the Open Cluster Management project
+
+package common
+
+import (
+	"context"
+
+	"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/reconcile"
+
+	policiesv1beta1 "open-cluster-management.io/governance-policy-propagator/api/v1beta1"
+)
+
+// EnqueueRequestsFromPolicySet adds reconcile requests for every policy in the policy set,
+// except on updates, it'll only add the diff between the old and new sets.
+type EnqueueRequestsFromPolicySet struct{}
+
+// mapPolicySetToRequests 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")
+
+	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
+}
+
+// Create implements EventHandler
+func (e *EnqueueRequestsFromPolicySet) Create(_ context.Context, evt event.CreateEvent,
+	q workqueue.RateLimitingInterface,
+) {
+	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 *EnqueueRequestsFromPolicySet) Update(_ context.Context, evt event.UpdateEvent,
+	q workqueue.RateLimitingInterface,
+) {
+	//nolint:forcetypeassert
+	newPolicySet := evt.ObjectNew.(*policiesv1beta1.PolicySet)
+	//nolint:forcetypeassert
+	oldPolicySet := evt.ObjectOld.(*policiesv1beta1.PolicySet)
+
+	newPoliciesMap := make(map[string]bool)
+	oldPoliciesMap := make(map[string]bool)
+	diffPolicies := []policiesv1beta1.NonEmptyString{}
+
+	for _, plc := range newPolicySet.Spec.Policies {
+		newPoliciesMap[string(plc)] = true
+	}
+
+	for _, plc := range oldPolicySet.Spec.Policies {
+		oldPoliciesMap[string(plc)] = true
+	}
+
+	for _, plc := range oldPolicySet.Spec.Policies {
+		if !newPoliciesMap[string(plc)] {
+			diffPolicies = append(diffPolicies, plc)
+		}
+	}
+
+	for _, plc := range newPolicySet.Spec.Policies {
+		if !oldPoliciesMap[string(plc)] {
+			diffPolicies = append(diffPolicies, plc)
+		}
+	}
+
+	for _, plc := range diffPolicies {
+		log.V(2).Info("Found reconciliation request from a policyset", "policyName", string(plc))
+
+		request := reconcile.Request{NamespacedName: types.NamespacedName{
+			Name:      string(plc),
+			Namespace: newPolicySet.GetNamespace(),
+		}}
+		q.Add(request)
+	}
+}
+
+// Delete implements EventHandler
+func (e *EnqueueRequestsFromPolicySet) Delete(_ context.Context, evt event.DeleteEvent,
+	q workqueue.RateLimitingInterface,
+) {
+	for _, policy := range mapPolicySetToRequests(evt.Object) {
+		q.Add(policy)
+	}
+}
+
+// Generic implements EventHandler
+func (e *EnqueueRequestsFromPolicySet) Generic(_ context.Context, evt event.GenericEvent,
+	q workqueue.RateLimitingInterface,
+) {
+	for _, policy := range mapPolicySetToRequests(evt.Object) {
+		q.Add(policy)
+	}
+}
diff --git 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{},
-			handler.EnqueueRequestsFromMapFunc(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 {