From 1965fde5854f260a7ac76fdc702d8a33d638c7e3 Mon Sep 17 00:00:00 2001 From: Nebojsa Prodana Date: Tue, 8 Jun 2021 12:32:39 +0200 Subject: [PATCH 01/13] Fix status update --- api/v1alpha1/condition_types.go | 4 + ...oproj.skyscanner.net_progressivesyncs.yaml | 2 +- controllers/progressivesync_controller.go | 74 +++++++++++++++---- internal/scheduler/scheduler.go | 8 +- 4 files changed, 73 insertions(+), 15 deletions(-) diff --git a/api/v1alpha1/condition_types.go b/api/v1alpha1/condition_types.go index 1bab8861..b841ca67 100644 --- a/api/v1alpha1/condition_types.go +++ b/api/v1alpha1/condition_types.go @@ -25,6 +25,10 @@ const ( // the progressive sync stages have been completed StagesCompleteReason string = "StagesCompleted" + // StagesCompleteReason represent the fact that all + // the progressive sync stages have been completed + StagesInitializedReason string = "StagesInitialized" + // StagesFailedReason represent the fact that a // progressive sync stage has failed StagesFailedReason string = "StagesFailed" diff --git a/config/crd/bases/argoproj.skyscanner.net_progressivesyncs.yaml b/config/crd/bases/argoproj.skyscanner.net_progressivesyncs.yaml index 93eec3ce..c37d3097 100644 --- a/config/crd/bases/argoproj.skyscanner.net_progressivesyncs.yaml +++ b/config/crd/bases/argoproj.skyscanner.net_progressivesyncs.yaml @@ -228,7 +228,7 @@ spec: type: string name: type: string - stage: + phase: description: StageStatusPhase defines the observed stage phase enum: - Progressing diff --git a/controllers/progressivesync_controller.go b/controllers/progressivesync_controller.go index 7dfb47c8..a9450edb 100644 --- a/controllers/progressivesync_controller.go +++ b/controllers/progressivesync_controller.go @@ -33,6 +33,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/retry" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -69,6 +70,11 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, client.IgnoreNotFound(err) } + if err := r.initializeStatus(ctx, &pr); err != nil { + log.Error(err, "unable to clear status") + return ctrl.Result{}, err + } + log = r.Log.WithValues("applicationset", pr.Spec.SourceRef.Name) if pr.ObjectMeta.DeletionTimestamp.IsZero() { @@ -143,7 +149,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ // TODO: status status - update targets for _, s := range scheduledApps { - r.Log.Info("syncing app", "app", s) + log.Info("syncing app", "app", s) _, err := r.syncApp(ctx, s.Name) @@ -164,7 +170,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ if scheduler.IsStageFailed(apps) { message := "stage failed" - r.Log.Info(message) + log.Info(message) if err := r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseFailed, &pr); err != nil { return ctrl.Result{}, err } @@ -176,14 +182,14 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ r.Log.Error(err, "failed to update object status") return ctrl.Result{}, err } - r.Log.Info("rollout failed") + log.Info("rollout failed") // We can set Requeue: true once we have a timeout in place return ctrl.Result{}, nil } if scheduler.IsStageInProgress(apps) { message := "stage in progress" - r.Log.Info(message) + log.Info(message) if err := r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseProgressing, &pr); err != nil { return ctrl.Result{}, err } @@ -193,13 +199,13 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ if scheduler.IsStageComplete(apps) { message := "stage completed" - r.Log.Info(message) + log.Info(message) if err := r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseSucceeded, &pr); err != nil { return ctrl.Result{}, err } } else { message := "stage is unknown" - r.Log.Info(message) + log.Info(message) if err := r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr); err != nil { return ctrl.Result{}, err } @@ -224,7 +230,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ // SetupWithManager adds the reconciler to the manager, so that it gets started when the manager is started. func (r *ProgressiveSyncReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). - For(&syncv1alpha1.ProgressiveSync{}). + For(&syncv1alpha1.ProgressiveSync{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). Watches( &source.Kind{Type: &argov1alpha1.Application{}}, handler.EnqueueRequestsFromMapFunc(r.requestsForApplicationChange)). @@ -252,6 +258,7 @@ func (r *ProgressiveSyncReconciler) requestsForApplicationChange(o client.Object r.Log.Error(err, "failed to convert object to application") return nil } + // r.Log.Info("received app event", "name", app.ObjectMeta.Name, "reconciledAt", app.Status.ReconciledAt.String()) if err := r.List(ctx, &list); err != nil { r.Log.Error(err, "failed to list ProgressiveSync") @@ -420,10 +427,51 @@ func (r *ProgressiveSyncReconciler) syncApp(ctx context.Context, appName string) // patchStatus patches the progressive sync object status func (r *ProgressiveSyncReconciler) patchStatus(ctx context.Context, pr *syncv1alpha1.ProgressiveSync) error { - key := client.ObjectKeyFromObject(pr) - latest := syncv1alpha1.ProgressiveSync{} - if err := r.Client.Get(ctx, key, &latest); err != nil { - return err - } - return r.Client.Status().Patch(ctx, pr, client.MergeFrom(&latest)) + // key := client.ObjectKeyFromObject(pr) + // latest := syncv1alpha1.ProgressiveSync{} + // if err := r.Client.Get(ctx, key, &latest); err != nil { + // return err + // } + // return r.Client.Status().Patch(ctx, pr, client.MergeFrom(&latest)) + + retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error { + + key := client.ObjectKeyFromObject(pr) + latest := syncv1alpha1.ProgressiveSync{} + if err := r.Client.Get(ctx, key, &latest); err != nil { + return err + } + latest.Status = pr.Status + if err := r.Client.Status().Update(ctx, &latest); err != nil { + return err + } + pr = &latest + return nil + + }) + return retryErr +} + +// initializeStatus initializes the progressive sync object status to prevent patching stale status +func (r *ProgressiveSyncReconciler) initializeStatus(ctx context.Context, pr *syncv1alpha1.ProgressiveSync) error { + + retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error { + + key := client.ObjectKeyFromObject(pr) + latest := syncv1alpha1.ProgressiveSync{} + if err := r.Client.Get(ctx, key, &latest); err != nil { + return err + } + latest.Status = syncv1alpha1.ProgressiveSyncStatus{} + condition := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesInitializedReason, "Stages initialized") + apimeta.SetStatusCondition(pr.GetStatusConditions(), condition) + + // if err := r.Client.Status().Update(ctx, &latest); err != nil { + // return err + // } + pr = &latest + return nil + + }) + return retryErr } diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 5e03a712..7f2da3f2 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -1,6 +1,8 @@ package scheduler import ( + "math" + syncv1alpha1 "github.com/Skyscanner/applicationset-progressive-sync/api/v1alpha1" "github.com/Skyscanner/applicationset-progressive-sync/internal/utils" argov1alpha1 "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1" @@ -61,7 +63,11 @@ func Scheduler(apps []argov1alpha1.Application, stage syncv1alpha1.ProgressiveSy return scheduledApps } - for i := 0; i < maxParallel-len(progressingApps); i++ { + // It may be that at point when apps were observed, no apps were progressing yet + // so maxParallel-len(progressingApps) might actually be greater than len(outOfSyncApps) + // causing the runtime to panic + p := (int)(math.Min((float64)(maxParallel-len(progressingApps)), (float64)(len(outOfSyncApps)))) + for i := 0; i < p; i++ { scheduledApps = append(scheduledApps, outOfSyncApps[i]) } return scheduledApps From ca87e605eeae54679d101136daaad79f267ce6b4 Mon Sep 17 00:00:00 2001 From: Nebojsa Prodana Date: Tue, 8 Jun 2021 14:13:30 +0200 Subject: [PATCH 02/13] Clean the code a bit --- api/v1alpha1/condition_types.go | 8 +++-- controllers/progressivesync_controller.go | 37 +++++++++++------------ 2 files changed, 23 insertions(+), 22 deletions(-) diff --git a/api/v1alpha1/condition_types.go b/api/v1alpha1/condition_types.go index b841ca67..f923c21a 100644 --- a/api/v1alpha1/condition_types.go +++ b/api/v1alpha1/condition_types.go @@ -25,10 +25,14 @@ const ( // the progressive sync stages have been completed StagesCompleteReason string = "StagesCompleted" - // StagesCompleteReason represent the fact that all - // the progressive sync stages have been completed + // StagesInitializedReason represent the fact that all + // the progressive sync stages have been initialized StagesInitializedReason string = "StagesInitialized" + // StagesProgressingReason represent the fact that some + // of the progressive sync stages are progressing + StagesProgressingReason string = "StagesProgressing" + // StagesFailedReason represent the fact that a // progressive sync stage has failed StagesFailedReason string = "StagesFailed" diff --git a/controllers/progressivesync_controller.go b/controllers/progressivesync_controller.go index a9450edb..31d06143 100644 --- a/controllers/progressivesync_controller.go +++ b/controllers/progressivesync_controller.go @@ -178,7 +178,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ failedMessage := fmt.Sprintf("%s stage failed", stage.Name) failed := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionTrue, syncv1alpha1.StagesFailedReason, failedMessage) apimeta.SetStatusCondition(pr.GetStatusConditions(), failed) - if err := r.patchStatus(ctx, &pr); err != nil { + if err := r.setStatus(ctx, &pr); err != nil { r.Log.Error(err, "failed to update object status") return ctrl.Result{}, err } @@ -188,8 +188,10 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ } if scheduler.IsStageInProgress(apps) { - message := "stage in progress" + message := fmt.Sprintf("%s stage in progress", stage.Name) log.Info(message) + progress := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesProgressingReason, message) + apimeta.SetStatusCondition(pr.GetStatusConditions(), progress) if err := r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseProgressing, &pr); err != nil { return ctrl.Result{}, err } @@ -198,14 +200,19 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ } if scheduler.IsStageComplete(apps) { - message := "stage completed" + message := fmt.Sprintf("%s stage completed", stage.Name) + log.Info(message) + progress := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesProgressingReason, message) + apimeta.SetStatusCondition(pr.GetStatusConditions(), progress) log.Info(message) if err := r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseSucceeded, &pr); err != nil { return ctrl.Result{}, err } } else { - message := "stage is unknown" + message := fmt.Sprintf("%s stage in unknown state", stage.Name) log.Info(message) + progress := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesProgressingReason, message) + apimeta.SetStatusCondition(pr.GetStatusConditions(), progress) if err := r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr); err != nil { return ctrl.Result{}, err } @@ -219,7 +226,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ // Progressive rollout completed completed := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionTrue, syncv1alpha1.StagesCompleteReason, "All stages completed") apimeta.SetStatusCondition(pr.GetStatusConditions(), completed) - if err := r.patchStatus(ctx, &pr); err != nil { + if err := r.setStatus(ctx, &pr); err != nil { r.Log.Error(err, "failed to update object status") return ctrl.Result{}, err } @@ -258,7 +265,6 @@ func (r *ProgressiveSyncReconciler) requestsForApplicationChange(o client.Object r.Log.Error(err, "failed to convert object to application") return nil } - // r.Log.Info("received app event", "name", app.ObjectMeta.Name, "reconciledAt", app.Status.ReconciledAt.String()) if err := r.List(ctx, &list); err != nil { r.Log.Error(err, "failed to list ProgressiveSync") @@ -409,7 +415,7 @@ func (r *ProgressiveSyncReconciler) updateStageStatus(ctx context.Context, name, ) nowTime := metav1.NewTime(time.Now()) pr.SetStageStatus(stageStatus, &nowTime) - if err := r.patchStatus(ctx, pr); err != nil { + if err := r.setStatus(ctx, pr); err != nil { r.Log.Error(err, "failed to update object status") return err } @@ -425,14 +431,8 @@ func (r *ProgressiveSyncReconciler) syncApp(ctx context.Context, appName string) return r.ArgoCDAppClient.Sync(ctx, &syncReq) } -// patchStatus patches the progressive sync object status -func (r *ProgressiveSyncReconciler) patchStatus(ctx context.Context, pr *syncv1alpha1.ProgressiveSync) error { - // key := client.ObjectKeyFromObject(pr) - // latest := syncv1alpha1.ProgressiveSync{} - // if err := r.Client.Get(ctx, key, &latest); err != nil { - // return err - // } - // return r.Client.Status().Patch(ctx, pr, client.MergeFrom(&latest)) +// setStatus updates the progressive sync object status with backoff +func (r *ProgressiveSyncReconciler) setStatus(ctx context.Context, pr *syncv1alpha1.ProgressiveSync) error { retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error { @@ -452,7 +452,7 @@ func (r *ProgressiveSyncReconciler) patchStatus(ctx context.Context, pr *syncv1a return retryErr } -// initializeStatus initializes the progressive sync object status to prevent patching stale status +// initializeStatus initializes the progressive sync object status to prevent conflicts when updating status later on func (r *ProgressiveSyncReconciler) initializeStatus(ctx context.Context, pr *syncv1alpha1.ProgressiveSync) error { retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error { @@ -463,12 +463,9 @@ func (r *ProgressiveSyncReconciler) initializeStatus(ctx context.Context, pr *sy return err } latest.Status = syncv1alpha1.ProgressiveSyncStatus{} - condition := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesInitializedReason, "Stages initialized") + condition := latest.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesInitializedReason, "Stages initialized") apimeta.SetStatusCondition(pr.GetStatusConditions(), condition) - // if err := r.Client.Status().Update(ctx, &latest); err != nil { - // return err - // } pr = &latest return nil From a0f7606772c9e9a6482961ed62463a238f4a3f71 Mon Sep 17 00:00:00 2001 From: Nebojsa Prodana Date: Tue, 8 Jun 2021 15:43:31 +0200 Subject: [PATCH 03/13] PR remarks, fix tests --- controllers/progressivesync_controller.go | 105 +++++++++++------- .../progressivesync_controller_test.go | 12 +- 2 files changed, 71 insertions(+), 46 deletions(-) diff --git a/controllers/progressivesync_controller.go b/controllers/progressivesync_controller.go index 31d06143..633ca6e4 100644 --- a/controllers/progressivesync_controller.go +++ b/controllers/progressivesync_controller.go @@ -71,7 +71,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ } if err := r.initializeStatus(ctx, &pr); err != nil { - log.Error(err, "unable to clear status") + log.Error(err, "unable to initialize status") return ctrl.Result{}, err } @@ -111,9 +111,13 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ if err != nil { message := "unable to fetch clusters" log.Error(err, message) - if err := r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr); err != nil { + r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr) + + if err := r.submitStatus(ctx, &pr); err != nil { + r.Log.Error(err, "failed to update object status") return ctrl.Result{}, err } + return ctrl.Result{}, err } r.Log.V(1).Info("clusters selected", "clusters", utils.GetClustersName(clusters.Items)) @@ -123,9 +127,13 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ if err != nil { message := "unable to fetch apps" log.Error(err, message) - if err := r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr); err != nil { + r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr) + + if err := r.submitStatus(ctx, &pr); err != nil { + r.Log.Error(err, "failed to update object status") return ctrl.Result{}, err } + return ctrl.Result{}, err } r.Log.V(1).Info("apps selected", "apps", utils.GetAppsName(apps)) @@ -136,7 +144,11 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ if err := r.removeAnnotationFromApps(ctx, outOfSyncApps, utils.ProgressiveSyncSyncedAtStageKey); err != nil { message := "unable to remove out-of-sync annotation" log.Error(err, message) - if err := r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr); err != nil { + + r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr) + + if err := r.submitStatus(ctx, &pr); err != nil { + r.Log.Error(err, "failed to update object status") return ctrl.Result{}, err } return ctrl.Result{}, err @@ -155,9 +167,16 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ if err != nil { if !strings.Contains(err.Error(), "another operation is already in progress") { - message := "unable to sync app" + message := "unable to remove out-of-sync annotation" log.Error(err, message, "message", err.Error()) - if err := r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr); err != nil { + + r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr) + + // TODO: Should this be reflected in the general condition? + // failed := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionTrue, syncv1alpha1.StagesFailedReason, message) + // apimeta.SetStatusCondition(pr.GetStatusConditions(), failed) + if err := r.submitStatus(ctx, &pr); err != nil { + r.Log.Error(err, "failed to update object status") return ctrl.Result{}, err } @@ -169,16 +188,14 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ } if scheduler.IsStageFailed(apps) { - message := "stage failed" - log.Info(message) - if err := r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseFailed, &pr); err != nil { - return ctrl.Result{}, err - } - failedMessage := fmt.Sprintf("%s stage failed", stage.Name) + log.Info(failedMessage) + + r.updateStageStatus(ctx, stage.Name, failedMessage, syncv1alpha1.PhaseFailed, &pr) + failed := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionTrue, syncv1alpha1.StagesFailedReason, failedMessage) apimeta.SetStatusCondition(pr.GetStatusConditions(), failed) - if err := r.setStatus(ctx, &pr); err != nil { + if err := r.submitStatus(ctx, &pr); err != nil { r.Log.Error(err, "failed to update object status") return ctrl.Result{}, err } @@ -190,11 +207,17 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ if scheduler.IsStageInProgress(apps) { message := fmt.Sprintf("%s stage in progress", stage.Name) log.Info(message) + + r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseProgressing, &pr) + progress := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesProgressingReason, message) apimeta.SetStatusCondition(pr.GetStatusConditions(), progress) - if err := r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseProgressing, &pr); err != nil { + + if err := r.submitStatus(ctx, &pr); err != nil { + r.Log.Error(err, "failed to update object status") return ctrl.Result{}, err } + // Stage in progress, we reconcile again until the stage is completed or failed return ctrl.Result{Requeue: true}, nil } @@ -202,20 +225,31 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ if scheduler.IsStageComplete(apps) { message := fmt.Sprintf("%s stage completed", stage.Name) log.Info(message) + + r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseSucceeded, &pr) + progress := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesProgressingReason, message) apimeta.SetStatusCondition(pr.GetStatusConditions(), progress) - log.Info(message) - if err := r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseSucceeded, &pr); err != nil { + + if err := r.submitStatus(ctx, &pr); err != nil { + r.Log.Error(err, "failed to update object status") return ctrl.Result{}, err } + } else { message := fmt.Sprintf("%s stage in unknown state", stage.Name) log.Info(message) + + r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr) + progress := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesProgressingReason, message) apimeta.SetStatusCondition(pr.GetStatusConditions(), progress) - if err := r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr); err != nil { + + if err := r.submitStatus(ctx, &pr); err != nil { + r.Log.Error(err, "failed to update object status") return ctrl.Result{}, err } + // We can set Requeue: true once we have a timeout in place return ctrl.Result{}, nil } @@ -226,7 +260,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ // Progressive rollout completed completed := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionTrue, syncv1alpha1.StagesCompleteReason, "All stages completed") apimeta.SetStatusCondition(pr.GetStatusConditions(), completed) - if err := r.setStatus(ctx, &pr); err != nil { + if err := r.submitStatus(ctx, &pr); err != nil { r.Log.Error(err, "failed to update object status") return ctrl.Result{}, err } @@ -407,7 +441,7 @@ func (r *ProgressiveSyncReconciler) removeAnnotationFromApps(ctx context.Context } // updateStageStatus updates the target stage given a stage name, message and phase -func (r *ProgressiveSyncReconciler) updateStageStatus(ctx context.Context, name, message string, phase syncv1alpha1.StageStatusPhase, pr *syncv1alpha1.ProgressiveSync) error { +func (r *ProgressiveSyncReconciler) updateStageStatus(ctx context.Context, name, message string, phase syncv1alpha1.StageStatusPhase, pr *syncv1alpha1.ProgressiveSync) { stageStatus := syncv1alpha1.NewStageStatus( name, message, @@ -415,11 +449,6 @@ func (r *ProgressiveSyncReconciler) updateStageStatus(ctx context.Context, name, ) nowTime := metav1.NewTime(time.Now()) pr.SetStageStatus(stageStatus, &nowTime) - if err := r.setStatus(ctx, pr); err != nil { - r.Log.Error(err, "failed to update object status") - return err - } - return nil } // syncApp sends a sync request for the target app @@ -431,8 +460,8 @@ func (r *ProgressiveSyncReconciler) syncApp(ctx context.Context, appName string) return r.ArgoCDAppClient.Sync(ctx, &syncReq) } -// setStatus updates the progressive sync object status with backoff -func (r *ProgressiveSyncReconciler) setStatus(ctx context.Context, pr *syncv1alpha1.ProgressiveSync) error { +// submitStatus updates the progressive sync object status with backoff +func (r *ProgressiveSyncReconciler) submitStatus(ctx context.Context, pr *syncv1alpha1.ProgressiveSync) error { retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error { @@ -455,20 +484,16 @@ func (r *ProgressiveSyncReconciler) setStatus(ctx context.Context, pr *syncv1alp // initializeStatus initializes the progressive sync object status to prevent conflicts when updating status later on func (r *ProgressiveSyncReconciler) initializeStatus(ctx context.Context, pr *syncv1alpha1.ProgressiveSync) error { - retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error { - - key := client.ObjectKeyFromObject(pr) - latest := syncv1alpha1.ProgressiveSync{} - if err := r.Client.Get(ctx, key, &latest); err != nil { - return err - } - latest.Status = syncv1alpha1.ProgressiveSyncStatus{} - condition := latest.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesInitializedReason, "Stages initialized") - apimeta.SetStatusCondition(pr.GetStatusConditions(), condition) + key := client.ObjectKeyFromObject(pr) + latest := syncv1alpha1.ProgressiveSync{} + if err := r.Client.Get(ctx, key, &latest); err != nil { + return err + } + latest.Status = syncv1alpha1.ProgressiveSyncStatus{} + condition := latest.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesInitializedReason, "Stages initialized") + apimeta.SetStatusCondition(pr.GetStatusConditions(), condition) - pr = &latest - return nil + pr = &latest + return nil - }) - return retryErr } diff --git a/controllers/progressivesync_controller_test.go b/controllers/progressivesync_controller_test.go index 6ab289d7..085073e7 100644 --- a/controllers/progressivesync_controller_test.go +++ b/controllers/progressivesync_controller_test.go @@ -333,7 +333,7 @@ var _ = Describe("ProgressiveRollout Controller", func() { ExpectStageStatus(ctx, prKey, "stage 0").Should(MatchStage(syncv1alpha1.StageStatus{ Name: "stage 0", Phase: syncv1alpha1.PhaseProgressing, - Message: "stage in progress", + Message: "stage 0 stage in progress", })) ExpectStagesInStatus(ctx, prKey).Should(Equal(1)) @@ -350,7 +350,7 @@ var _ = Describe("ProgressiveRollout Controller", func() { ExpectStageStatus(ctx, prKey, "stage 0").Should(MatchStage(syncv1alpha1.StageStatus{ Name: "stage 0", Phase: syncv1alpha1.PhaseSucceeded, - Message: "stage completed", + Message: "stage 0 stage completed", })) By("progressing in second application") @@ -369,7 +369,7 @@ var _ = Describe("ProgressiveRollout Controller", func() { ExpectStageStatus(ctx, prKey, "stage 1").Should(MatchStage(syncv1alpha1.StageStatus{ Name: "stage 1", Phase: syncv1alpha1.PhaseProgressing, - Message: "stage in progress", + Message: "stage 1 stage in progress", })) ExpectStagesInStatus(ctx, prKey).Should(Equal(2)) @@ -386,7 +386,7 @@ var _ = Describe("ProgressiveRollout Controller", func() { ExpectStageStatus(ctx, prKey, "stage 1").Should(MatchStage(syncv1alpha1.StageStatus{ Name: "stage 1", Phase: syncv1alpha1.PhaseSucceeded, - Message: "stage completed", + Message: "stage 1 stage completed", })) createdPR := syncv1alpha1.ProgressiveSync{} @@ -475,7 +475,7 @@ var _ = Describe("ProgressiveRollout Controller", func() { ExpectStageStatus(ctx, prKey, "stage 0").Should(MatchStage(syncv1alpha1.StageStatus{ Name: "stage 0", Phase: syncv1alpha1.PhaseProgressing, - Message: "stage in progress", + Message: "stage 0 stage in progress", })) ExpectStagesInStatus(ctx, prKey).Should(Equal(1)) @@ -492,7 +492,7 @@ var _ = Describe("ProgressiveRollout Controller", func() { ExpectStageStatus(ctx, prKey, "stage 0").Should(MatchStage(syncv1alpha1.StageStatus{ Name: "stage 0", Phase: syncv1alpha1.PhaseFailed, - Message: "stage failed", + Message: "stage 0 stage failed", })) createdPR := syncv1alpha1.ProgressiveSync{} From 1659d0496397a580091b47b0002e471d6dac7790 Mon Sep 17 00:00:00 2001 From: Nebojsa Prodana Date: Tue, 8 Jun 2021 15:55:51 +0200 Subject: [PATCH 04/13] More PR remarks --- controllers/progressivesync_controller.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/controllers/progressivesync_controller.go b/controllers/progressivesync_controller.go index 633ca6e4..049cf847 100644 --- a/controllers/progressivesync_controller.go +++ b/controllers/progressivesync_controller.go @@ -70,7 +70,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, client.IgnoreNotFound(err) } - if err := r.initializeStatus(ctx, &pr); err != nil { + if err := r.resetStatus(ctx, &pr); err != nil { log.Error(err, "unable to initialize status") return ctrl.Result{}, err } @@ -481,8 +481,8 @@ func (r *ProgressiveSyncReconciler) submitStatus(ctx context.Context, pr *syncv1 return retryErr } -// initializeStatus initializes the progressive sync object status to prevent conflicts when updating status later on -func (r *ProgressiveSyncReconciler) initializeStatus(ctx context.Context, pr *syncv1alpha1.ProgressiveSync) error { +// resetStatus initializes the progressive sync object status to prevent conflicts when updating status later on +func (r *ProgressiveSyncReconciler) resetStatus(ctx context.Context, pr *syncv1alpha1.ProgressiveSync) error { key := client.ObjectKeyFromObject(pr) latest := syncv1alpha1.ProgressiveSync{} @@ -491,7 +491,7 @@ func (r *ProgressiveSyncReconciler) initializeStatus(ctx context.Context, pr *sy } latest.Status = syncv1alpha1.ProgressiveSyncStatus{} condition := latest.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesInitializedReason, "Stages initialized") - apimeta.SetStatusCondition(pr.GetStatusConditions(), condition) + apimeta.SetStatusCondition(latest.GetStatusConditions(), condition) pr = &latest return nil From ee37d13e8593014c95b643fa8514c86abe949578 Mon Sep 17 00:00:00 2001 From: Matteo Ruina Date: Thu, 10 Jun 2021 09:39:26 +0200 Subject: [PATCH 05/13] Refactor min calculation --- controllers/progressivesync_controller.go | 3 --- internal/scheduler/scheduler.go | 11 ++++++----- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/controllers/progressivesync_controller.go b/controllers/progressivesync_controller.go index 049cf847..dd3fcc15 100644 --- a/controllers/progressivesync_controller.go +++ b/controllers/progressivesync_controller.go @@ -474,7 +474,6 @@ func (r *ProgressiveSyncReconciler) submitStatus(ctx context.Context, pr *syncv1 if err := r.Client.Status().Update(ctx, &latest); err != nil { return err } - pr = &latest return nil }) @@ -492,8 +491,6 @@ func (r *ProgressiveSyncReconciler) resetStatus(ctx context.Context, pr *syncv1a latest.Status = syncv1alpha1.ProgressiveSyncStatus{} condition := latest.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesInitializedReason, "Stages initialized") apimeta.SetStatusCondition(latest.GetStatusConditions(), condition) - - pr = &latest return nil } diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 7f2da3f2..202b1dd7 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -1,8 +1,6 @@ package scheduler import ( - "math" - syncv1alpha1 "github.com/Skyscanner/applicationset-progressive-sync/api/v1alpha1" "github.com/Skyscanner/applicationset-progressive-sync/internal/utils" argov1alpha1 "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1" @@ -63,10 +61,13 @@ func Scheduler(apps []argov1alpha1.Application, stage syncv1alpha1.ProgressiveSy return scheduledApps } - // It may be that at point when apps were observed, no apps were progressing yet - // so maxParallel-len(progressingApps) might actually be greater than len(outOfSyncApps) + // Because of eventual consistency, there might be a time where + // maxParallel-len(progressingApps) might actually be greater than len(outOfSyncApps) // causing the runtime to panic - p := (int)(math.Min((float64)(maxParallel-len(progressingApps)), (float64)(len(outOfSyncApps)))) + p := maxParallel - len(progressingApps) + if p > len(outOfSyncApps) { + p = len(outOfSyncApps) + } for i := 0; i < p; i++ { scheduledApps = append(scheduledApps, outOfSyncApps[i]) } From 48e7d6e8d16d43fe881e333bb83389ed8f42c516 Mon Sep 17 00:00:00 2001 From: Matteo Ruina Date: Thu, 10 Jun 2021 09:41:07 +0200 Subject: [PATCH 06/13] Replace r.log with just log --- controllers/progressivesync_controller.go | 30 +++++++++++------------ 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/controllers/progressivesync_controller.go b/controllers/progressivesync_controller.go index dd3fcc15..c83478e9 100644 --- a/controllers/progressivesync_controller.go +++ b/controllers/progressivesync_controller.go @@ -83,7 +83,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ if !controllerutil.ContainsFinalizer(&pr, syncv1alpha1.ProgressiveSyncFinalizer) { controllerutil.AddFinalizer(&pr, syncv1alpha1.ProgressiveSyncFinalizer) if err := r.Update(ctx, &pr); err != nil { - r.Log.Error(err, "failed to update object") + log.Error(err, "failed to update object") return ctrl.Result{}, err } // Requeue after adding the finalizer @@ -95,7 +95,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ // Remove our finalizer from the list and update it controllerutil.RemoveFinalizer(&pr, syncv1alpha1.ProgressiveSyncFinalizer) if err := r.Update(ctx, &pr); err != nil { - r.Log.Error(err, "failed to update object") + log.Error(err, "failed to update object") return ctrl.Result{}, err } } @@ -104,7 +104,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ } for _, stage := range pr.Spec.Stages { - log = r.Log.WithValues("stage", stage.Name) + log = log.WithValues("stage", stage.Name) // Get the clusters to update clusters, err := r.getClustersFromSelector(ctx, stage.Targets.Clusters.Selector) @@ -114,13 +114,13 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr) if err := r.submitStatus(ctx, &pr); err != nil { - r.Log.Error(err, "failed to update object status") + log.Error(err, "failed to update object status") return ctrl.Result{}, err } return ctrl.Result{}, err } - r.Log.V(1).Info("clusters selected", "clusters", utils.GetClustersName(clusters.Items)) + log.V(1).Info("clusters selected", "clusters", utils.GetClustersName(clusters.Items)) // Get only the Applications owned by the ProgressiveSync targeting the selected clusters apps, err := r.getOwnedAppsFromClusters(ctx, clusters, &pr) @@ -130,13 +130,13 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr) if err := r.submitStatus(ctx, &pr); err != nil { - r.Log.Error(err, "failed to update object status") + log.Error(err, "failed to update object status") return ctrl.Result{}, err } return ctrl.Result{}, err } - r.Log.V(1).Info("apps selected", "apps", utils.GetAppsName(apps)) + log.V(1).Info("apps selected", "apps", utils.GetAppsName(apps)) // Remove the annotation from the OutOfSync Applications before passing them to the Scheduler // This action allows the Scheduler to keep track at which stage an Application has been synced. @@ -148,7 +148,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr) if err := r.submitStatus(ctx, &pr); err != nil { - r.Log.Error(err, "failed to update object status") + log.Error(err, "failed to update object status") return ctrl.Result{}, err } return ctrl.Result{}, err @@ -176,7 +176,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ // failed := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionTrue, syncv1alpha1.StagesFailedReason, message) // apimeta.SetStatusCondition(pr.GetStatusConditions(), failed) if err := r.submitStatus(ctx, &pr); err != nil { - r.Log.Error(err, "failed to update object status") + log.Error(err, "failed to update object status") return ctrl.Result{}, err } @@ -196,7 +196,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ failed := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionTrue, syncv1alpha1.StagesFailedReason, failedMessage) apimeta.SetStatusCondition(pr.GetStatusConditions(), failed) if err := r.submitStatus(ctx, &pr); err != nil { - r.Log.Error(err, "failed to update object status") + log.Error(err, "failed to update object status") return ctrl.Result{}, err } log.Info("rollout failed") @@ -214,7 +214,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ apimeta.SetStatusCondition(pr.GetStatusConditions(), progress) if err := r.submitStatus(ctx, &pr); err != nil { - r.Log.Error(err, "failed to update object status") + log.Error(err, "failed to update object status") return ctrl.Result{}, err } @@ -232,7 +232,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ apimeta.SetStatusCondition(pr.GetStatusConditions(), progress) if err := r.submitStatus(ctx, &pr); err != nil { - r.Log.Error(err, "failed to update object status") + log.Error(err, "failed to update object status") return ctrl.Result{}, err } @@ -246,7 +246,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ apimeta.SetStatusCondition(pr.GetStatusConditions(), progress) if err := r.submitStatus(ctx, &pr); err != nil { - r.Log.Error(err, "failed to update object status") + log.Error(err, "failed to update object status") return ctrl.Result{}, err } @@ -261,10 +261,10 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ completed := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionTrue, syncv1alpha1.StagesCompleteReason, "All stages completed") apimeta.SetStatusCondition(pr.GetStatusConditions(), completed) if err := r.submitStatus(ctx, &pr); err != nil { - r.Log.Error(err, "failed to update object status") + log.Error(err, "failed to update object status") return ctrl.Result{}, err } - r.Log.Info("rollout completed") + log.Info("rollout completed") return ctrl.Result{}, nil } From 70ebbd943238632ba367d744421e0c9473b96c45 Mon Sep 17 00:00:00 2001 From: Matteo Ruina Date: Thu, 10 Jun 2021 17:16:56 +0200 Subject: [PATCH 07/13] Remove resetStatus as we don't need it --- controllers/progressivesync_controller.go | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/controllers/progressivesync_controller.go b/controllers/progressivesync_controller.go index c83478e9..ca331b0b 100644 --- a/controllers/progressivesync_controller.go +++ b/controllers/progressivesync_controller.go @@ -70,11 +70,6 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, client.IgnoreNotFound(err) } - if err := r.resetStatus(ctx, &pr); err != nil { - log.Error(err, "unable to initialize status") - return ctrl.Result{}, err - } - log = r.Log.WithValues("applicationset", pr.Spec.SourceRef.Name) if pr.ObjectMeta.DeletionTimestamp.IsZero() { @@ -479,18 +474,3 @@ func (r *ProgressiveSyncReconciler) submitStatus(ctx context.Context, pr *syncv1 }) return retryErr } - -// resetStatus initializes the progressive sync object status to prevent conflicts when updating status later on -func (r *ProgressiveSyncReconciler) resetStatus(ctx context.Context, pr *syncv1alpha1.ProgressiveSync) error { - - key := client.ObjectKeyFromObject(pr) - latest := syncv1alpha1.ProgressiveSync{} - if err := r.Client.Get(ctx, key, &latest); err != nil { - return err - } - latest.Status = syncv1alpha1.ProgressiveSyncStatus{} - condition := latest.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesInitializedReason, "Stages initialized") - apimeta.SetStatusCondition(latest.GetStatusConditions(), condition) - return nil - -} From c0831811fcc361d74e36b408173474706082f0e5 Mon Sep 17 00:00:00 2001 From: Matteo Ruina Date: Thu, 10 Jun 2021 17:20:26 +0200 Subject: [PATCH 08/13] Semplify add/remove finalizer logic by removing the if/else loop --- controllers/progressivesync_controller.go | 40 ++++++++++------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/controllers/progressivesync_controller.go b/controllers/progressivesync_controller.go index ca331b0b..f6b80071 100644 --- a/controllers/progressivesync_controller.go +++ b/controllers/progressivesync_controller.go @@ -66,38 +66,34 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ // Get the ProgressiveSync object pr := syncv1alpha1.ProgressiveSync{} if err := r.Get(ctx, req.NamespacedName, &pr); err != nil { - log.Error(err, "unable to fetch ProgressiveSync") + log.Error(err, "unable to fetch progressivesync object") return ctrl.Result{}, client.IgnoreNotFound(err) } log = r.Log.WithValues("applicationset", pr.Spec.SourceRef.Name) - if pr.ObjectMeta.DeletionTimestamp.IsZero() { - // The object is not being deleted, so if it does not have our finalizer, - // then lets add the finalizer and update the object - if !controllerutil.ContainsFinalizer(&pr, syncv1alpha1.ProgressiveSyncFinalizer) { - controllerutil.AddFinalizer(&pr, syncv1alpha1.ProgressiveSyncFinalizer) - if err := r.Update(ctx, &pr); err != nil { - log.Error(err, "failed to update object") - return ctrl.Result{}, err - } - // Requeue after adding the finalizer - return ctrl.Result{Requeue: true}, nil - } - } else { - // The object is being deleted - if controllerutil.ContainsFinalizer(&pr, syncv1alpha1.ProgressiveSyncFinalizer) { - // Remove our finalizer from the list and update it - controllerutil.RemoveFinalizer(&pr, syncv1alpha1.ProgressiveSyncFinalizer) - if err := r.Update(ctx, &pr); err != nil { - log.Error(err, "failed to update object") - return ctrl.Result{}, err - } + // If the object is being deleted, remove finalizer and don't requeue it + if !pr.ObjectMeta.DeletionTimestamp.IsZero() { + controllerutil.RemoveFinalizer(&pr, syncv1alpha1.ProgressiveSyncFinalizer) + if err := r.Update(ctx, &pr); err != nil { + log.Error(err, "failed to update object when removing finalizer") + return ctrl.Result{}, err } // Stop reconciliation as the item is being deleted return ctrl.Result{}, nil } + // Add finalizer if it doesn't exist + if !controllerutil.ContainsFinalizer(&pr, syncv1alpha1.ProgressiveSyncFinalizer) { + controllerutil.AddFinalizer(&pr, syncv1alpha1.ProgressiveSyncFinalizer) + if err := r.Update(ctx, &pr); err != nil { + log.Error(err, "failed to update object when adding finalizer") + return ctrl.Result{}, err + } + // Requeue after adding the finalizer + return ctrl.Result{Requeue: true}, nil + } + for _, stage := range pr.Spec.Stages { log = log.WithValues("stage", stage.Name) From 99e9bf059fb76f3d074311ad106784cb43fd896b Mon Sep 17 00:00:00 2001 From: Matteo Ruina Date: Thu, 10 Jun 2021 17:38:27 +0200 Subject: [PATCH 09/13] Rename submitStatus to updateStatusWithRetry to reflect its behaviour --- controllers/progressivesync_controller.go | 24 +++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/controllers/progressivesync_controller.go b/controllers/progressivesync_controller.go index f6b80071..6bc2217d 100644 --- a/controllers/progressivesync_controller.go +++ b/controllers/progressivesync_controller.go @@ -61,7 +61,7 @@ type ProgressiveSyncReconciler struct { // Reconcile performs the reconciling for a single named ProgressiveSync object func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - log := r.Log.WithValues("ProgressiveSync", req.NamespacedName) + log := r.Log.WithValues("progressivesync", req.NamespacedName) // Get the ProgressiveSync object pr := syncv1alpha1.ProgressiveSync{} @@ -104,7 +104,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ log.Error(err, message) r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr) - if err := r.submitStatus(ctx, &pr); err != nil { + if err := r.updateStatusWithRetry(ctx, &pr); err != nil { log.Error(err, "failed to update object status") return ctrl.Result{}, err } @@ -120,7 +120,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ log.Error(err, message) r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr) - if err := r.submitStatus(ctx, &pr); err != nil { + if err := r.updateStatusWithRetry(ctx, &pr); err != nil { log.Error(err, "failed to update object status") return ctrl.Result{}, err } @@ -138,7 +138,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr) - if err := r.submitStatus(ctx, &pr); err != nil { + if err := r.updateStatusWithRetry(ctx, &pr); err != nil { log.Error(err, "failed to update object status") return ctrl.Result{}, err } @@ -166,7 +166,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ // TODO: Should this be reflected in the general condition? // failed := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionTrue, syncv1alpha1.StagesFailedReason, message) // apimeta.SetStatusCondition(pr.GetStatusConditions(), failed) - if err := r.submitStatus(ctx, &pr); err != nil { + if err := r.updateStatusWithRetry(ctx, &pr); err != nil { log.Error(err, "failed to update object status") return ctrl.Result{}, err } @@ -186,7 +186,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ failed := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionTrue, syncv1alpha1.StagesFailedReason, failedMessage) apimeta.SetStatusCondition(pr.GetStatusConditions(), failed) - if err := r.submitStatus(ctx, &pr); err != nil { + if err := r.updateStatusWithRetry(ctx, &pr); err != nil { log.Error(err, "failed to update object status") return ctrl.Result{}, err } @@ -204,7 +204,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ progress := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesProgressingReason, message) apimeta.SetStatusCondition(pr.GetStatusConditions(), progress) - if err := r.submitStatus(ctx, &pr); err != nil { + if err := r.updateStatusWithRetry(ctx, &pr); err != nil { log.Error(err, "failed to update object status") return ctrl.Result{}, err } @@ -222,7 +222,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ progress := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesProgressingReason, message) apimeta.SetStatusCondition(pr.GetStatusConditions(), progress) - if err := r.submitStatus(ctx, &pr); err != nil { + if err := r.updateStatusWithRetry(ctx, &pr); err != nil { log.Error(err, "failed to update object status") return ctrl.Result{}, err } @@ -236,7 +236,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ progress := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesProgressingReason, message) apimeta.SetStatusCondition(pr.GetStatusConditions(), progress) - if err := r.submitStatus(ctx, &pr); err != nil { + if err := r.updateStatusWithRetry(ctx, &pr); err != nil { log.Error(err, "failed to update object status") return ctrl.Result{}, err } @@ -251,7 +251,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ // Progressive rollout completed completed := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionTrue, syncv1alpha1.StagesCompleteReason, "All stages completed") apimeta.SetStatusCondition(pr.GetStatusConditions(), completed) - if err := r.submitStatus(ctx, &pr); err != nil { + if err := r.updateStatusWithRetry(ctx, &pr); err != nil { log.Error(err, "failed to update object status") return ctrl.Result{}, err } @@ -451,8 +451,8 @@ func (r *ProgressiveSyncReconciler) syncApp(ctx context.Context, appName string) return r.ArgoCDAppClient.Sync(ctx, &syncReq) } -// submitStatus updates the progressive sync object status with backoff -func (r *ProgressiveSyncReconciler) submitStatus(ctx context.Context, pr *syncv1alpha1.ProgressiveSync) error { +// updateStatusWithRetry updates the progressive sync object status with backoff +func (r *ProgressiveSyncReconciler) updateStatusWithRetry(ctx context.Context, pr *syncv1alpha1.ProgressiveSync) error { retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error { From 63c692d6aff934c7bba579e6e42029b451759661 Mon Sep 17 00:00:00 2001 From: Matteo Ruina Date: Thu, 10 Jun 2021 17:42:07 +0200 Subject: [PATCH 10/13] Replace log.V(1).Info with log.Info --- controllers/progressivesync_controller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/progressivesync_controller.go b/controllers/progressivesync_controller.go index 6bc2217d..dfd25f8c 100644 --- a/controllers/progressivesync_controller.go +++ b/controllers/progressivesync_controller.go @@ -111,7 +111,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, err } - log.V(1).Info("clusters selected", "clusters", utils.GetClustersName(clusters.Items)) + log.Info("clusters selected", "clusters", utils.GetClustersName(clusters.Items)) // Get only the Applications owned by the ProgressiveSync targeting the selected clusters apps, err := r.getOwnedAppsFromClusters(ctx, clusters, &pr) @@ -127,7 +127,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, err } - log.V(1).Info("apps selected", "apps", utils.GetAppsName(apps)) + log.Info("apps selected", "apps", utils.GetAppsName(apps)) // Remove the annotation from the OutOfSync Applications before passing them to the Scheduler // This action allows the Scheduler to keep track at which stage an Application has been synced. From 6760629d2b5849de7d44f5b48a86a09552e20571 Mon Sep 17 00:00:00 2001 From: Matteo Ruina Date: Thu, 10 Jun 2021 23:03:56 +0200 Subject: [PATCH 11/13] Move stage reconciliation under separate method --- controllers/progressivesync_controller.go | 270 ++++++++---------- .../progressivesync_controller_test.go | 2 +- 2 files changed, 123 insertions(+), 149 deletions(-) diff --git a/controllers/progressivesync_controller.go b/controllers/progressivesync_controller.go index dfd25f8c..aed2dfb1 100644 --- a/controllers/progressivesync_controller.go +++ b/controllers/progressivesync_controller.go @@ -64,7 +64,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ log := r.Log.WithValues("progressivesync", req.NamespacedName) // Get the ProgressiveSync object - pr := syncv1alpha1.ProgressiveSync{} + var pr syncv1alpha1.ProgressiveSync if err := r.Get(ctx, req.NamespacedName, &pr); err != nil { log.Error(err, "unable to fetch progressivesync object") return ctrl.Result{}, client.IgnoreNotFound(err) @@ -95,159 +95,20 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ } for _, stage := range pr.Spec.Stages { - log = log.WithValues("stage", stage.Name) - - // Get the clusters to update - clusters, err := r.getClustersFromSelector(ctx, stage.Targets.Clusters.Selector) - if err != nil { - message := "unable to fetch clusters" - log.Error(err, message) - r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr) - - if err := r.updateStatusWithRetry(ctx, &pr); err != nil { - log.Error(err, "failed to update object status") - return ctrl.Result{}, err - } - - return ctrl.Result{}, err - } - log.Info("clusters selected", "clusters", utils.GetClustersName(clusters.Items)) - - // Get only the Applications owned by the ProgressiveSync targeting the selected clusters - apps, err := r.getOwnedAppsFromClusters(ctx, clusters, &pr) - if err != nil { - message := "unable to fetch apps" - log.Error(err, message) - r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr) - - if err := r.updateStatusWithRetry(ctx, &pr); err != nil { - log.Error(err, "failed to update object status") - return ctrl.Result{}, err - } + log = r.Log.WithValues("stage", stage.Name) + pr, result, err, requeue := r.reconcileStage(ctx, pr, stage) + if err := r.updateStatusWithRetry(ctx, &pr); err != nil { return ctrl.Result{}, err } - log.Info("apps selected", "apps", utils.GetAppsName(apps)) - - // Remove the annotation from the OutOfSync Applications before passing them to the Scheduler - // This action allows the Scheduler to keep track at which stage an Application has been synced. - outOfSyncApps := utils.GetAppsBySyncStatusCode(apps, argov1alpha1.SyncStatusCodeOutOfSync) - if err := r.removeAnnotationFromApps(ctx, outOfSyncApps, utils.ProgressiveSyncSyncedAtStageKey); err != nil { - message := "unable to remove out-of-sync annotation" - log.Error(err, message) - - r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr) - - if err := r.updateStatusWithRetry(ctx, &pr); err != nil { - log.Error(err, "failed to update object status") - return ctrl.Result{}, err - } - return ctrl.Result{}, err - } - - // Get the Applications to update - // TODO: update the scheduler to return the InProgress apps - scheduledApps := scheduler.Scheduler(apps, stage) - - // TODO: status status - update targets - - for _, s := range scheduledApps { - log.Info("syncing app", "app", s) - - _, err := r.syncApp(ctx, s.Name) - - if err != nil { - if !strings.Contains(err.Error(), "another operation is already in progress") { - message := "unable to remove out-of-sync annotation" - log.Error(err, message, "message", err.Error()) - - r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr) - - // TODO: Should this be reflected in the general condition? - // failed := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionTrue, syncv1alpha1.StagesFailedReason, message) - // apimeta.SetStatusCondition(pr.GetStatusConditions(), failed) - if err := r.updateStatusWithRetry(ctx, &pr); err != nil { - log.Error(err, "failed to update object status") - return ctrl.Result{}, err - } - - // TODO: stage status - update failed clusters - - return ctrl.Result{}, err - } - } - } - if scheduler.IsStageFailed(apps) { - failedMessage := fmt.Sprintf("%s stage failed", stage.Name) - log.Info(failedMessage) - - r.updateStageStatus(ctx, stage.Name, failedMessage, syncv1alpha1.PhaseFailed, &pr) - - failed := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionTrue, syncv1alpha1.StagesFailedReason, failedMessage) - apimeta.SetStatusCondition(pr.GetStatusConditions(), failed) - if err := r.updateStatusWithRetry(ctx, &pr); err != nil { - log.Error(err, "failed to update object status") - return ctrl.Result{}, err - } - log.Info("rollout failed") - // We can set Requeue: true once we have a timeout in place - return ctrl.Result{}, nil + if requeue { + log.Info("requeuing stage") + return result, err } - if scheduler.IsStageInProgress(apps) { - message := fmt.Sprintf("%s stage in progress", stage.Name) - log.Info(message) - - r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseProgressing, &pr) - - progress := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesProgressingReason, message) - apimeta.SetStatusCondition(pr.GetStatusConditions(), progress) - - if err := r.updateStatusWithRetry(ctx, &pr); err != nil { - log.Error(err, "failed to update object status") - return ctrl.Result{}, err - } - - // Stage in progress, we reconcile again until the stage is completed or failed - return ctrl.Result{Requeue: true}, nil - } - - if scheduler.IsStageComplete(apps) { - message := fmt.Sprintf("%s stage completed", stage.Name) - log.Info(message) - - r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseSucceeded, &pr) - - progress := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesProgressingReason, message) - apimeta.SetStatusCondition(pr.GetStatusConditions(), progress) - - if err := r.updateStatusWithRetry(ctx, &pr); err != nil { - log.Error(err, "failed to update object status") - return ctrl.Result{}, err - } - - } else { - message := fmt.Sprintf("%s stage in unknown state", stage.Name) - log.Info(message) - - r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr) - - progress := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesProgressingReason, message) - apimeta.SetStatusCondition(pr.GetStatusConditions(), progress) - - if err := r.updateStatusWithRetry(ctx, &pr); err != nil { - log.Error(err, "failed to update object status") - return ctrl.Result{}, err - } - - // We can set Requeue: true once we have a timeout in place - return ctrl.Result{}, nil - } } - log.Info("all stages completed") - // Progressive rollout completed completed := pr.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionTrue, syncv1alpha1.StagesCompleteReason, "All stages completed") apimeta.SetStatusCondition(pr.GetStatusConditions(), completed) @@ -255,7 +116,7 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ log.Error(err, "failed to update object status") return ctrl.Result{}, err } - log.Info("rollout completed") + log.Info("sync completed") return ctrl.Result{}, nil } @@ -331,7 +192,7 @@ func (r *ProgressiveSyncReconciler) requestsForSecretChange(o client.Object) []r return nil } - r.Log.V(1).Info("received secret event", "name", s.Name, "Namespace", s.Namespace) + r.Log.Info("received secret event", "name", s.Name, "Namespace", s.Namespace) if !utils.IsArgoCDCluster(s.GetLabels()) { return nil @@ -470,3 +331,116 @@ func (r *ProgressiveSyncReconciler) updateStatusWithRetry(ctx context.Context, p }) return retryErr } + +// reconcileStage reconcile a ProgressiveSyncStage +func (r *ProgressiveSyncReconciler) reconcileStage(ctx context.Context, ps syncv1alpha1.ProgressiveSync, stage syncv1alpha1.ProgressiveSyncStage) (syncv1alpha1.ProgressiveSync, reconcile.Result, error, bool) { + log := r.Log.WithValues("stage", stage.Name) + + // Get the clusters to update + clusters, err := r.getClustersFromSelector(ctx, stage.Targets.Clusters.Selector) + if err != nil { + message := "unable to fetch clusters from selector" + log.Error(err, message) + + // Set stage status + r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseFailed, &ps) + // Set ProgressiveSync status + failed := ps.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesFailedReason, message) + apimeta.SetStatusCondition(ps.GetStatusConditions(), failed) + + return ps, ctrl.Result{}, err, false + } + log.Info("clusters selected", "clusters", utils.GetClustersName(clusters.Items)) + + // Get only the Applications owned by the ProgressiveSync targeting the selected clusters + apps, err := r.getOwnedAppsFromClusters(ctx, clusters, &ps) + if err != nil { + message := "unable to fetch apps from clusters" + log.Error(err, message) + + r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseFailed, &ps) + failed := ps.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesFailedReason, message) + apimeta.SetStatusCondition(ps.GetStatusConditions(), failed) + return ps, ctrl.Result{}, err, false + } + log.Info("apps selected", "apps", utils.GetAppsName(apps)) + + // Remove the annotation from the OutOfSync Applications before passing them to the Scheduler + // This action allows the Scheduler to keep track at which stage an Application has been synced. + outOfSyncApps := utils.GetAppsBySyncStatusCode(apps, argov1alpha1.SyncStatusCodeOutOfSync) + if err := r.removeAnnotationFromApps(ctx, outOfSyncApps, utils.ProgressiveSyncSyncedAtStageKey); err != nil { + message := "failed to remove out-of-sync annotation from apps" + log.Error(err, message) + + r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseFailed, &ps) + // Set ProgressiveSync status + failed := ps.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesFailedReason, message) + apimeta.SetStatusCondition(ps.GetStatusConditions(), failed) + + return ps, ctrl.Result{}, err, false + } + + // Get the Applications to update + scheduledApps := scheduler.Scheduler(apps, stage) + + for _, s := range scheduledApps { + log.Info("syncing app", "app", s) + + _, err := r.syncApp(ctx, s.Name) + + if err != nil { + if !strings.Contains(err.Error(), "another operation is already in progress") { + message := "failed to sync app" + log.Error(err, message, "message", err.Error()) + + r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseFailed, &ps) + // Set ProgressiveSync status + failed := ps.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesFailedReason, message) + apimeta.SetStatusCondition(ps.GetStatusConditions(), failed) + return ps, ctrl.Result{}, err, false + } + } + } + + if scheduler.IsStageFailed(apps) { + message := fmt.Sprintf("%s stage failed", stage.Name) + log.Info(message) + + r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseFailed, &ps) + + failed := ps.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesFailedReason, message) + apimeta.SetStatusCondition(ps.GetStatusConditions(), failed) + + log.Info("sync failed") + // We can set Requeue: true once we have a timeout in place + return ps, ctrl.Result{}, nil, false + } + + if scheduler.IsStageInProgress(apps) { + message := fmt.Sprintf("%s stage in progress", stage.Name) + log.Info(message) + + r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseProgressing, &ps) + + progress := ps.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesProgressingReason, message) + apimeta.SetStatusCondition(ps.GetStatusConditions(), progress) + + // Stage in progress, we reconcile again until the stage is completed or failed + return ps, ctrl.Result{Requeue: true}, nil, true + } + + if scheduler.IsStageComplete(apps) { + message := fmt.Sprintf("%s stage completed", stage.Name) + log.Info(message) + + r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseSucceeded, &ps) + + progress := ps.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesProgressingReason, message) + apimeta.SetStatusCondition(ps.GetStatusConditions(), progress) + + return ps, ctrl.Result{}, nil, false + + } + + return ps, ctrl.Result{}, nil, false +} diff --git a/controllers/progressivesync_controller_test.go b/controllers/progressivesync_controller_test.go index 085073e7..82eac840 100644 --- a/controllers/progressivesync_controller_test.go +++ b/controllers/progressivesync_controller_test.go @@ -502,7 +502,7 @@ var _ = Describe("ProgressiveRollout Controller", func() { }).Should(Equal(1)) Expect(createdPR.ObjectMeta.Finalizers[0]).To(Equal(syncv1alpha1.ProgressiveSyncFinalizer)) - expected := failedStagePR.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionTrue, syncv1alpha1.StagesFailedReason, "stage 0 stage failed") + expected := failedStagePR.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesFailedReason, "stage 0 stage failed") ExpectCondition(&failedStagePR, expected.Type).Should(HaveStatus(expected.Status, expected.Reason, expected.Message)) deletedPR := syncv1alpha1.ProgressiveSync{} From 9dcee079b207d75c9c7155cd730f1cb87528db2f Mon Sep 17 00:00:00 2001 From: Matteo Ruina Date: Mon, 14 Jun 2021 09:52:30 +0200 Subject: [PATCH 12/13] Use result.Requeue instead of a separate bool to reduce complexity --- controllers/progressivesync_controller.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/controllers/progressivesync_controller.go b/controllers/progressivesync_controller.go index aed2dfb1..02b768e4 100644 --- a/controllers/progressivesync_controller.go +++ b/controllers/progressivesync_controller.go @@ -97,12 +97,12 @@ func (r *ProgressiveSyncReconciler) Reconcile(ctx context.Context, req ctrl.Requ for _, stage := range pr.Spec.Stages { log = r.Log.WithValues("stage", stage.Name) - pr, result, err, requeue := r.reconcileStage(ctx, pr, stage) + pr, result, err := r.reconcileStage(ctx, pr, stage) if err := r.updateStatusWithRetry(ctx, &pr); err != nil { return ctrl.Result{}, err } - if requeue { + if result.Requeue { log.Info("requeuing stage") return result, err } @@ -333,7 +333,7 @@ func (r *ProgressiveSyncReconciler) updateStatusWithRetry(ctx context.Context, p } // reconcileStage reconcile a ProgressiveSyncStage -func (r *ProgressiveSyncReconciler) reconcileStage(ctx context.Context, ps syncv1alpha1.ProgressiveSync, stage syncv1alpha1.ProgressiveSyncStage) (syncv1alpha1.ProgressiveSync, reconcile.Result, error, bool) { +func (r *ProgressiveSyncReconciler) reconcileStage(ctx context.Context, ps syncv1alpha1.ProgressiveSync, stage syncv1alpha1.ProgressiveSyncStage) (syncv1alpha1.ProgressiveSync, reconcile.Result, error) { log := r.Log.WithValues("stage", stage.Name) // Get the clusters to update @@ -348,7 +348,7 @@ func (r *ProgressiveSyncReconciler) reconcileStage(ctx context.Context, ps syncv failed := ps.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesFailedReason, message) apimeta.SetStatusCondition(ps.GetStatusConditions(), failed) - return ps, ctrl.Result{}, err, false + return ps, ctrl.Result{}, err } log.Info("clusters selected", "clusters", utils.GetClustersName(clusters.Items)) @@ -361,7 +361,7 @@ func (r *ProgressiveSyncReconciler) reconcileStage(ctx context.Context, ps syncv r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseFailed, &ps) failed := ps.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesFailedReason, message) apimeta.SetStatusCondition(ps.GetStatusConditions(), failed) - return ps, ctrl.Result{}, err, false + return ps, ctrl.Result{}, err } log.Info("apps selected", "apps", utils.GetAppsName(apps)) @@ -377,7 +377,7 @@ func (r *ProgressiveSyncReconciler) reconcileStage(ctx context.Context, ps syncv failed := ps.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesFailedReason, message) apimeta.SetStatusCondition(ps.GetStatusConditions(), failed) - return ps, ctrl.Result{}, err, false + return ps, ctrl.Result{}, err } // Get the Applications to update @@ -397,7 +397,7 @@ func (r *ProgressiveSyncReconciler) reconcileStage(ctx context.Context, ps syncv // Set ProgressiveSync status failed := ps.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesFailedReason, message) apimeta.SetStatusCondition(ps.GetStatusConditions(), failed) - return ps, ctrl.Result{}, err, false + return ps, ctrl.Result{}, err } } } @@ -413,7 +413,7 @@ func (r *ProgressiveSyncReconciler) reconcileStage(ctx context.Context, ps syncv log.Info("sync failed") // We can set Requeue: true once we have a timeout in place - return ps, ctrl.Result{}, nil, false + return ps, ctrl.Result{}, nil } if scheduler.IsStageInProgress(apps) { @@ -426,7 +426,7 @@ func (r *ProgressiveSyncReconciler) reconcileStage(ctx context.Context, ps syncv apimeta.SetStatusCondition(ps.GetStatusConditions(), progress) // Stage in progress, we reconcile again until the stage is completed or failed - return ps, ctrl.Result{Requeue: true}, nil, true + return ps, ctrl.Result{Requeue: true}, nil } if scheduler.IsStageComplete(apps) { @@ -438,9 +438,9 @@ func (r *ProgressiveSyncReconciler) reconcileStage(ctx context.Context, ps syncv progress := ps.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionFalse, syncv1alpha1.StagesProgressingReason, message) apimeta.SetStatusCondition(ps.GetStatusConditions(), progress) - return ps, ctrl.Result{}, nil, false + return ps, ctrl.Result{}, nil } - return ps, ctrl.Result{}, nil, false + return ps, ctrl.Result{}, nil } From 9db2555818fc8959c37a1ffced81bf50189a45dc Mon Sep 17 00:00:00 2001 From: Matteo Ruina Date: Mon, 14 Jun 2021 10:50:29 +0200 Subject: [PATCH 13/13] Remove unused StagesInitializedReason --- api/v1alpha1/condition_types.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/api/v1alpha1/condition_types.go b/api/v1alpha1/condition_types.go index f923c21a..ccef48e7 100644 --- a/api/v1alpha1/condition_types.go +++ b/api/v1alpha1/condition_types.go @@ -25,10 +25,6 @@ const ( // the progressive sync stages have been completed StagesCompleteReason string = "StagesCompleted" - // StagesInitializedReason represent the fact that all - // the progressive sync stages have been initialized - StagesInitializedReason string = "StagesInitialized" - // StagesProgressingReason represent the fact that some // of the progressive sync stages are progressing StagesProgressingReason string = "StagesProgressing"