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

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

Merged
merged 2 commits into from
Feb 13, 2024

Conversation

JAORMX
Copy link
Contributor

@JAORMX JAORMX commented Jan 19, 2024

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.

@JAORMX JAORMX requested a review from a team as a code owner January 19, 2024 14:59
@evankanderson
Copy link

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.

@JAORMX JAORMX marked this pull request as draft January 29, 2024 11:39
@JAORMX JAORMX marked this pull request as ready for review January 29, 2024 11:39
@JAORMX JAORMX changed the title Add flag to wait for migrations to complete (or not) feat: Add flag to wait for migrations to complete (or not) Jan 29, 2024
@rhamzeh
Copy link
Member

rhamzeh commented Jan 29, 2024

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
Copy link
Collaborator

jon-whit commented Jan 30, 2024

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

JAORMX commented Jan 30, 2024

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
Copy link
Collaborator

@JAORMX bump the chartVersion and then we can merge 👍

@JAORMX
Copy link
Contributor Author

JAORMX commented Feb 12, 2024

@jon-whit done! thanks

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.
@jon-whit jon-whit merged commit f3109a3 into openfga:main Feb 13, 2024
2 checks passed
@JAORMX JAORMX deleted the wait-for-migrations branch February 13, 2024 16:54
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.

4 participants