address comments

Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

Author: britaniar

apis/placement/v1beta1/stageupdate_types.go

@@ -374,9 +374,10 @@ type UpdateRunStatus struct {
 	// +kubebuilder:validation:Optional
 	PolicyObservedClusterCount int `json:"policyObservedClusterCount,omitempty"`
 
-	// ResourceSnapshotName records the name of the master resource snapshot that the update run is based on.
-	// +kubebuilder:validation:Optional
-	ResourceSnapshotName string `json:"resourceSnapshotName,omitempty"`
+	// ResourceSnapshotIndexUsed records the resource snapshot index that the update run is based on.
+	// The index represents the same resource snapshots as specified in the spec field, or the latest.
+	// +kubbebuilder:validation:Optional
+	ResourceSnapshotIndexUsed string `json:"resourceSnapshotIndexUsed,omitempty"`
 
 	// ApplyStrategy is the apply strategy that the stagedUpdateRun is using.
 	// It is the same as the apply strategy in the CRP when the staged update run starts.

config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdateruns.yaml

@@ -1894,9 +1894,10 @@ spec:
                   All clusters involved in the update run are selected from the list of clusters scheduled by the CRP according
                   to the current policy.
                 type: string
-              resourceSnapshotName:
-                description: ResourceSnapshotName records the name of the master resource
-                  snapshot that the update run is based on.
+              resourceSnapshotIndexUsed:
+                description: |-
+                  ResourceSnapshotIndexUsed records the resource snapshot index that the update run is based on.
+                  The index represents the same resource snapshots as specified in the spec field, or the latest.
                 type: string
               stagedUpdateStrategySnapshot:
                 description: |-

config/crd/bases/placement.kubernetes-fleet.io_stagedupdateruns.yaml

@@ -814,9 +814,10 @@ spec:
                   All clusters involved in the update run are selected from the list of clusters scheduled by the CRP according
                   to the current policy.
                 type: string
-              resourceSnapshotName:
-                description: ResourceSnapshotName records the name of the master resource
-                  snapshot that the update run is based on.
+              resourceSnapshotIndexUsed:
+                description: |-
+                  ResourceSnapshotIndexUsed records the resource snapshot index that the update run is based on.
+                  The index represents the same resource snapshots as specified in the spec field, or the latest.
                 type: string
               stagedUpdateStrategySnapshot:
                 description: |-

pkg/controllers/updaterun/execution.go

@@ -21,6 +21,7 @@ import (
 	"errors"
 	"fmt"
 	"reflect"
+	"strconv"
 	"time"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -92,8 +93,11 @@ func (r *Reconciler) executeUpdatingStage(
 	toBeUpdatedBindings []placementv1beta1.BindingObj,
 ) (time.Duration, error) {
 	updateRunStatus := updateRun.GetUpdateRunStatus()
+	updateRunSpec := updateRun.GetUpdateRunSpec()
 	updatingStageStatus := &updateRunStatus.StagesStatus[updatingStageIndex]
-	resourceSnapshotName := updateRunStatus.ResourceSnapshotName
+	// The parse error is ignored because the initialization should have caught it.
+	resourceIndex, _ := strconv.Atoi(updateRunStatus.ResourceSnapshotIndexUsed)
+	resourceSnapshotName := fmt.Sprintf(placementv1beta1.ResourceSnapshotNameFmt, updateRunSpec.PlacementName, resourceIndex)
 	updateRunRef := klog.KObj(updateRun)
 	// Create the map of the toBeUpdatedBindings.
 	toBeUpdatedBindingsMap := make(map[string]placementv1beta1.BindingObj, len(toBeUpdatedBindings))

pkg/controllers/updaterun/execution_integration_test.go

@@ -160,7 +160,7 @@ var _ = Describe("UpdateRun execution tests - double stages", func() {
 			Expect(k8sClient.Create(ctx, updateRun)).To(Succeed())
 
 			By("Validating the initialization succeeded and the execution started")
-			initialized := generateSucceededInitializationStatus(crp, updateRun, policySnapshot, updateStrategy, clusterResourceOverride)
+			initialized := generateSucceededInitializationStatus(crp, updateRun, testResourceSnapshotIndex, policySnapshot, updateStrategy, clusterResourceOverride)
 			wantStatus = generateExecutionStartedStatus(updateRun, initialized)
 			validateClusterStagedUpdateRunStatus(ctx, updateRun, wantStatus, "")
 
@@ -521,7 +521,7 @@ var _ = Describe("UpdateRun execution tests - double stages", func() {
 			Expect(k8sClient.Create(ctx, updateRun)).To(Succeed())
 
 			By("Validating the initialization succeeded and the execution started")
-			initialized := generateSucceededInitializationStatus(crp, updateRun, policySnapshot, updateStrategy, clusterResourceOverride)
+			initialized := generateSucceededInitializationStatus(crp, updateRun, testResourceSnapshotIndex, policySnapshot, updateStrategy, clusterResourceOverride)
 			wantStatus = generateExecutionStartedStatus(updateRun, initialized)
 			validateClusterStagedUpdateRunStatus(ctx, updateRun, wantStatus, "")
 
@@ -680,7 +680,7 @@ var _ = Describe("UpdateRun execution tests - single stage", func() {
 			Expect(k8sClient.Create(ctx, updateRun)).To(Succeed())
 
 			By("Validating the initialization succeeded and the execution started")
-			initialized := generateSucceededInitializationStatusForSmallClusters(crp, updateRun, policySnapshot, updateStrategy)
+			initialized := generateSucceededInitializationStatusForSmallClusters(crp, updateRun, testResourceSnapshotIndex, policySnapshot, updateStrategy)
 			wantStatus = generateExecutionStartedStatus(updateRun, initialized)
 			validateClusterStagedUpdateRunStatus(ctx, updateRun, wantStatus, "")
 
@@ -774,7 +774,7 @@ var _ = Describe("UpdateRun execution tests - single stage", func() {
 			Expect(k8sClient.Create(ctx, updateRun)).To(Succeed())
 
 			By("Validating the initialization succeeded and the execution started")
-			initialized := generateSucceededInitializationStatusForSmallClusters(crp, updateRun, policySnapshot, updateStrategy)
+			initialized := generateSucceededInitializationStatusForSmallClusters(crp, updateRun, testResourceSnapshotIndex, policySnapshot, updateStrategy)
 			wantStatus = generateExecutionStartedStatus(updateRun, initialized)
 			validateClusterStagedUpdateRunStatus(ctx, updateRun, wantStatus, "")
 
@@ -883,7 +883,7 @@ var _ = Describe("UpdateRun execution tests - single stage", func() {
 			Expect(k8sClient.Create(ctx, updateRun)).To(Succeed())
 
 			By("Validating the initialization succeeded and the execution started")
-			initialized := generateSucceededInitializationStatusForSmallClusters(crp, updateRun, policySnapshot, updateStrategy)
+			initialized := generateSucceededInitializationStatusForSmallClusters(crp, updateRun, testResourceSnapshotIndex, policySnapshot, updateStrategy)
 			wantStatus = generateExecutionStartedStatus(updateRun, initialized)
 			validateClusterStagedUpdateRunStatus(ctx, updateRun, wantStatus, "")
 
@@ -1014,7 +1014,7 @@ var _ = Describe("UpdateRun execution tests - single stage", func() {
 			Expect(k8sClient.Create(ctx, updateRun)).To(Succeed())
 
 			By("Validating the initialization succeeded and the execution started")
-			initialized := generateSucceededInitializationStatusForSmallClusters(crp, updateRun, policySnapshot, updateStrategy)
+			initialized := generateSucceededInitializationStatusForSmallClusters(crp, updateRun, testResourceSnapshotIndex, policySnapshot, updateStrategy)
 			wantStatus = generateExecutionStartedStatus(updateRun, initialized)
 			validateClusterStagedUpdateRunStatus(ctx, updateRun, wantStatus, "")
 
@@ -1106,7 +1106,7 @@ var _ = Describe("UpdateRun execution tests - single stage", func() {
 			Expect(k8sClient.Create(ctx, updateRun)).To(Succeed())
 
 			By("Validating the initialization succeeded and the execution started")
-			initialized := generateSucceededInitializationStatusForSmallClusters(crp, updateRun, policySnapshot, updateStrategy)
+			initialized := generateSucceededInitializationStatusForSmallClusters(crp, updateRun, testResourceSnapshotIndex, policySnapshot, updateStrategy)
 			wantStatus = generateExecutionStartedStatus(updateRun, initialized)
 			validateClusterStagedUpdateRunStatus(ctx, updateRun, wantStatus, "")
 
@@ -1163,7 +1163,7 @@ var _ = Describe("UpdateRun execution tests - single stage", func() {
 			Expect(k8sClient.Create(ctx, updateRun)).To(Succeed())
 
 			By("Validating the initialization succeeded and the execution started")
-			initialized := generateSucceededInitializationStatusForSmallClusters(crp, updateRun, policySnapshot, updateStrategy)
+			initialized := generateSucceededInitializationStatusForSmallClusters(crp, updateRun, testResourceSnapshotIndex, policySnapshot, updateStrategy)
 			wantStatus = generateExecutionStartedStatus(updateRun, initialized)
 			validateClusterStagedUpdateRunStatus(ctx, updateRun, wantStatus, "")
 		})

pkg/controllers/updaterun/initialization.go

@@ -471,9 +471,8 @@ func validateAfterStageTask(tasks []placementv1beta1.StageTask) error {
 func (r *Reconciler) recordOverrideSnapshots(ctx context.Context, placementKey types.NamespacedName, updateRun placementv1beta1.UpdateRunObj) error {
 	updateRunRef := klog.KObj(updateRun)
 	updateRunSpec := updateRun.GetUpdateRunSpec()
-	placementName := placementKey.Name
 
-	resourceSnapshotObjs, err := r.getResourceSnapshotObjs(ctx, updateRunSpec, placementName, placementKey, updateRunRef)
+	resourceSnapshotObjs, err := r.getResourceSnapshotObjs(ctx, placementKey, updateRun)
 	if err != nil {
 		return err
 	}
@@ -487,6 +486,7 @@ func (r *Reconciler) recordOverrideSnapshots(ctx context.Context, placementKey t
 			break
 		}
 	}
+
 	// No masterResourceSnapshot found.
 	if masterResourceSnapshot == nil {
 		err := controller.NewUnexpectedBehaviorError(fmt.Errorf("no master resourceSnapshot found for placement %s", placementKey))
@@ -497,14 +497,10 @@ func (r *Reconciler) recordOverrideSnapshots(ctx context.Context, placementKey t
 
 	klog.V(2).InfoS("Found master resourceSnapshot", "placement", placementKey, "masterResourceSnapshot", masterResourceSnapshot.GetName(), "updateRun", updateRunRef)
 
-	// Update the resource snapshot name in the UpdateRun status.
+	// Record the resource snapshot index used.
 	updateRunStatus := updateRun.GetUpdateRunStatus()
-	updateRunStatus.ResourceSnapshotName = masterResourceSnapshot.GetName()
-	if updateErr := r.Client.Status().Update(ctx, updateRun); updateErr != nil {
-		klog.ErrorS(updateErr, "Failed to update the UpdateRun status with resource snapshot name", "updateRun", klog.KObj(updateRun), "resourceSnapshot", klog.KObj(masterResourceSnapshot))
-		// updateErr can be retried.
-		return controller.NewUpdateIgnoreConflictError(updateErr)
-	}
+	updateRunStatus.ResourceSnapshotIndexUsed = masterResourceSnapshot.GetLabels()[placementv1beta1.ResourceIndexLabel]
+	updateRun.SetUpdateRunStatus(*updateRunStatus)
 
 	resourceSnapshotRef := klog.KObj(masterResourceSnapshot)
 	// Fetch all the matching overrides.
@@ -534,20 +530,24 @@ func (r *Reconciler) recordOverrideSnapshots(ctx context.Context, placementKey t
 
 // getResourceSnapshotObjs retrieves the list of resource snapshot objects from the specified ResourceSnapshotIndex.
 // If ResourceSnapshotIndex is unspecified, it returns the list of latest resource snapshots.
-func (r *Reconciler) getResourceSnapshotObjs(ctx context.Context, updateRunSpec *placementv1beta1.UpdateRunSpec, placementName string, placementKey types.NamespacedName, updateRunRef klog.ObjectRef) ([]placementv1beta1.ResourceSnapshotObj, error) {
+func (r *Reconciler) getResourceSnapshotObjs(ctx context.Context, placementKey types.NamespacedName, updateRun placementv1beta1.UpdateRunObj) ([]placementv1beta1.ResourceSnapshotObj, error) {
+	updateRunRef := klog.KObj(updateRun)
+	updateRunSpec := updateRun.GetUpdateRunSpec()
 	var resourceSnapshotObjs []placementv1beta1.ResourceSnapshotObj
 	if updateRunSpec.ResourceSnapshotIndex != "" {
 		snapshotIndex, err := strconv.Atoi(updateRunSpec.ResourceSnapshotIndex)
 		if err != nil || snapshotIndex < 0 {
 			err := controller.NewUserError(fmt.Errorf("invalid resource snapshot index `%s` provided, expected an integer >= 0", updateRunSpec.ResourceSnapshotIndex))
 			klog.ErrorS(err, "Failed to parse the resource snapshot index", "updateRun", updateRunRef)
+			// no more retries here.
 			return nil, fmt.Errorf("%w: %s", errInitializedFailed, err.Error())
 		}
 
-		resourceSnapshotList, err := controller.ListAllResourceSnapshotWithAnIndex(ctx, r.Client, updateRunSpec.ResourceSnapshotIndex, placementName, placementKey.Namespace)
+		resourceSnapshotList, err := controller.ListAllResourceSnapshotWithAnIndex(ctx, r.Client, updateRunSpec.ResourceSnapshotIndex, placementKey.Name, placementKey.Namespace)
 		if err != nil {
 			klog.ErrorS(err, "Failed to list the resourceSnapshots associated with the placement",
 				"placement", placementKey, "resourceSnapshotIndex", snapshotIndex, "updateRun", updateRunRef)
+			// list err can be retried.
 			return nil, controller.NewAPIServerError(true, err)
 		}
 
@@ -561,18 +561,20 @@ func (r *Reconciler) getResourceSnapshotObjs(ctx context.Context, updateRunSpec
 		return resourceSnapshotObjs, nil
 	}
 
+	klog.V(2).InfoS("No resource snapshot index specified, fetching latest resource snapshots", "placement", placementKey, "updateRun", updateRunRef)
 	latestResourceSnapshots, err := controller.ListLatestResourceSnapshots(ctx, r.Client, placementKey)
 	if err != nil {
 		klog.ErrorS(err, "Failed to list the latest resourceSnapshots associated with the placement",
 			"placement", placementKey, "updateRun", updateRunRef)
+		// list err can be retried.
 		return nil, controller.NewAPIServerError(true, err)
 	}
 
 	resourceSnapshotObjs = latestResourceSnapshots.GetResourceSnapshotObjs()
 	if len(resourceSnapshotObjs) == 0 {
-		err := controller.NewUserError(fmt.Errorf("no resourceSnapshots found for placement `%s`", placementKey))
-		klog.ErrorS(err, "No resourceSnapshots found", "updateRun", updateRunRef)
-		// no more retries here.
+		err := fmt.Errorf("no latest resourceSnapshots found for placement `%s`. This might be a transient state, need retry", placementKey)
+		klog.ErrorS(err, "No latest resourceSnapshots found for placement. This might be transient, need retry", "placement", placementKey, "updateRun", updateRunRef)
+		// retryable error.
 		return resourceSnapshotObjs, fmt.Errorf("%w: %s", errInitializedFailed, err.Error())
 	}
 	return resourceSnapshotObjs, nil