invidious icon indicating copy to clipboard operation
invidious copied to clipboard

[Feature request] Automatic migration system

Open broquemonsieur opened this issue 2 years ago • 8 comments

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 avatar Oct 21 '23 05:10 broquemonsieur

Related: https://github.com/iv-org/invidious/issues/2620

unixfox avatar Oct 21 '23 08:10 unixfox

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

SamantazFox avatar Oct 21 '23 14:10 SamantazFox

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.

unixfox avatar Dec 06 '23 13:12 unixfox

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

broquemonsieur avatar Dec 21 '23 10:12 broquemonsieur

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

unixfox avatar Jun 07 '24 17:06 unixfox

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

broquemonsieur avatar Jun 09 '24 21:06 broquemonsieur

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.

matthewmcgarvey avatar Jun 11 '24 23:06 matthewmcgarvey

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.

syeopite avatar Jun 13 '24 13:06 syeopite