Bug #22704
closed
Rails postinst can "succeed" in a broken state if database seeding fails
Description
Consider this function in source:build/rails-package-scripts/postinst.sh which gets called in the main body:
prepare_database() {
# Prevent PostgreSQL from trying to page output
unset PAGER
DB_MIGRATE_STATUS=`"$BUNDLE" exec bin/rake db:migrate:status 2>&1 || true`
if echo "$DB_MIGRATE_STATUS" | grep -qF 'Schema migrations table does not exist yet.'; then
# The database exists, but the migrations table doesn't.
run_and_report "Setting up database" "$BUNDLE" exec bin/rake db:schema:load db:seed
elif echo "$DB_MIGRATE_STATUS" | grep -q '^database: '; then
run_and_report "Running db:migrate" "$BUNDLE" exec bin/rake db:migrate
elif echo "$DB_MIGRATE_STATUS" | grep -q 'database .* does not exist'; then
run_and_report "Running db:setup" "$BUNDLE" exec bin/rake db:setup
else
# We don't have enough configuration to even check the database.
return 1
fi
}
Imagine you install arvados-api-server on a system where all the prerequisites are met (it has a good cluster configuration with an existing database and valid credentials, etc.) EXCEPT there is no smtpd listening on port 25. The first time you try to install the package, prepare_database will go down the first branch, and the schema will get loaded, but then db:seed will fail because of #22703. (There might be other ways to get db:seed to fail but this is the one in front of me now.)
The next time you try to install arvados-api-server, it will go down the second branch, try to run db:migrate, that'll be a noop, and it will say that everything is good. However, not everything is good: you still have not seeded the database, so you don't have records like a root user, and your RailsAPI server is functionally useless. If you're being good and following the install docs, you won't notice this until you run arvados-client diagnostics, it fails on the "get current user" step, and you go to track down why.
This function should do more to check whether the database is seeded and retry the rake task if not. I realize this is a much more delicate operation than it sounds and probably would benefit from some team discussion about the "right" way to do this.
Updated by Peter Amstutz
Updated by Tom Clegg 9 months ago
22704-db-load-and-seed @ f6b0a9edd3cb80ed743f1324021af93f6e827e3d -- developer-run-tests: #4824
- All agreed upon points are implemented / addressed. Describe changes from pre-implementation design.
- ✅ db:seed is idempotent, so an easy fix is to always run it before db:migrate
- Anything not implemented (discovered or discussed during work) has a follow-up story.
- n/a
- Code is tested and passing, both automated and manual, what manual testing was done is described.
- n/a
- New or changed UI/UX has gotten feedback from stakeholders.
- n/a
- Documentation has been updated.
- Added a note in source:services/api/app/models/database_seeds.rb that we're counting on it being non-destructive.
- Behaves appropriately at the intended scale (describe intended scale).
- n/a
- Considered backwards and forwards compatibility issues between client and server.
- n/a
- Follows our coding standards and GUI style guidelines.
- ✅
Updated by Brett Smith 9 months ago
Tom Clegg wrote in #note-6:
22704-db-load-and-seed @ f6b0a9edd3cb80ed743f1324021af93f6e827e3d -- developer-run-tests: #4824
I'm glad this looks straightforward.
What happens if we make a release where we add a seed that depends on a migration in the same release? e.g., a new model table seeded with an initial record.
Doing some quick reading, it looks like nothing magically makes the database seeds idempotent, they're idempotent because we write database_seeds.rb that way. Is that right? If so I think your new comment is good, just double-checking I'm following right.
Thanks.
Updated by Tom Clegg 9 months ago
Brett Smith wrote in #note-7:
What happens if we make a release where we add a seed that depends on a migration in the same release? e.g., a new model table seeded with an initial record.
Good point. I've swapped the order to "rake db:migrate db:seed".
The logic here doesn't actually detect whether migrations are needed -- only whether it's possible to run db:migrate -- so if either one of those fails, package install will fail and both will run next time. IOW this still makes it impossible for a sequence of install attempts to skip db:seed and exit 0.
Doing some quick reading, it looks like nothing magically makes the database seeds idempotent, they're idempotent because we write
database_seeds.rbthat way. Is that right? If so I think your new comment is good, just double-checking I'm following right.
Yes, that's right.
22704-db-load-and-seed @ d639865dfcc898dff71a834676894c09d6059bb3
Updated by Brett Smith 9 months ago
Tom Clegg wrote in #note-8:
22704-db-load-and-seed @ d639865dfcc898dff71a834676894c09d6059bb3
LGTM, thanks.
Updated by Tom Clegg 9 months ago
- Status changed from In Progress to Resolved
Applied in changeset arvados|0c89cd103459b54788efca583b1753f5c438fe2f.