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

[Feature request] Automatic migration system #4190

Open
broquemonsieur opened this issue Oct 21, 2023 · 8 comments
Open

[Feature request] Automatic migration system #4190

broquemonsieur opened this issue Oct 21, 2023 · 8 comments
Labels
feature-request Request of a new feature

Comments

@broquemonsieur
Copy link

Is your feature request related to a problem? Please describe.

Since 2021, a host of PRs have been blocked because there is no way to migrate the currently-running Invidious instances' database to the database proposed by the PRs.

Describe the solution you'd like

A two-part solution

  1. Establish a protocol:
  • For each PR, determine the following

    • For each new table or column, decide what the default and initial values are
  1. A script to carry out the migration. Each script will be coded on a per PR basis.

Describe alternatives you've considered

Ask all GitHub contributors to include a script with their PR should it contain database changes

Additional context

We can unblock some of the best that the Invidious community has to offer! #4032 #2653 #2469 #2254 not to mention the countless future PRs that can continue keeping Invidious the game-changer that it is today!

@broquemonsieur broquemonsieur added the feature-request Request of a new feature label Oct 21, 2023
@unixfox
Copy link
Member

unixfox commented Oct 21, 2023

Related: #2620

@SamantazFox
Copy link
Member

For the record, there is already the required logic for migrations in invidious' code. Now the problem is mostly "how to make everyone use it, without hurdles?"

In essence, you have a bunch of instance who deployed their DB using the old .sql scripts located in config/sql; I want them to transition to that new migration system/process before merging any PR that requires migrations.

For people not using docker, this is trivial: save the DB, run the migration, check integrity, and you're good to go.

However, for docker, it's a bit more involved. We have to provide clear instructions on how to backup the DB and restore it in case of a failure, we have to decide whether the migration is automatically ran (so automatic backups?) and probably other things.

I won't lie, containers are still quite new to me (I started using docker only because invidious relies on it), and I haven't had the time to look deeper into that subject... Help is appreciated :D

@unixfox
Copy link
Member

unixfox commented Dec 6, 2023

Ideas from matrix:
Basically one can run multiple invidious processes for the same postgresql database. Running automatic migration may break their database.
So I had the idea to detect if multiple postgresql clients not related to the main invidious process would be detected. If detected we do not do the automatic migration.

And for the backup we could copy the schema "public" to another schema. Only when the database is less than 10GB in order to avoid filling up the disk, if more than 10GB we abort the automatic migration.

@broquemonsieur
Copy link
Author

broquemonsieur commented Dec 21, 2023

I have successfully enabled backup-creation whenever new migrations are detected: https://github.com/iv-org/invidious/compare/master...broquemonsieur:invidious:invidious_migrations?expand=1

Before:

Screenshot 2023-12-21 at 2 05 55 AM

At this point I add a migration playlists2

After running ./invidious --migrate, this gets detected and terminal outputs "New migration(s) found: creating database backup":
Screenshot 2023-12-21 at 2 12 02 AM

Here's everything together: notice the playlists2 row in the public schema (the migration was carried as usual):
Screenshot 2023-12-21 at 2 12 25 AM

And this backup schema has actual data? Yes, check this out - I created this playlist before running the migration
Screenshot 2023-12-21 at 2 16 10 AM

@unixfox
Copy link
Member

unixfox commented Jun 7, 2024

I'm getting kinda sick that this hasn't moved for years, let's face it this will never see the light. Many many good PR hasn't been merged due to that. I'm for the idea of dropping the idea of automatic migrations.

If we set to only do 2 or 3 database migrations per year, I think that's ok. People are now used to breakage in Invidious due to YouTube anyway. It is not yet another planned breakage that will do much harm.

Immich a very popular project even have breaking changes every month: https://github.com/immich-app/immich/releases!!

We would use the release notes for announcing the breaking changes and with a clear migration path (BACKUP + migration commands). Also, we could have something to keep track of the state of the database. Like, we have a number that we increment everytime someone execute successful migrations using ./invidious migrate. If we detect that invidious is not at the correct versioning, we make Invidious exiting, this way this is safe.

What do you think @SamantazFox @syeopite @TheFrenchGhosty

@broquemonsieur
Copy link
Author

I'm willing to concede. Does this mean my compilations PR is unblocked?

@matthewmcgarvey
Copy link
Contributor

Just taking a peek around after being gone for so long. I'm sorry that this is still proving to be an issue. If I could give my two cents... I think it might be sensible to update the migrations code to have a method for signifying whether or not it is breaking/required to be run for the app to work as expected. The pending migrations check could be added back in on app startup but change it to only give a warning for pending migration but fail if the pending migration is marked in the aforementioned way. Thinking about it... it's really hard to say which migrations would actually be considered non-breaking so I'm not sure if that's best. It could be a general rule that any PRs with migration should seek to be non-breaking unless absolutely required.

@syeopite
Copy link
Member

It'd also be a good idea to add a new rule to the instance list for regular database backups, maybe even one each release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request of a new feature
Projects
None yet
Development

No branches or pull requests

5 participants