From 2ca74fa30a622a40163d411f0e44a5df24a8f7de Mon Sep 17 00:00:00 2001 From: changzhen Date: Thu, 25 Sep 2025 20:17:03 +0800 Subject: [PATCH] reduce unnecessary watch events and bindingSpec change in the binding controller Signed-off-by: changzhen --- pkg/controllers/binding/binding_controller.go | 25 +- .../cluster_resource_binding_controller.go | 24 +- pkg/controllers/binding/common.go | 36 ++ pkg/controllers/binding/common_test.go | 371 ++++++++++++++++++ 4 files changed, 454 insertions(+), 2 deletions(-) diff --git a/pkg/controllers/binding/binding_controller.go b/pkg/controllers/binding/binding_controller.go index 536c8bea7402..af87c931403e 100644 --- a/pkg/controllers/binding/binding_controller.go +++ b/pkg/controllers/binding/binding_controller.go @@ -31,9 +31,11 @@ import ( "k8s.io/klog/v2" "k8s.io/utils/ptr" controllerruntime "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -185,9 +187,30 @@ func (c *ResourceBindingController) checkDirectPurgeOrphanWorks(ctx context.Cont // SetupWithManager creates a controller and register to controller manager. func (c *ResourceBindingController) SetupWithManager(mgr controllerruntime.Manager) error { + bindingPredicateFn := predicate.Funcs{ + CreateFunc: func(_ event.CreateEvent) bool { + return true + }, + UpdateFunc: func(updateEvent event.UpdateEvent) bool { + oldBinding := updateEvent.ObjectOld.(*workv1alpha2.ResourceBinding) + newBinding := updateEvent.ObjectNew.(*workv1alpha2.ResourceBinding) + if oldBinding.DeletionTimestamp != newBinding.DeletionTimestamp { + return true + } + return careBindingSpecChanged(oldBinding.Spec, newBinding.Spec) + }, + DeleteFunc: func(_ event.DeleteEvent) bool { + // The binding object has a finalizer. When the deletion timestamp is not nil, + // it means the object is being deleted and the controller will handle the + // deletion process, so the delete event can be ignored. + return false + }, + GenericFunc: func(_ event.GenericEvent) bool { return false }, + } + return controllerruntime.NewControllerManagedBy(mgr). Named(ControllerName). - For(&workv1alpha2.ResourceBinding{}). + For(&workv1alpha2.ResourceBinding{}, builder.WithPredicates(bindingPredicateFn)). WithEventFilter(predicate.GenerationChangedPredicate{}). Watches(&policyv1alpha1.OverridePolicy{}, handler.EnqueueRequestsFromMapFunc(c.newOverridePolicyFunc())). Watches(&policyv1alpha1.ClusterOverridePolicy{}, handler.EnqueueRequestsFromMapFunc(c.newOverridePolicyFunc())). diff --git a/pkg/controllers/binding/cluster_resource_binding_controller.go b/pkg/controllers/binding/cluster_resource_binding_controller.go index 8acca3a3d886..04499958234f 100644 --- a/pkg/controllers/binding/cluster_resource_binding_controller.go +++ b/pkg/controllers/binding/cluster_resource_binding_controller.go @@ -31,9 +31,11 @@ import ( "k8s.io/klog/v2" "k8s.io/utils/ptr" controllerruntime "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -183,9 +185,29 @@ func (c *ClusterResourceBindingController) checkDirectPurgeOrphanWorks(ctx conte // SetupWithManager creates a controller and register to controller manager. func (c *ClusterResourceBindingController) SetupWithManager(mgr controllerruntime.Manager) error { + bindingPredicateFn := predicate.Funcs{ + CreateFunc: func(_ event.CreateEvent) bool { + return true + }, + UpdateFunc: func(updateEvent event.UpdateEvent) bool { + oldBinding := updateEvent.ObjectOld.(*workv1alpha2.ClusterResourceBinding) + newBinding := updateEvent.ObjectNew.(*workv1alpha2.ClusterResourceBinding) + if oldBinding.DeletionTimestamp != newBinding.DeletionTimestamp { + return true + } + return careBindingSpecChanged(oldBinding.Spec, newBinding.Spec) + }, + DeleteFunc: func(_ event.DeleteEvent) bool { + // The binding object has a finalizer. When the deletion timestamp is not nil, + // it means the object is being deleted and the controller will handle the + // deletion process, so the delete event can be ignored. + return false + }, + GenericFunc: func(_ event.GenericEvent) bool { return false }, + } return controllerruntime.NewControllerManagedBy(mgr). Named(ClusterResourceBindingControllerName). - For(&workv1alpha2.ClusterResourceBinding{}). + For(&workv1alpha2.ClusterResourceBinding{}, builder.WithPredicates(bindingPredicateFn)). Watches(&policyv1alpha1.ClusterOverridePolicy{}, handler.EnqueueRequestsFromMapFunc(c.newOverridePolicyFunc())). WithEventFilter(predicate.GenerationChangedPredicate{}). WithOptions(controller.Options{RateLimiter: ratelimiterflag.DefaultControllerRateLimiter[controllerruntime.Request](c.RateLimiterOptions)}). diff --git a/pkg/controllers/binding/common.go b/pkg/controllers/binding/common.go index a56492c7b253..98814f405eef 100644 --- a/pkg/controllers/binding/common.go +++ b/pkg/controllers/binding/common.go @@ -18,6 +18,7 @@ package binding import ( "context" + "reflect" "strconv" "time" @@ -354,3 +355,38 @@ func shouldSuspendDispatching(suspension *workv1alpha2.Suspension, targetCluster } return suspendDispatching } + +type bindingSpecDigest struct { + Resource workv1alpha2.ObjectReference + Clusters []workv1alpha2.TargetCluster + GracefulEvictionTasks []workv1alpha2.GracefulEvictionTask + RequiredBy []workv1alpha2.BindingSnapshot + ConflictResolution policyv1alpha1.ConflictResolution + Suspension *workv1alpha2.Suspension + PreserveResourcesOnDeletion *bool +} + +// careBindingSpecChanged checks whether the fields in bindingSpec that the binding controller cares about have changed. +// This helps avoid triggering the binding controller to resync when irrelevant fields are modified. +func careBindingSpecChanged(oldBindingSpec, newBindingSpec workv1alpha2.ResourceBindingSpec) bool { + oldDigest := bindingSpecDigest{ + Resource: oldBindingSpec.Resource, + Clusters: oldBindingSpec.Clusters, + GracefulEvictionTasks: oldBindingSpec.GracefulEvictionTasks, + RequiredBy: oldBindingSpec.RequiredBy, + ConflictResolution: oldBindingSpec.ConflictResolution, + Suspension: oldBindingSpec.Suspension, + PreserveResourcesOnDeletion: oldBindingSpec.PreserveResourcesOnDeletion, + } + newDigest := bindingSpecDigest{ + Resource: newBindingSpec.Resource, + Clusters: newBindingSpec.Clusters, + GracefulEvictionTasks: newBindingSpec.GracefulEvictionTasks, + RequiredBy: newBindingSpec.RequiredBy, + ConflictResolution: newBindingSpec.ConflictResolution, + Suspension: newBindingSpec.Suspension, + PreserveResourcesOnDeletion: newBindingSpec.PreserveResourcesOnDeletion, + } + + return !reflect.DeepEqual(oldDigest, newDigest) +} diff --git a/pkg/controllers/binding/common_test.go b/pkg/controllers/binding/common_test.go index 0bd53292d4eb..072e9e136b20 100644 --- a/pkg/controllers/binding/common_test.go +++ b/pkg/controllers/binding/common_test.go @@ -20,6 +20,7 @@ import ( "reflect" "sort" "testing" + "time" v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -527,3 +528,373 @@ func Test_divideReplicasByJobCompletions(t *testing.T) { }) } } + +func Test_careBindingSpecChanged(t *testing.T) { + // Create a base ResourceBindingSpec for testing + baseTime := metav1.Now() + baseSpec := workv1alpha2.ResourceBindingSpec{ + Resource: workv1alpha2.ObjectReference{ + APIVersion: "apps/v1", + Kind: "Deployment", + Namespace: "default", + Name: "test-deployment", + }, + Clusters: []workv1alpha2.TargetCluster{ + {Name: "cluster1", Replicas: 1}, + {Name: "cluster2", Replicas: 2}, + }, + GracefulEvictionTasks: []workv1alpha2.GracefulEvictionTask{ + { + FromCluster: "cluster3", + PurgeMode: policyv1alpha1.PurgeModeGracefully, + Reason: workv1alpha2.EvictionReasonTaintUntolerated, + Message: "test eviction", + Producer: workv1alpha2.EvictionProducerTaintManager, + }, + }, + RequiredBy: []workv1alpha2.BindingSnapshot{ + { + Namespace: "test-ns", + Name: "test-binding", + Clusters: []workv1alpha2.TargetCluster{ + {Name: "cluster4", Replicas: 1}, + }, + }, + }, + ConflictResolution: policyv1alpha1.ConflictAbort, + Suspension: &workv1alpha2.Suspension{ + Suspension: policyv1alpha1.Suspension{ + Dispatching: ptr.To(true), + }, + }, + PreserveResourcesOnDeletion: ptr.To(false), + RescheduleTriggeredAt: &baseTime, + } + + tests := []struct { + name string + oldBindingSpec workv1alpha2.ResourceBindingSpec + newBindingSpec workv1alpha2.ResourceBindingSpec + want bool + }{ + { + name: "identical specs", + oldBindingSpec: baseSpec, + newBindingSpec: baseSpec, + want: false, + }, + { + name: "Resource field changed - APIVersion", + oldBindingSpec: baseSpec, + newBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.Resource.APIVersion = "apps/v2" + return spec + }(), + want: true, + }, + { + name: "Resource field changed - Kind", + oldBindingSpec: baseSpec, + newBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.Resource.Kind = "StatefulSet" + return spec + }(), + want: true, + }, + { + name: "Resource field changed - Namespace", + oldBindingSpec: baseSpec, + newBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.Resource.Namespace = "other-namespace" + return spec + }(), + want: true, + }, + { + name: "Resource field changed - Name", + oldBindingSpec: baseSpec, + newBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.Resource.Name = "other-deployment" + return spec + }(), + want: true, + }, + { + name: "Clusters field changed - different cluster name", + oldBindingSpec: baseSpec, + newBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.Clusters = []workv1alpha2.TargetCluster{ + {Name: "cluster1", Replicas: 1}, + {Name: "cluster3", Replicas: 2}, // cluster2 -> cluster3 + } + return spec + }(), + want: true, + }, + { + name: "Clusters field changed - different replicas", + oldBindingSpec: baseSpec, + newBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.Clusters = []workv1alpha2.TargetCluster{ + {Name: "cluster1", Replicas: 1}, + {Name: "cluster2", Replicas: 3}, // 2 -> 3 + } + return spec + }(), + want: true, + }, + { + name: "Clusters field changed - different length", + oldBindingSpec: baseSpec, + newBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.Clusters = []workv1alpha2.TargetCluster{ + {Name: "cluster1", Replicas: 1}, + } + return spec + }(), + want: true, + }, + { + name: "Clusters field changed - empty to non-empty", + oldBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.Clusters = []workv1alpha2.TargetCluster{} + return spec + }(), + newBindingSpec: baseSpec, + want: true, + }, + { + name: "GracefulEvictionTasks field changed - different FromCluster", + oldBindingSpec: baseSpec, + newBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.GracefulEvictionTasks = []workv1alpha2.GracefulEvictionTask{ + { + FromCluster: "cluster4", // cluster3 -> cluster4 + PurgeMode: policyv1alpha1.PurgeModeGracefully, + Reason: workv1alpha2.EvictionReasonTaintUntolerated, + Message: "test eviction", + Producer: workv1alpha2.EvictionProducerTaintManager, + }, + } + return spec + }(), + want: true, + }, + { + name: "GracefulEvictionTasks field changed - different PurgeMode", + oldBindingSpec: baseSpec, + newBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.GracefulEvictionTasks = []workv1alpha2.GracefulEvictionTask{ + { + FromCluster: "cluster3", + PurgeMode: policyv1alpha1.PurgeModeDirectly, // PurgeModeGracefully -> PurgeModeDirectly + Reason: workv1alpha2.EvictionReasonTaintUntolerated, + Message: "test eviction", + Producer: workv1alpha2.EvictionProducerTaintManager, + }, + } + return spec + }(), + want: true, + }, + { + name: "GracefulEvictionTasks field changed - empty to non-empty", + oldBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.GracefulEvictionTasks = []workv1alpha2.GracefulEvictionTask{} + return spec + }(), + newBindingSpec: baseSpec, + want: true, + }, + { + name: "RequiredBy field changed - different namespace", + oldBindingSpec: baseSpec, + newBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.RequiredBy = []workv1alpha2.BindingSnapshot{ + { + Namespace: "other-ns", // test-ns -> other-ns + Name: "test-binding", + Clusters: []workv1alpha2.TargetCluster{ + {Name: "cluster4", Replicas: 1}, + }, + }, + } + return spec + }(), + want: true, + }, + { + name: "RequiredBy field changed - different name", + oldBindingSpec: baseSpec, + newBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.RequiredBy = []workv1alpha2.BindingSnapshot{ + { + Namespace: "test-ns", + Name: "other-binding", // test-binding -> other-binding + Clusters: []workv1alpha2.TargetCluster{ + {Name: "cluster4", Replicas: 1}, + }, + }, + } + return spec + }(), + want: true, + }, + { + name: "RequiredBy field changed - different clusters", + oldBindingSpec: baseSpec, + newBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.RequiredBy = []workv1alpha2.BindingSnapshot{ + { + Namespace: "test-ns", + Name: "test-binding", + Clusters: []workv1alpha2.TargetCluster{ + {Name: "cluster5", Replicas: 2}, // cluster4 -> cluster5, 1 -> 2 + }, + }, + } + return spec + }(), + want: true, + }, + { + name: "RequiredBy field changed - empty to non-empty", + oldBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.RequiredBy = []workv1alpha2.BindingSnapshot{} + return spec + }(), + newBindingSpec: baseSpec, + want: true, + }, + { + name: "ConflictResolution field changed", + oldBindingSpec: baseSpec, + newBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.ConflictResolution = policyv1alpha1.ConflictOverwrite // ConflictAbort -> ConflictOverwrite + return spec + }(), + want: true, + }, + { + name: "Suspension field changed - nil to non-nil", + oldBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.Suspension = nil + return spec + }(), + newBindingSpec: baseSpec, + want: true, + }, + { + name: "Suspension field changed - non-nil to nil", + oldBindingSpec: baseSpec, + newBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.Suspension = nil + return spec + }(), + want: true, + }, + { + name: "Suspension field changed - Dispatching value", + oldBindingSpec: baseSpec, + newBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.Suspension = &workv1alpha2.Suspension{ + Suspension: policyv1alpha1.Suspension{ + Dispatching: ptr.To(false), // true -> false + }, + } + return spec + }(), + want: true, + }, + { + name: "PreserveResourcesOnDeletion field changed - false to true", + oldBindingSpec: baseSpec, + newBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.PreserveResourcesOnDeletion = ptr.To(true) // false -> true + return spec + }(), + want: true, + }, + { + name: "PreserveResourcesOnDeletion field changed - nil to non-nil", + oldBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.PreserveResourcesOnDeletion = nil + return spec + }(), + newBindingSpec: baseSpec, + want: true, + }, + { + name: "PreserveResourcesOnDeletion field changed - non-nil to nil", + oldBindingSpec: baseSpec, + newBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.PreserveResourcesOnDeletion = nil + return spec + }(), + want: true, + }, + { + name: "irrelevant field changed - RescheduleTriggeredAt (should not trigger)", + oldBindingSpec: baseSpec, + newBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + newTime := metav1.NewTime(baseTime.Add(1 * time.Hour)) + spec.RescheduleTriggeredAt = &newTime + return spec + }(), + want: false, // RescheduleTriggeredAt is not a field that careBindingSpecChanged cares about + }, + { + name: "irrelevant field changed - Replicas (should not trigger)", + oldBindingSpec: baseSpec, + newBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.Replicas = 10 // Added Replicas field + return spec + }(), + want: false, // Replicas is not a field that careBindingSpecChanged cares about + }, + { + name: "multiple relevant fields changed", + oldBindingSpec: baseSpec, + newBindingSpec: func() workv1alpha2.ResourceBindingSpec { + spec := baseSpec + spec.Resource.Name = "other-deployment" + spec.ConflictResolution = policyv1alpha1.ConflictOverwrite + spec.PreserveResourcesOnDeletion = ptr.To(true) + return spec + }(), + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := careBindingSpecChanged(tt.oldBindingSpec, tt.newBindingSpec); got != tt.want { + t.Errorf("careBindingSpecChanged() = %v, want %v", got, tt.want) + } + }) + } +}