Run database migrations only when pending

[ehelms]

Why are you introducing these changes? (Problem description, related links)

Foreman database migration containers currently run on deployments where their databases are already up to date. These containers add unnecessary startup time. Database migrations should be checked explicitly, and the migration containers should only run when pending migrations are reported.

What are the changes introduced in this pull request?

• Add one-shot Foreman migration-check containers.
• Run the migration container only when the migration check reports pending migrations.
• Remove database seeding from the Foreman migration container; Foreman performs seeding during application startup.

How to test this pull request

Steps to reproduce:

• Run ANSIBLE_COLLECTIONS_PATH="$PWD/build/collections/foremanctl" ANSIBLE_COLLECTIONS_SCAN_SYS_PATH=false bash -c 'source .venv/bin/activate && cd src && ansible-lint'; expect no findings.
• Run source .venv/bin/activate && ruff check tests/ src/ development/scripts/ inventories/; expect all checks to pass.
• Run git diff --check; expect no whitespace errors.
• Deploy on a provisioned test VM and verify that the migration service runs when migrations are pending and is skipped when the database is current.

Checklist

• Tests added/updated (if applicable)
• Documentation updated (if applicable)

[shubhamsg199]

Ack, similar approach I suggested in #769 (comment)

[evgeni]

I really wish we could run db:abort_if_pending_migrations and ask Rails whether migrations are needed, instead of guessing.

[ehelms]

I really wish we could run db:abort_if_pending_migrations and ask Rails whether migrations are needed, instead of guessing.

We can. I tried this approach as well and it works. It's just not as efficient because booting our environment is slow. When I researched how to make our boot time faster I basically encountered needing to go explore bootsnap which we only enable in development. Based on the time savings of other big apps (e.g. Shopify) we could possibly get our boot time down to sub 5 seconds.

[evgeni]

Stupid question: Why can't we migrate on app boot (so in config.after_initialize or so)?

[ehelms]

Stupid question: Why can't we migrate on app boot (so in config.after_initialize or so)?

I think we avoided this primarily due to potential race conditions with Puma having multiple workers, the need to ensure orchestration of the foreman instance before dynflow, and to keep the door open to multi-container instances in the future.

[ehelms]

So I realized there is a signal missing in this design that would cause problems and that is if a feature is enabled that enables a plugin this would not signal the migration. That leaves us with two options:

1. Keep this design but add a check if the quadlet changed and use that as a signal that migration is needed
2. Swap to check if there are pending migrations, accept the cost of the Rails environment boot up for now and try to tackle that problem instead.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: dcedd372-6e4b-431e-bb1e-bf611e95a756

📥 Commits

Reviewing files that changed from the base of the PR and between 8439731 and 0048e96.

📒 Files selected for processing (3)

• src/roles/foreman/tasks/main.yaml
• src/roles/pulp/defaults/main.yaml
• src/roles/pulp/tasks/main.yaml

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Foreman and Pulp now check for pending database migrations before running migration services. Foreman removes migration ordering dependencies and no longer seeds during migration. Pulp adds a configurable always-run option.

Changes

Database migration gating

Layer / File(s)	Summary
Foreman migration gating src/roles/foreman/tasks/main.yaml	Foreman and Dynflow no longer order against the migration service. A migration-check service detects pending migrations, and the migration service runs only when required. Database seeding is removed.
Pulp migration gating src/roles/pulp/defaults/main.yaml, src/roles/pulp/tasks/main.yaml	Pulp adds pulp_always_run_migrations. When disabled, a migration-check service controls whether the database migration task runs.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 179b4

Foreman and Pulp now avoid unnecessary migration runs while preserving Pulp’s existing always-run behavior by default. No actionable merge-blocking risk remains.

Sequence Diagram(s)

sequenceDiagram
  participant ForemanRoleTasks
  participant ForemanMigrationCheckService
  participant ForemanDatabase
  participant ForemanMigrationService
  ForemanRoleTasks->>ForemanMigrationCheckService: restart migration check
  ForemanMigrationCheckService->>ForemanDatabase: run db:abort_if_pending_migrations
  ForemanRoleTasks->>ForemanMigrationCheckService: read ExecMainStatus
  ForemanRoleTasks->>ForemanMigrationService: restart when status is 1

Loading

sequenceDiagram
  participant PulpRoleTasks
  participant PulpMigrationCheckService
  participant PulpDatabase
  participant PulpMigrationService
  PulpRoleTasks->>PulpMigrationCheckService: restart when always-run is false
  PulpMigrationCheckService->>PulpDatabase: run pulpcore-manager migrate --check
  PulpRoleTasks->>PulpMigrationCheckService: read ExecMainStatus
  PulpRoleTasks->>PulpMigrationService: migrate when status is 1 or always-run is true

Loading

Suggested reviewers: evgeni, pablomh, archanaserver

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the primary change: database migrations run only when pending migrations exist.
Description check	✅ Passed	The description directly explains the migration checks, conditional execution, Foreman seeding removal, and testing steps in the changeset.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[ehelms]

I have reworked this to the check if there are pending migrations model as that is the most reliable. I tried out checking for image change or quadlet change but those both still felt like there could be edge cases.

[evgeni]

If it must always run, why bother checking if it needs to run? :)

[ehelms]

Great question, this came from https://github.com/theforeman/puppet-pulpcore/blob/master/manifests/database.pp#L39. Re-reading the PR (theforeman/puppet-pulpcore#351) I see we did this as a quirk of puppet idempotency.

[ehelms]

I removed the Pulp handling from the code, this just does Foreman now.