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

v3: fix the migration mess #1211

Closed
wants to merge 17 commits into from
Closed

v3: fix the migration mess #1211

wants to merge 17 commits into from

Conversation

ewjoachim
Copy link
Member

@ewjoachim ewjoachim commented Oct 5, 2024

Closes #1189

Warning

the word "mess" is is solely directed at the mess I created trhough unclear rules and overly complex processes, the work done so far by everyone has been amazing

Introduce a new migration process: pre-migrations and post-migrations.

  • They will be tied to the corresponding release
  • Reworked v3 migrations with this new template in mind
  • They allow us to provide a much saner path for blue-green deployment
  • The process is currently not tested (but nor was the previous system)
  • We will need to be careful when we do a release to rename the migrations in ongoing PRs (as today)
  • We (hopefully) won't have to do multiple releases just for the sake of blue-green compatibility.
  • I'm adding versioning in the name of all internal objects (functions, triggers, etc) (as mentionned here)

So the idea is that:

  • Procrastinate v2 with the pre-migrations & without the post-migrations should work
  • Procrastinate v3 with the pre-migrations & without the post-migrations should work
  • Procrastinate v3 with all migrations should be what we currently have in v3

(I'm 80% sure that I have missed a few corner cases)

Is it going in the right direction according to you @medihack @onlyann ?

Successful PR Checklist:

  • Tests
    • (not applicable?)
  • Documentation
    • (not applicable?)

PR label(s):

@ewjoachim ewjoachim requested a review from a team as a code owner October 5, 2024 18:42
@github-actions github-actions bot added the PR type: breaking 💥 Contains breaking changes label Oct 5, 2024
@onlyann
Copy link
Contributor

onlyann commented Oct 6, 2024

Let's see if I understand.

pre-script version N can be executed while the code version N-1 is running.
post-script version N is not compatible with code version N-1 and must be executed once code version N is running.

Is the idea that procrastinate semantic versioning is at the programmatic API level and that any new version can possibly introduce database breaking changes as long as there is the ability to split them in pre-scripts and post-scripts to maintain blue-green deployments of the codebase?

What are your thoughts about testing out that pre-migration script is backward compatible?

I like it.

@ewjoachim ewjoachim changed the title WIP v3: fix the migration mess Oct 6, 2024
@ewjoachim
Copy link
Member Author

Is the idea that procrastinate semantic versioning is at the programmatic API level and that any new version can possibly introduce database breaking changes as long as there is the ability to split them in pre-scripts and post-scripts to maintain blue-green deployments of the codebase?

Yep. I think this change removes some hurdles that make keeping blue-green possible (and sane) while the current state of affairs made it more confusing than it needed to be and harder to achieve. We can iterate on the tooling to make this actually easy in the future.

What are your thoughts about testing out that pre-migration script is backward compatible?

My ideas:
1/ we could run the tests on n-1 & pre. (The n-1 tests supposedly should work, though it will be very annoying to fix if they don't for some reason)
2/ for n & pre, i'm tempted to run the normal tests with an escape hatch allowing to easily skip the tests that can't run yet because they don't have the DB ready
3/ an alternative could be to redo our end to end tests to be a bit more versatile: test a large part of the external facing API and not necessarily handle the setup of the database.
This way, we may just run the e2e tests on n-1&pre and n&pre I think this would cover what we actually want to check and would be unlikely to cause issues. I'm not entirely sure if the end to end tests that would run on n-1&pre would be the end to end tests as defined on the n codebase or on the n-1 codebase. Both have merits.

DROP FUNCTION IF EXISTS procrastinate_sync_abort_requested_with_status_v1;

-- Create a new enum type without 'aborting'
CREATE TYPE procrastinate_job_status_v1 AS ENUM (
Copy link
Member

Choose a reason for hiding this comment

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

Must this not be created in pre? Some functions already use it there.

Copy link
Member

Choose a reason for hiding this comment

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

I still think this is true. I will move it over to procrastinate/sql/migrations/03.00.00_01_pre_cancel_notification.sql

@medihack
Copy link
Member

medihack commented Oct 6, 2024

I think I get the idea now and also like it 🙂. The separation between pre and post makes things clearer, as does the versioning of those functions. Should we also give the remaining functions and types a version to make it consistent (procrastinate_job_event_type, procrastinate_defer_job, procrastinate_defer_periodic_job, procrastinate_unlink_periodic_defers, procrastinate_trigger_delete_jobs?

@ewjoachim
Copy link
Member Author

I'm delighted by your answers. You're most probably right that we'll need to continue adjusting a few things.

Given that we need v3+pre to work, we need v3's queries to work with pre, which means the new enum needs to be supported in pre.

Post can only be used to clean the mess.

@medihack
Copy link
Member

@ewjoachim Do you need a helping hand here? I have some time left in the upcoming days and can take care of this issue (at least I could try).

@ewjoachim
Copy link
Member Author

I probably won't have time to work on it Saturday, there might be a chance Sunday and then probably not for a few days, so I'd say: feel free to try something as long as we communicate where we're at.
Alternatively if you want, we can try to do it in pair programming on Sunday or on an evening next week :)

@medihack
Copy link
Member

I probably won't have time to work on it Saturday, there might be a chance Sunday and then probably not for a few days, so I'd say: feel free to try something as long as we communicate where we're at. Alternatively if you want, we can try to do it in pair programming on Sunday or on an evening next week :)

Alright, let's see how far I get. I am probably not yet fully aware of the extent of this PR. And yes, pair programming sounds fun (I have never done anything like this before), but it sounds more like you will be tutoring me then ;-)


-- Recreate and update the functions
CREATE OR REPLACE FUNCTION procrastinate_fetch_job(
-- Recreate and update the functions to use the new column
Copy link
Member

@medihack medihack Oct 19, 2024

Choose a reason for hiding this comment

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

I don't think this is necessary when just changing the column type (it is compatible with the new type). We have also already created those functions in procrastinate/sql/migrations/03.00.00_01_pre_cancel_notification.sql.

@medihack
Copy link
Member

Based on this one, I will work on a new PR (#1227). So we can throw mine away if I go in the wrong direction.

@ewjoachim
Copy link
Member Author

(closed in favor of #1227 )

@ewjoachim ewjoachim closed this Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR type: breaking 💥 Contains breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants