From 62be8910e5a592d29f7ec5676fa868a5473fde82 Mon Sep 17 00:00:00 2001 From: rohan-019 Date: Thu, 25 Sep 2025 11:05:16 +0530 Subject: [PATCH] optimizing-binding-controller Signed-off-by: rohan-019 --- pkg/controllers/binding/binding_controller.go | 3 +- pkg/controllers/binding/binding_predicate.go | 88 ++++++ .../binding/binding_predicate_test.go | 288 ++++++++++++++++++ .../cluster_resource_binding_controller.go | 3 +- 4 files changed, 380 insertions(+), 2 deletions(-) create mode 100644 pkg/controllers/binding/binding_predicate.go create mode 100644 pkg/controllers/binding/binding_predicate_test.go diff --git a/pkg/controllers/binding/binding_controller.go b/pkg/controllers/binding/binding_controller.go index 536c8bea7402..5febad0cfcbc 100644 --- a/pkg/controllers/binding/binding_controller.go +++ b/pkg/controllers/binding/binding_controller.go @@ -31,6 +31,7 @@ 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" @@ -187,7 +188,7 @@ func (c *ResourceBindingController) checkDirectPurgeOrphanWorks(ctx context.Cont func (c *ResourceBindingController) SetupWithManager(mgr controllerruntime.Manager) error { return controllerruntime.NewControllerManagedBy(mgr). Named(ControllerName). - For(&workv1alpha2.ResourceBinding{}). + For(&workv1alpha2.ResourceBinding{}, builder.WithPredicates(newBindingPredicate())). WithEventFilter(predicate.GenerationChangedPredicate{}). Watches(&policyv1alpha1.OverridePolicy{}, handler.EnqueueRequestsFromMapFunc(c.newOverridePolicyFunc())). Watches(&policyv1alpha1.ClusterOverridePolicy{}, handler.EnqueueRequestsFromMapFunc(c.newOverridePolicyFunc())). diff --git a/pkg/controllers/binding/binding_predicate.go b/pkg/controllers/binding/binding_predicate.go new file mode 100644 index 000000000000..0030ec704286 --- /dev/null +++ b/pkg/controllers/binding/binding_predicate.go @@ -0,0 +1,88 @@ +/* +Copyright 2025 The Karmada Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package binding + +import ( + "reflect" + + "k8s.io/klog/v2" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/predicate" + + workv1alpha2 "github.com/karmada-io/karmada/pkg/apis/work/v1alpha2" +) + +// hasSignificantSpecFieldChanged checks if any significant spec fields have changed +func hasSignificantSpecFieldChanged(oldObj, newObj interface{}) bool { + var oldSpec, newSpec workv1alpha2.ResourceBindingSpec + + switch oldBinding := oldObj.(type) { + case *workv1alpha2.ResourceBinding: + oldSpec = oldBinding.Spec + case *workv1alpha2.ClusterResourceBinding: + oldSpec = oldBinding.Spec + default: + klog.V(4).InfoS("Unknown object type in predicate", "type", reflect.TypeOf(oldObj)) + return false + } + + switch newBinding := newObj.(type) { + case *workv1alpha2.ResourceBinding: + newSpec = newBinding.Spec + case *workv1alpha2.ClusterResourceBinding: + newSpec = newBinding.Spec + default: + klog.V(4).InfoS("Unknown object type in predicate", "type", reflect.TypeOf(newObj)) + return false + } + + // Check critical fields that require reconciliation + return !reflect.DeepEqual(oldSpec.RequiredBy, newSpec.RequiredBy) || + !reflect.DeepEqual(oldSpec.Clusters, newSpec.Clusters) || + !reflect.DeepEqual(oldSpec.GracefulEvictionTasks, newSpec.GracefulEvictionTasks) || + !reflect.DeepEqual(oldSpec.Resource, newSpec.Resource) || + !reflect.DeepEqual(oldSpec.Suspension, newSpec.Suspension) || + !reflect.DeepEqual(oldSpec.PreserveResourcesOnDeletion, newSpec.PreserveResourcesOnDeletion) || + oldSpec.ConflictResolution != newSpec.ConflictResolution +} + +// newBindingPredicate returns an inline predicate for ResourceBinding and ClusterResourceBinding +// to optimize event filtering and reduce unnecessary reconciliations +func newBindingPredicate() predicate.Predicate { + return predicate.Funcs{ + CreateFunc: func(event.CreateEvent) bool { + return true + }, + UpdateFunc: func(e event.UpdateEvent) bool { + // Trigger reconciliation when deletion starts to ensure timely finalizer/cleanup handling + if e.ObjectOld.GetDeletionTimestamp() != e.ObjectNew.GetDeletionTimestamp() { + return true + } + // Check if any critical spec fields have changed + return hasSignificantSpecFieldChanged(e.ObjectOld, e.ObjectNew) + }, + DeleteFunc: func(event.DeleteEvent) bool { + // Ignore delete events; reconciliation is driven by generation bump and finalizers. + // Cleanup is handled by controller logic (e.g., work deletion). + return false + }, + GenericFunc: func(event.GenericEvent) bool { + // Process generic events (rarely used) + return true + }, + } +} diff --git a/pkg/controllers/binding/binding_predicate_test.go b/pkg/controllers/binding/binding_predicate_test.go new file mode 100644 index 000000000000..f47abf13dbb4 --- /dev/null +++ b/pkg/controllers/binding/binding_predicate_test.go @@ -0,0 +1,288 @@ +/* +Copyright 2025 The Karmada Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package binding + +import ( + "testing" + "time" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/event" + + workv1alpha2 "github.com/karmada-io/karmada/pkg/apis/work/v1alpha2" +) + +func TestBindingPredicate_Create(t *testing.T) { + predicate := newBindingPredicate() + + // Test ResourceBinding create event + rb := &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rb", + Namespace: "default", + }, + } + + createEvent := event.CreateEvent{Object: rb} + if !predicate.Create(createEvent) { + t.Error("Expected Create event to return true") + } + + // Test ClusterResourceBinding create event + crb := &workv1alpha2.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-crb", + }, + } + + createEventCRB := event.CreateEvent{Object: crb} + if !predicate.Create(createEventCRB) { + t.Error("Expected Create event for ClusterResourceBinding to return true") + } +} + +func TestBindingPredicate_Delete(t *testing.T) { + predicate := newBindingPredicate() + + // Test ResourceBinding delete event + rb := &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rb", + Namespace: "default", + }, + } + + deleteEvent := event.DeleteEvent{Object: rb} + if predicate.Delete(deleteEvent) { + t.Error("Expected Delete event to return false") + } + + // Test ClusterResourceBinding delete event + crb := &workv1alpha2.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-crb", + }, + } + + deleteEventCRB := event.DeleteEvent{Object: crb} + if predicate.Delete(deleteEventCRB) { + t.Error("Expected Delete event for ClusterResourceBinding to return false") + } +} + +func TestBindingPredicate_Update(t *testing.T) { + predicate := newBindingPredicate() + + tests := []struct { + name string + oldObj *workv1alpha2.ResourceBinding + newObj *workv1alpha2.ResourceBinding + expected bool + }{ + { + name: "deletion timestamp set", + oldObj: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rb", + Namespace: "default", + }, + }, + newObj: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rb", + Namespace: "default", + DeletionTimestamp: &metav1.Time{Time: time.Now()}, + }, + }, + expected: true, + }, + { + name: "clusters field change", + oldObj: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rb", + Namespace: "default", + }, + Spec: workv1alpha2.ResourceBindingSpec{ + Clusters: []workv1alpha2.TargetCluster{ + {Name: "cluster1"}, + }, + }, + }, + newObj: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rb", + Namespace: "default", + }, + Spec: workv1alpha2.ResourceBindingSpec{ + Clusters: []workv1alpha2.TargetCluster{ + {Name: "cluster1"}, + {Name: "cluster2"}, + }, + }, + }, + expected: true, + }, + { + name: "resource field change", + oldObj: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rb", + Namespace: "default", + }, + Spec: workv1alpha2.ResourceBindingSpec{ + Resource: workv1alpha2.ObjectReference{ + APIVersion: "apps/v1", + Kind: "Deployment", + Name: "test-deployment", + Namespace: "default", + }, + }, + }, + newObj: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rb", + Namespace: "default", + }, + Spec: workv1alpha2.ResourceBindingSpec{ + Resource: workv1alpha2.ObjectReference{ + APIVersion: "apps/v1", + Kind: "Deployment", + Name: "test-deployment-v2", + Namespace: "default", + }, + }, + }, + expected: true, + }, + { + name: "conflict resolution change", + oldObj: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rb", + Namespace: "default", + }, + Spec: workv1alpha2.ResourceBindingSpec{ + ConflictResolution: "Abort", + }, + }, + newObj: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rb", + Namespace: "default", + }, + Spec: workv1alpha2.ResourceBindingSpec{ + ConflictResolution: "Overwrite", + }, + }, + expected: true, + }, + { + name: "no significant change", + oldObj: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rb", + Namespace: "default", + }, + Spec: workv1alpha2.ResourceBindingSpec{ + Resource: workv1alpha2.ObjectReference{ + APIVersion: "apps/v1", + Kind: "Deployment", + Name: "test-deployment", + Namespace: "default", + }, + }, + }, + newObj: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rb", + Namespace: "default", + Labels: map[string]string{"test": "label"}, + }, + Spec: workv1alpha2.ResourceBindingSpec{ + Resource: workv1alpha2.ObjectReference{ + APIVersion: "apps/v1", + Kind: "Deployment", + Name: "test-deployment", + Namespace: "default", + }, + }, + }, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + updateEvent := event.UpdateEvent{ + ObjectOld: tt.oldObj, + ObjectNew: tt.newObj, + } + + result := predicate.Update(updateEvent) + if result != tt.expected { + t.Errorf("Expected %v, got %v", tt.expected, result) + } + }) + } +} + +func TestResourceBindingPredicate_Generic(t *testing.T) { + predicate := newBindingPredicate() + + genericEvent := event.GenericEvent{Object: &workv1alpha2.ResourceBinding{}} + if !predicate.Generic(genericEvent) { + t.Error("Expected Generic event to return true") + } +} + +func TestResourceBindingPredicate_ClusterResourceBinding(t *testing.T) { + predicate := newBindingPredicate() + + // Test ClusterResourceBinding update with clusters change + oldCRB := &workv1alpha2.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-crb", + }, + Spec: workv1alpha2.ResourceBindingSpec{ + Clusters: []workv1alpha2.TargetCluster{ + {Name: "cluster1"}, + }, + }, + } + + newCRB := &workv1alpha2.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-crb", + }, + Spec: workv1alpha2.ResourceBindingSpec{ + Clusters: []workv1alpha2.TargetCluster{ + {Name: "cluster1"}, + {Name: "cluster2"}, + }, + }, + } + + updateEvent := event.UpdateEvent{ + ObjectOld: oldCRB, + ObjectNew: newCRB, + } + + if !predicate.Update(updateEvent) { + t.Error("Expected ClusterResourceBinding update with clusters change to return true") + } +} diff --git a/pkg/controllers/binding/cluster_resource_binding_controller.go b/pkg/controllers/binding/cluster_resource_binding_controller.go index 8acca3a3d886..5300fd0a96cc 100644 --- a/pkg/controllers/binding/cluster_resource_binding_controller.go +++ b/pkg/controllers/binding/cluster_resource_binding_controller.go @@ -31,6 +31,7 @@ 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" @@ -185,7 +186,7 @@ func (c *ClusterResourceBindingController) checkDirectPurgeOrphanWorks(ctx conte func (c *ClusterResourceBindingController) SetupWithManager(mgr controllerruntime.Manager) error { return controllerruntime.NewControllerManagedBy(mgr). Named(ClusterResourceBindingControllerName). - For(&workv1alpha2.ClusterResourceBinding{}). + For(&workv1alpha2.ClusterResourceBinding{}, builder.WithPredicates(newBindingPredicate())). Watches(&policyv1alpha1.ClusterOverridePolicy{}, handler.EnqueueRequestsFromMapFunc(c.newOverridePolicyFunc())). WithEventFilter(predicate.GenerationChangedPredicate{}). WithOptions(controller.Options{RateLimiter: ratelimiterflag.DefaultControllerRateLimiter[controllerruntime.Request](c.RateLimiterOptions)}).