Skip to content

Commit 1c44de9

Browse files
committed
address comments
Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>
1 parent bff9508 commit 1c44de9

File tree

6 files changed

+67
-43
lines changed

6 files changed

+67
-43
lines changed

apis/placement/v1beta1/stageupdate_types.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ type UpdateRunSpec struct {
159159

160160
// The resource snapshot index of the selected resources to be updated across clusters.
161161
// The index represents a group of resource snapshots that includes all the resources a ResourcePlacement selected.
162-
// +kubebuilder:validation:optional
162+
// +kubebuilder:validation:Optional
163163
ResourceSnapshotIndex string `json:"resourceSnapshotIndex"`
164164

165165
// The name of the update strategy that specifies the stages and the sequence
@@ -335,6 +335,10 @@ type UpdateRunStatus struct {
335335
// +kubebuilder:validation:Optional
336336
PolicyObservedClusterCount int `json:"policyObservedClusterCount,omitempty"`
337337

338+
// ResourceSnapshotName records the name of the resource snapshot that the update run is based on.
339+
// +kubebuilder:validation:Optional
340+
ResourceSnapshotName string `json:"resourceSnapshotName,omitempty"`
341+
338342
// ApplyStrategy is the apply strategy that the stagedUpdateRun is using.
339343
// It is the same as the apply strategy in the CRP when the staged update run starts.
340344
// The apply strategy is not updated during the update run even if it changes in the CRP.

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1175,7 +1175,6 @@ spec:
11751175
type: string
11761176
required:
11771177
- placementName
1178-
- resourceSnapshotIndex
11791178
- stagedRolloutStrategyName
11801179
type: object
11811180
x-kubernetes-validations:
@@ -1856,6 +1855,10 @@ spec:
18561855
All clusters involved in the update run are selected from the list of clusters scheduled by the CRP according
18571856
to the current policy.
18581857
type: string
1858+
resourceSnapshotName:
1859+
description: ResourceSnapshotName records the name of the resource
1860+
snapshot that the update run is based on.
1861+
type: string
18591862
stagedUpdateStrategySnapshot:
18601863
description: |-
18611864
UpdateStrategySnapshot is the snapshot of the UpdateStrategy used for the update run.

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,6 @@ spec:
9494
type: string
9595
required:
9696
- placementName
97-
- resourceSnapshotIndex
9897
- stagedRolloutStrategyName
9998
type: object
10099
x-kubernetes-validations:
@@ -775,6 +774,10 @@ spec:
775774
All clusters involved in the update run are selected from the list of clusters scheduled by the CRP according
776775
to the current policy.
777776
type: string
777+
resourceSnapshotName:
778+
description: ResourceSnapshotName records the name of the resource
779+
snapshot that the update run is based on.
780+
type: string
778781
stagedUpdateStrategySnapshot:
779782
description: |-
780783
UpdateStrategySnapshot is the snapshot of the UpdateStrategy used for the update run.

pkg/controllers/updaterun/execution.go

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ import (
2121
"errors"
2222
"fmt"
2323
"reflect"
24-
"strconv"
2524
"time"
2625

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

pkg/controllers/updaterun/initialization.go

Lines changed: 44 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -469,42 +469,18 @@ func validateAfterStageTask(tasks []placementv1beta1.StageTask) error {
469469

470470
// recordOverrideSnapshots finds all the override snapshots that are associated with each cluster and record them in the UpdateRun status.
471471
func (r *Reconciler) recordOverrideSnapshots(ctx context.Context, placementKey types.NamespacedName, updateRun placementv1beta1.UpdateRunObj) error {
472-
var resourceSnapshotObjs []placementv1beta1.ResourceSnapshotObj
473-
var snapshotIndex int
474472
updateRunRef := klog.KObj(updateRun)
475473
updateRunSpec := updateRun.GetUpdateRunSpec()
476474
placementName := placementKey.Name
477-
if updateRunSpec.ResourceSnapshotIndex != "" {
478-
snapshotIndex, err := strconv.Atoi(updateRunSpec.ResourceSnapshotIndex)
479-
if err != nil || snapshotIndex < 0 {
480-
err := controller.NewUserError(fmt.Errorf("invalid resource snapshot index `%s` provided, expected an integer >= 0", updateRunSpec.ResourceSnapshotIndex))
481-
klog.ErrorS(err, "Failed to parse the resource snapshot index", "updateRun", updateRunRef)
482-
// no more retries here.
483-
return fmt.Errorf("%w: %s", errInitializedFailed, err.Error())
484-
}
485475

486-
resourceSnapshotList, err := controller.ListAllResourceSnapshotWithAnIndex(ctx, r.Client, updateRunSpec.ResourceSnapshotIndex, placementName, placementKey.Namespace)
487-
if err != nil {
488-
klog.ErrorS(err, "Failed to list the resourceSnapshots associated with the placement",
489-
"placement", placementKey, "resourceSnapshotIndex", snapshotIndex, "updateRun", updateRunRef)
490-
// err can be retried.
491-
return controller.NewAPIServerError(true, err)
492-
}
493-
resourceSnapshotObjs = resourceSnapshotList.GetResourceSnapshotObjs()
494-
} else {
495-
latestResourceSnapshot, err := controller.ListLatestResourceSnapshots(ctx, r.Client, placementKey)
496-
if err != nil {
497-
klog.ErrorS(err, "Failed to list the latest resourceSnapshots associated with the placement",
498-
"placement", placementKey, "updateRun", updateRunRef)
499-
// err can be retried.
500-
return controller.NewAPIServerError(true, err)
501-
}
502-
resourceSnapshotObjs = latestResourceSnapshot.GetResourceSnapshotObjs()
476+
resourceSnapshotObjs, err := r.getResourceSnapshotObjs(ctx, updateRunSpec, placementName, placementKey, updateRunRef)
477+
if err != nil {
478+
return err
503479
}
504480

505481
if len(resourceSnapshotObjs) == 0 {
506-
err := controller.NewUserError(fmt.Errorf("no resourceSnapshots with index `%d` found for placement `%s`", snapshotIndex, placementKey))
507-
klog.ErrorS(err, "No specified resourceSnapshots found", "updateRun", updateRunRef)
482+
err := controller.NewUserError(fmt.Errorf("no resourceSnapshots found for placement `%s`", placementKey))
483+
klog.ErrorS(err, "No resourceSnapshots found", "updateRun", updateRunRef)
508484
// no more retries here.
509485
return fmt.Errorf("%w: %s", errInitializedFailed, err.Error())
510486
}
@@ -518,15 +494,23 @@ func (r *Reconciler) recordOverrideSnapshots(ctx context.Context, placementKey t
518494
break
519495
}
520496
}
521-
522497
// No masterResourceSnapshot found.
523498
if masterResourceSnapshot == nil {
524-
err := controller.NewUnexpectedBehaviorError(fmt.Errorf("no master resourceSnapshot found for placement `%s` with index `%d`", placementKey, snapshotIndex))
499+
err := controller.NewUnexpectedBehaviorError(fmt.Errorf("no master resourceSnapshot found for placement %s", placementKey))
525500
klog.ErrorS(err, "Failed to find master resourceSnapshot", "updateRun", updateRunRef)
526501
// no more retries here.
527502
return fmt.Errorf("%w: %s", errInitializedFailed, err.Error())
528503
}
529-
klog.V(2).InfoS("Found master resourceSnapshot", "placement", placementKey, "index", snapshotIndex, "updateRun", updateRunRef)
504+
505+
klog.InfoS("Found master resourceSnapshot", "placement", placementKey, "index", updateRun.GetUpdateRunSpec().ResourceSnapshotIndex, "updateRun", updateRunRef)
506+
507+
// Update the resource snapshot name in the UpdateRun status.
508+
updateRun.GetUpdateRunStatus().ResourceSnapshotName = masterResourceSnapshot.GetName()
509+
if updateErr := r.Client.Status().Update(ctx, updateRun); updateErr != nil {
510+
klog.ErrorS(updateErr, "Failed to update the UpdateRun status with resource snapshot name", "updateRun", klog.KObj(updateRun), "resourceSnapshot", klog.KObj(masterResourceSnapshot))
511+
// updateErr can be retried.
512+
return controller.NewUpdateIgnoreConflictError(updateErr)
513+
}
530514

531515
resourceSnapshotRef := klog.KObj(masterResourceSnapshot)
532516
// Fetch all the matching overrides.
@@ -555,6 +539,34 @@ func (r *Reconciler) recordOverrideSnapshots(ctx context.Context, placementKey t
555539
return nil
556540
}
557541

542+
// getResourceSnapshotObjs retrieves the resource snapshot objects either by index or latest snapshots.
543+
func (r *Reconciler) getResourceSnapshotObjs(ctx context.Context, updateRunSpec *placementv1beta1.UpdateRunSpec, placementName string, placementKey types.NamespacedName, updateRunRef klog.ObjectRef) ([]placementv1beta1.ResourceSnapshotObj, error) {
544+
if updateRunSpec.ResourceSnapshotIndex != "" {
545+
snapshotIndex, err := strconv.Atoi(updateRunSpec.ResourceSnapshotIndex)
546+
if err != nil || snapshotIndex < 0 {
547+
err := controller.NewUserError(fmt.Errorf("invalid resource snapshot index `%s` provided, expected an integer >= 0", updateRunSpec.ResourceSnapshotIndex))
548+
klog.ErrorS(err, "Failed to parse the resource snapshot index", "updateRun", updateRunRef)
549+
return nil, fmt.Errorf("%w: %s", errInitializedFailed, err.Error())
550+
}
551+
552+
resourceSnapshotList, err := controller.ListAllResourceSnapshotWithAnIndex(ctx, r.Client, updateRunSpec.ResourceSnapshotIndex, placementName, placementKey.Namespace)
553+
if err != nil {
554+
klog.ErrorS(err, "Failed to list the resourceSnapshots associated with the placement",
555+
"placement", placementKey, "resourceSnapshotIndex", snapshotIndex, "updateRun", updateRunRef)
556+
return nil, controller.NewAPIServerError(true, err)
557+
}
558+
return resourceSnapshotList.GetResourceSnapshotObjs(), nil
559+
}
560+
561+
latestResourceSnapshots, err := controller.ListLatestResourceSnapshots(ctx, r.Client, placementKey)
562+
if err != nil {
563+
klog.ErrorS(err, "Failed to list the latest resourceSnapshots associated with the placement",
564+
"placement", placementKey, "updateRun", updateRunRef)
565+
return nil, controller.NewAPIServerError(true, err)
566+
}
567+
return latestResourceSnapshots.GetResourceSnapshotObjs(), nil
568+
}
569+
558570
// recordInitializationSucceeded records the successful initialization condition in the UpdateRun status.
559571
func (r *Reconciler) recordInitializationSucceeded(ctx context.Context, updateRun placementv1beta1.UpdateRunObj) error {
560572
updateRunStatus := updateRun.GetUpdateRunStatus()

pkg/controllers/updaterun/initialization_integration_test.go

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -655,6 +655,7 @@ var _ = Describe("Updaterun initialization tests", func() {
655655
}
656656

657657
want := generateSucceededInitializationStatus(crp, updateRun, policySnapshot, updateStrategy, clusterResourceOverride)
658+
want.ResourceSnapshotName = "" // resourceSnapshot not created in this test
658659
// No clusters should be selected in the first stage.
659660
want.StagesStatus[0].Clusters = []placementv1beta1.ClusterUpdatingStatus{}
660661
// All clusters should be selected in the second stage and sorted by name.
@@ -697,6 +698,7 @@ var _ = Describe("Updaterun initialization tests", func() {
697698
}
698699

699700
want := generateSucceededInitializationStatus(crp, updateRun, policySnapshot, updateStrategy, clusterResourceOverride)
701+
want.ResourceSnapshotName = "" // resourceSnapshot not created in this test
700702
for i := range want.StagesStatus[0].Clusters {
701703
// Remove the CROs, as they are not added in this test.
702704
want.StagesStatus[0].Clusters[i].ClusterResourceOverrideSnapshots = nil
@@ -777,7 +779,7 @@ var _ = Describe("Updaterun initialization tests", func() {
777779
Expect(k8sClient.Create(ctx, updateRun)).To(Succeed())
778780

779781
By("Validating the initialization failed")
780-
validateFailedInitCondition(ctx, updateRun, "no resourceSnapshots with index `0` found")
782+
validateFailedInitCondition(ctx, updateRun, "no resourceSnapshots found for placement")
781783

782784
By("Checking update run status metrics are emitted")
783785
validateUpdateRunMetricsEmitted(generateInitializationFailedMetric(updateRun))
@@ -792,7 +794,7 @@ var _ = Describe("Updaterun initialization tests", func() {
792794
Expect(k8sClient.Create(ctx, updateRun)).To(Succeed())
793795

794796
By("Validating the initialization failed")
795-
validateFailedInitCondition(ctx, updateRun, "no resourceSnapshots with index `0` found")
797+
validateFailedInitCondition(ctx, updateRun, "no resourceSnapshots found for placement")
796798

797799
By("Checking update run status metrics are emitted")
798800
validateUpdateRunMetricsEmitted(generateInitializationFailedMetric(updateRun))
@@ -807,7 +809,7 @@ var _ = Describe("Updaterun initialization tests", func() {
807809
Expect(k8sClient.Create(ctx, updateRun)).To(Succeed())
808810

809811
By("Validating the initialization failed")
810-
validateFailedInitCondition(ctx, updateRun, "no resourceSnapshots with index `0` found")
812+
validateFailedInitCondition(ctx, updateRun, "no resourceSnapshots found for placement")
811813

812814
By("Checking update run status metrics are emitted")
813815
validateUpdateRunMetricsEmitted(generateInitializationFailedMetric(updateRun))
@@ -841,6 +843,7 @@ var _ = Describe("Updaterun initialization tests", func() {
841843

842844
By("Validating the clusterStagedUpdateRun stats")
843845
initialized := generateSucceededInitializationStatus(crp, updateRun, policySnapshot, updateStrategy, clusterResourceOverride)
846+
initialized.ResourceSnapshotName = testResourceSnapshotName
844847
want := generateExecutionStartedStatus(updateRun, initialized)
845848
validateClusterStagedUpdateRunStatus(ctx, updateRun, want, "")
846849

@@ -863,6 +866,7 @@ var _ = Describe("Updaterun initialization tests", func() {
863866

864867
By("Validating the clusterStagedUpdateRun stats")
865868
initialized := generateSucceededInitializationStatus(crp, updateRun, policySnapshot, updateStrategy, clusterResourceOverride)
869+
initialized.ResourceSnapshotName = testResourceSnapshotName
866870
want := generateExecutionStartedStatus(updateRun, initialized)
867871
validateClusterStagedUpdateRunStatus(ctx, updateRun, want, "")
868872

@@ -902,6 +906,7 @@ func generateSucceededInitializationStatus(
902906
status := &placementv1beta1.UpdateRunStatus{
903907
PolicySnapshotIndexUsed: policySnapshot.Labels[placementv1beta1.PolicyIndexLabel],
904908
PolicyObservedClusterCount: 10,
909+
ResourceSnapshotName: testResourceSnapshotName,
905910
ApplyStrategy: crp.Spec.Strategy.ApplyStrategy.DeepCopy(),
906911
UpdateStrategySnapshot: &updateStrategy.Spec,
907912
StagesStatus: []placementv1beta1.StageUpdatingStatus{
@@ -962,6 +967,7 @@ func generateSucceededInitializationStatusForSmallClusters(
962967
status := &placementv1beta1.UpdateRunStatus{
963968
PolicySnapshotIndexUsed: policySnapshot.Labels[placementv1beta1.PolicyIndexLabel],
964969
PolicyObservedClusterCount: 3,
970+
ResourceSnapshotName: testResourceSnapshotName,
965971
ApplyStrategy: crp.Spec.Strategy.ApplyStrategy.DeepCopy(),
966972
UpdateStrategySnapshot: &updateStrategy.Spec,
967973
StagesStatus: []placementv1beta1.StageUpdatingStatus{

0 commit comments

Comments
 (0)