diff --git a/pkg/sync/sync_context.go b/pkg/sync/sync_context.go index f0b8f4998..ef7bd45bf 100644 --- a/pkg/sync/sync_context.go +++ b/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 { diff --git a/pkg/sync/sync_context_test.go b/pkg/sync/sync_context_test.go index d575f3270..fe8f954c3 100644 --- a/pkg/sync/sync_context_test.go +++ b/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 diff --git a/pkg/sync/sync_task.go b/pkg/sync/sync_task.go index 4a4ab7664..bbe6319f9 100644 --- a/pkg/sync/sync_task.go +++ b/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) }