-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
change configmap-env helm hook weight to be installed before job-db-migrate #74
base: main
Are you sure you want to change the base?
Conversation
From my understanding, adding a hook means that the ConfigMap will be removed once the helm install completes? https://helm.sh/docs/topics/charts_hooks/#hook-deletion-policies In which case, either we need to pass everything the Job needs to it directly, or we create a separate ConfigMap just for the Job, which will also be cleared up after. |
Oh, great catch! but are we sure? This part of the docs makes it seem like they are left alone:
So potentially we could just add a But the doc you linked says:
Those feel conflicting, but I think it means that the configMap in this instance would only be deleted before a new resource is launched, so at the time of another
We could do that! I'd need to know if it needs everything from the configmap though?
That could also work, but we still need to know if that job needs absolutely all the same config map values? I can modify this PR to do this quick and dirty by just copying UPDATEI tested the full helm install and the hook is not automatically deleted right now, but we can add the |
After I gave this PR a shot in my local cluster to make sure the behavior is as expected, it looks like the db-migrate job still fails because it needs to connect to redis, so I guess there's another question: is there a way to wait for the sub-charts that are dependencies of this one before installing the configmap and db-migreate job? Can we tell helm to finish setting up redis and postgres/mariadb before doing the migration job? 🤔 Actually, after reading this stack overflow post, I was thiking: would it make more sense to have an init container on the main deployment instead of all this helm hook stuff? That way we just run these commands on init: - bundle
- exec
- rake
- db:migrate I don't write ruby very often at all, so does We'd probably also need a container for making sure postgres/mariadb were ready, similar to how we do in nextcloud/helm like: {{- else if .Values.postgresql.enabled }}
- name: postgresql-isready
image: {{ .Values.postgresql.image.repository }}:{{ .Values.postgresql.image.tag }}
env:
- name: POSTGRES_USER
valueFrom:
secretKeyRef:
name: {{ .Values.externalDatabase.existingSecret.secretName | default (printf "%s-%s" .Release.Name "db") }}
key: {{ .Values.externalDatabase.existingSecret.usernameKey | default "db-username" }}
command:
- "sh"
- "-c"
- {{ printf "until pg_isready -h %s-postgresql -U ${POSTGRES_USER} ; do sleep 2 ; done" .Release.Name }}
{{- end }} |
So, if I use both an external postgresql database and an external redis host, everything works fine after the adding the helm hook to the config map. I'm doing that over at jessebot/mastodon-helm-chart if anyone would like to take inspiration for their own forks. I've also made parameters for existingSecrets for all secure values, so you should be in a better state overall. I'll try to submit additional PRs to merge more changes back into this repo for the other changes too. |
@@ -4,6 +4,9 @@ metadata: | |||
name: {{ include "mastodon.fullname" . }}-env | |||
labels: | |||
{{- include "mastodon.labels" . | nindent 4 }} | |||
annotations: | |||
"helm.sh/hook": pre-install | |||
"helm.sh/hook-weight": "-3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"helm.sh/hook-weight": "-3" | |
"helm.sh/hook-weight": "-3" | |
"helm.sh/resource-policy": "keep" |
Adding this future proofs against any changes to hook deletion in helm 3 in the future.
This is to solve #73 where the db-migrate job tries to run before the configmap it needs is in place. This change will just install the configmap before the db-migrate job.