Hook Deletion Policies HookSucceeded should be run after whole Hook succeed and not only Resource succeed

[mcanevet]

I have an issue deploying gitlab's chart using ArgoCD (v1.7.6). One of the subchart, shared-secrets, uses helm hooks to temporarily create a Job, ServiceAccount, Role and RoleBinding (https://gitlab.com/gitlab-org/charts/gitlab/-/tree/v4.1.12/charts/shared-secrets/templates).

The Pod created by the Job fails to be created because of Error creating: pods "gitlab-shared-secrets.1-l3b-" is forbidden: error looking up service account gitlab/gitlab-shared-secrets: serviceaccount "gitlab-shared-secrets" not found while I have this in application controller's logs https://gist.github.com/mcanevet/2207866c78f68c76124af17aa4bd4c81

time="2020-09-21T06:53:42Z" level=info msg=syncing application=gitlab skipHooks=false started=true syncId=00085-OahHe
time="2020-09-21T06:53:42Z" level=info msg=tasks application=gitlab syncId=00085-OahHe tasks="[PreSync/-11 hook /ConfigMap:gitlab/gitlab-gitlab-upgrade-check obj->obj (,Succeeded,gitlab-gitlab-upgrade-check create
d), PreSync/-10 hook batch/Job:gitlab/gitlab-gitlab-upgrade-check nil->obj (,Succeeded,job.batch/gitlab-gitlab-upgrade-check created), PreSync/-5 hook /ServiceAccount:gitlab/gitlab-shared-secrets nil->obj (,Succee
ded,gitlab-shared-secrets created), PreSync/-5 hook rbac.authorization.k8s.io/Role:gitlab/gitlab-shared-secrets nil->obj (,Succeeded,gitlab-shared-secrets created), PreSync/-5 hook rbac.authorization.k8s.io/RoleBi
nding:gitlab/gitlab-shared-secrets nil->obj (,Succeeded,gitlab-shared-secrets created), PreSync/-3 hook /ConfigMap:gitlab/gitlab-shared-secrets nil->obj (,Succeeded,gitlab-shared-secrets created), PreSync/0 hook b
atch/Job:gitlab/gitlab-shared-secrets.1-l3b obj->obj (,Running,job.batch/gitlab-shared-secrets.1-l3b created), Sync/0 resource policy/PodDisruptionBudget:gitlab/gitlab-gitaly obj->obj (,,), Sync/0 resource policy/
PodDisruptionBudget:gitlab/gitlab-gitlab-shell obj->obj (,,), Sync/0 resource policy/PodDisruptionBudget:gitlab/gitlab-registry-v1 obj->obj (,,), Sync/0 resource policy/PodDisruptionBudget:gitlab/gitlab-sidekiq-al
l-in-1-v1 obj->obj (,,), Sync/0 resource policy/PodDisruptionBudget:gitlab/gitlab-webservice obj->obj (,,), Sync/0 resource /ConfigMap:gitlab/gitlab-gitaly obj->obj (,,), Sync/0 resource /ConfigMap:gitlab/gitlab-g
itlab-chart-info obj->obj (,,), Sync/0 resource /ConfigMap:gitlab/gitlab-gitlab-exporter obj->obj (,,), Sync/0 resource /ConfigMap:gitlab/gitlab-gitlab-runner obj->obj (,,), Sync/0 resource /ConfigMap:gitlab/gitla
b-gitlab-shell obj->obj (,,), Sync/0 resource /ConfigMap:gitlab/gitlab-gitlab-shell-sshd obj->obj (,,), Sync/0 resource /ConfigMap:gitlab/gitlab-migrations obj->obj (,,), Sync/0 resource /ConfigMap:gitlab/gitlab-n
ginx-ingress-tcp obj->obj (,,), Sync/0 resource /ConfigMap:gitlab/gitlab-redis obj->obj (,,), Sync/0 resource /ConfigMap:gitlab/gitlab-redis-health obj->obj (,,), Sync/0 resource /ConfigMap:gitlab/gitlab-registry
obj->obj (,,), Sync/0 resource /ConfigMap:gitlab/gitlab-sidekiq obj->obj (,,), Sync/0 resource /ConfigMap:gitlab/gitlab-sidekiq-all-in-1 obj->obj (,,), Sync/0 resource /ConfigMap:gitlab/gitlab-task-runner obj->obj
 (,,), Sync/0 resource /ConfigMap:gitlab/gitlab-webservice obj->obj (,,), Sync/0 resource /ConfigMap:gitlab/gitlab-webservice-tests obj->obj (,,), Sync/0 resource /ConfigMap:gitlab/gitlab-workhorse-config obj->obj
 (,,), Sync/0 resource /ServiceAccount:gitlab/gitlab-gitlab-runner obj->obj (,,), Sync/0 resource rbac.authorization.k8s.io/Role:gitlab/gitlab-gitlab-runner obj->obj (,,), Sync/0 resource rbac.authorization.k8s.io
