Fixes #33277: Change Puma default workers to 1.5 * CPU, max threads to 5 #986
Conversation
ehelms
changed the title
|
We could borrow from Gitlab and do the min of 1.5 * CPU and (memory - 1.5).
…On Tue, Aug 17, 2021, 6:04 PM Ewoud Kohl van Wijngaarden < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In manifests/params.pp
<#986 (comment)>
:
> @@ -64,8 +64,8 @@
$foreman_service_ensure = 'running'
$foreman_service_enable = true
$foreman_service_puma_threads_min = undef
- $foreman_service_puma_threads_max = 16
- $foreman_service_puma_workers = 2
+ $foreman_service_puma_threads_max = 5
+ $foreman_service_puma_workers = $facts['processors']['count'] * 1.5
Should we have some upper bound as well? In the past we've seen that too
many workers could also easily consume way too much memory.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#986 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACHT46Z2B6T7O44HXPH2TTT5LMFFANCNFSM5CKU5GIA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
What's memory here? Memory in GB? |
Correct |
|
I am working on a version with memory considered, while I fix up test this would look something like: |
|
Something else: we talked about making things more dynamic. If we do this in |
That does make this more robust to memory or CPU changes which is a nice feature. Users would get an automatically scaling number of workers. The only downside is users understanding that in the installer when looking to the installers answer file or the help text for knowing what the currently configured value is. They have to know to go look at the systemd file or use systemctl to print out that info. While not a blocker, it just adds knowledge complexity. So two workflows:
|
|
That's why I said "consider". I'm fine with leaving that out for now. However, we should try to find an answer. Perhaps do a poll in the community? |
spec/classes/foreman_spec.rb
Outdated
| @@ -6,6 +6,7 @@ | |||
| let(:facts) { facts } | |||
| let(:params) { {} } | |||
| let(:apache_user) { facts[:osfamily] == 'Debian' ? 'www-data' : 'apache' } | |||
| let(:puma_workers) { [facts[:processors]['count'] * 1.5, ((facts[:memory]['system']['total_bytes'] * 1024 * 1024 * 1024) - 1.5)].min.floor } | |||
There was a problem hiding this comment.
How about we override the processors and memory facts instead? I think if you modify line 6 to be something like this it should work (but I may have missed some braces somewhere):
| let(:puma_workers) { [facts[:processors]['count'] * 1.5, ((facts[:memory]['system']['total_bytes'] * 1024 * 1024 * 1024) - 1.5)].min.floor } | |
| let(:facts) { override_facts(facts, processors: { count: 3 }, memory: { system: { total_bytes: SOMETHING }}) } |
|
Thinking more, if we did set this optional it would allow things like
installer tuning profiled to set this value via hiera given where those
tunings exist in the heirarchy.
…On Wed, Aug 18, 2021, 1:42 PM Ewoud Kohl van Wijngaarden < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In spec/classes/foreman_spec.rb
<#986 (comment)>
:
> @@ -3,7 +3,7 @@
describe 'foreman' do
on_supported_os.each do |os, facts|
context "on #{os}" do
- let(:facts) { facts }
+ let(:facts) { override_facts(facts, processors: { count: 3 }, memory: { system: { total_bytes: 10737418240}}) }
In Ruby it's valid to write:
⬇️ Suggested change
- let(:facts) { override_facts(facts, processors: { count: 3 }, memory: { system: { total_bytes: 10737418240}}) }
+ let(:facts) { override_facts(facts, processors: { count: 3 }, memory: { system: { total_bytes: 10_737_418_240}}) }
I think that makes it easier to spot it's 10 GB.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#986 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACHT42JPFVFY4RLHUNDONDT5PWITANCNFSM5CKU5GIA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
|
This now moves Puma workers to be optional, and if not defined then it uses the prior mentioned calculation to determine what to set this as. This will allow more dynamic setting of this value based on system resources and allow tuning profiles to set these values. |
This resets the foreman_server_puma_workers value to pick up the newest default which is undef. The change from theforeman/puppet-foreman#986 makes the default a dynamically calculated value that should provide better out of the box experience for users.
This resets the foreman_server_puma_workers value to pick up the newest default which is undef. The change from theforeman/puppet-foreman#986 makes the default a dynamically calculated value that should provide better out of the box experience for users.
|
Installer migration -- theforeman/foreman-installer#714 |
The Puma documentation recommends for CRuby based installations to start with number of CPUs times 1.5 for workers and defaults to a maxium of 5 threads when using CRuby. The previous maximum threads of 16 was based on an incorrect reading of the Puma documentation and is only the default on non-CRuby Ruby.
This resets the foreman_server_puma_workers value to pick up the newest default which is undef. The change from theforeman/puppet-foreman#986 makes the default a dynamically calculated value that should provide better out of the box experience for users.
This initially sets the Puma tuning values based on the same ideas from theforeman/puppet-foreman#986 statically calculated for each of the CPU and memory values that exist for the various profiles.
|
We discussed this some more and one idea that came up was making This allows users to start with a baseline that is based on the recommendations from Puma documentation. Then as we gain knowledge about what works best for Katello specifically, those values can be defined in the tuning profiles and will override the module defaults here unless the user has already supplied their own values. The idea is that the tuning profiles can also start with initial values that are based on the same calculations used in the module defaults, and can be updated in the future as our knowledge improves. An installer migration would reset manually values, with a release note explaining the change. Users of the individual puppet modules would still have any of their settings persisted unless they clear them to opt-in to the new defaults defined here. Users of foreman-installer would have their settings reset at first, but they could be manually set again. If they are not manually set again, foreman-installer users would get settings from their applied tuning profile. As the tuning profiles get updated based on new data, they would automatically receive the performance improvement benefits unless they have opted out by supplying their own values. |
|
I feel like that would be premature optimization now. At this point we're not really certain what the best practice will be. If we migrate all our settings from one style to the other and then find out it doesn't work well it's going to be painful. Having a static value does mean you can use --(full-)help to see the value which can make it easier to admins to know what's going to happen on their system. Currently we have a technical limitation where we can't modify answers with our tuning, but perhaps there are ways around that. We'll also need to think about how we want to really implement auto tuning. There are several ways I can think of. So for now I'd like to table that a bit, especially since I'm really not sure if we'll end up tuning threads dynamically. |
| @@ -4,4 +4,4 @@ Environment=FOREMAN_ENV=<%= scope['foreman::rails_env'] %> | |||
| Environment=FOREMAN_HOME=<%= scope['foreman::app_root'] %> | |||
| Environment=FOREMAN_PUMA_THREADS_MIN=<%= scope['foreman::config::min_puma_threads'] %> | |||
| Environment=FOREMAN_PUMA_THREADS_MAX=<%= scope['foreman::foreman_service_puma_threads_max'] %> | |||
| Environment=FOREMAN_PUMA_WORKERS=<%= scope['foreman::foreman_service_puma_workers'] %> | |||
| Environment=FOREMAN_PUMA_WORKERS=<%= scope['foreman::config::puma_workers'] %> | |||
There was a problem hiding this comment.
This is generated from the foreman::config scope so I think this is equal:
| Environment=FOREMAN_PUMA_WORKERS=<%= scope['foreman::config::puma_workers'] %> | |
| Environment=FOREMAN_PUMA_WORKERS=<%= @puma_workers %> |
The same is true for min_puma_threads.
This resets the foreman_server_puma_workers value to pick up the newest default which is undef. The change from theforeman/puppet-foreman#986 makes the default a dynamically calculated value that should provide better out of the box experience for users.
…uma_threads_max if set to default value This resets the foreman_server_puma_workers value to pick up the newest default which is undef. The change from theforeman/puppet-foreman#986 makes the default a dynamically calculated value that should provide better out of the box experience for users. This resets the value of foreman_service_puma_threads_max to the new default value coming from puppet-foreman of 5. The original default of 16 has been determined to likely be too high for Ruby MRI based installations and resetting to the recommended default by Puma should result in less wasted resources. Users can opt to increase this value based on their environments.
…uma_threads_max if set to default value This resets the foreman_server_puma_workers value to pick up the newest default which is undef. The change from theforeman/puppet-foreman#986 makes the default a dynamically calculated value that should provide better out of the box experience for users. This resets the value of foreman_service_puma_threads_max to the new default value coming from puppet-foreman of 5. The original default of 16 has been determined to likely be too high for Ruby MRI based installations and resetting to the recommended default by Puma should result in less wasted resources. Users can opt to increase this value based on their environments.
…uma_threads_max if set to default value This resets the foreman_server_puma_workers value to pick up the newest default which is undef. The change from theforeman/puppet-foreman#986 makes the default a dynamically calculated value that should provide better out of the box experience for users. This resets the value of foreman_service_puma_threads_max to the new default value coming from puppet-foreman of 5. The original default of 16 has been determined to likely be too high for Ruby MRI based installations and resetting to the recommended default by Puma should result in less wasted resources. Users can opt to increase this value based on their environments. (cherry picked from commit 4c129ef)
…uma_threads_max if set to default value This resets the foreman_server_puma_workers value to pick up the newest default which is undef. The change from theforeman/puppet-foreman#986 makes the default a dynamically calculated value that should provide better out of the box experience for users. This resets the value of foreman_service_puma_threads_max to the new default value coming from puppet-foreman of 5. The original default of 16 has been determined to likely be too high for Ruby MRI based installations and resetting to the recommended default by Puma should result in less wasted resources. Users can opt to increase this value based on their environments. (cherry picked from commit 4c129ef)
The Puma documentation recommends for MRI based installations to
start with number of CPUs times 1.5 and defaults to 5 on Ruby MRI.
The previous maximum threads of 16 was based on an incorrect reading
of the Puma documentation and is only the default on non-MRI Ruby.
I am recommending we make these default changes (and consider resetting existing installations that match the old defaults by way of an installer migration) based upon some of the following info: