Fix service startup and deployment

[itwasntandy]

Firstly with mysql-server, it was being restarted with phab config
delayed, which during initial run (or during kitchen test run), meant
that phd service was being created before mysql had been restarted.
Thus when mysql was restarted it was restarting phd, causing the
behaviour noticed previously in a comment.

Secondly, with the run_storage_upgrade task, switched to using a guard
not_if rather than the preivous mechanism, as this should be more robust
and is cleaner.

Lastly with the phd upstart script, configured it to not depend on mysql starting
when mysql server is not local.

[kimtore]

Thanks for the patch. I haven't converged yet, but when reading the code I noticed that there is no trigger for the Phabricator storage upgrade. It must be performed after pulling the updated Git repositories. Am I missing something?

[itwasntandy]

Sure, I removed the trigger, as I didn't see it as necessary? I might be missing something but:

Without the notifies on first deploy it will run the storage upgrade as I removed the action :nothing and left it to the default (:run)

         * phabricator_config[mysql.host] action set
           - Configure Phabricator: set phabricator_config[mysql.host]

           - Configure Phabricator: set phabricator_config[mysql.port]

           - Configure Phabricator: set phabricator_config[mysql.user]

           - Configure Phabricator: set phabricator_config[mysql.pass]

       - execute /opt/phabricator/phabricator/bin/storage upgrade --force

I tried re-adding the notifies, and looking at logs pretty much the same happens as above:

         * phabricator_config[mysql.host] action set
           - Configure Phabricator: set phabricator_config[mysql.host]

           - Configure Phabricator: set phabricator_config[mysql.port]
         * phabricator_config[mysql.user] action set
           - Configure Phabricator: set phabricator_config[mysql.user]
         * phabricator_config[mysql.pass] action set
           - Configure Phabricator: set phabricator_config[mysql.pass]

           - execute /opt/phabricator/phabricator/bin/storage upgrade --force

In the case of an upgrade… then obviously the node['phabricator']['storage_upgrade_done'] attribute would need to be unset, but the same would have been true before.
Arguably there may be a delay of a few seconds in that case, which the notify could help with... but only if the notifies was immediate, rather than delayed (default).

[kimtore]

The key point is that the storage upgrade must run every time Phabricator is upgraded. Today's mechanism works, in the way that notifies :run, "execute[upgrade_phabricator_databases]" triggers it when the Git repositories are updated.

The storage_upgrade_done is a workaround. I don't remember the particulars, but as far as I recall, there was some catch 22 that needed to be solved during the very first converge. It will be better though, if that mess can be cleaned up while ensuring that the storage upgrade is run every time. :)

[itwasntandy]

Gotcha. I think I know what I'll do now… (The reason I started looking at this was that I noticed before was phd was being stopped and not restarted…)

So if storage_upgrade_done gets set to the git tag of that version at the end of a run, instead of true, and we do a comparison on that, that way it will trigger a database if there's a change, but otherwise it won't.

Feels like that would be quite clean. What do you think?

[kimtore]

Why compare on git tag when we can trigger it with a notification?

IIRC, the workaround was added because the daemon and mysql database have to be stopped beforehand, and started at a later point. But those servers will need to run directly after the storage upgrade, in order to configure Phabricator settings.

[itwasntandy]

so the reason for that, would be that the notify would be triggered on any of the git repositories changing.
Do you really want phd and php-fpm being restarted if (for example) arcanist is updated?

[itwasntandy]

could still trigger via a notifies of course, just thinking that having a guard to only run that block when necessary would be useful…

[kimtore]

As the storage upgrade overhead in restarting the services only takes a few seconds, I'd say that the cost of restarting outweighs the additional complexity added by having to make a special case for it in the source code.

What would be the purpose of a guard when the default action is :nothing?