From db38835460139a190e6bdd55cadfc2399ea2fb0d Mon Sep 17 00:00:00 2001 From: Jeffrey Luo Date: Tue, 17 Oct 2023 10:11:15 -0400 Subject: [PATCH] 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 --- controllers/common/policyset_handler.go | 114 +++++++++++++++++++++ controllers/propagator/rootpolicy_setup.go | 48 +-------- 2 files changed, 115 insertions(+), 47 deletions(-) create mode 100644 controllers/common/policyset_handler.go diff --git a/controllers/common/policyset_handler.go b/controllers/common/policyset_handler.go new file mode 100644 index 00000000..aff32046 --- /dev/null +++ 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 index 2ec7d396..aadd7fee 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{}, - 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 {