Add custom migration implementation #2878

matthewmcgarvey · 2022-02-09T06:59:20Z

Instead of adding a migration library that is unmaintained, restrictive, or we'd have to tweak anyways, why not make our own? As you can see it really doesn't take that much code to get a minimal version working.

This implementation has things to point out:

Migrations are versioned, that version is provided inside the class (filename is ignored but useful for file ordering, could be changed in the future)
Migrations are run in the order of the versions, but ALL unapplied migrations are run. So if version 8 has run, but version 6 hasn't it will still be applied the next time migrations are run. This is useful if you have a migration you always want to run last but add migrations after that one. It can be version 9999 but not force later migrations to be 10000.
No rollback capability, maybe the concept is useful for local dev but it can be added later if we need it
No CLI
No ability to create or drop the database
Each individual migration is run within a transaction so if any of the sql statements fail, the migration will not bel applied

Feel free to push back on this, my feelings won't be hurt 😄

src/invidious/migrations/0007_create_playlists_table.cr

matthewmcgarvey · 2022-02-09T07:01:48Z

src/invidious/migration.cr

@@ -0,0 +1,38 @@
+abstract class Invidious::Migration
+  macro inherited
+    Invidious::Migrator.migrations << self


migrations are automatically added to this list when the file is required which is why there's no need to do anything with migrations after they're defined.

matthewmcgarvey · 2022-02-09T07:02:28Z

src/invidious/migration.cr

+    @db.transaction do |txn|
+      up(txn.connection)
+      track(txn.connection)
+      @completed = true
+    end


This is the core code that runs the migration

unixfox · 2022-02-09T08:28:28Z

Thank you for your work!

I'm definitively going to try it on https://github.com/iv-org/invidious/pull/2469/files#diff-64021f64c486d8a36eae29fce4c1b13e9c95f5cbb713ca7bcd26f8d38517eeb8

src/invidious/migrator.cr

src/invidious/migrations/0007_create_playlists_table.cr

src/invidious/migration.cr

src/invidious/migrations/0000_create_channels_table.cr

SamantazFox · 2022-02-10T19:54:00Z

src/invidious/migrations/0001_create_videos_table.cr

+
+    def up(conn : DB::Connection)
+      conn.exec <<-SQL
+      CREATE UNLOGGED TABLE IF NOT EXISTS public.videos


Given that UNLOGGED was a recent addition by @unixfox, most tables on the different instances haven't been altered. It would be preferrable to have a migration for that (ALTER TABLE ...).

I left this as it was and made a new migration altering the table

src/invidious/migrations/0007_create_playlists_table.cr

matthewmcgarvey · 2022-02-11T04:40:19Z

src/invidious.cr

@@ -111,6 +112,8 @@ end
 OUTPUT = CONFIG.output.upcase == "STDOUT" ? STDOUT : File.open(CONFIG.output, mode: "a")
 LOGGER = Invidious::LogHandler.new(OUTPUT, CONFIG.log_level)

+# Run migrations
+Invidious::Database::Migrator.new(PG_DB).migrate


One thing to keep in mind with this right now is... if someone is deploying multiple instances of invidious at the same time, this could cause issues. I think we could have this code lock the migrations table so one instance will claim the migration rollout but I'm not sure if that will work. I've not personally seen it done that way. Typically migrations are run before the deploy. We could move this line to a separate file that people could run before deploying if they want? Is it even necessary?

We could do the following:

Check for migrations

If migrations have to be run, exit with an informational message (and a specific error code)

If --migrate-db is passed, run migrations

That way, it's at the discretion of the instance owner to do as they want: if invidious exits with specified error code, they can hook up a wrapper script to stop all other instances, run a backup, and then migrate.

Mmmh this definitively need to be tackled properly. I'm running 10 instances of Invidious at the same time, so I don't want this to become an issue haha.

matthewmcgarvey · 2022-02-11T04:42:29Z

src/invidious/database/migrations/0008_create_playlists_table.cr

