diff --git a/controllers/progressivesync_controller.go b/controllers/progressivesync_controller.go index 622f0ee2..b608862d 100644 --- a/controllers/progressivesync_controller.go +++ b/controllers/progressivesync_controller.go @@ -173,6 +173,7 @@ func (r *ProgressiveSyncReconciler) requestsForApplicationChange(o client.Object Namespace: pr.Namespace, Name: pr.Name, }}) + r.Log.Info("application changed", "app", fmt.Sprintf("%s/%s", app.Namespace, app.Name), "sync.status", app.Status.Sync.Status, "health.status", app.Status.Health.Status) } } @@ -450,5 +451,7 @@ func (r *ProgressiveSyncReconciler) reconcileStage(ctx context.Context, ps syncv } + message := fmt.Sprintf("%s is in an unknown state!", stage.Name) + log.Info(message) return ps, ctrl.Result{Requeue: true}, nil } diff --git a/controllers/progressivesync_controller_test.go b/controllers/progressivesync_controller_test.go index b2c94c97..3437771c 100644 --- a/controllers/progressivesync_controller_test.go +++ b/controllers/progressivesync_controller_test.go @@ -3,10 +3,11 @@ package controllers import ( "context" "fmt" - "sigs.k8s.io/controller-runtime/pkg/cache" "testing" "time" + "sigs.k8s.io/controller-runtime/pkg/cache" + syncv1alpha1 "github.com/Skyscanner/applicationset-progressive-sync/api/v1alpha1" "github.com/Skyscanner/applicationset-progressive-sync/internal/consts" "github.com/Skyscanner/applicationset-progressive-sync/internal/utils" @@ -725,6 +726,37 @@ var _ = Describe("ProgressiveRollout Controller", func() { expected := ps.NewStatusCondition(syncv1alpha1.CompletedCondition, metav1.ConditionTrue, syncv1alpha1.StagesCompleteReason, "All stages completed") ExpectCondition(&ps, expected.Type).Should(HaveStatus(expected.Status, expected.Reason, expected.Message)) + //Make sure that we can recover from transient app failures, due to flaky healthchecks for instance + Eventually(func() error { + return setAppStatusFailed(ctx, "account1-eu-west-1a-1", argoNamespace) + }).Should(Succeed()) + + message = "one cluster as canary in eu-west-1 stage failed" + ExpectStageStatus(ctx, psKey, "one cluster as canary in eu-west-1").Should(MatchStage(syncv1alpha1.StageStatus{ + Name: "one cluster as canary in eu-west-1", + Phase: syncv1alpha1.PhaseFailed, + Message: message, + })) + + Eventually(func() error { + return setAppStatusProgressing(ctx, "account1-eu-west-1a-2", argoNamespace) + }).Should(Succeed()) + + Eventually(func() error { + return setAppStatusCompleted(ctx, "account1-eu-west-1a-2", argoNamespace) + }).Should(Succeed()) + + Eventually(func() error { + return setAppStatusCompleted(ctx, "account1-eu-west-1a-1", argoNamespace) + }).Should(Succeed()) + + message = "one cluster as canary in eu-west-1 stage completed" + ExpectStageStatus(ctx, psKey, "one cluster as canary in eu-west-1").Should(MatchStage(syncv1alpha1.StageStatus{ + Name: "one cluster as canary in eu-west-1", + Phase: syncv1alpha1.PhaseSucceeded, + Message: message, + })) + }) It("should fail if unable to sync an application", func() { diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index b43eff55..0cffa583 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -38,8 +38,7 @@ func Scheduler(log logr.Logger, apps []argov1alpha1.Application, stage syncv1alp log.Info("fetched out-of-sync apps", "apps", utils.GetAppsName(outOfSyncApps)) - healthyApps := utils.GetAppsByHealthStatusCode(apps, health.HealthStatusHealthy) - syncedInCurrentStage := utils.GetSyncedAppsByStage(healthyApps, stage, pss) + syncedInCurrentStage := utils.GetSyncedAppsByStage(apps, stage, pss) log.Info("fetched synced-in-current-stage apps", "apps", utils.GetAppsName(syncedInCurrentStage)) progressingApps := utils.GetAppsByHealthStatusCode(apps, health.HealthStatusProgressing) @@ -79,12 +78,6 @@ func Scheduler(log logr.Logger, apps []argov1alpha1.Application, stage syncv1alp scheduledApps = append(scheduledApps, outOfSyncApps[i]) } - // To recover from a case where something triggers an Application sync, the scheulder also return - // all the progressing apps but still out of sync, so we can mark them as synced and take back control of the app - - progressingOutOfSyncApps := utils.GetAppsBySyncStatusCode(progressingApps, argov1alpha1.SyncStatusCodeOutOfSync) - scheduledApps = append(scheduledApps, progressingOutOfSyncApps...) - return scheduledApps } diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index 56fd01ef..f42517a4 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -730,7 +730,7 @@ func TestScheduler(t *testing.T) { expected: ([]argov1alpha1.Application)(nil), }, { - name: "Applications: outOfSync 4, syncedInCurrentStage 2, progressing 1, syncedInPreviousStage 2 | Stage: maxTargets 3, maxParallel 3 | Expected: scheduled 2", + name: "Applications: outOfSync 4, syncedInCurrentStage 2, progressing 1, syncedInPreviousStage 2 | Stage: maxTargets 3, maxParallel 3 | Expected: scheduled 1", apps: []argov1alpha1.Application{ { ObjectMeta: metav1.ObjectMeta{ @@ -854,6 +854,39 @@ func TestScheduler(t *testing.T) { }, }, }, + }, + }, + { + name: "Applications: outOfSync 1, syncedInCurrentStage 3, progressing 2 | Stage: maxTargets 3, maxParallel 3 | Expected: scheduled 0", + apps: []argov1alpha1.Application{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "app-one", + Namespace: SchedulerTestNamespace, + }, + Status: argov1alpha1.ApplicationStatus{ + Sync: argov1alpha1.SyncStatus{ + Status: argov1alpha1.SyncStatusCodeSynced, + }, + Health: argov1alpha1.HealthStatus{ + Status: health.HealthStatusHealthy, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "app-two", + Namespace: SchedulerTestNamespace, + }, + Status: argov1alpha1.ApplicationStatus{ + Sync: argov1alpha1.SyncStatus{ + Status: argov1alpha1.SyncStatusCodeSynced, + }, + Health: argov1alpha1.HealthStatus{ + Status: health.HealthStatusProgressing, + }, + }, + }, { ObjectMeta: metav1.ObjectMeta{ Name: "app-three", @@ -861,14 +894,40 @@ func TestScheduler(t *testing.T) { }, Status: argov1alpha1.ApplicationStatus{ Sync: argov1alpha1.SyncStatus{ - Status: argov1alpha1.SyncStatusCodeOutOfSync, + Status: argov1alpha1.SyncStatusCodeSynced, }, Health: argov1alpha1.HealthStatus{ Status: health.HealthStatusProgressing, }, }, }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "app-four", + Namespace: SchedulerTestNamespace, + }, + Status: argov1alpha1.ApplicationStatus{ + Sync: argov1alpha1.SyncStatus{ + Status: argov1alpha1.SyncStatusCodeOutOfSync, + }, + Health: argov1alpha1.HealthStatus{ + Status: health.HealthStatusHealthy, + }, + }, + }, }, + stage: syncv1alpha1.ProgressiveSyncStage{ + Name: StageName, + MaxParallel: intstr.Parse("3"), + MaxTargets: intstr.Parse("3"), + Targets: syncv1alpha1.ProgressiveSyncTargets{}, + }, + syncedAtStage: map[string]string{ + "app-one": StageName, + "app-two": StageName, + "app-three": StageName, + }, + expected: ([]argov1alpha1.Application)(nil), }, } diff --git a/internal/utils/state.go b/internal/utils/state.go index 241f17f3..a31ae78f 100644 --- a/internal/utils/state.go +++ b/internal/utils/state.go @@ -5,7 +5,6 @@ import ( syncv1alpha1 "github.com/Skyscanner/applicationset-progressive-sync/api/v1alpha1" argov1alpha1 "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1" - "github.com/argoproj/gitops-engine/pkg/health" "k8s.io/apimachinery/pkg/util/intstr" ) @@ -150,8 +149,6 @@ func (s *InMemorySyncState) RefreshState(apps []argov1alpha1.Application, stage s.MaxTargetsPerStage[stage.Name] = maxTargets } - healthyApps := GetAppsByHealthStatusCode(unmarkedApps, health.HealthStatusHealthy) - _, ok := s.SyncedAppsPerStage[stage.Name] if !ok { s.SyncedAppsPerStage[stage.Name] = 0 @@ -159,12 +156,12 @@ func (s *InMemorySyncState) RefreshState(apps []argov1alpha1.Application, stage appsToMark := s.MaxTargetsPerStage[stage.Name] - s.SyncedAppsPerStage[stage.Name] - if len(healthyApps) < appsToMark { - appsToMark = len(healthyApps) + if len(unmarkedApps) < appsToMark { + appsToMark = len(unmarkedApps) } for i := 0; i < appsToMark; i++ { - s.SyncedAtStage[healthyApps[i].Name] = stage.Name + s.SyncedAtStage[unmarkedApps[i].Name] = stage.Name s.SyncedAppsPerStage[stage.Name]++ }