openfga / helm-charts

Official Helm charts for the OpenFGA project.
https://openfga.dev
Apache License 2.0
19 stars 35 forks source link

feat: Add flag to wait for migrations to complete (or not) #99

Closed JAORMX closed 7 months ago

JAORMX commented 8 months ago

This makes the init container that waits for migrations optional via a waitForMigrations flag. If disabled, it does not render the initContainer alongside the role and role binding it uses.

In tools such as ArgoCD, the waves (which can be set throught he helm annotations we already have) ensure that a resource is applied before another one. So this waiting init container may not be needed. Let's make it configurable for deployments that don't require this.

Note that the option defaults to true as to keep the current logic and functionality as it is.

evankanderson commented 8 months ago

In particular, setting waitForMigrations: false also removes a reliability concern that the OpenFGA deployment might fail to start because the Job in question ran successfully but has since been deleted.

I don't have full background on the init container's story, but I'd expect that it's redundant with the default values for migrate.annotations:

    helm.sh/hook: "post-install, post-upgrade, post-rollback, post-delete"
    helm.sh/hook-weight: "-5"
    helm.sh/hook-delete-policy: "before-hook-creation"

Though I'm not sure why we're running the migration post-delete, now that I think about it. That seems like a separate issue / PR, though.

rhamzeh commented 8 months ago

Thanks for the PR @JAORMX - the team should be reviewing this soon, we're just busy getting a few other things out of the door and we'll take a look! 👀

jon-whit commented 8 months ago

@JAORMX @evankanderson we have a datastore.applyMigrations value that can be set to false already. If that flag is set to false we don't create the wait-for-migration init container. See https://github.com/openfga/helm-charts/blob/main/charts/openfga/templates/deployment.yaml#L37-L47

Could you help me understand where we'd want waitForMigrations to be a value different than datastore.applyMigrations. If you're applying the migrations in the helm install ... then presumably you want to wait for them to complete before the server starts up (e.g. wait on the job to complete). Otherwise I assume that if datastore.applyMigrations: false you are managing the schema migrations out-of-band and therefore don't need to wait on anything because you'd be managing that migration prior to a helm rollout.

JAORMX commented 8 months ago

We are applying the migrations via the helm chart. We are not doing it out of band. And waiting for them is done as part of the ArgoCD hook system.

In our case we want to apply the migrations via the chart but don't want the init container to wait since it's redundant.

Does that make sense @jon-whit ?

this was initially confusing because I accidentally also added the new flat to the migration job, which was not the intention.

jon-whit commented 7 months ago

@JAORMX bump the chartVersion and then we can merge 👍

JAORMX commented 7 months ago

@jon-whit done! thanks