Skip to content
73 changes: 68 additions & 5 deletions applicationset/controllers/applicationset_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,8 @@ func (r *ApplicationSetReconciler) Reconcile(ctx context.Context, req ctrl.Reque

// appSyncMap tracks which apps will be synced during this reconciliation.
appSyncMap := map[string]bool{}
var appDependencyList [][]string
var appStepMap map[string]int
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var appDependencyList [][]string
var appStepMap map[string]int
var (
appDependencyList [][]string
appStepMap map[string]int
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to put the appSyncMap too.


if r.EnableProgressiveSyncs {
if !isRollingSyncStrategy(&applicationSetInfo) && len(applicationSetInfo.Status.ApplicationStatus) > 0 {
Expand All @@ -243,7 +245,7 @@ func (r *ApplicationSetReconciler) Reconcile(ctx context.Context, req ctrl.Reque
return ctrl.Result{}, fmt.Errorf("failed to clear previous AppSet application statuses for %v: %w", applicationSetInfo.Name, err)
}
} else if isRollingSyncStrategy(&applicationSetInfo) {
appSyncMap, err = r.performProgressiveSyncs(ctx, logCtx, applicationSetInfo, currentApplications, generatedApplications)
appSyncMap, appDependencyList, appStepMap, err = r.performProgressiveSyncs(ctx, logCtx, applicationSetInfo, currentApplications, generatedApplications)
if err != nil {
return ctrl.Result{}, fmt.Errorf("failed to perform progressive sync reconciliation for application set: %w", err)
}
Expand Down Expand Up @@ -298,6 +300,8 @@ func (r *ApplicationSetReconciler) Reconcile(ctx context.Context, req ctrl.Reque
// trigger appropriate application syncs if RollingSync strategy is enabled
if progressiveSyncsRollingSyncStrategyEnabled(&applicationSetInfo) {
validApps = r.syncDesiredApplications(logCtx, &applicationSetInfo, appSyncMap, validApps)
// Filter applications to only create/update those in the current eligible step
validApps = r.filterApplicationsByProgressiveStep(logCtx, &applicationSetInfo, validApps, appDependencyList, appStepMap)
}
}

Expand Down Expand Up @@ -944,12 +948,12 @@ func (r *ApplicationSetReconciler) removeOwnerReferencesOnDeleteAppSet(ctx conte
return nil
}

func (r *ApplicationSetReconciler) performProgressiveSyncs(ctx context.Context, logCtx *log.Entry, appset argov1alpha1.ApplicationSet, applications []argov1alpha1.Application, desiredApplications []argov1alpha1.Application) (map[string]bool, error) {
func (r *ApplicationSetReconciler) performProgressiveSyncs(ctx context.Context, logCtx *log.Entry, appset argov1alpha1.ApplicationSet, applications []argov1alpha1.Application, desiredApplications []argov1alpha1.Application) (map[string]bool, [][]string, map[string]int, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a method comment that also states why are we returning specific values like slice of slice of strings etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it

appDependencyList, appStepMap := r.buildAppDependencyList(logCtx, appset, desiredApplications)

_, err := r.updateApplicationSetApplicationStatus(ctx, logCtx, &appset, applications, appStepMap)
if err != nil {
return nil, fmt.Errorf("failed to update applicationset app status: %w", err)
return nil, nil, nil, fmt.Errorf("failed to update applicationset app status: %w", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i understand why you returned nil here because there's probably no use of appDepList or appStepMap. However, it's better to return those values since we may need them in future. Returning nil simply means either they haven't been built. I'm of mixed opinions here. @ranakan19 wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah makes sense, i’m fine keeping them returned for now in case they’re needed later. we can clean it up once we’re sure they aren’t used anywhere else.

}

logCtx.Infof("ApplicationSet %v step list:", appset.Name)
Expand All @@ -962,12 +966,12 @@ func (r *ApplicationSetReconciler) performProgressiveSyncs(ctx context.Context,

_, err = r.updateApplicationSetApplicationStatusProgress(ctx, logCtx, &appset, appsToSync, appStepMap)
if err != nil {
return nil, fmt.Errorf("failed to update applicationset application status progress: %w", err)
return nil, nil, nil, fmt.Errorf("failed to update applicationset application status progress: %w", err)
}

_ = r.updateApplicationSetApplicationStatusConditions(ctx, &appset)

return appsToSync, nil
return appsToSync, appDependencyList, appStepMap, nil
}

// this list tracks which Applications belong to each RollingUpdate step
Expand Down Expand Up @@ -1590,6 +1594,65 @@ func (r *ApplicationSetReconciler) syncDesiredApplications(logCtx *log.Entry, ap
return rolloutApps
}

// filterApplicationsByProgressiveStep filters applications to only include those in the current eligible step.
// Applications are only created/updated if all applications in their current step's prerequisite steps are healthy.
func (r *ApplicationSetReconciler) filterApplicationsByProgressiveStep(logCtx *log.Entry, appset *argov1alpha1.ApplicationSet, applications []argov1alpha1.Application, appDependencyList [][]string, appStepMap map[string]int) []argov1alpha1.Application {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please write unit tests for this function.

if len(appDependencyList) == 0 || len(appStepMap) == 0 {
// No progressive sync configuration, return all apps
return applications
}

// Find the highest step index that is complete (all apps in that step are healthy or non-existent but needed)
// Step 0 is always enabled for creation
// Step N is enabled if all apps in step N-1 exist and are healthy
maxCreateableStep := 0 // Start with step 0 always creatable

for stepIndex := 1; stepIndex < len(appDependencyList); stepIndex++ {
// Check if all apps in the previous step are healthy
previousStepComplete := true
for _, appName := range appDependencyList[stepIndex-1] {
idx := findApplicationStatusIndex(appset.Status.ApplicationStatus, appName)
if idx == -1 {
// App status not found or doesn't exist yet
previousStepComplete = false
break
}

appStatus := appset.Status.ApplicationStatus[idx]
if appStatus.Status != argov1alpha1.ProgressiveSyncHealthy {
previousStepComplete = false
break
}
}

if !previousStepComplete {
break // Stop checking further steps if current step's prerequisite isn't complete
}
maxCreateableStep = stepIndex
}

logCtx.Infof("Progressive sync: max creatable step is %d (out of %d)", maxCreateableStep, len(appDependencyList)-1)

// Only include apps from steps up to and including maxCreateableStep
filteredApps := []argov1alpha1.Application{}
for _, app := range applications {
if stepNum, ok := appStepMap[app.Name]; ok {
// Application belongs to a step - only include if within creatable steps
if stepNum <= maxCreateableStep {
filteredApps = append(filteredApps, app)
logCtx.Debugf("Including app %s from step %d (max creatable: %d)", app.Name, stepNum, maxCreateableStep)
} else {
logCtx.Debugf("Excluding app %s from step %d (max creatable: %d)", app.Name, stepNum, maxCreateableStep)
}
} else {
// Application doesn't belong to any step, include it
filteredApps = append(filteredApps, app)
}
}

return filteredApps
}

// used by the RollingSync Progressive Sync strategy to trigger a sync of a particular Application resource
func syncApplication(application argov1alpha1.Application, prune bool) argov1alpha1.Application {
operation := argov1alpha1.Operation{
Expand Down
5 changes: 0 additions & 5 deletions test/e2e/app_management_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3079,11 +3079,6 @@ func TestDeletionConfirmation(t *testing.T) {
When().ConfirmDeletion().
Then().Expect(OperationPhaseIs(OperationSucceeded)).
When().Delete(true).
Then().
And(func(app *Application) {
assert.NotNil(t, app.DeletionTimestamp)
}).
When().ConfirmDeletion().
Then().Expect(DoesNotExist())
}

Expand Down
Loading
Loading