Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[9.x] Make migrate command isolated #44743

Merged
merged 12 commits into from
Oct 31, 2022

Conversation

olivernybroe
Copy link
Contributor

@olivernybroe olivernybroe commented Oct 26, 2022

This PR makes the migrate command isolated which limits the migration command to only have one process active at once. Meaning two servers cannot both run migrate at the same time.

Problem

When deploying on multiple servers at once, it needs to be decided which of these servers have the responsibility for running migrations, however this means that all the deployments to all the servers are not identical, to simplify deployments.

For example when using k8s, you would use something like initContainers for running things like migrations. However as this will happen on each replica it could result in two servers running the same migration at the same time, as there is no locking so only one server will run one migration.

Other people talking about this issue

Solution

Make migrate command isolated. This could in theory still result in the command running twice in the same deployment, however not at the same time, so it won't cause any issues, it will however result in queries to the database to check if migrations are missing where we already know there are none missing.

This solution works by adding the Isolated interface to a command which marks the command as an isolated command and it will now only run on one server. This implementation would allow to easily make other commands run in isolated when needed.

class MyCommand extends Command implements Isolated
{
   ...
}

An alternative solution could be to instead make this a flag called isolated so the user can choose when running the command to run it isolated or not. This solution could be added as a parameter available for all commands by default.

src/Illuminate/Console/CacheCommandMutex.php Outdated Show resolved Hide resolved
src/Illuminate/Console/CommandMutex.php Outdated Show resolved Hide resolved
src/Illuminate/Console/CacheCommandMutex.php Outdated Show resolved Hide resolved
src/Illuminate/Console/CacheCommandMutex.php Show resolved Hide resolved
src/Illuminate/Console/CacheCommandMutex.php Show resolved Hide resolved
src/Illuminate/Console/CacheCommandMutex.php Outdated Show resolved Hide resolved
src/Illuminate/Console/CacheCommandMutex.php Outdated Show resolved Hide resolved
src/Illuminate/Console/CacheCommandMutex.php Outdated Show resolved Hide resolved
src/Illuminate/Console/CommandMutex.php Outdated Show resolved Hide resolved
src/Illuminate/Console/CommandMutex.php Outdated Show resolved Hide resolved
tests/Console/CacheCommandMutexTest.php Outdated Show resolved Hide resolved
@olivernybroe olivernybroe marked this pull request as ready for review October 28, 2022 10:04
@deleugpn
Copy link
Contributor

Interesting overall, but when you look at a generic Isolated interface without knowing the underlying Command, I'm not sure how I feel about exit successfully. I keep imagining a process executing php artisan something and getting a successful response back "because I was not able to acquire a lock". In HTTP world, we have the 3xx but in CLI we unfortunately only have 0 and non-zero. Maybe I'm chasing ghosts, but I see this end up getting evolved into "let's ask the underlying command what I should do if I can't acquire the lock" and then it just feels easier if you just wrap the migrator yourself?

@olivernybroe
Copy link
Contributor Author

@deleugpn Would you feel better about it if it was a flag?

So you say php artisan migrate --isolated and then it is enabled. This way you actively choose that the command can finish early because another server is running it already.

@taylorotwell
Copy link
Member

I definitely think this should be behind some kind of flag to reduce the chance of breaking changes.

@taylorotwell taylorotwell marked this pull request as draft October 28, 2022 13:51
@olivernybroe
Copy link
Contributor Author

Alright, I have added a flag named isolated to it instead. 👍

@olivernybroe olivernybroe marked this pull request as ready for review October 31, 2022 07:50
@taylorotwell
Copy link
Member

Added ability to customize the lock expiration time. Also added ability to specify the exit code (--isolated=12) per @deleugpn's comment.

@taylorotwell taylorotwell merged commit c2798fd into laravel:9.x Oct 31, 2022
@taylorotwell
Copy link
Member

Interface renamed to Isolatable.

@GrahamCampbell GrahamCampbell changed the title Make migrate command isolated [9.x] Make migrate command isolated Nov 6, 2022
@bytestream
Copy link
Contributor

Why does this use store()->add() instead of store()->lock()?

store->add() is affected by cache:clear while ->lock() is not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants