"Preloader" process for preload + rolling restart

[nateberkopec]

#1861 (comment)

[nateberkopec]

@cjlarose kind of reminds me of the puma-wild proposal you made.

[cjlarose]

Yeah, I see the connection. In #2018 (comment) I described a way to separate the gems of the puma master process from those of the workers in a way that was more robust than the existing prune_bundler solution.

The comments in #1861 describe a way to leverage the benefit of preload_app (faster worker startup, leverage COW memory) in a system that also supports phased restarts. To start workers, the puma master first forks into what @evanphx calls a "stem-cell" process. The "stem-cell" process preloads the application and then later forks to make the actual worker processes. When it's time to perform a phased restart, the puma master forks a new "stem-cell" process, from which the new workers are forked.

In that new architecture, fixing the kinds of bugs related to #2018 would only require a small change: Whenever the puma master forks to form the "stem-cell" worker-generator process, it must immediately exec into a new ruby process, removing any references from puma master process's gems. After the exec, that process can preload the application and later fork workers. This change can make it possible for CD tools to automatically remove the gems from disk associated with the puma master. It'll also make it possible to upgrade gems like nio4r in new versions of the application, without requiring a hard restart, since the gems of the master process are separated from the gems of the worker-generator and worker processes.

The architecture proposed in my comments in #2018 doesn't require the new "stem-cell"/worker-generator process idea, but is definitely compatible with it. This strategy seems like a good way forward.

I might have some time to work on this in the next couple of days if no one's actively working on it already.

[nateberkopec]

no one's actively working on it already.

Just confirming no one in Puma core has spiked this yet.

[cjlarose]

I was able to spend some time on this during a company hackathon. Early results are great: I was able to confirm that with the new architecture, it's possible to support both preload_app and phased restarts. The cluster integration test suite passes.

The code's not great and it's not ready for review, but it's here: cjlarose@a1f7c11

It doesn't yet exec to form the worker generator processes (so it doesn't actually fix #2018 yet), but I think that can be a follow-up change. The master process doesn't yet correctly forward some signals to the worker generators. The master process doesn't yet detect when worker generators die unexpectedly and restart them. Worker generator processes don't yet detect when the master dies unexpectedly and kill themselves. In short, a bunch of stuff isn't ready for production yet, but at least it's clear to me now that it's all doable.

From here, I'll spend some time cleaning up the code, breaking up the change into smaller, easier-to-review commits (maybe even separate PRs), fixing tests, writing new tests, and all that jazz.

[nateberkopec]

Thanks for your efforts here @cjlarose!

[cjlarose]

I noticed there's some behavior of Puma::Cluster that isn't covered by the test suite. I think it'd be good to get some better coverage before I introduce the worker-generator process change.

I opened up #2165 to backfill tests related to SIGHUP and Puma::Cluster#redirect_io. There might be a few more PRs in the same vein.

[wjordan]

Worth mentioning that #2099 is an existing solution to this issue. The fork_worker option 'preloads' the app in worker 0, which serves as the 'stem-cell' process from which the rest of the workers are forked, so phased_restart should work as usual.

The tiny difference between a 'refork' and a phased_restart is that in a 'refork', the stem-cell (worker 0) doesn't get reloaded:

puma/lib/puma/cluster.rb

Lines 420 to 423 in 53839f9

	if (worker = @workers.find {|w| w.index == 0})
	worker.phase += 1
	end
	phased_restart

[gingerlime]

@wjordan thanks for the update. Did #2099 fix the problem reported on #2018 ? sorry, my familiarity with puma is extremely limited, but we're still stuck on puma 3.x because of the phased-restart issues with 4.x and I'm just trying to figure out if there's even a temporary workaround at this point.

[wjordan]

Did #2099 fix the problem reported on #2018?

No, according to the Docker test in cjlarose/puma-phased-restart-could-not-find-gem-errors, that issue is not fixed by the fork_worker option alone. That option provides the ability to use phased restart with a 'preloaded' app that only needs to be initialized once, but doesn't do anything additional to isolate activated gems in worker processes via exec.

[gingerlime]

Thanks for clarifying @wjordan !! much appreciated.

[cjlarose]

Happy to see that #2099 is merged. I was kinda discouraged from fixing #2018 while #2099 was still in development, but I can take another look now that it's merged!

[gingerlime]

Sorry to nag, but is there any way to help move this forward? We're not deeply familiar with Puma unfortunately, but if there's something we can do to help, we'd like to at least try :) we're getting a bit worried that we're still "stuck" on 3.x and unable to upgrade to 4.x due to issues with the phased restarts...

[cjlarose]

I finally got around to taking a look at implementing exec-after-fork for worker 0, provided that the fork_worker option is enabled. I was able to put together a proof-of-concept here: cjlarose@57e3042

When the puma master process creates worker 0, worker 0 execs into a new process image (no shared memory with the puma master). This probably reduces some of the benefit of COW memory since the the puma master and worker 0 won't share as much memory as they did before. However, I'm fairly confident that it does fix #2018. Since the workers no longer share any memory with the puma master, they're free to use totally different versions of gems without any conflict.

