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

WIP: Replace Thin with Puma #2261

Closed
wants to merge 1 commit into from

Conversation

andy-paine
Copy link
Contributor

This is a first pass based on the equivalent BOSH PR. CC uses a runner.rb rather than embedding the server startup directly in the bin/ script so move stuff there.

Still needs a lot of work:

  • Not feature flagged (probably should be)
  • Does not allow configuration of puma workers/threads
  • Check signal handling works with BOSH release/k8s as this just lets Puma handle them directly rather than intercepting them
  • Periodic metrics are added on every worker but most are gauges about the individual thread (see vitals) so will clash and cause confusing numbers as each worker reports. We could probably use https://github.com/yob/puma-plugin-statsd to replace the thread_info (EM) metrics
  • Starting EM in a different thread means it may not get shutdown properly but given it just does metrics now I'm not sure this really matters anymore? (If you don't spawn the new thread it blocks the main thread)
  • Are we really benefitting from preload_app!? - we don't really need fast startup time for workers as we probably won't cycle them and it means we have to disconnect the DB after loading the models. It might be simpler to just start the CC after forking which should hopefully mean the DB only connects after forking
  • Some config options like DB connection pool will need expressing/updating to be per-worker to avoid over-allocating

Hopefully this helps at least have a play around with it. I'm pretty stuck on what to do about metrics - maybe try and uniquely identify a worker and put that identifier in the metric? We use Telegraf for collecting our StatsD metrics which supports tagging but not every StatsD implementation does so that may not be a great option.

cc: @sethboyles

Still needs a lot of work:
* Not feature flagged (probably should be)
* Periodic metrics are added on every worker but most are gauges about
the individual thread (see vitals) so will clash and cause confusing
numbers as each worker reports
* Starting EM in a different thread means it _may_ not get shutdown
properly but given it just does metrics now I'm not sure this really
matters anymore?
* Are we really benefitting from preload_app! - we don't really need
fast startup time for workers as we probably won't cycle them and it
means we have to disconnect the DB after loading the models. It might be
simpler to just start the CC after forking which should hopefully mean
the DB only connects after forking
* Some config options like DB connection pool will need
expressing/updating to be per-worker to avoid over-allocating
@sethboyles
Copy link
Member

sethboyles commented Sep 26, 2022

We released Puma as an experimental option in https://github.com/cloudfoundry/capi-release/releases/tag/1.138.0. This spike was extremely helpful getting us to that point--thanks!

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.

3 participants