+      SQL
+    end
+
+    private def privacy_type_exists?(conn : DB::Connection) : Bool


Instead of the weird pgsql query, I extracted out this check to look for the privacy type and only creating it if this method returns false.

src/invidious/database/migrations/0008_create_playlists_table.cr

src/invidious/database/migrator.cr

SamantazFox

Looks good to me!

matthewmcgarvey · 2022-02-12T04:44:14Z

src/invidious.cr

+  parser.on("--migrate", "Run any migrations") do
+    Invidious::Database::Migrator.new(PG_DB).migrate
+    exit
+  end


This was a good idea, only issue is that it really messes with my flow of using docker-compose

Oh, right. We should probably make a wrapper script for that?

Also, we probably shouldn't exit if migrations are run. Passing --migrate would always bump the DB no matter what, without breaking the execution process. And we could make that the default on Docker, so owners of small instances aren't too surprised.

So I'm concerned that we're going to roll out a breaking change to everyone with this PR. Everyone will update as normal and then find out that they can't run the service and will have to update their command, and I'm not sure the best way to roll that out.

What I'm going to do is leave this as it is, and remove the pending migration check. That way we can merge this change, it won't break anyone, and people can test out the migrations process. Then we can make a follow-up PR with the implementation that requires the migrations.

I am struggling to run these CLI arguments, when I type sudo docker exec -it invidious-invidious-1 invidious -v I get

OCI runtime exec failed: exec failed: unable to start container process: exec: "invidious": executable file not found in $PATH: unknown

matthewmcgarvey · 2022-02-12T04:45:01Z

src/invidious.cr

+# Run migrations
+if Invidious::Database::Migrator.new(PG_DB).pending_migrations?
+  puts "There are pending migrations. Run `invidious --migrate` to apply the migrations."
+  exit 46


You said use a specific error code 😛. But for real... what error code should I return?

I don't know xD I'll have to double check that, to ensure that we don't use something that either kemal or crystal already do. (Also I'd have expected that you'd go with 42 xD)

This is so that we don't break deploys with this PR. Instead we only ship the 'invidious --migrate' cli command and let people test that. Maybe even ship a new migration that wouldn't break apps that don't run the migrations. Then we roll out the functionality that requires migrations.

unixfox · 2022-02-12T15:31:07Z

@SamantazFox @TheFrenchGhosty Please don't merge this PR without extensive testing. I would even consider asking some people to test before merging that to the public.
Don't merge before I've tested it too, I really don't want to break yewtu.be.

SamantazFox · 2022-02-12T15:51:32Z

@unixfox Sure!

SamantazFox · 2022-02-12T15:56:30Z

src/invidious.cr

-# Run migrations
-if Invidious::Database::Migrator.new(PG_DB).pending_migrations?
-  puts "There are pending migrations. Run `invidious --migrate` to apply the migrations."
-  exit 46
-end


"This is so that we don't break deploys with this PR."

Well, when we'll make any DB changes (like #2469), the code won't correspond to the DB anymore, so invidious will still be broken until the migration is done.