The Docker test in cjlarose/puma-phased-restart-could-not-find-gem-errors passes with my change.

I've got to clean it up and make sure I didn't break anything critical, and add tests and everything, but at least we know the idea is sound.

[gingerlime]

Thank you @cjlarose. I'm so happy to hear. With the release of 5.0 I was really starting to get nervous that we're still stuck on 3.x. Is there anything we can do to support you to move this forward?

side note / question:

Given the little perceived interest in #2018 I am wondering if we're "doing it wrong" in some way with Puma and our deployment process?

We're still using symlinks and phased restarts to achieve zero downtime deploys. I always assumed this is the Rails way of deploying, but things are obviously moving to docker, kubernetes and other methods I guess. Should we consider moving along to something better? if so, is there a new recommended way to deploy Rails these days?

[cjlarose]

Is there anything we can do to support you to move this forward?

I think when we get a PR merged, it'd be great if you could deploy it and see if it fixes problems for your application. That'd help make sure we got it!

We're still using symlinks and phased restarts to achieve zero downtime deploys. I always assumed this is the Rails way of deploying, but things are obviously moving to docker, kubernetes and other methods I guess. Should we consider moving along to something better? if so, is there a new recommended way to deploy Rails these days?

The product I work on is in a similar spot. We use the same deployment strategy (I think this matches the behavior of capistrano, though we don't use capistrano anymore). I think that deployment strategy makes sense for applications deployed on bare metal or onto VMs (such as in EC2) that you keep around between releases (this is sometimes called mutable infrastructure). I think you're right though that one reason why phased restarts might get less attention these days is because folks are deploying puma applications using container orchestration. If you use kubernetes or ECS or any other container orchestrator, phased restarts are irrelevant because you can more easily handle restarts and rollouts by spinning up new containers and killing the old ones.

I don't have the numbers on what deployment environment is most popular for puma apps these days, but I my guess is that there are still many applications like yours that use a more traditional deployment strategy. Puma still supports features that really only make sense for mutable infrastructure (hot restarts, phased restarts, and re-evaluating the release directory symlink on restart), and to my knowledge, Puma has hasn't expressed any interest in deprecating those features.

With the release of 5.0 I was really starting to get nervous that we're still stuck on 3.x

I don't think I made it clear in #2018, but my team did upgrade to puma 4.x despite the nio4r issue. We changed our deployment process to stop deleting old release directories. We added monitoring to make sure we don't exceed the disk space on our VMs. Our autoscaling rules basically guarantee that no VM lives longer than 24 hours, so as long as we don't deploy so many releases in that window that we exceed the available space on disk, we're good: phased restarts work reliably. Clearly, it's not ideal and I don't think puma users should be expected to do the same. And even with these practices in place, we still can't, for example, upgrade nio4r without downtime (workers spin up and die immediately if we try).

[gingerlime]

Thank you so much for your thoughtful response, @cjlarose.

I think when we get a PR merged, it'd be great if you could deploy it and see if it fixes problems for your application. That'd help make sure we got it!

Absolutely. I hope to also test the PR on our staging environment next week and report back on what we see.

As far as how people are deploying, I also wonder. I would have thought that if this method was very prevalent, then there would be many people commenting and reporting a similar problem. I don't know how to interpret the relative "quiet".

We're also using a capistrano-like process, without using capistrano. However, our VM is much longer-lived, and we don't have any auto-scaling processes to clean it up regularly unfortunately. We also run on only one web server, so we can't even rely on load balancing to do a rolling restart... So we have to rely on the folder cleanup and phased restarts... It's definitely not cutting-edge, but are we becoming old fossils?

We do use docker for development, and it's great. But it still feels like a big leap to switch to kubernetes or something else. That's why I'm curious to understand what other people are using and perhaps also learning about a migration path forward. Thanks for sharing your setup, Chris.

I do hope that Puma continues to support this use-case, although I guess there's an increasingly stronger argument to rely on generic orchestration tools for continuous deployment, and it can help simplify the puma code base and reduce its surface area.

[jarthod]

I don't know how to interpret the relative "quiet"

I just wanted to point out that some people like my company are totally in this situation (standard stuff: capistrano, bare metal, in need for reliable and fast rolling restart for 40+ processes on a single server). We are currently stuck on 3.x for the same reasons and are looking forward to try 5.x, just didn't feel the need to comment here as it wouldn't bring any new information, I don't want to put more pressure on the dev team who is doing what they can. So I just subscribed and ":+1:".

[gingerlime]

Hi @jarthod and thank you. Yes, you're right. It's always hard to strike a balance between too many +1 and "me too" posts that don't add value to the discussion, and not really knowing how many people are affected by an issue :) I didn't mean to imply that I encourage lots of noise, but just that I didn't know if this was an important issue affecting many or not.

[cjlarose]

Happy to hear my work will be useful for folks other than myself 😂

I opened #2374 which is just a refactor that'll make it easier to implement a fix for #2018. If that gets merged, I'll open the follow-up PR which should fix #2018.

[cjlarose]

I think that #2099 by @wjordan solves the original problem described in this issue.

I think we can close this issue and continue the conversion about #2018 and exec-on-worker-fork in that issue.