Fixes #14410 - Raise error on pending migrations on production

[dLobatog]

Problem

During foreman-installer --upgrade, maybe there's some error that will
make migrations not run, such as proxy unavailable, etc.. In that case,
if the user checks the application, everything will look fine. However,
when migrations are pending, permissions from plugins will not be
loaded into Foreman::AccessControl:
(https://github.com/theforeman/foreman/blob/develop/app/services/foreman/plugin.rb#L217)

This is a problem for plugins. I can set the 'view_activation_keys'
permissions because it's on the db, but if it's not loaded, it's
useless. This means the user won't be able to use these permissions and
won't be able to access any plugin resource.

Solution

I looked at the source, and the only option is to show a 500. This
leaves these instructions to run the migrations. We should show a nicer
error IMO, so we can either catch pending_migrations or patch Rails to
allow redirecting to an URL when migrations are pending.

[domcleal]

We should show a nicer
error IMO, so we can either catch pending_migrations

Could you do this please? I think this is a very useful change, but the error's not great, particularly because we recommend using foreman-rake in other parts of the UI, this won't work for most users.

[theforeman-bot]

There were the following issues with the commit message:

• commit message for a18ec4f is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available in Coding Standards or on the Foreman wiki.

---

This message was auto-generated by Foreman's prprocessor

[dLobatog]

@domcleal Updated, I catch it manually now instead of with Rails' setting. Rake tasks are not protected, but that should be fine as this is meant to fix the problem with unloaded permissions in plugins. I'm not sure if applying this check everywhere but on 'db:migrate' other things could break, hence I just allow all tasks to pass it.

[ohadlevy]

I wonder if we can introduce a maintenance mode? maybe in a package pre and then we can skip the entire stack.

I could see this also implemented in hammer to recognize this state and report correctly.
what do you think? /cc @iNecas

[iNecas]

What about catching this in the controller responding with proper HTML/JSON for hammer

[ares]

We can just add few mod_rewrite rules that will check if tmp/maintanance.txt exists and if it does, respond with 503, otherwise pass the request to passenger. Example

Installer could just touch the file and remove it at the end if everything was ok.

EDIT: we'd probably need to let the localhost requests pass though since smart proxy might need to register during the maintenance mode. If that's too complicated change for this PR, maybe we can address that in a separate issue.

[iNecas]

Alternative PR to address the issue with nicer error message + optimization for production, that the Rails's pending_migration is not really suitable (as it checks the db/migrate directory at every request) #3561

[dLobatog]

@iNecas I don't understand why checking for new migrations on every request (even if it's a cached value)? Seems like overkill since only if the app is restarted there could be any new migrations. In that case the app shouldn't even start and the Foreman administrator can start it again when it has ran all migrations. Hammer users don't need to know about this, just the Foreman admin.

@ohadlevy I don't think I understand well what you mean. What's the maintenance mode needed for and what does it have to do with the pending migrations & plugins problem? If foreman-installer runs properly, Foreman can run and respond all requests until httpd is restarted without problem. If there's any problem like this, migrations that didn't run, it won't start. Which I reckon is fine & requires no extra code.

[iNecas]

@dLobatog even for non-admins, it's nicer to see that the service is up but not ready yet, rather than generic 503 error.
Also, in more complex scenarios (as with katello migrations), it's needed to restart httpd before the installer runs due to pulp.

[ohadlevy]

should we close this or #3561? Just wondering what is the next step here?

[ares]

I prefer #3561 since it has better error message, @dLobatog would renaming the exception there help so it does not indicate any maintenance mode? Proper maintenance mode implemention is out of the scope of issue #14410

[dLobatog]

Closed in favor of #3561