From 9b142c799a0c09c42f4aa603d0ca711e76639957 Mon Sep 17 00:00:00 2001 From: Alexander Matyushentsev Date: Tue, 28 Apr 2020 08:49:07 -0700 Subject: [PATCH] fix: 'argocd sync' does not take into account IgnoreExtraneous annotation (#3486) --- cmd/argocd/commands/app.go | 42 ++++++++++++++++++++++----------- controller/sync.go | 6 ++--- test/e2e/app_management_test.go | 8 ++++++- test/e2e/kustomize_test.go | 2 -- 4 files changed, 38 insertions(+), 20 deletions(-) diff --git a/cmd/argocd/commands/app.go b/cmd/argocd/commands/app.go index ed8330989c6a8..28ccb3b0f7381 100644 --- a/cmd/argocd/commands/app.go +++ b/cmd/argocd/commands/app.go @@ -1488,15 +1488,15 @@ func NewApplicationSyncCommand(clientOpts *argocdclient.ClientOptions) *cobra.Co app, err := waitOnApplicationStatus(acdClient, appName, timeout, false, false, true, false, selectedResources) errors.CheckError(err) - // Only get resources to be pruned if sync was application-wide - if len(selectedResources) == 0 { - pruningRequired := app.Status.OperationState.SyncResult.Resources.PruningRequired() - if pruningRequired > 0 { - log.Fatalf("%d resources require pruning", pruningRequired) - } - - if !app.Status.OperationState.Phase.Successful() && !dryRun { - os.Exit(1) + if !dryRun { + if !app.Status.OperationState.Phase.Successful() { + log.Fatalf("Operation has completed with phase: %s", app.Status.OperationState.Phase) + } else if len(selectedResources) == 0 && app.Status.Sync.Status != argoappv1.SyncStatusCodeSynced { + // Only get resources to be pruned if sync was application-wide and final status is not synced + pruningRequired := app.Status.OperationState.SyncResult.Resources.PruningRequired() + if pruningRequired > 0 { + log.Fatalf("%d resources require pruning", pruningRequired) + } } } } @@ -1652,7 +1652,7 @@ func waitOnApplicationStatus(acdClient apiclient.Client, appName string, timeout // time when the sync status lags behind when an operation completes refresh := false - printFinalStatus := func(app *argoappv1.Application) { + printFinalStatus := func(app *argoappv1.Application) *argoappv1.Application { var err error if refresh { conn, appClient := acdClient.NewApplicationClientOrDie() @@ -1675,6 +1675,7 @@ func waitOnApplicationStatus(acdClient apiclient.Client, appName string, timeout printAppResources(w, app) _ = w.Flush() } + return app } if timeout != 0 { @@ -1695,8 +1696,21 @@ func waitOnApplicationStatus(acdClient apiclient.Client, appName string, timeout for appEvent := range appEventCh { app = &appEvent.Application + + operationInProgress := false + // consider the operation is in progress if app.Operation != nil { + // if it just got requested + operationInProgress = true refresh = true + } else if app.Status.OperationState != nil { + if app.Status.OperationState.FinishedAt == nil { + // if it is not finished yet + operationInProgress = true + } else if app.Status.ReconciledAt == nil || app.Status.ReconciledAt.Before(app.Status.OperationState.FinishedAt) { + // if it is just finished and we need to wait for controller to reconcile app once after syncing + operationInProgress = true + } } var selectedResourcesAreReady bool @@ -1716,8 +1730,8 @@ func waitOnApplicationStatus(acdClient apiclient.Client, appName string, timeout selectedResourcesAreReady = checkResourceStatus(watchSync, watchHealth, watchOperation, watchSuspended, app.Status.Health.Status, string(app.Status.Sync.Status), appEvent.Application.Operation) } - if selectedResourcesAreReady { - printFinalStatus(app) + if selectedResourcesAreReady && !operationInProgress { + app = printFinalStatus(app) return app, nil } @@ -1727,7 +1741,7 @@ func waitOnApplicationStatus(acdClient apiclient.Client, appName string, timeout stateKey := newState.Key() if prevState, found := prevStates[stateKey]; found { if watchHealth && prevState.Health != argoappv1.HealthStatusUnknown && prevState.Health != argoappv1.HealthStatusDegraded && newState.Health == argoappv1.HealthStatusDegraded { - printFinalStatus(app) + _ = printFinalStatus(app) return nil, fmt.Errorf("application '%s' health state has transitioned from %s to %s", appName, prevState.Health, newState.Health) } doPrint = prevState.Merge(newState) @@ -1741,7 +1755,7 @@ func waitOnApplicationStatus(acdClient apiclient.Client, appName string, timeout } _ = w.Flush() } - printFinalStatus(app) + _ = printFinalStatus(app) return nil, fmt.Errorf("timed out (%ds) waiting for app %q match desired state", timeout, appName) } diff --git a/controller/sync.go b/controller/sync.go index ebfe86ba3c043..6b5b6307d7e78 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -571,10 +571,10 @@ func (sc *syncContext) applyObject(targetObj *unstructured.Unstructured, dryRun, // pruneObject deletes the object if both prune is true and dryRun is false. Otherwise appropriate message func (sc *syncContext) pruneObject(liveObj *unstructured.Unstructured, prune, dryRun bool) (v1alpha1.ResultCode, string) { - if !prune { - return v1alpha1.ResultCodePruneSkipped, "ignored (requires pruning)" - } else if resource.HasAnnotationOption(liveObj, common.AnnotationSyncOptions, "Prune=false") { + if resource.HasAnnotationOption(liveObj, common.AnnotationSyncOptions, "Prune=false") { return v1alpha1.ResultCodePruneSkipped, "ignored (no prune)" + } else if !prune { + return v1alpha1.ResultCodePruneSkipped, "ignored (requires pruning)" } else { if dryRun { return v1alpha1.ResultCodePruned, "pruned (dry run)" diff --git a/test/e2e/app_management_test.go b/test/e2e/app_management_test.go index d49c3b769b9a2..a0be7fe5363fa 100644 --- a/test/e2e/app_management_test.go +++ b/test/e2e/app_management_test.go @@ -811,6 +811,7 @@ func TestSyncOptionValidateFalse(t *testing.T) { // make sure that, if we have a resource that needs pruning, but we're ignoring it, the app is in-sync func TestCompareOptionIgnoreExtraneous(t *testing.T) { Given(t). + Prune(false). Path("two-nice-pods"). When(). PatchFile("pod-1.yaml", `[{"op": "add", "path": "/metadata/annotations", "value": {"argocd.argoproj.io/compare-options": "IgnoreExtraneous"}}]`). @@ -826,7 +827,12 @@ func TestCompareOptionIgnoreExtraneous(t *testing.T) { Expect(SyncStatusIs(SyncStatusCodeSynced)). And(func(app *Application) { assert.Len(t, app.Status.Resources, 2) - assert.Equal(t, SyncStatusCodeOutOfSync, app.Status.Resources[1].Status) + statusByName := map[string]SyncStatusCode{} + for _, r := range app.Status.Resources { + statusByName[r.Name] = r.Status + } + assert.Equal(t, SyncStatusCodeOutOfSync, statusByName["pod-1"]) + assert.Equal(t, SyncStatusCodeSynced, statusByName["pod-2"]) }). When(). Sync(). diff --git a/test/e2e/kustomize_test.go b/test/e2e/kustomize_test.go index 74af85ff93e45..ec8c652ee8af4 100644 --- a/test/e2e/kustomize_test.go +++ b/test/e2e/kustomize_test.go @@ -88,10 +88,8 @@ func TestSyncStatusOptionIgnore(t *testing.T) { Then(). // this is standard logging from the command - tough one - true statement When(). - IgnoreErrors(). Sync(). Then(). - Expect(Error("", "1 resources require pruning")). Expect(OperationPhaseIs(OperationSucceeded)). // this is a key check - we expect the app to be healthy because, even though we have a resources that needs // pruning, because it is annotated with IgnoreExtraneous it should not contribute to the sync status