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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding Pause Support on Redis #1442

Closed
wants to merge 4 commits into from
Closed

Conversation

dbpolito
Copy link
Contributor

@dbpolito dbpolito commented May 9, 2024

This PR adds support to use redis for pause / continue horizons instances, without removing existing behavior.

Motivation

The current implementation relies on you being able to ssh into the server is running horizon so you can pause, which can be limiting in the following scenarios:

Multiple Servers

If you have multiple horizons instances running, in case you need to pause, you would need to ssh on each instance and call pause.

Blue/green deploying on Kubernetes

On Kubernetes it's quite a challenge ssh into the container, specially if you have replicas, etc.

Also, if you want to pause while deploying for example, you probably want to use something like initContainer or before helm hook to run that, which will spawn a new container to run that.

On current implementation it's quite impossible.

In my case, as the old horizon is still running while we're deploying, we can have race conditions, running job on old code until we finish deploy and kill the old one. We should always recommend running pause before deploying i think (not sure it's already done somewhere) 馃

Looking for Feedback

I kept the existing signals approach so we kind of do the pause logic twice... what we could do:

  • Add some config to choose between approaches
  • Remove signals and rely on cache / redis for this
  • I added a docker-compose.yml to make easier to test locally
  • I changed the test worker driver to predis as it skips the requirement of redis extension on local

src/Console/TerminateCommand.php Outdated Show resolved Hide resolved
src/Pause.php Show resolved Hide resolved
src/Pause.php Show resolved Hide resolved
@crynobone crynobone marked this pull request as draft May 14, 2024 14:14
@dbpolito dbpolito requested a review from crynobone May 14, 2024 14:31
@dbpolito dbpolito marked this pull request as ready for review May 14, 2024 14:31
@dbpolito
Copy link
Contributor Author

@crynobone done

not sure i had to change back to ready to review...

@taylorotwell
Copy link
Member

Don't quite have time to dig into this one right, but I'm also not sure it would totally solve all issues in this regard. Pausing wouldn't stop or prevent any jobs that had just started running. Those would continue to run on old code during your deployment. If they are long running jobs they could even run until after your new deployment is finished on the old code.

@dbpolito
Copy link
Contributor Author

Well, in my case i use https://github.com/TheDragonCode/laravel-deploy-operations, if you push a new job into the Queue, the old horizon instance would pick the job and fail with class doesn't exists...

So in that scenario, not picking new jobs is enough...

But the existing behavior wouldn't kill the running jobs, right? So the behavior would be consistent, just not picking new jobs... which makes sense for me...

The problem is, with current implementation, we can't have a way to implement it using events...

@taylorotwell would you be willing to at least accept a PR adding an event like SupervisorLooping to dispatch before the loop start? around here? https://github.com/laravel/horizon/blob/5.x/src/Supervisor.php#L286

That would make at least possible to implement it on the user land.

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.

None yet

3 participants