temp

Signed-off-by: Ryan Zhang <[email protected]>

Author: Ryan Zhang

.github/.copilot/breadcrumbs/2025-06-24-0900-rollout-controller-binding-interface-refactor.md

@@ -118,7 +118,7 @@ func (r *Reconciler) validateEviction(ctx context.Context, eviction *placementv1
 	}
 
 	// set default values for CRP.
-	defaulter.SetDefaultsClusterResourcePlacement(&crp)
+	defaulter.SetDefaultsPlacement(&crp)
 
 	if crp.DeletionTimestamp != nil {
 		klog.V(2).InfoS(condition.EvictionInvalidDeletingCRPMessage, "clusterResourcePlacementEviction", eviction.Name, "clusterResourcePlacement", eviction.Spec.PlacementName)

pkg/controllers/clusterresourceplacementeviction/controller.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.SetDefaultsPlacement(tc.wantValidationResult.crp)
 			}
 
 			if diff := cmp.Diff(tc.wantValidationResult, gotValidationResult, validationResultCmpOptions...); diff != "" {

pkg/controllers/clusterresourceplacementeviction/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)
 			}

pkg/controllers/rollout/controller.go

@@ -17,26 +17,33 @@ limitations under the License.
 package binding
 
 import (
+	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/klog/v2"
 
 	placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1"
 	"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 {
+	conditions := binding.GetBindingStatus().Conditions
+	generation := binding.GetGeneration()
+
 	for i := condition.OverriddenCondition; i <= condition.AvailableCondition; i++ {
-		if condition.IsConditionStatusFalse(binding.GetCondition(string(i.ResourceBindingConditionType())), binding.Generation) {
+		cond := meta.FindStatusCondition(conditions, string(i.ResourceBindingConditionType()))
+		if condition.IsConditionStatusFalse(cond, generation) {
 			// 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 {
-	diffReportCondition := binding.GetCondition(string(placementv1beta1.ResourceBindingDiffReported))
-	return diffReportCondition != nil && diffReportCondition.ObservedGeneration == binding.Generation
+func IsBindingDiffReported(binding placementv1beta1.BindingObj) bool {
+	conditions := binding.GetBindingStatus().Conditions
+	generation := binding.GetGeneration()
+	diffReportCondition := meta.FindStatusCondition(conditions, string(placementv1beta1.ResourceBindingDiffReported))
+	return diffReportCondition != nil && diffReportCondition.ObservedGeneration == generation
 }

pkg/controllers/rollout/controller_test.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)

pkg/utils/binding/binding.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{
+// SetDefaultsPlacement sets the default values for placement.
+func SetDefaultsPlacement(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))
 	}
 }
 

pkg/utils/controller/placement_resolver.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)
+			SetDefaultsPlacement(tt.obj)
 			if diff := cmp.Diff(tt.wantObj, tt.obj); diff != "" {
 				t.Errorf("SetDefaultsClusterResourcePlacement() mismatch (-want +got):\n%s", diff)
 			}