/RoleBinding:gitlab/gitlab-gitlab-runner obj->obj (,,), Sync/0 resource /Service:gitlab/gitlab-gitaly obj->obj (,,), Sync/0 resource /Service:gitlab/gitlab-gitlab-exporter obj->obj (,,), Sync/0 resource /Service:g
itlab/gitlab-gitlab-shell obj->obj (,,), Sync/0 resource /Service:gitlab/gitlab-redis-headless obj->obj (,,), Sync/0 resource /Service:gitlab/gitlab-redis-master obj->obj (,,), Sync/0 resource /Service:gitlab/gitl
ab-redis-metrics obj->obj (,,), Sync/0 resource /Service:gitlab/gitlab-registry obj->obj (,,), Sync/0 resource /Service:gitlab/gitlab-webservice obj->obj (,,), Sync/0 resource apps/Deployment:gitlab/gitlab-gitlab-
exporter obj->obj (,,), Sync/0 resource apps/Deployment:gitlab/gitlab-gitlab-runner obj->obj (,,), Sync/0 resource apps/Deployment:gitlab/gitlab-gitlab-shell obj->obj (,,), Sync/0 resource apps/Deployment:gitlab/g
itlab-registry obj->obj (,,), Sync/0 resource apps/Deployment:gitlab/gitlab-sidekiq-all-in-1-v1 obj->obj (,,), Sync/0 resource apps/Deployment:gitlab/gitlab-task-runner obj->obj (,,), Sync/0 resource apps/Deployme
nt:gitlab/gitlab-webservice obj->obj (,,), Sync/0 resource apps/StatefulSet:gitlab/gitlab-gitaly obj->obj (,,), Sync/0 resource apps/StatefulSet:gitlab/gitlab-redis-master obj->obj (,,), Sync/0 resource batch/Job:
gitlab/gitlab-migrations.1 nil->obj (,,), Sync/0 resource batch/CronJob:gitlab/gitlab-task-runner-backup obj->obj (,,), Sync/0 resource extensions/Ingress:gitlab/gitlab-registry obj->obj (,,), Sync/0 resource exte
nsions/Ingress:gitlab/gitlab-webservice obj->obj (,,), Sync/0 resource autoscaling/HorizontalPodAutoscaler:gitlab/gitlab-gitlab-shell obj->obj (,,), Sync/0 resource monitoring.coreos.com/ServiceMonitor:gitlab/gitl
ab-redis obj->obj (,,), Sync/0 resource autoscaling/HorizontalPodAutoscaler:gitlab/gitlab-registry obj->obj (,,), Sync/0 resource autoscaling/HorizontalPodAutoscaler:gitlab/gitlab-sidekiq-all-in-1-v1 obj->obj (,,)
, Sync/0 resource autoscaling/HorizontalPodAutoscaler:gitlab/gitlab-webservice obj->obj (,,)]"
time

I guess that the helm.sh/hook-delete-policy on hook-succeeded is executed per resource or sync wave and not per sync?
That would explain why the ServiceAccount, with a hook weigth of -5 is created than instantaneously destroyed before the Job with no hook weigh (hence sync wave of 0?) is launched.

[mcanevet]

It should have been fixed by that : argoproj/gitops-engine#92 but somehow it is not.

[mayzhang2000]

helm.sh/hook-delete-policy on hook-succeeded is executed per sync wave.

[jessesuen]

I guess that the helm.sh/hook-delete-policy on hook-succeeded is executed per resource or sync wave and not per sync?

I think it will be better to move it at end of sync to accommodate helm chart compatibility.

[aweis89]

Noticing the same issue with nginx-ingress chart: https://github.com/kubernetes/ingress-nginx/tree/master/charts/ingress-nginx

[matlegit]

Hi there, we are also experiencing this issue, any plans to fix it soon?

[mayzhang2000]

PR for this fix is created: argoproj/gitops-engine#144