Elgato_dark/argo-cd
1
0
Fork 0
mirror of https://github.com/argoproj/argo-cd synced 2026-04-21 08:57:17 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 14

fix(hook): Fixed hook code issues that caused stuck applications on "Deleting" state (Issues #18355 and #17191) (#26724)

Browse source 
Signed-off-by: Nikolaos Astyrakakis <nastyrakakis@gmail.com>
This commit is contained in:
Nikolaos Astyrakakis 2026-04-09 17:19:38 +02:00 committed by GitHub
parent 9e80e058e7
commit ae10c0c6c3
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 195 additions and 5 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

27
controller/hook.go
View file

@ -11,6 +11,7 @@ import (
"github.com/argoproj/argo-cd/gitops-engine/pkg/sync/hook"
"github.com/argoproj/argo-cd/gitops-engine/pkg/utils/kube"
log "github.com/sirupsen/logrus"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/client-go/rest"
@ -103,6 +104,7 @@ func (ctrl *ApplicationController) executeHooks(hookType HookType, app *appv1.Ap
revisions = append(revisions, src.TargetRevision)
}
// Fetch target objects from Git to know which hooks should exist
targets, _, _, err := ctrl.appStateManager.GetRepoObjs(context.Background(), app, app.Spec.GetSources(), appLabelKey, revisions, false, false, false, proj, true)
if err != nil {
return false, err
@ -125,14 +127,14 @@ func (ctrl *ApplicationController) executeHooks(hookType HookType, app *appv1.Ap
if !isHookOfType(obj, hookType) {
continue
}
if runningHook := runningHooks[kube.GetResourceKey(obj)]; runningHook == nil {
if _, alreadyExists := runningHooks[kube.GetResourceKey(obj)]; !alreadyExists {
expectedHook[kube.GetResourceKey(obj)] = obj
}
}
// Create hooks that don't exist yet
createdCnt := 0
for _, obj := range expectedHook {
for key, obj := range expectedHook {
// Add app instance label so the hook can be tracked and cleaned up
labels := obj.GetLabels()
if labels == nil {
@ -141,8 +143,13 @@ func (ctrl *ApplicationController) executeHooks(hookType HookType, app *appv1.Ap
labels[appLabelKey] = app.InstanceName(ctrl.namespace)
obj.SetLabels(labels)
logCtx.Infof("Creating %s hook resource: %s", hookType, key)
_, err = ctrl.kubectl.CreateResource(context.Background(), config, obj.GroupVersionKind(), obj.GetName(), obj.GetNamespace(), obj, metav1.CreateOptions{})
if err != nil {
if apierrors.IsAlreadyExists(err) {
logCtx.Warnf("Hook resource %s already exists, skipping", key)
continue
}
return false, err
}
createdCnt++
@ -163,7 +170,8 @@ func (ctrl *ApplicationController) executeHooks(hookType HookType, app *appv1.Ap
progressingHooksCount := 0
var failedHooks []string
var failedHookObjects []*unstructured.Unstructured
for _, obj := range runningHooks {
for key, obj := range runningHooks {
hookHealth, err := health.GetResourceHealth(obj, healthOverrides)
if err != nil {
return false, err
@ -180,12 +188,17 @@ func (ctrl *ApplicationController) executeHooks(hookType HookType, app *appv1.Ap
Status: health.HealthStatusHealthy,
}
}
switch hookHealth.Status {
case health.HealthStatusProgressing:
logCtx.Debugf("Hook %s is progressing", key)
progressingHooksCount++
case health.HealthStatusDegraded:
logCtx.Warnf("Hook %s is degraded: %s", key, hookHealth.Message)
failedHooks = append(failedHooks, fmt.Sprintf("%s/%s", obj.GetNamespace(), obj.GetName()))
failedHookObjects = append(failedHookObjects, obj)
case health.HealthStatusHealthy:
logCtx.Debugf("Hook %s is healthy", key)
}
}
@ -194,7 +207,7 @@ func (ctrl *ApplicationController) executeHooks(hookType HookType, app *appv1.Ap
logCtx.Infof("Deleting %d failed %s hook(s) to allow retry", len(failedHookObjects), hookType)
for _, obj := range failedHookObjects {
err = ctrl.kubectl.DeleteResource(context.Background(), config, obj.GroupVersionKind(), obj.GetName(), obj.GetNamespace(), metav1.DeleteOptions{})
if err != nil {
if err != nil && !apierrors.IsNotFound(err) {
logCtx.WithError(err).Warnf("Failed to delete failed hook %s/%s", obj.GetNamespace(), obj.GetName())
}
}
@ -241,6 +254,10 @@ func (ctrl *ApplicationController) cleanupHooks(hookType HookType, liveObjs map[
hooks = append(hooks, obj)
}
if len(hooks) == 0 {
return true, nil
}
// Process hooks for deletion
for _, obj := range hooks {
deletePolicies := hook.DeletePolicies(obj)
@ -267,7 +284,7 @@ func (ctrl *ApplicationController) cleanupHooks(hookType HookType, liveObjs map[
}
logCtx.Infof("Deleting %s hook %s/%s", hookType, obj.GetNamespace(), obj.GetName())
err = ctrl.kubectl.DeleteResource(context.Background(), config, obj.GroupVersionKind(), obj.GetName(), obj.GetNamespace(), metav1.DeleteOptions{})
if err != nil {
if err != nil && !apierrors.IsNotFound(err) {
return false, err
}
}

173
controller/hook_test.go
View file

@ -3,8 +3,10 @@ package controller
import (
"testing"
"github.com/argoproj/argo-cd/gitops-engine/pkg/utils/kube"
"github.com/stretchr/testify/assert"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
)
func TestIsHookOfType(t *testing.T) {
@ -312,3 +314,174 @@ func TestMultiHookOfType(t *testing.T) {
})
}
}
func TestExecuteHooksAlreadyExistsLogic(t *testing.T) {
newObj := func(name string, annot map[string]string) *unstructured.Unstructured {
obj := &unstructured.Unstructured{}
obj.SetGroupVersionKind(schema.GroupVersionKind{Group: "batch", Version: "v1", Kind: "Job"})
obj.SetName(name)
obj.SetNamespace("default")
obj.SetAnnotations(annot)
return obj
}
tests := []struct {
name string
hookType []HookType
targetAnnot map[string]string
liveAnnot map[string]string // nil -> object doesn't exist in cluster
expectCreated bool
}{
// PRE DELETE TESTS
{
name: "PreDelete (argocd): Not in cluster - should be created",
hookType: []HookType{PreDeleteHookType},
targetAnnot: map[string]string{"argocd.argoproj.io/hook": "PreDelete"},
liveAnnot: nil,
expectCreated: true,
},
{
name: "PreDelete (helm): Not in cluster - should be created",
hookType: []HookType{PreDeleteHookType},
targetAnnot: map[string]string{"helm.sh/hook": "pre-delete"},
liveAnnot: nil,
expectCreated: true,
},
{
name: "PreDelete (argocd): Already exists - should be skipped",
hookType: []HookType{PreDeleteHookType},
targetAnnot: map[string]string{"argocd.argoproj.io/hook": "PreDelete"},
liveAnnot: map[string]string{"argocd.argoproj.io/hook": "PreDelete"},
expectCreated: false,
},
{
name: "PreDelete (argocd): Already exists - should be skipped",
hookType: []HookType{PreDeleteHookType},
targetAnnot: map[string]string{"helm.sh/hook": "pre-delete"},
liveAnnot: map[string]string{"helm.sh/hook": "pre-delete"},
expectCreated: false,
},
{
name: "PreDelete (helm+argocd): One of two already exists - should be skipped",
hookType: []HookType{PreDeleteHookType},
targetAnnot: map[string]string{"helm.sh/hook": "pre-delete", "argocd.argoproj.io/hook": "PreDelete"},
liveAnnot: map[string]string{"helm.sh/hook": "pre-delete"},
expectCreated: false,
},
{
name: "PreDelete (helm+argocd): One of two already exists - should be skipped",
hookType: []HookType{PreDeleteHookType},
targetAnnot: map[string]string{"helm.sh/hook": "pre-delete", "argocd.argoproj.io/hook": "PreDelete"},
liveAnnot: map[string]string{"argocd.argoproj.io/hook": "PreDelete"},
expectCreated: false,
},
// POST DELETE TESTS
{
name: "PostDelete (argocd): Not in cluster - should be created",
hookType: []HookType{PostDeleteHookType},
targetAnnot: map[string]string{"argocd.argoproj.io/hook": "PostDelete"},
liveAnnot: nil,
expectCreated: true,
},
{
name: "PostDelete (helm): Not in cluster - should be created",
hookType: []HookType{PostDeleteHookType},
targetAnnot: map[string]string{"helm.sh/hook": "post-delete"},
liveAnnot: nil,
expectCreated: true,
},
{
name: "PostDelete (argocd): Already exists - should be skipped",
hookType: []HookType{PostDeleteHookType},
targetAnnot: map[string]string{"argocd.argoproj.io/hook": "PostDelete"},
liveAnnot: map[string]string{"argocd.argoproj.io/hook": "PostDelete"},
expectCreated: false,
},
{
name: "PostDelete (helm): Already exists - should be skipped",
hookType: []HookType{PostDeleteHookType},
targetAnnot: map[string]string{"helm.sh/hook": "post-delete"},
liveAnnot: map[string]string{"helm.sh/hook": "post-delete"},
expectCreated: false,
},
{
name: "PostDelete (helm+argocd): Already exists - should be skipped",
hookType: []HookType{PostDeleteHookType},
targetAnnot: map[string]string{"helm.sh/hook": "post-delete", "argocd.argoproj.io/hook": "PostDelete"},
liveAnnot: map[string]string{"helm.sh/hook": "post-delete", "argocd.argoproj.io/hook": "PostDelete"},
expectCreated: false,
},
{
name: "PostDelete (helm+argocd): One of two already exists - should be skipped",
hookType: []HookType{PostDeleteHookType},
targetAnnot: map[string]string{"helm.sh/hook": "post-delete", "argocd.argoproj.io/hook": "PostDelete"},
liveAnnot: map[string]string{"helm.sh/hook": "post-delete"},
expectCreated: false,
},
{
name: "PostDelete (helm+argocd): One of two already exists - should be skipped",
hookType: []HookType{PostDeleteHookType},
targetAnnot: map[string]string{"helm.sh/hook": "post-delete", "argocd.argoproj.io/hook": "PostDelete"},
liveAnnot: map[string]string{"argocd.argoproj.io/hook": "PostDelete"},
expectCreated: false,
},
// MULTI HOOK TESTS - SKIP LOGIC
{
name: "Multi-hook (argocd): Target is (Pre,Post), Cluster has (Pre,Post) - should be skipped",
hookType: []HookType{PreDeleteHookType, PostDeleteHookType},
targetAnnot: map[string]string{"argocd.argoproj.io/hook": "PreDelete,PostDelete"},
liveAnnot: map[string]string{"argocd.argoproj.io/hook": "PreDelete,PostDelete"},
expectCreated: false,
},
{
name: "Multi-hook (helm): Target is (Pre,Post), Cluster has (Pre,Post) - should be skipped",
hookType: []HookType{PreDeleteHookType, PostDeleteHookType},
targetAnnot: map[string]string{"helm.sh/hook": "post-delete,pre-delete"},
liveAnnot: map[string]string{"helm.sh/hook": "post-delete,pre-delete"},
expectCreated: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
targetObj := newObj("my-hook", tt.targetAnnot)
targetKey := kube.GetResourceKey(targetObj)
liveObjs := make(map[kube.ResourceKey]*unstructured.Unstructured)
if tt.liveAnnot != nil {
liveObjs[targetKey] = newObj("my-hook", tt.liveAnnot)
}
runningHooks := map[kube.ResourceKey]*unstructured.Unstructured{}
for key, obj := range liveObjs {
for _, hookType := range tt.hookType {
if isHookOfType(obj, hookType) {
runningHooks[key] = obj
}
}
}
expectedHooksToCreate := map[kube.ResourceKey]*unstructured.Unstructured{}
targets := []*unstructured.Unstructured{targetObj}
for _, obj := range targets {
for _, hookType := range tt.hookType {
if !isHookOfType(obj, hookType) {
continue
}
}
objKey := kube.GetResourceKey(obj)
if _, alreadyExists := runningHooks[objKey]; !alreadyExists {
expectedHooksToCreate[objKey] = obj
}
}
if tt.expectCreated {
assert.NotEmpty(t, expectedHooksToCreate, "Expected hook to be marked for creation")
} else {
assert.Empty(t, expectedHooksToCreate, "Expected hook to be skipped (already exists)")
}
})
}
}