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

Print warning when running one-worker cluster #2565

Merged
merged 4 commits into from Mar 9, 2021
Merged

Print warning when running one-worker cluster #2565

merged 4 commits into from Mar 9, 2021

Conversation

CGA1123
Copy link
Contributor

@CGA1123 CGA1123 commented Mar 6, 2021

Running Puma in cluster-mode is likely a misconfiguration in most
scenarios.

Cluster mode has some overhead of running an addtional 'control' process
in order to manage the cluster. If only running a single worker it is
likely not worth paying that overhead vs running in single mode with
additional threads instead.

There are some scenarios where running cluster mode with a single worker
may still be warranted and valid under certain deployment scenarios, see
the linked issue for details.

From experience at work, we were able to migrate our Rails applications
from single worker cluster to single-mode and saw a reduction of RAM
usage of around ~15%.

Closes #2534

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] or [ci skip] to the pull request title.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

Copy link
Member

@dentarg dentarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to be able to silence the warning, as outlined in #2534, there might be users who really want to run Puma this way.

lib/puma/cluster.rb Outdated Show resolved Hide resolved
@dentarg
Copy link
Member

dentarg commented Mar 6, 2021

A test would also be good, so future refactoring can be done with confidence. Looks like test_run_hooks_and_exception could provide some inspiration for that.

@CGA1123
Copy link
Contributor Author

CGA1123 commented Mar 6, 2021

@dentarg Thanks for taking a look!

What would be the preferred way to suppress this warning? Via an environment variable? A new configuration option?

@dentarg
Copy link
Member

dentarg commented Mar 6, 2021

I would go for environment variable

@nateberkopec
Copy link
Member

Env variable seems like the wrong choice @dentarg - wouldnt you want this warning to be supressed "for all time"? Env variables mean that other devs in dev environments, for example, would have to also have the env variable set.

@dentarg
Copy link
Member

dentarg commented Mar 6, 2021

@nateberkopec you're right, that makes sense. I was thinking of other types of software where ENVs is often used to hide warnings.

I guess an ENV is good if you're in a situation where you can't change the config for some reason but you have control over the environment. (Might not be a common thing with this case)

@CGA1123
Copy link
Contributor Author

CGA1123 commented Mar 7, 2021

In that case then should this be implemented as a new method in Puma::DSL or as a new keyword argument to Puma::DSL#workers? or something else?

One potential issue with adding it as a new keyword argument to #workers is that many people may be relying on the WEB_CONCURRENCY environment variable to manage worker counts.
So it may be more appropriate to add it as a new DSL method? silence_single_worker_warning or something like that?

@dentarg
Copy link
Member

dentarg commented Mar 7, 2021

So it may be more appropriate to add it as a new DSL method? silence_single_worker_warning or something like that?

Sounds good to me 👍 The warning could inform about this setting

Running Puma in cluster-mode is likely a misconfiguration in most
scenarios.

Cluster mode has some overhead of running an addtional 'control' process
in order to manage the cluster. If only running a single worker it is
likely not worth paying that overhead vs running in single mode with
additional threads instead.

There are some scenarios where running cluster mode with a single worker
may still be warranted and valid under certain deployment scenarios, see
the linked issue for details.

From experience at work, we were able to migrate our Rails applications
from single worker cluster to single-mode and saw a reduction of RAM
usage of around ~15%.

Closes #2534
@CGA1123
Copy link
Contributor Author

CGA1123 commented Mar 8, 2021

I'm not sure that the failing test is related to these changes, seems like a flake!

@nateberkopec nateberkopec merged commit 81d26e9 into puma:master Mar 9, 2021
@CGA1123 CGA1123 deleted the ISSUE-2534/one-worker-warning branch March 9, 2021 16:16
JuanitoFatas pushed a commit to JuanitoFatas/puma that referenced this pull request Sep 9, 2022
* Print warning when running one-worker cluster

Running Puma in cluster-mode is likely a misconfiguration in most
scenarios.

Cluster mode has some overhead of running an addtional 'control' process
in order to manage the cluster. If only running a single worker it is
likely not worth paying that overhead vs running in single mode with
additional threads instead.

There are some scenarios where running cluster mode with a single worker
may still be warranted and valid under certain deployment scenarios, see
the linked issue for details.

From experience at work, we were able to migrate our Rails applications
from single worker cluster to single-mode and saw a reduction of RAM
usage of around ~15%.

Closes puma#2534

* Remove link to issue

* Add #silence_single_worker_warning option

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

Successfully merging this pull request may close these issues.

Log warning when max worker count is 1
3 participants