test

ryanzhang-oss · ryanzhang-oss · commit 1681e8a6366f · 2025-06-27T23:51:22.000Z
diff --git a/.github/.copilot/breadcrumbs/2025-06-24-0900-rollout-controller-binding-interface-refactor.md b/.github/.copilot/breadcrumbs/2025-06-24-0900-rollout-controller-binding-interface-refactor.md
diff --git a/apis/placement/v1beta1/binding_types.go b/apis/placement/v1beta1/binding_types.go
@@ -20,6 +20,8 @@ import (
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	"github.com/kubefleet-dev/kubefleet/apis"
 )
 
 const (
@@ -52,7 +54,7 @@ type BindingStatusGetterSetter interface {
 // A BindingObj offers an abstract way to work with fleet binding objects.
 // +kubebuilder:object:generate=false
 type BindingObj interface {
-	client.Object
+	apis.ConditionedObj
 	BindingSpecGetterSetter
 	BindingStatusGetterSetter
 }
diff --git a/apis/placement/v1beta1/clusterresourceplacement_types.go b/apis/placement/v1beta1/clusterresourceplacement_types.go
@@ -22,6 +22,8 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	"github.com/kubefleet-dev/kubefleet/apis"
 )
 
 const (
@@ -58,7 +60,7 @@ type PlacementStatusGetterSetter interface {
 // PlacementObj offers the functionality to work with fleet placement object.
 // +kubebuilder:object:generate=false
 type PlacementObj interface {
-	client.Object
+	apis.ConditionedObj
 	PlacementSpecGetterSetter
 	PlacementStatusGetterSetter
 }
diff --git a/apis/placement/v1beta1/policysnapshot_types.go b/apis/placement/v1beta1/policysnapshot_types.go
@@ -20,6 +20,8 @@ import (
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	"github.com/kubefleet-dev/kubefleet/apis"
 )
 
 const (
@@ -57,7 +59,7 @@ type PolicySnapshotStatusGetterSetter interface {
 // A PolicySnapshotObj offers an abstract way to work with a fleet policy snapshot object.
 // +kubebuilder:object:generate=false
 type PolicySnapshotObj interface {
-	client.Object
+	apis.ConditionedObj
 	PolicySnapshotSpecGetterSetter
 	PolicySnapshotStatusGetterSetter
 }
diff --git a/apis/placement/v1beta1/resourcesnapshot_types.go b/apis/placement/v1beta1/resourcesnapshot_types.go
@@ -21,6 +21,8 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	"github.com/kubefleet-dev/kubefleet/apis"
 )
 
 const (
@@ -71,7 +73,7 @@ type ResourceSnapshotStatusGetterSetter interface {
 // A ResourceSnapshotObj offers an abstract way to work with a resource snapshot object.
 // +kubebuilder:object:generate=false
 type ResourceSnapshotObj interface {
-	client.Object
+	apis.ConditionedObj
 	ResourceSnapshotSpecGetterSetter
 	ResourceSnapshotStatusGetterSetter
 }
diff --git a/pkg/controllers/clusterresourceplacementeviction/controller.go b/pkg/controllers/clusterresourceplacementeviction/controller.go
@@ -118,7 +118,7 @@ func (r *Reconciler) validateEviction(ctx context.Context, eviction *placementv1
 	}
 
 	// set default values for CRP.
-	defaulter.SetDefaultsClusterResourcePlacement(&crp)
+	defaulter.SetPlacementDefaults(&crp)
 
 	if crp.DeletionTimestamp != nil {
 		klog.V(2).InfoS(condition.EvictionInvalidDeletingCRPMessage, "clusterResourcePlacementEviction", eviction.Name, "clusterResourcePlacement", eviction.Spec.PlacementName)
diff --git a/pkg/controllers/clusterresourceplacementeviction/controller_test.go b/pkg/controllers/clusterresourceplacementeviction/controller_test.go
@@ -258,7 +258,7 @@ func TestValidateEviction(t *testing.T) {
 			// Since default values are applied to the affected CRP in the eviction controller; the
 			// the same must be done on the expected result as well.
 			if tc.wantValidationResult.crp != nil {
-				defaulter.SetDefaultsClusterResourcePlacement(tc.wantValidationResult.crp)
+				defaulter.SetPlacementDefaults(tc.wantValidationResult.crp)
 			}
 
 			if diff := cmp.Diff(tc.wantValidationResult, gotValidationResult, validationResultCmpOptions...); diff != "" {
diff --git a/pkg/controllers/rollout/controller.go b/pkg/controllers/rollout/controller.go
diff --git a/pkg/controllers/rollout/controller_test.go b/pkg/controllers/rollout/controller_test.go
@@ -264,7 +264,7 @@ func TestWaitForResourcesToCleanUp(t *testing.T) {
 	for name, tt := range tests {
 		crp := &fleetv1beta1.ClusterResourcePlacement{}
 		t.Run(name, func(t *testing.T) {
-			gotWait, err := waitForResourcesToCleanUp(tt.allBindings, crp)
+			gotWait, err := waitForResourcesToCleanUp(controller.ConvertCRBArrayToBindingObjs(tt.allBindings), crp)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("waitForResourcesToCleanUp test `%s` error = %v, wantErr %v", name, err, tt.wantErr)
 				return
@@ -618,7 +618,7 @@ func TestUpdateBindings(t *testing.T) {
 				}
 				if tt.desiredBindingsSpec != nil {
 					inputs[i].desiredBinding = tt.bindings[i].DeepCopy()
-					inputs[i].desiredBinding.Spec = tt.desiredBindingsSpec[i]
+					inputs[i].desiredBinding.SetBindingSpec(tt.desiredBindingsSpec[i])
 				}
 			}
 			err := r.updateBindings(ctx, inputs)
@@ -2172,7 +2172,7 @@ func TestPickBindingsToRoll(t *testing.T) {
 					Name: tt.latestResourceSnapshotName,
 				},
 			}
-			gotUpdatedBindings, gotStaleUnselectedBindings, gotUpToDateBoundBindings, gotNeedRoll, gotWaitTime, err := r.pickBindingsToRoll(context.Background(), tt.allBindings, resourceSnapshot, tt.crp, tt.matchedCROs, tt.matchedROs)
+			gotUpdatedBindings, gotStaleUnselectedBindings, gotUpToDateBoundBindings, gotNeedRoll, gotWaitTime, err := r.pickBindingsToRoll(context.Background(), controller.ConvertCRBArrayToBindingObjs(tt.allBindings), resourceSnapshot, tt.crp, tt.matchedCROs, tt.matchedROs)
 			if (err != nil) != (tt.wantErr != nil) || err != nil && !errors.Is(err, tt.wantErr) {
 				t.Fatalf("pickBindingsToRoll() error = %v, wantErr %v", err, tt.wantErr)
 			}
@@ -2183,21 +2183,23 @@ func TestPickBindingsToRoll(t *testing.T) {
 			wantTobeUpdatedBindings := make([]toBeUpdatedBinding, len(tt.wantTobeUpdatedBindings))
 			for i, index := range tt.wantTobeUpdatedBindings {
 				// Unscheduled bindings are only removed in a single rollout cycle.
-				if tt.allBindings[index].Spec.State != fleetv1beta1.BindingStateUnscheduled {
+				bindingSpec := tt.allBindings[index].GetBindingSpec()
+				if bindingSpec.State != fleetv1beta1.BindingStateUnscheduled {
 					wantTobeUpdatedBindings[i].currentBinding = tt.allBindings[index]
 					wantTobeUpdatedBindings[i].desiredBinding = tt.allBindings[index].DeepCopy()
-					wantTobeUpdatedBindings[i].desiredBinding.Spec = tt.wantDesiredBindingsSpec[index]
+					wantTobeUpdatedBindings[i].desiredBinding.SetBindingSpec(tt.wantDesiredBindingsSpec[index])
 				} else {
 					wantTobeUpdatedBindings[i].currentBinding = tt.allBindings[index]
 				}
 			}
 			wantStaleUnselectedBindings := make([]toBeUpdatedBinding, len(tt.wantStaleUnselectedBindings))
 			for i, index := range tt.wantStaleUnselectedBindings {
 				// Unscheduled bindings are only removed in a single rollout cycle.
-				if tt.allBindings[index].Spec.State != fleetv1beta1.BindingStateUnscheduled {
+				bindingSpec := tt.allBindings[index].GetBindingSpec()
+				if bindingSpec.State != fleetv1beta1.BindingStateUnscheduled {
 					wantStaleUnselectedBindings[i].currentBinding = tt.allBindings[index]
 					wantStaleUnselectedBindings[i].desiredBinding = tt.allBindings[index].DeepCopy()
-					wantStaleUnselectedBindings[i].desiredBinding.Spec = tt.wantDesiredBindingsSpec[index]
+					wantStaleUnselectedBindings[i].desiredBinding.SetBindingSpec(tt.wantDesiredBindingsSpec[index])
 				} else {
 					wantStaleUnselectedBindings[i].currentBinding = tt.allBindings[index]
 				}
@@ -2892,7 +2894,7 @@ func TestCheckAndUpdateStaleBindingsStatus(t *testing.T) {
 				Client: fakeClient,
 			}
 			ctx := context.Background()
-			if err := r.checkAndUpdateStaleBindingsStatus(ctx, tt.bindings); err != nil {
+			if err := r.checkAndUpdateStaleBindingsStatus(ctx, controller.ConvertCRBArrayToBindingObjs(tt.bindings)); err != nil {
 				t.Fatalf("checkAndUpdateStaleBindingsStatus() got error %v, want no err", err)
 			}
 			bindingList := &fleetv1beta1.ClusterResourceBindingList{}
@@ -3168,7 +3170,7 @@ func TestProcessApplyStrategyUpdates(t *testing.T) {
 				Client: fakeClient,
 			}
 
-			applyStrategyUpdated, err := r.processApplyStrategyUpdates(ctx, tc.crp, tc.allBindings)
+			applyStrategyUpdated, err := r.processApplyStrategyUpdates(ctx, tc.crp, controller.ConvertCRBArrayToBindingObjs(tc.allBindings))
 			if err != nil {
 				t.Errorf("processApplyStrategyUpdates() error = %v, want no error", err)
 			}
diff --git a/pkg/scheduler/framework/frameworkutils_test.go b/pkg/scheduler/framework/frameworkutils_test.go
@@ -126,28 +126,6 @@ func TestGenerateBinding(t *testing.T) {
 					t.Errorf("expected empty namespace for ClusterResourceBinding, got %s", crb.Namespace)
 				}
 
-				// Verify name pattern: placement-cluster-uuid (8 chars)
-				nameParts := strings.Split(crb.Name, "-")
-				if len(nameParts) < 3 {
-					t.Errorf("expected binding name to have at least 3 parts separated by '-', got %s", crb.Name)
-				} else {
-					// Last part should be 8-character UUID
-					uuidPart := nameParts[len(nameParts)-1]
-					if len(uuidPart) != 8 {
-						t.Errorf("expected UUID part to be 8 characters, got %d characters: %s", len(uuidPart), uuidPart)
-					}
-				}
-
-				// Verify labels
-				if crb.Labels[placementv1beta1.CRPTrackingLabel] != tt.expectedLabel {
-					t.Errorf("expected CRPTrackingLabel %s, got %s", tt.expectedLabel, crb.Labels[placementv1beta1.CRPTrackingLabel])
-				}
-
-				// Verify finalizers
-				if len(crb.Finalizers) != 1 || crb.Finalizers[0] != placementv1beta1.SchedulerCRBCleanupFinalizer {
-					t.Errorf("expected finalizer %s, got %v", placementv1beta1.SchedulerCRBCleanupFinalizer, crb.Finalizers)
-				}
-
 			case "ResourceBinding":
 				rb, ok := binding.(*placementv1beta1.ResourceBinding)
 				if !ok {
@@ -158,30 +136,31 @@ func TestGenerateBinding(t *testing.T) {
 					t.Errorf("expected namespace %s, got %s", tt.expectedNamespace, rb.Namespace)
 				}
 
-				// Verify name pattern: placement-cluster-uuid (8 chars)
-				nameParts := strings.Split(rb.Name, "-")
-				if len(nameParts) < 3 {
-					t.Errorf("expected binding name to have at least 3 parts separated by '-', got %s", rb.Name)
-				} else {
-					// Last part should be 8-character UUID
-					uuidPart := nameParts[len(nameParts)-1]
-					if len(uuidPart) != 8 {
-						t.Errorf("expected UUID part to be 8 characters, got %d characters: %s", len(uuidPart), uuidPart)
-					}
-				}
+			default:
+				t.Errorf("unexpected expected type: %s", tt.expectedType)
+			}
 
-				// Verify labels
-				if rb.Labels[placementv1beta1.CRPTrackingLabel] != tt.expectedLabel {
-					t.Errorf("expected CRPTrackingLabel %s, got %s", tt.expectedLabel, rb.Labels[placementv1beta1.CRPTrackingLabel])
+			// Verify name pattern: placement-cluster-uuid (8 chars), the placement and cluster name part are already validated
+			// in NewBindingName UT.
+			nameParts := strings.Split(binding.GetName(), "-")
+			if len(nameParts) < 3 {
+				t.Errorf("expected binding name to have at least 3 parts separated by '-', got %s", binding.GetName())
+			} else {
+				// Last part should be 8-character UUID
+				uuidPart := nameParts[len(nameParts)-1]
+				if len(uuidPart) != 8 {
+					t.Errorf("expected UUID part to be 8 characters, got %d characters: %s", len(uuidPart), uuidPart)
 				}
+			}
 
-				// Verify finalizers
-				if len(rb.Finalizers) != 1 || rb.Finalizers[0] != placementv1beta1.SchedulerCRBCleanupFinalizer {
-					t.Errorf("expected finalizer %s, got %v", placementv1beta1.SchedulerCRBCleanupFinalizer, rb.Finalizers)
-				}
+			// Verify labels
+			if binding.GetLabels()[placementv1beta1.CRPTrackingLabel] != tt.expectedLabel {
+				t.Errorf("expected CRPTrackingLabel %s, got %s", tt.expectedLabel, binding.GetLabels()[placementv1beta1.CRPTrackingLabel])
+			}
 
-			default:
-				t.Errorf("unexpected expected type: %s", tt.expectedType)
+			// Verify finalizers
+			if len(binding.GetFinalizers()) != 1 || binding.GetFinalizers()[0] != placementv1beta1.SchedulerCRBCleanupFinalizer {
+				t.Errorf("expected finalizer %s, got %v", placementv1beta1.SchedulerCRBCleanupFinalizer, binding.GetFinalizers())
 			}
 		})
 	}
diff --git a/pkg/utils/binding/binding.go b/pkg/utils/binding/binding.go
@@ -23,20 +23,20 @@ import (
 	"github.com/kubefleet-dev/kubefleet/pkg/utils/condition"
 )
 
-// HasBindingFailed checks if ClusterResourceBinding has failed based on its conditions.
-func HasBindingFailed(binding *placementv1beta1.ClusterResourceBinding) bool {
+// HasBindingFailed checks if BindingObj has failed based on its conditions.
+func HasBindingFailed(binding placementv1beta1.BindingObj) bool {
 	for i := condition.OverriddenCondition; i <= condition.AvailableCondition; i++ {
-		if condition.IsConditionStatusFalse(binding.GetCondition(string(i.ResourceBindingConditionType())), binding.Generation) {
+		if condition.IsConditionStatusFalse(binding.GetCondition(string(i.ResourceBindingConditionType())), binding.GetGeneration()) {
 			// TODO: parse the reason of the condition to see if the failure is recoverable/retriable or not
-			klog.V(2).Infof("binding %s has condition %s with status false", binding.Name, string(i.ResourceBindingConditionType()))
+			klog.V(2).Infof("binding %s has condition %s with status false", binding.GetName(), string(i.ResourceBindingConditionType()))
 			return true
 		}
 	}
 	return false
 }
 
 // IsBindingDiffReported checks if the binding is in diffReported state.
-func IsBindingDiffReported(binding *placementv1beta1.ClusterResourceBinding) bool {
+func IsBindingDiffReported(binding placementv1beta1.BindingObj) bool {
 	diffReportCondition := binding.GetCondition(string(placementv1beta1.ResourceBindingDiffReported))
-	return diffReportCondition != nil && diffReportCondition.ObservedGeneration == binding.Generation
+	return diffReportCondition != nil && diffReportCondition.ObservedGeneration == binding.GetGeneration()
 }
diff --git a/pkg/utils/controller/placement_resolver.go b/pkg/utils/controller/placement_resolver.go
@@ -22,6 +22,7 @@ import (
 	"strings"
 
 	"k8s.io/apimachinery/pkg/types"
+	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	fleetv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1"
@@ -76,6 +77,17 @@ func GetPlacementKeyFromObj(obj fleetv1beta1.PlacementObj) queue.PlacementKey {
 	}
 }
 
+// GetPlacementKeyFromObj generates a PlacementKey from a placement object.
+func GetPlacementKeyFromRequest(req ctrl.Request) queue.PlacementKey {
+	if req.Namespace == "" {
+		// Cluster-scoped placement
+		return queue.PlacementKey(req.Name)
+	} else {
+		// Namespaced placement
+		return queue.PlacementKey(req.Namespace + namespaceSeparator + req.Name)
+	}
+}
+
 // ExtractNamespaceNameFromKey resolves a PlacementKey to a (namespace, name) tuple of the placement object.
 func ExtractNamespaceNameFromKey(placementKey queue.PlacementKey) (string, string, error) {
 	keyStr := string(placementKey)
diff --git a/pkg/utils/defaulter/clusterresourceplacement.go b/pkg/utils/defaulter/clusterresourceplacement.go
@@ -42,34 +42,35 @@ const (
 	DefaultRevisionHistoryLimitValue = 10
 )
 
-// SetDefaultsClusterResourcePlacement sets the default values for ClusterResourcePlacement.
-func SetDefaultsClusterResourcePlacement(obj *fleetv1beta1.ClusterResourcePlacement) {
-	if obj.Spec.Policy == nil {
-		obj.Spec.Policy = &fleetv1beta1.PlacementPolicy{
+// SetPlacementDefaults sets the default values for placement.
+func SetPlacementDefaults(obj fleetv1beta1.PlacementObj) {
+	spec := obj.GetPlacementSpec()
+	if spec.Policy == nil {
+		spec.Policy = &fleetv1beta1.PlacementPolicy{
 			PlacementType: fleetv1beta1.PickAllPlacementType,
 		}
 	}
 
-	if obj.Spec.Policy.TopologySpreadConstraints != nil {
-		for i := range obj.Spec.Policy.TopologySpreadConstraints {
-			if obj.Spec.Policy.TopologySpreadConstraints[i].MaxSkew == nil {
-				obj.Spec.Policy.TopologySpreadConstraints[i].MaxSkew = ptr.To(int32(DefaultMaxSkewValue))
+	if spec.Policy.TopologySpreadConstraints != nil {
+		for i := range spec.Policy.TopologySpreadConstraints {
+			if spec.Policy.TopologySpreadConstraints[i].MaxSkew == nil {
+				spec.Policy.TopologySpreadConstraints[i].MaxSkew = ptr.To(int32(DefaultMaxSkewValue))
 			}
-			if obj.Spec.Policy.TopologySpreadConstraints[i].WhenUnsatisfiable == "" {
-				obj.Spec.Policy.TopologySpreadConstraints[i].WhenUnsatisfiable = fleetv1beta1.DoNotSchedule
+			if spec.Policy.TopologySpreadConstraints[i].WhenUnsatisfiable == "" {
+				spec.Policy.TopologySpreadConstraints[i].WhenUnsatisfiable = fleetv1beta1.DoNotSchedule
 			}
 		}
 	}
 
-	if obj.Spec.Policy.Tolerations != nil {
-		for i := range obj.Spec.Policy.Tolerations {
-			if obj.Spec.Policy.Tolerations[i].Operator == "" {
-				obj.Spec.Policy.Tolerations[i].Operator = corev1.TolerationOpEqual
+	if spec.Policy.Tolerations != nil {
+		for i := range spec.Policy.Tolerations {
+			if spec.Policy.Tolerations[i].Operator == "" {
+				spec.Policy.Tolerations[i].Operator = corev1.TolerationOpEqual
 			}
 		}
 	}
 
-	strategy := &obj.Spec.Strategy
+	strategy := &spec.Strategy
 	if strategy.Type == "" {
 		strategy.Type = fleetv1beta1.RollingUpdateRolloutStrategyType
 	}
@@ -88,13 +89,13 @@ func SetDefaultsClusterResourcePlacement(obj *fleetv1beta1.ClusterResourcePlacem
 		}
 	}
 
-	if obj.Spec.Strategy.ApplyStrategy == nil {
-		obj.Spec.Strategy.ApplyStrategy = &fleetv1beta1.ApplyStrategy{}
+	if spec.Strategy.ApplyStrategy == nil {
+		spec.Strategy.ApplyStrategy = &fleetv1beta1.ApplyStrategy{}
 	}
-	SetDefaultsApplyStrategy(obj.Spec.Strategy.ApplyStrategy)
+	SetDefaultsApplyStrategy(spec.Strategy.ApplyStrategy)
 
-	if obj.Spec.RevisionHistoryLimit == nil {
-		obj.Spec.RevisionHistoryLimit = ptr.To(int32(DefaultRevisionHistoryLimitValue))
+	if spec.RevisionHistoryLimit == nil {
+		spec.RevisionHistoryLimit = ptr.To(int32(DefaultRevisionHistoryLimitValue))
 	}
 }
 
diff --git a/pkg/utils/defaulter/clusterresourceplacement_test.go b/pkg/utils/defaulter/clusterresourceplacement_test.go
@@ -167,7 +167,7 @@ func TestSetDefaultsClusterResourcePlacement(t *testing.T) {
 	}
 	for name, tt := range tests {
 		t.Run(name, func(t *testing.T) {
-			SetDefaultsClusterResourcePlacement(tt.obj)
+			SetPlacementDefaults(tt.obj)
 			if diff := cmp.Diff(tt.wantObj, tt.obj); diff != "" {
 				t.Errorf("SetDefaultsClusterResourcePlacement() mismatch (-want +got):\n%s", diff)
 			}

Original file line number	Diff line number	Diff line change
`@@ -118,7 +118,7 @@ func (r Reconciler) validateEviction(ctx context.Context, eviction placementv1`
`118`	`118`	`}`
`119`	`119`
`120`	`120`	`// set default values for CRP.`
`121`		`- defaulter.SetDefaultsClusterResourcePlacement(&crp)`
	`121`	`+ defaulter.SetPlacementDefaults(&crp)`
`122`	`122`
`123`	`123`	`if crp.DeletionTimestamp != nil {`
`124`	`124`	`klog.V(2).InfoS(condition.EvictionInvalidDeletingCRPMessage, "clusterResourcePlacementEviction", eviction.Name, "clusterResourcePlacement", eviction.Spec.PlacementName)`
Original file line number	Diff line number	Diff line change
`@@ -258,7 +258,7 @@ func TestValidateEviction(t *testing.T) {`
`258`	`258`	`// Since default values are applied to the affected CRP in the eviction controller; the`
`259`	`259`	`// the same must be done on the expected result as well.`
`260`	`260`	`if tc.wantValidationResult.crp != nil {`
`261`		`- defaulter.SetDefaultsClusterResourcePlacement(tc.wantValidationResult.crp)`
	`261`	`+ defaulter.SetPlacementDefaults(tc.wantValidationResult.crp)`
`262`	`262`	`}`
`263`	`263`
`264`	`264`	`if diff := cmp.Diff(tc.wantValidationResult, gotValidationResult, validationResultCmpOptions...); diff != "" {`