Skip to content

Commit

Permalink
fix(applicationset): ensure that older applicationStatus is updated w…
Browse files Browse the repository at this point in the history 
…ith new required values (argoproj#19165) (argoproj#19216)

Signed-off-by: wparr-circle <[email protected]>
Co-authored-by: William Parr <[email protected]>
  • Loading branch information
@gcp-cherry-pick-bot @wparr-circle
gcp-cherry-pick-bot[bot] and wparr-circle authored Jul 25, 2024
1 parent d926310 commit 004cabb
Show file tree
Hide file tree
Showing 2 changed files with 217 additions and 48 deletions.
33 changes: 33 additions & 0 deletions applicationset/controllers/applicationset_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,11 @@ func (r *ApplicationSetReconciler) Reconcile(ctx context.Context, req ctrl.Reque
return ctrl.Result{}, nil
}

if err := r.migrateStatus(ctx, &applicationSetInfo); err != nil {
logCtx.Errorf("failed to migrate status subresource %v", err)
return ctrl.Result{}, err
}

// Log a warning if there are unrecognized generators
_ = utils.CheckInvalidGenerators(&applicationSetInfo)
// desiredApplications is the main list of all expected Applications from all generators in this appset.
Expand Down Expand Up @@ -1151,6 +1156,13 @@ func (r *ApplicationSetReconciler) updateApplicationSetApplicationStatus(ctx con
} else {
// we have an existing AppStatus
currentAppStatus = applicationSet.Status.ApplicationStatus[idx]

// upgrade any existing AppStatus that might have been set by an older argo-cd version
// note: currentAppStatus.TargetRevisions may be set to empty list earlier during migrations,
// to prevent other usage of r.Client.Status().Update to fail before reaching here.
if currentAppStatus.TargetRevisions == nil || len(currentAppStatus.TargetRevisions) == 0 {
currentAppStatus.TargetRevisions = app.Status.GetRevisions()
}
}

appOutdated := false
Expand Down Expand Up @@ -1348,6 +1360,27 @@ func findApplicationStatusIndex(appStatuses []argov1alpha1.ApplicationSetApplica
return -1
}

// migrateStatus run migrations on the status subresource of ApplicationSet early during the run of ApplicationSetReconciler.Reconcile
// this handles any defaulting of values - which would otherwise cause the references to r.Client.Status().Update to fail given missing required fields.
func (r *ApplicationSetReconciler) migrateStatus(ctx context.Context, appset *argov1alpha1.ApplicationSet) error {
update := false
if statusList := appset.Status.ApplicationStatus; statusList != nil {
for idx := range statusList {
if statusList[idx].TargetRevisions == nil {
statusList[idx].TargetRevisions = []string{}
update = true
}
}
}

if update {
if err := r.Client.Status().Update(ctx, appset); err != nil {
return fmt.Errorf("unable to set application set status: %w", err)
}
}
return nil
}

func (r *ApplicationSetReconciler) updateResourcesStatus(ctx context.Context, logCtx *log.Entry, appset *argov1alpha1.ApplicationSet, apps []argov1alpha1.Application) error {
statusMap := getResourceStatusMap(appset)
statusMap = buildResourceStatus(statusMap, apps)
Expand Down
232 changes: 184 additions & 48 deletions applicationset/controllers/applicationset_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4761,6 +4761,58 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
},
},
},
{
name: "handles an outdated list of statuses with a healthy application, setting required variables",
appSet: v1alpha1.ApplicationSet{
ObjectMeta: metav1.ObjectMeta{
Name: "name",
Namespace: "argocd",
},
Spec: v1alpha1.ApplicationSetSpec{
Strategy: &v1alpha1.ApplicationSetStrategy{
Type: "RollingSync",
RollingSync: &v1alpha1.ApplicationSetRolloutStrategy{},
},
},
Status: v1alpha1.ApplicationSetStatus{
ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{
{
Application: "app1",
Message: "Application resource is already Healthy, updating status from Waiting to Healthy.",
Status: "Healthy",
Step: "1",
},
},
},
},
apps: []v1alpha1.Application{
{
ObjectMeta: metav1.ObjectMeta{
Name: "app1",
},
Status: v1alpha1.ApplicationStatus{
Health: v1alpha1.HealthStatus{
Status: health.HealthStatusHealthy,
},
OperationState: &v1alpha1.OperationState{
Phase: common.OperationSucceeded,
},
Sync: v1alpha1.SyncStatus{
Status: v1alpha1.SyncStatusCodeSynced,
},
},
},
},
expectedAppStatus: []v1alpha1.ApplicationSetApplicationStatus{
{
Application: "app1",
Message: "Application resource is already Healthy, updating status from Waiting to Healthy.",
Status: "Healthy",
Step: "1",
TargetRevisions: []string{},
},
},
},
{
name: "progresses an OutOfSync RollingSync application to waiting",
appSet: v1alpha1.ApplicationSet{
Expand Down Expand Up @@ -4850,10 +4902,11 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
Status: v1alpha1.ApplicationSetStatus{
ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{
{
Application: "app1",
Message: "",
Status: "Pending",
Step: "1",
Application: "app1",
Message: "",
Status: "Pending",
Step: "1",
TargetRevisions: []string{"Next"},
},
},
},
Expand All @@ -4872,15 +4925,16 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
},
expectedAppStatus: []v1alpha1.ApplicationSetApplicationStatus{
{
Application: "app1",
Message: "Application resource became Progressing, updating status from Pending to Progressing.",
Status: "Progressing",
Step: "1",
Application: "app1",
Message: "Application resource became Progressing, updating status from Pending to Progressing.",
Status: "Progressing",
Step: "1",
TargetRevisions: []string{"Next"},
},
},
},
{
name: "progresses a pending syncing application to progressing",
name: "progresses a pending synced application to progressing",
appSet: v1alpha1.ApplicationSet{
ObjectMeta: metav1.ObjectMeta{
Name: "name",
Expand All @@ -4895,10 +4949,11 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
Status: v1alpha1.ApplicationSetStatus{
ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{
{
Application: "app1",
Message: "",
Status: "Pending",
Step: "1",
Application: "app1",
Message: "",
Status: "Pending",
Step: "1",
TargetRevisions: []string{"Current"},
},
},
},
Expand All @@ -4923,10 +4978,11 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
},
expectedAppStatus: []v1alpha1.ApplicationSetApplicationStatus{
{
Application: "app1",
Message: "Application resource became Progressing, updating status from Pending to Progressing.",
Status: "Progressing",
Step: "1",
Application: "app1",
Message: "Application resource became Progressing, updating status from Pending to Progressing.",
Status: "Progressing",
Step: "1",
TargetRevisions: []string{"Current"},
},
},
},
Expand All @@ -4946,10 +5002,11 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
Status: v1alpha1.ApplicationSetStatus{
ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{
{
Application: "app1",
Message: "",
Status: "Progressing",
Step: "1",
Application: "app1",
Message: "",
Status: "Progressing",
Step: "1",
TargetRevisions: []string{"Next"},
},
},
},
Expand All @@ -4974,10 +5031,11 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
},
expectedAppStatus: []v1alpha1.ApplicationSetApplicationStatus{
{
Application: "app1",
Message: "Application resource became Healthy, updating status from Progressing to Healthy.",
Status: "Healthy",
Step: "1",
Application: "app1",
Message: "Application resource became Healthy, updating status from Progressing to Healthy.",
Status: "Healthy",
Step: "1",
TargetRevisions: []string{"Next"},
},
},
},
Expand All @@ -4997,10 +5055,11 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
Status: v1alpha1.ApplicationSetStatus{
ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{
{
Application: "app1",
Message: "",
Status: "Waiting",
Step: "1",
Application: "app1",
Message: "",
Status: "Waiting",
Step: "1",
TargetRevisions: []string{"Current"},
},
},
},
Expand All @@ -5025,10 +5084,11 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
},
expectedAppStatus: []v1alpha1.ApplicationSetApplicationStatus{
{
Application: "app1",
Message: "Application resource is already Healthy, updating status from Waiting to Healthy.",
Status: "Healthy",
Step: "1",
Application: "app1",
Message: "Application resource is already Healthy, updating status from Waiting to Healthy.",
Status: "Healthy",
Step: "1",
TargetRevisions: []string{"Current"},
},
},
},
Expand Down Expand Up @@ -5304,16 +5364,18 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
Status: v1alpha1.ApplicationSetStatus{
ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{
{
Application: "app1",
Message: "Application has pending changes, setting status to Waiting.",
Status: "Waiting",
Step: "1",
Application: "app1",
Message: "Application has pending changes, setting status to Waiting.",
Status: "Waiting",
Step: "1",
TargetRevisions: []string{"Current"},
},
{
Application: "app2",
Message: "Application has pending changes, setting status to Waiting.",
Status: "Waiting",
Step: "1",
Application: "app2",
Message: "Application has pending changes, setting status to Waiting.",
Status: "Waiting",
Step: "1",
TargetRevisions: []string{"Current"},
},
},
},
Expand All @@ -5338,10 +5400,11 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
},
expectedAppStatus: []v1alpha1.ApplicationSetApplicationStatus{
{
Application: "app1",
Message: "Application resource is already Healthy, updating status from Waiting to Healthy.",
Status: "Healthy",
Step: "1",
Application: "app1",
Message: "Application resource is already Healthy, updating status from Waiting to Healthy.",
Status: "Healthy",
Step: "1",
TargetRevisions: []string{"Current"},
},
},
},
Expand Down Expand Up @@ -5503,9 +5566,10 @@ func TestUpdateApplicationSetApplicationStatusProgress(t *testing.T) {
Status: v1alpha1.ApplicationSetStatus{
ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{
{
Application: "app1",
Message: "Application is out of date with the current AppSet generation, setting status to Waiting.",
Status: "Waiting",
Application: "app1",
Message: "Application is out of date with the current AppSet generation, setting status to Waiting.",
Status: "Waiting",
TargetRevisions: []string{"Next"},
},
},
},
Expand All @@ -5523,6 +5587,7 @@ func TestUpdateApplicationSetApplicationStatusProgress(t *testing.T) {
Message: "Application moved to Pending status, watching for the Application resource to start Progressing.",
Status: "Pending",
Step: "1",
TargetRevisions: []string{"Next"},
},
},
},
Expand Down Expand Up @@ -6529,3 +6594,74 @@ func TestOwnsHandler(t *testing.T) {
})
}
}

