diff --git a/Makefile b/Makefile index 6eccdf423894b..80803a4678a7e 100644 --- a/Makefile +++ b/Makefile @@ -135,7 +135,7 @@ dep-ensure: lint: # golangci-lint does not do a good job of formatting imports goimports -local github.com/argoproj/argo-cd -w `find . ! -path './vendor/*' ! -path './pkg/client/*' -type f -name '*.go'` - golangci-lint run --fix --verbose + golangci-lint run --fix --verbose --concurrency 4 .PHONY: build build: diff --git a/controller/sync.go b/controller/sync.go index 896a05fc29a24..1652fb48bd2ea 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -259,7 +259,7 @@ func (sc *syncContext) sync() { // If no sync tasks were generated (e.g., in case all application manifests have been removed), // the sync operation is successful. if len(tasks) == 0 { - sc.setOperationPhase(v1alpha1.OperationSucceeded, "successfully synced") + sc.setOperationPhase(v1alpha1.OperationSucceeded, "successfully synced (no more tasks)") return } @@ -267,6 +267,11 @@ func (sc *syncContext) sync() { phase := tasks.phase() wave := tasks.wave() + // if it is the last phase/wave and the only remaining tasks are non-hooks, the we are successful + // EVEN if those objects subsequently degraded + // This handles the common case where neither hooks or waves are used and a sync equates to simply an (asynchronous) kubectl apply of manifests, which succeeds immediately. + complete := tasks.Find(func(t *syncTask) bool { return t.phase != phase || wave != t.wave() || t.isHook() }) == nil + sc.log.WithFields(log.Fields{"phase": phase, "wave": wave, "tasks": tasks}).Debug("filtering tasks in correct phase and wave") tasks = tasks.Filter(func(t *syncTask) bool { return t.phase == phase && t.wave() == wave }) @@ -275,6 +280,10 @@ func (sc *syncContext) sync() { sc.log.WithFields(log.Fields{"tasks": tasks}).Debug("wet-run") if !sc.runTasks(tasks, false) { sc.setOperationPhase(v1alpha1.OperationFailed, "one or more objects failed to apply") + } else { + if complete { + sc.setOperationPhase(v1alpha1.OperationSucceeded, "successfully synced (all tasks run)") + } } } diff --git a/controller/sync_test.go b/controller/sync_test.go index d6af8ce3f0970..ed9233b26ca46 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -105,7 +105,7 @@ func TestSyncCreateInSortedOrder(t *testing.T) { }}, } syncCtx.sync() - assert.Equal(t, v1alpha1.OperationRunning, syncCtx.opState.Phase) + assert.Equal(t, v1alpha1.OperationSucceeded, syncCtx.opState.Phase) assert.Len(t, syncCtx.syncRes.Resources, 2) for i := range syncCtx.syncRes.Resources { result := syncCtx.syncRes.Resources[i] @@ -188,7 +188,7 @@ func TestSyncSuccessfully(t *testing.T) { }}, } syncCtx.sync() - assert.Equal(t, v1alpha1.OperationRunning, syncCtx.opState.Phase) + assert.Equal(t, v1alpha1.OperationSucceeded, syncCtx.opState.Phase) assert.Len(t, syncCtx.syncRes.Resources, 2) for i := range syncCtx.syncRes.Resources { result := syncCtx.syncRes.Resources[i] @@ -220,7 +220,7 @@ func TestSyncDeleteSuccessfully(t *testing.T) { }}, } syncCtx.sync() - assert.Equal(t, v1alpha1.OperationRunning, syncCtx.opState.Phase) + assert.Equal(t, v1alpha1.OperationSucceeded, syncCtx.opState.Phase) for i := range syncCtx.syncRes.Resources { result := syncCtx.syncRes.Resources[i] if result.Kind == "Pod" { @@ -315,7 +315,7 @@ func TestDontPrunePruneFalse(t *testing.T) { syncCtx.sync() - assert.Equal(t, v1alpha1.OperationRunning, syncCtx.opState.Phase) + assert.Equal(t, v1alpha1.OperationSucceeded, syncCtx.opState.Phase) assert.Len(t, syncCtx.syncRes.Resources, 1) assert.Equal(t, v1alpha1.ResultCodePruneSkipped, syncCtx.syncRes.Resources[0].Status) assert.Equal(t, "ignored (no prune)", syncCtx.syncRes.Resources[0].Message) diff --git a/test/e2e/app_management_test.go b/test/e2e/app_management_test.go index ea6269910ac5a..818f28a1b1395 100644 --- a/test/e2e/app_management_test.go +++ b/test/e2e/app_management_test.go @@ -299,7 +299,7 @@ func TestResourceDiffing(t *testing.T) { } func TestDeprecatedExtensions(t *testing.T) { - testEdgeCasesApplicationResources(t, "deprecated-extensions", OperationRunning, HealthStatusProgressing) + testEdgeCasesApplicationResources(t, "deprecated-extensions", OperationSucceeded, HealthStatusProgressing) } func TestCRDs(t *testing.T) { diff --git a/test/e2e/fixture/app/expectation.go b/test/e2e/fixture/app/expectation.go index d543b2429afa5..1397e246979a6 100644 --- a/test/e2e/fixture/app/expectation.go +++ b/test/e2e/fixture/app/expectation.go @@ -81,6 +81,29 @@ func ResourceHealthIs(kind, resource string, expected HealthStatusCode) Expectat return simple(actual == expected, fmt.Sprintf("resource '%s/%s' health should be %s, is %s", kind, resource, expected, actual)) } } +func ResourceResultNumbering(num int) Expectation { + return func(c *Consequences) (state, string) { + actualNum := len(c.app().Status.OperationState.SyncResult.Resources) + if actualNum < num { + return pending, fmt.Sprintf("not enough results yet, want %d, got %d", num, actualNum) + } else if actualNum == num { + return succeeded, fmt.Sprintf("right number of results, want %d, got %d", num, actualNum) + } else { + return failed, fmt.Sprintf("too many results, want %d, got %d", num, actualNum) + } + } +} + +func ResourceResultIs(result ResourceResult) Expectation { + return func(c *Consequences) (state, string) { + for _, res := range c.app().Status.OperationState.SyncResult.Resources { + if *res == result { + return succeeded, fmt.Sprintf("found resource result %v", result) + } + } + return pending, fmt.Sprintf("waiting for resource result %v", result) + } +} func DoesNotExist() Expectation { return func(c *Consequences) (state, string) { diff --git a/test/e2e/hook_test.go b/test/e2e/hook_test.go index c1084ecf6b374..b2b8500a14015 100644 --- a/test/e2e/hook_test.go +++ b/test/e2e/hook_test.go @@ -13,19 +13,21 @@ import ( ) func TestPreSyncHookSuccessful(t *testing.T) { - testHookSuccessful(t, HookTypePreSync) + // special-case that the pod remains in the running state, but we don't really care, because this is only used for + // determining overall operation status is a sync with >1 wave/phase + testHookSuccessful(t, HookTypePreSync, OperationRunning) } func TestSyncHookSuccessful(t *testing.T) { - testHookSuccessful(t, HookTypeSync) + testHookSuccessful(t, HookTypeSync, OperationSucceeded) } func TestPostSyncHookSuccessful(t *testing.T) { - testHookSuccessful(t, HookTypePostSync) + testHookSuccessful(t, HookTypePostSync, OperationSucceeded) } // make sure we can run a standard sync hook -func testHookSuccessful(t *testing.T, hookType HookType) { +func testHookSuccessful(t *testing.T, hookType HookType, podHookPhase OperationPhase) { Given(t). Path("hook"). When(). @@ -38,13 +40,9 @@ func testHookSuccessful(t *testing.T, hookType HookType) { Expect(ResourceSyncStatusIs("Pod", "pod", SyncStatusCodeSynced)). Expect(ResourceHealthIs("Pod", "pod", HealthStatusHealthy)). Expect(Pod(func(p v1.Pod) bool { return p.Name == "hook" })). - And(func(app *Application) { - // a retentive check on the resource results - _, pod := app.Status.OperationState.SyncResult.Resources.Find("", "Pod", DeploymentNamespace(), "pod", SyncPhaseSync) - assert.Equal(t, ResourceResult{Version: "v1", Kind: "Pod", Namespace: DeploymentNamespace(), Name: "pod", Status: ResultCodeSynced, Message: "pod/pod created", HookPhase: OperationSucceeded, SyncPhase: SyncPhaseSync}, *pod) - _, hook := app.Status.OperationState.SyncResult.Resources.Find("", "Pod", DeploymentNamespace(), "hook", SyncPhase(hookType)) - assert.Equal(t, ResourceResult{Version: "v1", Kind: "Pod", Namespace: DeploymentNamespace(), Name: "hook", Message: "pod/hook created", HookType: hookType, HookPhase: OperationSucceeded, SyncPhase: SyncPhase(hookType)}, *hook) - }) + Expect(ResourceResultNumbering(2)). + Expect(ResourceResultIs(ResourceResult{Version: "v1", Kind: "Pod", Namespace: DeploymentNamespace(), Name: "pod", Status: ResultCodeSynced, Message: "pod/pod created", HookPhase: podHookPhase, SyncPhase: SyncPhaseSync})). + Expect(ResourceResultIs(ResourceResult{Version: "v1", Kind: "Pod", Namespace: DeploymentNamespace(), Name: "hook", Message: "pod/hook created", HookType: hookType, HookPhase: OperationSucceeded, SyncPhase: SyncPhase(hookType)})) } // make sure that if pre-sync fails, we fail the app and we do not create the pod @@ -61,6 +59,7 @@ func TestPreSyncHookFailure(t *testing.T) { Expect(OperationPhaseIs(OperationFailed)). // if a pre-sync hook fails, we should not start the main sync Expect(SyncStatusIs(SyncStatusCodeOutOfSync)). + Expect(ResourceResultNumbering(1)). Expect(ResourceSyncStatusIs("Pod", "pod", SyncStatusCodeOutOfSync)) } @@ -77,6 +76,7 @@ func TestSyncHookFailure(t *testing.T) { Expect(OperationPhaseIs(OperationFailed)). // even thought the hook failed, we expect the pod to be in sync Expect(SyncStatusIs(SyncStatusCodeSynced)). + Expect(ResourceResultNumbering(2)). Expect(ResourceSyncStatusIs("Pod", "pod", SyncStatusCodeSynced)) } @@ -93,6 +93,7 @@ func TestPostSyncHookFailure(t *testing.T) { Then(). Expect(OperationPhaseIs(OperationFailed)). Expect(SyncStatusIs(SyncStatusCodeSynced)). + Expect(ResourceResultNumbering(2)). Expect(ResourceSyncStatusIs("Pod", "pod", SyncStatusCodeSynced)) } @@ -111,6 +112,7 @@ func TestPostSyncHookPodFailure(t *testing.T) { Expect(SyncStatusIs(SyncStatusCodeSynced)). Expect(ResourceSyncStatusIs("Pod", "pod", SyncStatusCodeSynced)). Expect(ResourceHealthIs("Pod", "pod", HealthStatusDegraded)). + Expect(ResourceResultNumbering(1)). Expect(NotPod(func(p v1.Pod) bool { return p.Name == "hook" })) } @@ -138,6 +140,7 @@ func TestHookDeletePolicyHookSucceededHookExit1(t *testing.T) { Sync(). Then(). Expect(OperationPhaseIs(OperationFailed)). + Expect(ResourceResultNumbering(2)). Expect(Pod(func(p v1.Pod) bool { return p.Name == "hook" })) } @@ -151,6 +154,7 @@ func TestHookDeletePolicyHookFailedHookExit0(t *testing.T) { Sync(). Then(). Expect(OperationPhaseIs(OperationSucceeded)). + Expect(ResourceResultNumbering(2)). Expect(Pod(func(p v1.Pod) bool { return p.Name == "hook" })) } @@ -165,6 +169,7 @@ func TestHookDeletePolicyHookFailedHookExit1(t *testing.T) { Sync(). Then(). Expect(OperationPhaseIs(OperationFailed)). + Expect(ResourceResultNumbering(2)). Expect(NotPod(func(p v1.Pod) bool { return p.Name == "hook" })) } @@ -179,6 +184,7 @@ func TestHookSkip(t *testing.T) { Sync(). Then(). Expect(OperationPhaseIs(OperationSucceeded)). + Expect(ResourceResultNumbering(1)). Expect(NotPod(func(p v1.Pod) bool { return p.Name == "pod" })) } diff --git a/test/e2e/sync_waves_test.go b/test/e2e/sync_waves_test.go index 34af946889f37..30a58fe21a102 100644 --- a/test/e2e/sync_waves_test.go +++ b/test/e2e/sync_waves_test.go @@ -18,6 +18,7 @@ func TestFixingDegradedApp(t *testing.T) { Expect(OperationPhaseIs(OperationFailed)). Expect(SyncStatusIs(SyncStatusCodeOutOfSync)). Expect(HealthIs(HealthStatusMissing)). + Expect(ResourceResultNumbering(1)). Expect(ResourceSyncStatusIs("Pod", "pod-1", SyncStatusCodeSynced)). Expect(ResourceHealthIs("Pod", "pod-1", HealthStatusDegraded)). Expect(ResourceSyncStatusIs("Pod", "pod-2", SyncStatusCodeOutOfSync)). @@ -34,6 +35,7 @@ func TestFixingDegradedApp(t *testing.T) { Expect(OperationPhaseIs(OperationFailed)). Expect(SyncStatusIs(SyncStatusCodeOutOfSync)). Expect(HealthIs(HealthStatusMissing)). + Expect(ResourceResultNumbering(1)). Expect(ResourceSyncStatusIs("Pod", "pod-1", SyncStatusCodeSynced)). Expect(ResourceHealthIs("Pod", "pod-1", HealthStatusHealthy)). Expect(ResourceSyncStatusIs("Pod", "pod-2", SyncStatusCodeOutOfSync)). @@ -44,8 +46,56 @@ func TestFixingDegradedApp(t *testing.T) { Expect(OperationPhaseIs(OperationSucceeded)). Expect(SyncStatusIs(SyncStatusCodeSynced)). Expect(HealthIs(HealthStatusHealthy)). + Expect(ResourceResultNumbering(2)). Expect(ResourceSyncStatusIs("Pod", "pod-1", SyncStatusCodeSynced)). Expect(ResourceHealthIs("Pod", "pod-1", HealthStatusHealthy)). Expect(ResourceSyncStatusIs("Pod", "pod-2", SyncStatusCodeSynced)). Expect(ResourceHealthIs("Pod", "pod-2", HealthStatusHealthy)) } + +func TestOneProgressingDeploymentIsSucceededAndSynced(t *testing.T) { + Given(t). + Path("one-deployment"). + When(). + // make this deployment get stuck in progressing due to "invalidimagename" + PatchFile("deployment.yaml", `[ + { + "op": "replace", + "path": "/spec/template/spec/containers/0/image", + "value": "alpine:ops!" + } +]`). + Create(). + Sync(). + Then(). + Expect(OperationPhaseIs(OperationSucceeded)). + Expect(HealthIs(HealthStatusProgressing)). + Expect(SyncStatusIs(SyncStatusCodeSynced)). + Expect(ResourceResultNumbering(1)) +} + +func TestDegradedDeploymentIsSucceededAndSynced(t *testing.T) { + Given(t). + Path("one-deployment"). + When(). + // make this deployment get stuck in progressing due to "invalidimagename" + PatchFile("deployment.yaml", `[ + { + "op": "replace", + "path": "/spec/progressDeadlineSeconds", + "value": 1 + }, + { + "op": "replace", + "path": "/spec/template/spec/containers/0/image", + "value": "alpine:ops!" + } +]`). + Create(). + Sync(). + Then(). + Expect(OperationPhaseIs(OperationSucceeded)). + Expect(HealthIs(HealthStatusDegraded)). + Expect(SyncStatusIs(SyncStatusCodeSynced)). + Expect(ResourceResultNumbering(1)) +} diff --git a/test/e2e/testdata/deprecated-extensions/deployment.yaml b/test/e2e/testdata/deprecated-extensions/deployment.yaml index 2f09b4eb1284c..b82457347dadf 100644 --- a/test/e2e/testdata/deprecated-extensions/deployment.yaml +++ b/test/e2e/testdata/deprecated-extensions/deployment.yaml @@ -16,7 +16,7 @@ spec: spec: containers: - name: extensions-deployment - image: "gcr.io/heptio-images/ks-guestbook-demo:0.1" + image: "gcr.io/heptio-images/ks-guestbook-demo:0.2" imagePullPolicy: IfNotPresent ports: - name: http diff --git a/test/e2e/testdata/guestbook/guestbook-ui-deployment.yaml b/test/e2e/testdata/guestbook/guestbook-ui-deployment.yaml index 8538b16e1cbba..1d7f77bb27f6e 100644 --- a/test/e2e/testdata/guestbook/guestbook-ui-deployment.yaml +++ b/test/e2e/testdata/guestbook/guestbook-ui-deployment.yaml @@ -15,6 +15,7 @@ spec: spec: containers: - image: gcr.io/heptio-images/ks-guestbook-demo:0.2 + imagePullPolicy: IfNotPresent name: guestbook-ui ports: - containerPort: 80 diff --git a/test/e2e/testdata/multi-namespace-hook/multi-namespace-hook.yaml b/test/e2e/testdata/multi-namespace-hook/multi-namespace-hook.yaml index 24779ed5ded2b..617bfa709a1ca 100644 --- a/test/e2e/testdata/multi-namespace-hook/multi-namespace-hook.yaml +++ b/test/e2e/testdata/multi-namespace-hook/multi-namespace-hook.yaml @@ -22,7 +22,8 @@ spec: spec: containers: - name: nginx - image: nginx:1.15.4 + image: nginx:latest + imagePullPolicy: IfNotPresent ports: - containerPort: 80 diff --git a/test/e2e/testdata/multi-namespace/deployment-with-namespace.yaml b/test/e2e/testdata/multi-namespace/deployment-with-namespace.yaml index ade2a4b323190..ffc0eb38e3ce9 100644 --- a/test/e2e/testdata/multi-namespace/deployment-with-namespace.yaml +++ b/test/e2e/testdata/multi-namespace/deployment-with-namespace.yaml @@ -17,7 +17,7 @@ spec: spec: containers: - name: helm-guestbook - image: "gcr.io/heptio-images/ks-guestbook-demo:0.1" + image: "gcr.io/heptio-images/ks-guestbook-demo:0.2" imagePullPolicy: IfNotPresent ports: - name: http diff --git a/test/e2e/testdata/multi-namespace/deployment.yaml b/test/e2e/testdata/multi-namespace/deployment.yaml index 37e8022338503..79b8fbecd7d7a 100644 --- a/test/e2e/testdata/multi-namespace/deployment.yaml +++ b/test/e2e/testdata/multi-namespace/deployment.yaml @@ -16,7 +16,7 @@ spec: spec: containers: - name: helm-guestbook - image: "gcr.io/heptio-images/ks-guestbook-demo:0.1" + image: "gcr.io/heptio-images/ks-guestbook-demo:0.2" imagePullPolicy: IfNotPresent ports: - name: http diff --git a/test/e2e/testdata/one-deployment/deployment.yaml b/test/e2e/testdata/one-deployment/deployment.yaml new file mode 100644 index 0000000000000..198397bc3dd53 --- /dev/null +++ b/test/e2e/testdata/one-deployment/deployment.yaml @@ -0,0 +1,21 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: main +spec: + replicas: 1 + progressDeadlineSeconds: 30 + selector: + matchLabels: + my-label: whatever + template: + metadata: + labels: + my-label: whatever + spec: + containers: + - name: main + command: ["sleep", "999"] + image: alpine:latest + imagePullPolicy: IfNotPresent +