No longer waits for healthy before completing sync op. Closes #1715 (#…

…1727)

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:

controller/sync.go

@@ -259,14 +259,19 @@ 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
 	}
 
 	// remove any tasks not in this wave
 	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)")
+		}
 	}
 }
 

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)

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) {

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) {

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" }))
 }
 

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))
+}

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

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

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
 

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

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

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
+