fix: Hook Deletion Policies HookSucceeded should be run after whole H…

pkg/sync/sync_context.go

@@ -345,22 +345,21 @@ func (sc *syncContext) Sync() {
 		return
 	}
 
-	// delete all completed hooks which have appropriate delete policy
-	hooksPendingDeletion := tasks.Filter(func(task *syncTask) bool {
-		return task.isHook() && task.liveObj != nil && !task.running() && task.deleteOnPhaseCompletion()
+	// collect all completed hooks which have appropriate delete policy
+	hooksPendingDeletionSuccessful := tasks.Filter(func(task *syncTask) bool {
+		return task.isHook() && task.liveObj != nil && !task.running() && task.deleteOnPhaseSuccessful()
+	})
+
+	hooksPendingDeletionFailed := tasks.Filter(func(task *syncTask) bool {
+		return task.isHook() && task.liveObj != nil && !task.running() && task.deleteOnPhaseFailed()
 	})
-	for _, task := range hooksPendingDeletion {
-		err := sc.deleteResource(task)
-		if err != nil && !apierr.IsNotFound(err) {
-			sc.setResourceResult(task, "", common.OperationError, fmt.Sprintf("failed to delete resource: %v", err))
-		}
-	}
 
 	// syncFailTasks only run during failure, so separate them from regular tasks
 	syncFailTasks, tasks := tasks.Split(func(t *syncTask) bool { return t.phase == common.SyncPhaseSyncFail })
 
 	// if there are any completed but unsuccessful tasks, sync is a failure.
 	if tasks.Any(func(t *syncTask) bool { return t.completed() && !t.successful() }) {
+		sc.deleteHooks(hooksPendingDeletionFailed)
 		sc.setOperationFailed(syncFailTasks, "one or more synchronization tasks completed unsuccessfully")
 		return
 	}
@@ -372,6 +371,8 @@ 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 {
+		// delete all completed hooks which have appropriate delete policy
+		sc.deleteHooks(hooksPendingDeletionSuccessful)
 		sc.setOperationPhase(common.OperationSucceeded, "successfully synced (no more tasks)")
 		return
 	}
@@ -394,9 +395,12 @@ func (sc *syncContext) Sync() {
 	runState := sc.runTasks(tasks, false)
 	switch runState {
 	case failed:
+		sc.deleteHooks(hooksPendingDeletionFailed)
 		sc.setOperationFailed(syncFailTasks, "one or more objects failed to apply")
 	case successful:
 		if remainingTasks.Len() == 0 {
+			// delete all completed hooks which have appropriate delete policy
+			sc.deleteHooks(hooksPendingDeletionSuccessful)
 			sc.setOperationPhase(common.OperationSucceeded, "successfully synced (all tasks run)")
 		} else {
 			sc.setRunningPhase(remainingTasks, false)
@@ -408,6 +412,15 @@ func (sc *syncContext) Sync() {
 	}
 }
 
+func (sc *syncContext) deleteHooks(hooksPendingDeletion syncTasks) {
+	for _, task := range hooksPendingDeletion {
+		err := sc.deleteResource(task)
+		if err != nil && !apierr.IsNotFound(err) {
+			sc.setResourceResult(task, "", common.OperationError, fmt.Sprintf("failed to delete resource: %v", err))
+		}
+	}
+}
+
 func (sc *syncContext) GetState() (common.OperationPhase, string, []common.ResourceSyncResult) {
 	var resourceRes []common.ResourceSyncResult
 	for _, v := range sc.syncRes {

pkg/sync/sync_context_test.go

@@ -774,6 +774,50 @@ func TestRunSync_HooksDeletedAfterPhaseCompleted(t *testing.T) {
 	assert.Equal(t, 2, deletedCount)
 }
 
+func TestRunSync_HooksDeletedAfterPhaseCompletedFailed(t *testing.T) {
+	completedHook1 := newHook(synccommon.HookTypeSync)
+	completedHook1.SetName("completed-hook1")
+	completedHook1.SetNamespace(FakeArgoCDNamespace)
+	_ = Annotate(completedHook1, synccommon.AnnotationKeyHookDeletePolicy, "HookFailed")
+
+	completedHook2 := newHook(synccommon.HookTypeSync)
+	completedHook2.SetNamespace(FakeArgoCDNamespace)
+	completedHook2.SetName("completed-hook2")
+	_ = Annotate(completedHook2, synccommon.AnnotationKeyHookDeletePolicy, "HookFailed")
+
+	syncCtx := newTestSyncCtx(
+		WithInitialState(synccommon.OperationRunning, "", []synccommon.ResourceSyncResult{{
+			ResourceKey: kube.GetResourceKey(completedHook1),
+			HookPhase:   synccommon.OperationSucceeded,
+			SyncPhase:   synccommon.SyncPhaseSync,
+		}, {
+			ResourceKey: kube.GetResourceKey(completedHook2),
+			HookPhase:   synccommon.OperationFailed,
+			SyncPhase:   synccommon.SyncPhaseSync,
+		}}))
+	fakeDynamicClient := fake.NewSimpleDynamicClient(runtime.NewScheme())
+	syncCtx.dynamicIf = fakeDynamicClient
+	deletedCount := 0
+	fakeDynamicClient.PrependReactor("delete", "*", func(action testcore.Action) (handled bool, ret runtime.Object, err error) {
+		deletedCount += 1
+		return true, nil, nil
+	})
+	syncCtx.resources = groupResources(ReconciliationResult{
+		Live:   []*unstructured.Unstructured{completedHook1, completedHook2},
+		Target: []*unstructured.Unstructured{nil, nil},
+	})
+	syncCtx.hooks = []*unstructured.Unstructured{completedHook1, completedHook2}
+
+	syncCtx.kubectl = &kubetest.MockKubectlCmd{
+		Commands: map[string]kubetest.KubectlOutput{},
+	}
+
+	syncCtx.Sync()
+
+	assert.Equal(t, synccommon.OperationFailed, syncCtx.phase)
+	assert.Equal(t, 2, deletedCount)
+}
+
 func Test_syncContext_liveObj(t *testing.T) {
 	type fields struct {
 		compareResult ReconciliationResult

pkg/sync/sync_task.go

@@ -103,10 +103,6 @@ func (t *syncTask) successful() bool {
 	return t.operationState.Successful()
 }
 
-func (t *syncTask) failed() bool {
-	return t.operationState.Failed()
-}
-
 func (t *syncTask) hookType() common.HookType {
 	if t.isHook() {
 		return common.HookType(t.phase)
@@ -133,6 +129,13 @@ func (t *syncTask) deleteBeforeCreation() bool {
 }
 
 func (t *syncTask) deleteOnPhaseCompletion() bool {
-	return t.liveObj != nil && (t.successful() && t.hasHookDeletePolicy(common.HookDeletePolicyHookSucceeded) ||
-		t.failed() && t.hasHookDeletePolicy(common.HookDeletePolicyHookFailed))
+	return t.deleteOnPhaseFailed() || t.deleteOnPhaseSuccessful()
+}
+
+func (t *syncTask) deleteOnPhaseSuccessful() bool {
+	return t.liveObj != nil && t.hasHookDeletePolicy(common.HookDeletePolicyHookSucceeded)
+}
+
+func (t *syncTask) deleteOnPhaseFailed() bool {
+	return t.liveObj != nil && t.hasHookDeletePolicy(common.HookDeletePolicyHookFailed)
 }