Project

General

Profile

Actions

Feature #19709

closed

Apply database migrations if needed when starting arvados-server boot

Added by Tom Clegg over 1 year ago. Updated over 1 year ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Story points:
1.0

Description

Currently, upgrading the arvados-server-easy package via apt-get upgrade does not handle database migrations, so if any are needed, railsapi doesn't come up.

This should be handled by arvados-boot when [re]starting, rather than by a package trigger script (otherwise, slow/failed migrations will cause apt-get upgrade to block/reverse other package changes too, and generally make a mess).


Subtasks 1 (0 open1 closed)

Task #19856: Review 19709-boot-db-migrateResolvedTom Clegg12/08/2022Actions

Related issues

Related to Arvados Epics - Idea #18337: Easy install via OS packageIn ProgressActions
Related to Arvados Epics - Idea #15941: arvados-bootNewActions
Actions #1

Updated by Tom Clegg over 1 year ago

  • Related to Idea #18337: Easy install via OS package added
Actions #2

Updated by Tom Clegg over 1 year ago

Actions #3

Updated by Tom Clegg over 1 year ago

  • Status changed from New to In Progress
Actions #4

Updated by Peter Amstutz over 1 year ago

  • Target version set to 2022-12-21 Sprint
Actions #5

Updated by Tom Clegg over 1 year ago

  • Story points set to 1.0
Actions #6

Updated by Peter Amstutz over 1 year ago

  • Target version changed from 2022-12-21 Sprint to 2023-01-18 sprint
Actions #7

Updated by Peter Amstutz over 1 year ago

  • Target version changed from 2023-01-18 sprint to 2022-12-21 Sprint
Actions #9

Updated by Brett Smith over 1 year ago

Tom Clegg wrote in #note-8:

19709-boot-db-migrate @ 41b9b83812826b77034fe13ea34047e194b1027f -- developer-run-tests: #3408

I don't have any big concerns about this code as-is, I mostly have questions about why this approach was chosen.

What are the reason(s) we cannot or should not just call db:migrate directly every time? It seems to handle the case of "no migrations need to be run" just fine. I see the comment about rake commands being "[relatively slow]" which, sure, I'll grant that, but is that the only reason?

If we can't run db:migrate every time, Rails provides a db:migrate:status task to report what migrations have and haven't been done. I understand this gets us back into slowness, but I feel like it's a pretty big upside to avoid reimplementing Rails' own logic for this, and potentially having subtle discrepancies. Are there other reasons besides slowness you decided against using this?

If we must do it ourselves, is it worth verifying that the migration version we pull out of the filename (d.Name()[:cut]) is numeric before we add it to the todo list? Rails itself seems to require this, so our implementation could catch stray files lying in that directory that Rails itself ignores. It shouldn't be too bad if it happens, just a noop db:migrate run, but if we're that concerned about avoiding those maybe it's worth it?

I think it would also be nice to see some of this "why this approach" discussion in the commit message, whatever we end up doing, in case future readers have questions.

Thanks.

Actions #10

Updated by Tom Clegg over 1 year ago

Yes, this is really just about timing. I've added this to make it less mysterious:

       // In principle, we could use "rake db:migrate:status" or skip
       // this check entirely and let "rake db:migrate" be a no-op
       // most of the time.  However, in the most common case when
       // there are no new migrations, that would add ~2s to startup
       // time / downtime during service restart.

I've added the only-digits-before-underscore check.

Going out of sync with Rails logic is a fair point. I think I can address that with a test case.

Actions #11

Updated by Tom Clegg over 1 year ago

Test that schema_migrations and db/migrate/*.rb line up the way we expect.

19709-boot-db-migrate @ 228028e523983a847239fd4a5530d3e54b27cfa5 -- developer-run-tests: #3414

Actions #12

Updated by Brett Smith over 1 year ago

Tom Clegg wrote in #note-10:

Yes, this is really just about timing. I've added this to make it less mysterious:

Okay, thanks. I'll just note, if it were me, I would've taken the other trade. Two seconds of downtime for an event that should be relatively rare seems okay to save us all this code. But I don't think it's my decision, especially not during the review.

Thanks for the test, that is nice.

My only concern with the new version is that it now fails if there are non-migration files in the db/migrate directory. This seems needlessly strict to me and admin-unfriendly to stop the boot because of these extra files. (I'm guessing the extra files that are most likely to end up there is backup files, or migrations intentionally moved out of the way, when the admin is in a bad spot and trying to un-wedge things. They might just break things more, but so could modifying the migrations in-place, and we're not defending against that.) I think it would be nicer to log each file being skipped but otherwise continue as normal. What do you think?

Actions #13

Updated by Tom Clegg over 1 year ago

Good point. Updated to warn (instead of erroring out) on unexpected files in db/migrate/.

19709-boot-db-migrate @ da416e33b7e9fb51c27662ef111bdc4fafcadbcf -- developer-run-tests: #3415

Actions #14

Updated by Brett Smith over 1 year ago

Tom Clegg wrote in #note-13:

Good point. Updated to warn (instead of erroring out) on unexpected files in db/migrate/.

19709-boot-db-migrate @ da416e33b7e9fb51c27662ef111bdc4fafcadbcf -- developer-run-tests: #3415

Thanks, LGTM.

Actions #15

Updated by Tom Clegg over 1 year ago

  • % Done changed from 0 to 100
  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF