Fixes #33214: Set minimum Puma threads equal to maximum puma threads … #984
Conversation
|
The alternative would be to drop the minimum parameter and always force |
manifests/params.pp
Outdated
| @@ -63,7 +63,7 @@ | |||
| $foreman_service = 'foreman' | |||
| $foreman_service_ensure = 'running' | |||
| $foreman_service_enable = true | |||
| $foreman_service_puma_threads_min = 0 | |||
| $foreman_service_puma_threads_min = 16 | |||
There was a problem hiding this comment.
How about we make it dynamic and set it to undef:
| $foreman_service_puma_threads_min = 16 | |
| $foreman_service_puma_threads_min = undef |
Then use $min = pick($foreman::foreman_service_puma_threads_min, $foreman::foreman_service_puma_threads_max). That way users don't need to specify it twice if they want them to match.
You probably also need to change the type to Optional[Integer[0]].
Also an installer migration that clears the answer if they match will be nice for users.
There was a problem hiding this comment.
I like the dynamic aspect from a code perspective. I am wondering if this will confuse users of the installer who look at it and see that the minimum is set to nothing. Asking out loud if that will lead to support posts or users to think that the minimum is therefore 0 if blank.
There was a problem hiding this comment.
We can document this in the parameter help that the default (undef) means matching max threads.
There was a problem hiding this comment.
Ehh, I'll update the documentation of the parameter and let that suffice.
|
Do you recall if min threads set to 1 also works around the leak? Or does matching mean it doesn't rotate out old threads which means the leak doesn't show up? |
I do not know if 1 also works around the leak. I can test this. My understanding was, that by setting |
|
Another note, the Satellite performance team found generally better overall performance when |
|
Sounds like we should start with |
|
Failing tests are getting fixed with theforeman/foreman-packaging#7001 |
| @@ -2,6 +2,6 @@ | |||
| User=<%= scope['foreman::user'] %> | |||
| Environment=FOREMAN_ENV=<%= scope['foreman::rails_env'] %> | |||
| Environment=FOREMAN_HOME=<%= scope['foreman::app_root'] %> | |||
| Environment=FOREMAN_PUMA_THREADS_MIN=<%= scope['foreman::foreman_service_puma_threads_min'] %> | |||
| Environment=FOREMAN_PUMA_THREADS_MIN=<%= @min_puma_threads %> | |||
There was a problem hiding this comment.
it feels a little funny to me that puppet's erb templating supports @variable in the local scope from where the template is called. is it more efficient in case the scope object is truly huge? consistently using the same syntax would be nicer to look at otherwise.
|
I read the previous note about a migration that would clear the value of the minimum if it matches the maximum. 👍 to that. I think we should also in that migration clear the value of the minimum if it used the old default of 0. this would alleviate the memory leaks for the vast majority of users not using any non-default params, while still allowing users to set a non-default value of the minimum if they really desire it. |
That's over here -- theforeman/foreman-installer#713 |
…by default This has been shown to prevent memory leaks that can appear in Foreman when running under Puma. This only changes the default to match and users must still be aware of this potential when changing the tunings.
There was a problem hiding this comment.
Overall this looks good to me. The new param documentation mentions the leaks and performance benefits of using min = max. It's possible there is some unknown future condition where separately configuring the min to be different from the max would still be desirable.
|
@ehelms I went with 'bug' for now. It's not so much adding a feature that we traditionally think of as an 'enhancement', and the intent behind it is very much 'fix a thing that wasn't working the way it was intended' |
…by default
This has been shown to prevent memory leaks that can appear in Foreman
when running under Puma. This only changes the default to match and
users must still be aware of this potential when changing the tunings.
Some guidance from Satellite performance team -- https://github.com/RedHatSatellite/satellite-performance-tuning/blob/devel/docs/satellite-configuration-tuning.rst#puma-tuning
Some info from upstream Puma -- puma/puma#2665
There is most likely one or more memory leaks within the application itself leading to this issue. This alleviates the issue for now by pre-allocating the needed memory and preventing spin-up and spin-down of threads that exacerbates the memory leak.