update generateStagesByStrategy function

Signed-off-by: Arvind Thirumurugan <[email protected]>

Author: Arvind Thirumurugan

.github/.copilot/breadcrumbs/2025-09-25-1430-initialize-refactor.md

@@ -362,10 +362,108 @@ func (r *Reconciler) computeRunStageStatus(
 
 **Integration**: The `generateStagesByStrategy` function now converts concrete binding arrays to interfaces before calling `computeRunStageStatus`, maintaining compatibility while enabling interface-based processing.
 
+### Phase 8: generateStagesByStrategy Refactor ✅
+
+**Target**: `generateStagesByStrategy` function - computes stages based on StagedUpdateStrategy
+
+**Implementation**: Applied interface-based pattern with new strategy resolver utility:
+
+**Before**:
+```go
+func (r *Reconciler) generateStagesByStrategy(
+	ctx context.Context,
+	scheduledBindings []*placementv1beta1.ClusterResourceBinding,
+	toBeDeletedBindings []*placementv1beta1.ClusterResourceBinding,
+	updateRun *placementv1beta1.ClusterStagedUpdateRun,
+) error {
+	// Direct Client.Get for ClusterStagedUpdateStrategy
+	var updateStrategy placementv1beta1.ClusterStagedUpdateStrategy
+	if err := r.Client.Get(ctx, client.ObjectKey{Name: updateRun.Spec.StagedUpdateStrategyName}, &updateStrategy); err != nil {
+		// Direct field access
+		updateRun.Status.StagedUpdateStrategySnapshot = &updateStrategy.Spec
+		binding.Spec.TargetCluster
+	}
+}
+```
+
+**After**:
+```go
+func (r *Reconciler) generateStagesByStrategy(
+	ctx context.Context,
+	scheduledBindings []placementv1beta1.BindingObj,
+	toBeDeletedBindings []placementv1beta1.BindingObj,
+	updateRun placementv1beta1.StagedUpdateRunObj,
+) error {
+	// Generic utility function handles type determination
+	strategyKey := types.NamespacedName{
+		Name:      updateRunSpec.StagedUpdateStrategyName,
+		Namespace: updateRun.GetNamespace(),
+	}
+	updateStrategy, err := controller.FetchStagedUpdateStrategyFromNamespacedName(ctx, r.Client, strategyKey)
+	// Interface-based access
+	updateRunStatus.StagedUpdateStrategySnapshot = updateStrategy.GetStagedUpdateStrategySpec()
+	bindingSpec.TargetCluster
+}
+```
+
+**Key Improvements**:
+1. **Generic Interface Parameters**: Uses `[]BindingObj` and `StagedUpdateStrategyObj` instead of concrete types
+2. **Strategy Resolver Utility**: Created `strategy_resolver.go` with `FetchStagedUpdateStrategyFromNamespacedName()`
+3. **Interface-Based Access**: Uses `GetStagedUpdateRunSpec()`, `GetStagedUpdateRunStatus()`, `GetBindingSpec()` 
+4. **Interface-Based Status Updates**: Uses `SetStagedUpdateRunStatus()` instead of direct field assignment
+5. **Automatic Type Determination**: Namespace presence determines ClusterStagedUpdateStrategy vs StagedUpdateStrategy
+6. **Generic Logging**: Updated terminology from cluster-specific to generic
+
+**New Utility**: Created `/pkg/utils/controller/strategy_resolver.go` following the same pattern as `binding_resolver.go`
+
+### Phase 9: removeWaitTimeFromUpdateRunStatus Refactor ✅
+
+**Target**: `removeWaitTimeFromUpdateRunStatus` utility function - removes waitTime from approval tasks
+
+**Implementation**: Updated from concrete type to interface-based approach:
+
+**Before**:
+```go
+func removeWaitTimeFromUpdateRunStatus(updateRun *placementv1beta1.ClusterStagedUpdateRun) {
+	// Direct field access
+	if updateRun.Status.StagedUpdateStrategySnapshot != nil {
+		for i := range updateRun.Status.StagedUpdateStrategySnapshot.Stages {
+			// Direct status modification
+			updateRun.Status.StagedUpdateStrategySnapshot.Stages[i].AfterStageTasks[j].WaitTime = nil
+		}
+	}
+}
+```
+
+**After**:
+```go
+func removeWaitTimeFromUpdateRunStatus(updateRun placementv1beta1.StagedUpdateRunObj) {
+	// Interface-based access
+	updateRunStatus := updateRun.GetStagedUpdateRunStatus()
+	if updateRunStatus.StagedUpdateStrategySnapshot != nil {
+		for i := range updateRunStatus.StagedUpdateStrategySnapshot.Stages {
+			// Modify status copy
+			updateRunStatus.StagedUpdateStrategySnapshot.Stages[i].AfterStageTasks[j].WaitTime = nil
+		}
+		// Update back using interface method
+		updateRun.SetStagedUpdateRunStatus(*updateRunStatus)
+	}
+}
+```
+
+**Key Improvements**:
+1. **Generic Interface Parameter**: Uses `StagedUpdateRunObj` instead of `*ClusterStagedUpdateRun`
+2. **Interface-Based Status Access**: Uses `GetStagedUpdateRunStatus()` and `SetStagedUpdateRunStatus()`
+3. **Proper Status Management**: Gets status copy, modifies it, then sets it back
+4. **Eliminated Type Assertion**: Removed the type assertion workaround in `generateStagesByStrategy`
+
+**Impact**: This utility function is now generic and works with both `ClusterStagedUpdateRun` and `StagedUpdateRun` objects.
+
 ## Current Status
 - ✅ `validatePlacement`: Fully generic, uses utility functions and interfaces
 - ✅ `determinePolicySnapshot`: Fully generic, uses utility functions and interfaces, unnecessary switch removed
 - ✅ `collectScheduledClusters`: Fully generic, uses utility functions and interfaces
 - ✅ `computeRunStageStatus`: Fully generic, uses interface-based access patterns
-- ⚠️ `initialize`: Still takes concrete ClusterStagedUpdateRun type, but now calls interface-based functions
-- ⚠️ `generateStagesByStrategy`: Still takes concrete types but converts to interfaces for `computeRunStageStatus`
+- ✅ `generateStagesByStrategy`: Fully generic, uses interface-based access patterns and new strategy resolver utility
+- ✅ `removeWaitTimeFromUpdateRunStatus`: Fully generic utility function using interface-based approach
+- ⚠️ `initialize`: Still takes concrete ClusterStagedUpdateRun type, but now calls interface-based functions

pkg/controllers/updaterun/controller.go

@@ -378,15 +378,19 @@ func emitUpdateRunStatusMetric(updateRun *placementv1beta1.ClusterStagedUpdateRu
 	klog.V(2).InfoS("There's no valid status condition on updateRun, status updating failed possibly", "updateRun", klog.KObj(updateRun))
 }
 
-func removeWaitTimeFromUpdateRunStatus(updateRun *placementv1beta1.ClusterStagedUpdateRun) {
+func removeWaitTimeFromUpdateRunStatus(updateRun placementv1beta1.StagedUpdateRunObj) {
 	// Remove waitTime from the updateRun status for AfterStageTask for type Approval.
-	if updateRun.Status.StagedUpdateStrategySnapshot != nil {
-		for i := range updateRun.Status.StagedUpdateStrategySnapshot.Stages {
-			for j := range updateRun.Status.StagedUpdateStrategySnapshot.Stages[i].AfterStageTasks {
-				if updateRun.Status.StagedUpdateStrategySnapshot.Stages[i].AfterStageTasks[j].Type == placementv1beta1.AfterStageTaskTypeApproval {
-					updateRun.Status.StagedUpdateStrategySnapshot.Stages[i].AfterStageTasks[j].WaitTime = nil
+	// Use interface methods to access and update status
+	updateRunStatus := updateRun.GetStagedUpdateRunStatus()
+	if updateRunStatus.StagedUpdateStrategySnapshot != nil {
+		for i := range updateRunStatus.StagedUpdateStrategySnapshot.Stages {
+			for j := range updateRunStatus.StagedUpdateStrategySnapshot.Stages[i].AfterStageTasks {
+				if updateRunStatus.StagedUpdateStrategySnapshot.Stages[i].AfterStageTasks[j].Type == placementv1beta1.AfterStageTaskTypeApproval {
+					updateRunStatus.StagedUpdateStrategySnapshot.Stages[i].AfterStageTasks[j].WaitTime = nil
 				}
 			}
 		}
+		// Update the status back using interface method
+		updateRun.SetStagedUpdateRunStatus(*updateRunStatus)
 	}
 }

pkg/controllers/updaterun/initialization.go

@@ -94,7 +94,8 @@ func (r *Reconciler) initialize(
 	}
 
 	// Compute the stages based on the StagedUpdateStrategy.
-	if err := r.generateStagesByStrategy(ctx, scheduledBindings, toBeDeletedBindings, updateRun); err != nil {
+	// Use interface objects directly - scheduledBindingObjs and toBeDeletedBindingObjs already exist
+	if err := r.generateStagesByStrategy(ctx, scheduledBindingObjs, toBeDeletedBindingObjs, updateRun); err != nil {
 		return nil, nil, err
 	}
 	// Record the override snapshots associated with each cluster.
@@ -295,51 +296,73 @@ func (r *Reconciler) collectScheduledClusters(
 	return selectedBindings, toBeDeletedBindings, nil
 }
 
-// generateStagesByStrategy computes the stages based on the ClusterStagedUpdateStrategy referenced by the ClusterStagedUpdateRun.
+// generateStagesByStrategy computes the stages based on the StagedUpdateStrategy referenced by the StagedUpdateRun.
 func (r *Reconciler) generateStagesByStrategy(
 	ctx context.Context,
-	scheduledBindings []*placementv1beta1.ClusterResourceBinding,
-	toBeDeletedBindings []*placementv1beta1.ClusterResourceBinding,
-	updateRun *placementv1beta1.ClusterStagedUpdateRun,
+	scheduledBindings []placementv1beta1.BindingObj,
+	toBeDeletedBindings []placementv1beta1.BindingObj,
+	updateRun placementv1beta1.StagedUpdateRunObj,
 ) error {
 	updateRunRef := klog.KObj(updateRun)
-	// Fetch the StagedUpdateStrategy referenced by StagedUpdateStrategyName.
-	var updateStrategy placementv1beta1.ClusterStagedUpdateStrategy
-	if err := r.Client.Get(ctx, client.ObjectKey{Name: updateRun.Spec.StagedUpdateStrategyName}, &updateStrategy); err != nil {
-		klog.ErrorS(err, "Failed to get StagedUpdateStrategy", "stagedUpdateStrategy", updateRun.Spec.StagedUpdateStrategyName, "clusterStagedUpdateRun", updateRunRef)
+
+	// Get updateRun spec using interface methods
+	updateRunSpec := updateRun.GetStagedUpdateRunSpec()
+
+	// Create NamespacedName for the StagedUpdateStrategy
+	// For ClusterStagedUpdateRun, namespace will be empty (cluster-scoped)
+	// For StagedUpdateRun, namespace will be the actual namespace (namespace-scoped)
+	strategyKey := types.NamespacedName{
+		Name:      updateRunSpec.StagedUpdateStrategyName,
+		Namespace: updateRun.GetNamespace(),
+	}
+
+	// Fetch the StagedUpdateStrategy using utility function
+	// This automatically determines whether to fetch ClusterStagedUpdateStrategy or StagedUpdateStrategy
+	updateStrategy, err := controller.FetchStagedUpdateStrategyFromNamespacedName(ctx, r.Client, strategyKey)
+	if err != nil {
+		klog.ErrorS(err, "Failed to get StagedUpdateStrategy", "stagedUpdateStrategy", updateRunSpec.StagedUpdateStrategyName, "stagedUpdateRun", updateRunRef)
 		if apierrors.IsNotFound(err) {
 			// we won't continue or retry the initialization if the StagedUpdateStrategy is not found.
-			strategyNotFoundErr := controller.NewUserError(errors.New("referenced clusterStagedUpdateStrategy not found: " + updateRun.Spec.StagedUpdateStrategyName))
+			strategyNotFoundErr := controller.NewUserError(errors.New("referenced stagedUpdateStrategy not found: " + updateRunSpec.StagedUpdateStrategyName))
 			return fmt.Errorf("%w: %s", errInitializedFailed, strategyNotFoundErr.Error())
 		}
 		// other err can be retried.
 		return controller.NewAPIServerError(true, err)
 	}
 
 	// This won't change even if the stagedUpdateStrategy changes or is deleted after the updateRun is initialized.
-	updateRun.Status.StagedUpdateStrategySnapshot = &updateStrategy.Spec
+	// Use interface methods to access and update status
+	updateRunStatus := updateRun.GetStagedUpdateRunStatus()
+	updateStrategySpec := updateStrategy.GetStagedUpdateStrategySpec()
+	updateRunStatus.StagedUpdateStrategySnapshot = updateStrategySpec
+	updateRun.SetStagedUpdateRunStatus(*updateRunStatus)
+
 	// Remove waitTime from the updateRun status for AfterStageTask for type Approval.
 	removeWaitTimeFromUpdateRunStatus(updateRun)
 
 	// Compute the update stages.
-	// Convert concrete binding arrays to interface arrays
-	scheduledBindingObjs := controller.ConvertCRBArrayToBindingObjs(scheduledBindings)
-	if err := r.computeRunStageStatus(ctx, scheduledBindingObjs, updateRun); err != nil {
+	// scheduledBindings already are interface objects, no conversion needed
+	if err := r.computeRunStageStatus(ctx, scheduledBindings, updateRun); err != nil {
 		return err
 	}
 	toBeDeletedClusters := make([]placementv1beta1.ClusterUpdatingStatus, len(toBeDeletedBindings))
 	for i, binding := range toBeDeletedBindings {
-		klog.V(2).InfoS("Adding a cluster to the delete stage", "cluster", binding.Spec.TargetCluster, "clusterStagedUpdateStrategy", updateStrategy.Name, "clusterStagedUpdateRun", updateRunRef)
-		toBeDeletedClusters[i].ClusterName = binding.Spec.TargetCluster
+		bindingSpec := binding.GetBindingSpec()
+		klog.V(2).InfoS("Adding a cluster to the delete stage", "cluster", bindingSpec.TargetCluster, "stagedUpdateStrategy", updateStrategy.GetName(), "stagedUpdateRun", updateRunRef)
+		toBeDeletedClusters[i].ClusterName = bindingSpec.TargetCluster
 	}
 	// Sort the clusters in the deletion stage by their names.
 	sort.Slice(toBeDeletedClusters, func(i, j int) bool {
 		return toBeDeletedClusters[i].ClusterName < toBeDeletedClusters[j].ClusterName
 	})
-	updateRun.Status.DeletionStageStatus = &placementv1beta1.StageUpdatingStatus{
+
+	// Update deletion stage status using interface methods
+	updateRunStatus = updateRun.GetStagedUpdateRunStatus()
+	updateRunStatus.DeletionStageStatus = &placementv1beta1.StageUpdatingStatus{
 		StageName: placementv1beta1.UpdateRunDeleteStageName,
 		Clusters:  toBeDeletedClusters,
 	}
+	updateRun.SetStagedUpdateRunStatus(*updateRunStatus)
 	return nil
 }
 

pkg/utils/controller/strategy_resolver.go

@@ -0,0 +1,45 @@
+/*
+Copyright 2025 The KubeFleet 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 controller
+
+import (
+	"context"
+
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1"
+)
+
+// FetchStagedUpdateStrategyFromNamespacedName resolves a NamespacedName to a concrete staged update strategy object that implements StagedUpdateStrategyObj.
+// If Namespace is empty, it fetches a ClusterStagedUpdateStrategy (cluster-scoped).
+// If Namespace is not empty, it fetches a StagedUpdateStrategy (namespace-scoped).
+func FetchStagedUpdateStrategyFromNamespacedName(ctx context.Context, c client.Reader, strategyKey types.NamespacedName) (placementv1beta1.StagedUpdateStrategyObj, error) {
+	var strategy placementv1beta1.StagedUpdateStrategyObj
+	// If Namespace is empty, it's a cluster-scoped strategy (ClusterStagedUpdateStrategy)
+	if strategyKey.Namespace == "" {
+		strategy = &placementv1beta1.ClusterStagedUpdateStrategy{}
+	} else {
+		// Otherwise, it's a namespaced strategy (StagedUpdateStrategy)
+		strategy = &placementv1beta1.StagedUpdateStrategy{}
+	}
+	err := c.Get(ctx, strategyKey, strategy)
+	if err != nil {
+		return nil, err
+	}
+	return strategy, nil
+}