Add more logs and fix scheduler logic (#130)

* Add more logs * Remove logic for progressing and out-of-sync apps in the scheduler * Schedule looks at all apps and not only healthy ones * Fix the test * PR remarks * Fix merge Co-authored-by: Nebojsa Prodana <nebojsa.prodana@skyscanner.net> Co-authored-by: Matteo Ruina <matteo.ruina@gmail.com>
Skyscanner · Aug 6, 2021 · 9cac96e · 9cac96e
1 parent 1f1e535
commit 9cac96e
Showing 5 changed files with 101 additions and 17 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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,21 +854,80 @@ 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",
 						Namespace: SchedulerTestNamespace,
 					},
 					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
@@ -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,21 +149,19 @@ 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
 	}
 
 	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]++
 	}