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

SP-20484 database init improvements #58

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

KirylKovaliov
Copy link
Collaborator

@KirylKovaliov KirylKovaliov commented Dec 26, 2024

This PR introduces a new Helm chart feature that executes SQL database migrations using a Kubernetes job. The job leverages Helm hooks (post-install and post-upgrade) to ensure the migrations run after chart installation or upgrade.

Questions:

Versioning: Since this feature works only with version 7.6, what is the best approach to handle versioning in the Helm chart? Should we introduce a conditional check or restrict upgrades if the product version is below 7.6?

Notes:

The Python code handles whether migrations are needed, ensuring that the installation works correctly for both setups: whether a database is required or not.

@KirylKovaliov KirylKovaliov requested a review from a team as a code owner December 26, 2024 13:58
@sdauhuchytsrf
Copy link
Contributor

sdauhuchytsrf commented Dec 26, 2024

Probably we could use some Helm logic like this if it's not possible to implement the same on code/application level:

{{- if or (hasPrefix "7.6" (default .Chart.AppVersion .Values.image.tag)) (eq "nightly" (default .Chart.AppVersion .Values.image.tag)) -}}
...
{{- end -}}

The logic is the following. If either of two is true (chart version/image tag starts with "7.6" or chart version/image tag is "nightly"), then apply some block of code. This is my assumption.
@AndreiPaulau WDYT?

P.S. Please note the failing tests for this PR.

@KirylKovaliov
Copy link
Collaborator Author

I ended up adding an additional option responsible for enabling and disabling the database migration job – dbMigrationsJob.enabled. This option is set to false by default. I believe we can keep it as is for the next couple of releases, using it only internally

@AndreiPaulau
Copy link
Contributor

Discussed with @KirylKovaliov, FYI @sdauhuchytsrf
MFA:

  • chart major version bump
  • move to pre-upgrade / pre-install hooks
  • checks over appVersion ( if >= 7.6)
  • run db-migration-job if sessionapi or chipVerification enabled
  • introduce/migrate to /api/healthz /api/readyz endpoints
  • research if we can prohibit latest version or at least provide Warning
  • Test. Verify over utilized DB.

@KirylKovaliov
Copy link
Collaborator Author

Discussed with @KirylKovaliov, FYI @sdauhuchytsrf MFA:

  • chart major version bump

  • move to pre-upgrade / pre-install hooks

  • checks over appVersion ( if >= 7.6)

  • run db-migration-job if sessionapi or chipVerification enabled

  • introduce/migrate to /api/healthz /api/readyz endpoints

  • research if we can prohibit latest version or at least provide Warning

  • Test. Verify over utilized DB.

More detailed testing revealed some more issues with pre-install hook. Leaving only pre-upgrade

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

Successfully merging this pull request may close these issues.

3 participants