Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jessebot
Copy link
Contributor

@jessebot jessebot commented Jul 6, 2023

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.

@grrywlsn
Copy link

grrywlsn commented Jul 6, 2023

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.

@jessebot
Copy link
Contributor Author

jessebot commented Jul 6, 2023

From my understanding, adding a hook means that the ConfigMap will be removed once the helm install completes?

Oh, great catch! but are we sure? This part of the docs makes it seem like they are left alone:

Hook resources are not managed with corresponding releases

The resources that a hook creates are currently not tracked or managed as part of the release. Once Helm verifies that the hook has reached its ready state, it will leave the hook resource alone. Garbage collection of hook resources when the corresponding release is deleted may be added to Helm 3 in the future, so any hook resources that must never be deleted should be annotated with helm.sh/resource-policy: keep.

So potentially we could just add a helm.sh/resource-policy: keep annotation? 🤔

But the doc you linked says:

before-hook-creation - Delete the previous resource before a new hook is launched (default)
...
If no hook deletion policy annotation is specified, the before-hook-creation behavior applies by default.

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 helm install/uninstall, correct? I can totally be wrong on this, but regardless to your second point:

In which case, either we need to pass everything the Job needs to it directly

We could do that! I'd need to know if it needs everything from the configmap though?

or we create a separate ConfigMap just for the Job, which will also be cleared up after.

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 configmap-env.yaml to configmap-dbmigrate-env.yaml and then reverting back the changes on configmap-env.yaml?

UPDATE

I tested the full helm install and the hook is not automatically deleted right now, but we can add the helm.sh/resource-policy: keep annotation anyway to be safe.

@jessebot
Copy link
Contributor Author

jessebot commented Jul 6, 2023

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 db:migrate do anything that we wouldn't want happening on each new pod creation? If it shouldn't be run everytime, we need some sort of location we can update to say it's done, and if it can run everything time, then I feel like an initContainer solves the whole shebang.

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 }}

@jessebot
Copy link
Contributor Author

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"
Copy link
Contributor Author

@jessebot jessebot Jul 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants