diff --git a/pkg/sync/sync_context.go b/pkg/sync/sync_context.go index a7b08fa5f..63abc9b51 100644 --- a/pkg/sync/sync_context.go +++ b/pkg/sync/sync_context.go @@ -1265,6 +1265,31 @@ func (sc *syncContext) runTasks(tasks syncTasks, dryRun bool) runState { createTasks = append(createTasks, task) } } + + // remove finalizers from previous sync on existing hooks to make sure the operation is idempotent + { + ss := newStateSync(state) + existingHooks := tasks.Filter(func(t *syncTask) bool { return t.isHook() && t.pending() && t.liveObj != nil }) + for _, task := range existingHooks { + t := task + ss.Go(func(state runState) runState { + logCtx := sc.log.WithValues("dryRun", dryRun, "task", t) + logCtx.V(1).Info("Removing finalizers") + if !dryRun { + if err := sc.removeHookFinalizer(t); err != nil { + state = failed + sc.setResourceResult(t, t.syncStatus, common.OperationError, fmt.Sprintf("failed to remove hook finalizer: %v", err)) + } + } + return state + }) + } + state = ss.Wait() + } + if state != successful { + return state + } + // prune first { if !sc.pruneConfirmed { @@ -1316,15 +1341,19 @@ func (sc *syncContext) runTasks(tasks syncTasks, dryRun bool) runState { for _, task := range hooksPendingDeletion { t := task ss.Go(func(state runState) runState { - sc.log.WithValues("dryRun", dryRun, "task", t).V(1).Info("Deleting") + log := sc.log.WithValues("dryRun", dryRun, "task", t).V(1) + log.Info("Deleting") if !dryRun { err := sc.deleteResource(t) if err != nil { // it is possible to get a race condition here, such that the resource does not exist when - // delete is requested, we treat this as a nop + // delete is requested, we treat this as a nopand remove the liveObj if !apierrors.IsNotFound(err) { state = failed - sc.setResourceResult(t, "", common.OperationError, fmt.Sprintf("failed to delete resource: %v", err)) + sc.setResourceResult(t, t.syncStatus, common.OperationError, fmt.Sprintf("failed to delete resource: %v", err)) + } else { + log.Info("Resource not found, treating as no-op and removing liveObj") + t.liveObj = nil } } else { // if there is anything that needs deleting, we are at best now in pending and diff --git a/pkg/sync/sync_context_test.go b/pkg/sync/sync_context_test.go index 5e7bc84b9..1d8c9fbe0 100644 --- a/pkg/sync/sync_context_test.go +++ b/pkg/sync/sync_context_test.go @@ -45,15 +45,15 @@ func newTestSyncCtx(getResourceFunc *func(ctx context.Context, config *rest.Conf &metav1.APIResourceList{ GroupVersion: "v1", APIResources: []metav1.APIResource{ - {Kind: "Pod", Group: "", Version: "v1", Namespaced: true, Verbs: standardVerbs}, - {Kind: "Service", Group: "", Version: "v1", Namespaced: true, Verbs: standardVerbs}, - {Kind: "Namespace", Group: "", Version: "v1", Namespaced: false, Verbs: standardVerbs}, + {Name: "pods", Kind: "Pod", Group: "", Version: "v1", Namespaced: true, Verbs: standardVerbs}, + {Name: "services", Kind: "Service", Group: "", Version: "v1", Namespaced: true, Verbs: standardVerbs}, + {Name: "namespaces", Kind: "Namespace", Group: "", Version: "v1", Namespaced: false, Verbs: standardVerbs}, }, }, &metav1.APIResourceList{ GroupVersion: "apps/v1", APIResources: []metav1.APIResource{ - {Kind: "Deployment", Group: "apps", Version: "v1", Namespaced: true, Verbs: standardVerbs}, + {Name: "deployments", Kind: "Deployment", Group: "apps", Version: "v1", Namespaced: true, Verbs: standardVerbs}, }, }) sc := syncContext{ @@ -807,6 +807,39 @@ func withReplaceAndServerSideApplyAnnotations(un *unstructured.Unstructured) *un return un } +func TestSync_HookWithReplaceAndBeforeHookCreation_AlreadyDeleted(t *testing.T) { + // This test a race condition when Delete is called on an already deleted object + // LiveObj is set, but then the resource is deleted asynchronously in kubernetes + syncCtx := newTestSyncCtx(nil) + + target := withReplaceAnnotation(testingutils.NewPod()) + target.SetNamespace(testingutils.FakeArgoCDNamespace) + target = testingutils.Annotate(target, synccommon.AnnotationKeyHookDeletePolicy, string(synccommon.HookDeletePolicyBeforeHookCreation)) + target = testingutils.Annotate(target, synccommon.AnnotationKeyHook, string(synccommon.SyncPhasePreSync)) + live := target.DeepCopy() + + syncCtx.resources = groupResources(ReconciliationResult{ + Live: []*unstructured.Unstructured{live}, + Target: []*unstructured.Unstructured{target}, + }) + syncCtx.hooks = []*unstructured.Unstructured{live} + + client := fake.NewSimpleDynamicClient(runtime.NewScheme()) + deleted := false + client.PrependReactor("delete", "pods", func(_ testcore.Action) (bool, runtime.Object, error) { + deleted = true + // simulate the race conditions where liveObj was not null, but is now deleted in k8s + return true, nil, apierrors.NewNotFound(corev1.Resource("pods"), live.GetName()) + }) + syncCtx.dynamicIf = client + + syncCtx.Sync() + + resourceOps, _ := syncCtx.resourceOps.(*kubetest.MockResourceOps) + assert.Equal(t, "create", resourceOps.GetLastResourceCommand(kube.GetResourceKey(target))) + assert.True(t, deleted) +} + func TestSync_ServerSideApply(t *testing.T) { testCases := []struct { name string