func TestMigrateStatus(t *testing.T) {
scheme := runtime.NewScheme()
err := v1alpha1.AddToScheme(scheme)
require.NoError(t, err)

err = v1alpha1.AddToScheme(scheme)
require.NoError(t, err)

for _, tc := range []struct {
name string
appset v1alpha1.ApplicationSet
expectedStatus v1alpha1.ApplicationSetStatus
}{
{
name: "status without applicationstatus target revisions set will default to empty list",
appset: v1alpha1.ApplicationSet{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Namespace: "test",
},
Status: v1alpha1.ApplicationSetStatus{
ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{
{},
},
},
},
expectedStatus: v1alpha1.ApplicationSetStatus{
ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{
{
TargetRevisions: []string{},
},
},
},
},
{
name: "status with applicationstatus target revisions set will do nothing",
appset: v1alpha1.ApplicationSet{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Namespace: "test",
},
Status: v1alpha1.ApplicationSetStatus{
ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{
{
TargetRevisions: []string{"Current"},
},
},
},
},
expectedStatus: v1alpha1.ApplicationSetStatus{
ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{
{
TargetRevisions: []string{"Current"},
},
},
},
},
} {
t.Run(tc.name, func(t *testing.T) {
client := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(&tc.appset).WithObjects(&tc.appset).Build()
r := ApplicationSetReconciler{
Client: client,
}

err := r.migrateStatus(context.Background(), &tc.appset)
require.NoError(t, err)
assert.Equal(t, tc.expectedStatus, tc.appset.Status)
})
}
}

0 comments on commit 004cabb

Please sign in to comment.