As an in-between, we could keep the check + message, but not exit (so it's easier to trace why invidious is broken).

I made this change because it seems like the rollout of requiring migrations to have run needs more thought which would could give more attention in a followup PR. By removing that, we can merge in the migration code without worrying about breaking users

matthewmcgarvey · 2022-03-11T01:48:49Z

@SamantazFox Is there a blocker for this anymore now that it won't run migrations unless the CLI command is used? If this is merged we could talk further about how we want migrations to be done by end users

SamantazFox · 2022-03-11T07:08:09Z

I don't see any. I was waiting on @unixfox approval, as per his previous message.

unixfox · 2022-03-11T08:11:54Z

I'm not sure if I'll have time to try, you may merge anyway as it is not automatic yet.

TheFrenchGhosty · 2022-03-11T20:19:13Z

@SamantazFox Does this mean we can finally merge PRs that implement database changes? (after they're changed to work, obviously)

unixfox · 2022-03-11T20:38:55Z

@SamantazFox Does this mean we can finally merge PRs that implement database changes? (after they're changed to work, obviously)

No because it's not automatic yet. Merging them will brake invidious.

Add custom migration implementation

8ec992a

matthewmcgarvey commented Feb 9, 2022

View reviewed changes

matthewmcgarvey marked this pull request as ready for review February 9, 2022 07:09

matthewmcgarvey requested a review from a team as a code owner February 9, 2022 07:09

matthewmcgarvey requested review from SamantazFox and removed request for a team February 9, 2022 07:09

SamantazFox reviewed Feb 10, 2022

View reviewed changes

matthewmcgarvey commented Feb 11, 2022

View reviewed changes

SamantazFox reviewed Feb 11, 2022

View reviewed changes

src/invidious/database/migrations/0008_create_playlists_table.cr Outdated Show resolved Hide resolved

src/invidious/database/migrator.cr Outdated Show resolved Hide resolved

Migrations tweaks

cf13c11

matthewmcgarvey force-pushed the migrations branch from 512209c to cf13c11 Compare February 12, 2022 04:29

SamantazFox approved these changes Feb 12, 2022

View reviewed changes

Run migrations through CLI instead of when app starts

5965428

matthewmcgarvey commented Feb 12, 2022

View reviewed changes

unixfox added blocked require something else first need-testing This feature needs to be deployed and tested to see if it's working, and doesn't break something labels Feb 12, 2022

SamantazFox reviewed Feb 12, 2022

View reviewed changes

unixfox mentioned this pull request Feb 28, 2022

Add ability to disable notifications #2937

Closed

SamantazFox merged commit 55da1e3 into iv-org:master Mar 11, 2022

matthewmcgarvey deleted the migrations branch March 11, 2022 21:02

syeopite mentioned this pull request Jul 25, 2023

Add 2fa to Invidious in the form of TOTP #2254

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom migration implementation #2878

Add custom migration implementation #2878

matthewmcgarvey commented Feb 9, 2022 •

edited

Loading

matthewmcgarvey Feb 9, 2022

matthewmcgarvey Feb 9, 2022

unixfox commented Feb 9, 2022

SamantazFox Feb 10, 2022

matthewmcgarvey Feb 11, 2022

matthewmcgarvey Feb 11, 2022

SamantazFox Feb 11, 2022

unixfox Feb 12, 2022

matthewmcgarvey Feb 11, 2022

SamantazFox left a comment

matthewmcgarvey Feb 12, 2022

SamantazFox Feb 12, 2022

matthewmcgarvey Feb 12, 2022

broquemonsieur Dec 18, 2023

matthewmcgarvey Feb 12, 2022

SamantazFox Feb 12, 2022 •

edited

Loading

unixfox commented Feb 12, 2022 •

edited

Loading

SamantazFox commented Feb 12, 2022

SamantazFox Feb 12, 2022

SamantazFox Feb 12, 2022

matthewmcgarvey Feb 12, 2022

matthewmcgarvey commented Mar 11, 2022

SamantazFox commented Mar 11, 2022

unixfox commented Mar 11, 2022

TheFrenchGhosty commented Mar 11, 2022

unixfox commented Mar 11, 2022

Add custom migration implementation #2878

Add custom migration implementation #2878

Conversation

matthewmcgarvey commented Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unixfox commented Feb 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamantazFox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamantazFox Feb 12, 2022 • edited Loading

Choose a reason for hiding this comment

unixfox commented Feb 12, 2022 • edited Loading

SamantazFox commented Feb 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewmcgarvey commented Mar 11, 2022

SamantazFox commented Mar 11, 2022

unixfox commented Mar 11, 2022

TheFrenchGhosty commented Mar 11, 2022

unixfox commented Mar 11, 2022

matthewmcgarvey commented Feb 9, 2022 •

edited

Loading

SamantazFox Feb 12, 2022 •

edited

Loading

unixfox commented Feb 12, 2022 •

edited

Loading