Fixes #28955 - Add puma configuration tuning options #790

sthirugn · 2020-01-23T22:23:48Z

~~I am creating this PR sooner with DO NOT MERGE tag to let everyone know how I am proceeding.~~
Any feedback is appreciated.

About the configuration file puma_production.erb

I have so far picked only 2 parameters - threads and workers. More will be picked in the future if need be. See sample config for more info.

Pending work in this PR

Externalize the parameters in the template (as environment variables?)
workers=4 is my test configuration. I need to test further to tune this to an appropriate default value.

ekohl · 2020-01-23T22:36:53Z

manifests/config.pp

+    ensure  => directory,
+  }
+
+  file { "${::foreman::app_root}/config/puma/production.rb":


I think this should actually live in http://github.com/theforeman/foreman. Since it's Ruby, it can use ENV and we can set that in our systemd overrides. This way container deployments can also benefit from this.

Maybe like creating puma/production.rb in https://github.com/theforeman/foreman/tree/develop/config. If so, where will the template (templates/puma_production.erb) live?

I think you can avoid to template it. In pure Ruby:

# Configure "min" to be the minimum number of threads to use to answer # requests and “max” the maximum. # # The default is "0, 16". # threads ENV.fetch('FOREMAN_PUMA_THREADS_MIN', 0).to_i, ENV.fetch('FOREMAN_PUMA_THREADS_MAX, 16).to_i # === Cluster mode === # How many worker processes to run. # # The default is "0". # workers ENV.fetch('FOREMAN_PUMA_WORKERS', 4).to_i

Untested, but I suspect this will work. Then you should be able to set these environment variables via systemd overrides.

@ekohl I tried the configuration you gave above in /usr/share/foreman/config/puma/production.rb

I provided overrides in file /usr/share/foreman-installer/modules/foreman/templates/foreman.service-overrides.erb like the following:

[Service] User=<%= scope['foreman::user'] %> Environment=FOREMAN_ENV=<%= scope['foreman::rails_env'] %> Environment=FOREMAN_HOME=<%= scope['foreman::app_root'] %> <% if scope['foreman::foreman_service_bind'] -%> Environment=FOREMAN_BIND=<%= scope['foreman::foreman_service_bind'] %> <% end -%> Environment=FOREMAN_PORT=<%= scope['foreman::foreman_service_port'] %> Environment=FOREMAN_PUMA_WORKERS=8 Environment=FOREMAN_PUMA_THREADS_MIN=4 Environment=FOREMAN_PUMA_THREADS_MAX=12

Initially I thought it was working, but later figured that it is not really picking up the environment variable values defined. I am looking into this further.

Just adding notes from discussion, I tested this method out and it worked great for me. We will want a Redmine issue since we'll be adding a baseline configuration file to Foreman core codebase as well as this overrides file.

@ehelms thank you for the test. Added redmine issue and updated this PR and the foreman-installer PR. Added link below in the comment section.

sthirugn · 2020-02-06T22:29:54Z

This is ready for review in conjunction with ~~theforeman/foreman-installer#464~~ theforeman/foreman#7421

ekohl

You should also add the values to manifests/init.pp.

ehelms · 2020-02-10T20:07:27Z

Something to consider is the db pool size we used given the threading nature of Puma. We likely want to tune this based on these settings.

https://devcenter.heroku.com/articles/concurrency-and-database-connections

sthirugn · 2020-02-11T19:44:46Z

You should also add the values to manifests/init.pp.

Thank you, @ekohl I added params to init.pp and confirmed that I can override them through custom-hiera.yaml parameters like the following:

# cat /etc/foreman-installer/custom-hiera.yaml | tail -n 3
foreman::foreman_puma_threads_min: 1
foreman::foreman_puma_threads_max: 14
foreman::foreman_puma_workers: 6

ekohl · 2020-02-11T19:56:27Z

It shouldn't be needed to specify it via hiera and should show up as --foreman-foreman-puma-threads-min.

manifests/init.pp

sthirugn · 2020-02-11T21:33:00Z

It shouldn't be needed to specify it via hiera and should show up as --foreman-foreman-puma-threads-min.

Good to know this.

sthirugn · 2020-02-11T21:42:56Z

Something to consider is the db pool size we used given the threading nature of Puma. We likely want to tune this based on these settings.

https://devcenter.heroku.com/articles/concurrency-and-database-connections

I am testing with different configurations. I will post here when I have more info.

sthirugn · 2020-02-12T23:09:36Z

Updated default puma workers to 2 as per theforeman/foreman#7421 (comment)

ehelms

The failures are unrelated and same as master. @ekohl good with this?

ekohl · 2020-02-19T12:53:30Z

manifests/init.pp

@@ -193,6 +193,12 @@
 #
 # $cors_domains::                 List of domains that show be allowed for Cross-Origin Resource Sharing. This requires Foreman 1.22+
 #
+# $foreman_service_puma_threads_min::     Minimum number of threads for puma. Relevant only when Puma service is used and ignored when Passenger is used.


Bit of a bikeshedding: should this leave out puma in the variable name? The use of puma vs Puma is also a bit inconsistent.

Suggested change

# $foreman_service_puma_threads_min:: Minimum number of threads for puma. Relevant only when Puma service is used and ignored when Passenger is used.

# $foreman_service_threads_min:: Minimum number of threads for Puma. Relevant only when Puma service is used and ignored when Passenger is used.

I think calling out puma explicitly in the variable name will help the users under it as puma threads vs. just foreman threads.

@ekohl Updated lowecase/uppercase inconsistencies, let me know what you think

ekohl

Other than the last item, I think this is good to be merged.

ekohl · 2020-02-19T16:50:49Z

manifests/init.pp

+# $foreman_service_puma_threads_max::     Maximum number of threads for Puma. Relevant only when Puma service is used and ignored when Passenger is used.
+#
+# $foreman_service_puma_workers::         Number of workers for Puma. Relevant only when Puma service is used and ignored when Passenger is used.
+


The empty line here breaks some tools. The comment block must be attached to the class.

Suggested change

#

Good catch, fixed it now. thanks!

ekohl · 2020-02-19T17:17:15Z

Tests are still running. The person merging this should make sure to squash the commits.

ehelms · 2020-02-19T20:19:40Z

Same failures as master, mergin

ekohl · 2020-02-19T20:25:25Z

@sthirugn the dash in the commit message appears to be needed for the PR processor. Now it wasn't checked. I also didn't check during the review, but let's be mindful about this in the future. Now it didn't hard fail because this repo doesn't hard require a Redmine issue.

sthirugn · 2020-02-19T20:32:04Z

@sthirugn the dash in the commit message appears to be needed for the PR processor. Now it wasn't checked. I also didn't check during the review, but let's be mindful about this in the future. Now it didn't hard fail because this repo doesn't hard require a Redmine issue.

ack thank you for pointing this out.

theforeman-bot added Needs testing Not yet reviewed labels Jan 23, 2020

ekohl reviewed Jan 23, 2020

View reviewed changes

sthirugn force-pushed the puma_config branch from b53179d to 3ff4a58 Compare February 6, 2020 22:27

sthirugn changed the title ~~[DO NOT MERGE] Initial commit for puma configuration~~ Added puma configuration Feb 6, 2020

sthirugn mentioned this pull request Feb 6, 2020

Added puma configuration theforeman/foreman-installer#464

Closed

sthirugn mentioned this pull request Feb 7, 2020

Fixes #28955 - Added puma configuration theforeman/foreman#7421

Merged

sthirugn force-pushed the puma_config branch 2 times, most recently from dae357d to a772fdc Compare February 10, 2020 19:12

ekohl requested changes Feb 10, 2020

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Feb 10, 2020

sthirugn changed the title ~~Added puma configuration~~ Fixes #28955 - Added puma configuration Feb 10, 2020

theforeman-bot added Needs re-review and removed Waiting on contributor labels Feb 11, 2020

sthirugn changed the title ~~Fixes #28955 - Added puma configuration~~ [DO NOT MERGE] Fixes #28955 - Added puma configuration Feb 11, 2020

ekohl reviewed Feb 11, 2020

View reviewed changes

manifests/init.pp Outdated Show resolved Hide resolved

manifests/init.pp Outdated Show resolved Hide resolved

sthirugn changed the title ~~[DO NOT MERGE] Fixes #28955 - Added puma configuration~~ Fixes #28955 - Added puma configuration Feb 12, 2020

sthirugn force-pushed the puma_config branch from 3daab0f to bfa5a63 Compare February 18, 2020 21:01

ehelms approved these changes Feb 19, 2020

View reviewed changes

theforeman-bot removed the Needs re-review label Feb 19, 2020

ekohl reviewed Feb 19, 2020

View reviewed changes

sthirugn force-pushed the puma_config branch from bfa5a63 to db1cfca Compare February 19, 2020 16:47

ekohl reviewed Feb 19, 2020

View reviewed changes

sthirugn force-pushed the puma_config branch from db1cfca to 6ae8e1e Compare February 19, 2020 16:53

ekohl approved these changes Feb 19, 2020

View reviewed changes

sthirugn force-pushed the puma_config branch 3 times, most recently from afa2997 to d1ceb50 Compare February 19, 2020 17:24

Fixes #28955 Add puma configuration tuning options

1e1a1e0

sthirugn force-pushed the puma_config branch from d1ceb50 to 1e1a1e0 Compare February 19, 2020 17:40

ehelms merged commit 94e03d5 into theforeman:master Feb 19, 2020

ekohl added the Enhancement label Feb 19, 2020

ekohl changed the title ~~Fixes #28955 - Added puma configuration~~ Fixes #28955 - Add puma configuration tuning options Feb 19, 2020

sthirugn deleted the puma_config branch February 19, 2020 20:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #28955 - Add puma configuration tuning options #790

Fixes #28955 - Add puma configuration tuning options #790

sthirugn commented Jan 23, 2020 •

edited

Loading

ekohl Jan 23, 2020

sthirugn Jan 23, 2020

ekohl Jan 23, 2020

sthirugn Feb 6, 2020

ehelms Feb 6, 2020

sthirugn Feb 6, 2020

sthirugn commented Feb 6, 2020 •

edited

Loading

ekohl left a comment

ehelms commented Feb 10, 2020

sthirugn commented Feb 11, 2020

ekohl commented Feb 11, 2020

sthirugn commented Feb 11, 2020

sthirugn commented Feb 11, 2020

sthirugn commented Feb 12, 2020

ehelms left a comment

ekohl Feb 19, 2020

sthirugn Feb 19, 2020

sthirugn Feb 19, 2020

ekohl left a comment

ekohl Feb 19, 2020

sthirugn Feb 19, 2020

ekohl commented Feb 19, 2020

ehelms commented Feb 19, 2020

ekohl commented Feb 19, 2020

sthirugn commented Feb 19, 2020

	# $foreman_service_puma_threads_min:: Minimum number of threads for puma. Relevant only when Puma service is used and ignored when Passenger is used.
	# $foreman_service_threads_min:: Minimum number of threads for Puma. Relevant only when Puma service is used and ignored when Passenger is used.

Fixes #28955 - Add puma configuration tuning options #790

Fixes #28955 - Add puma configuration tuning options #790

Conversation

sthirugn commented Jan 23, 2020 • edited Loading

About the configuration file puma_production.erb

Pending work in this PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthirugn commented Feb 6, 2020 • edited Loading

ekohl left a comment

Choose a reason for hiding this comment

ehelms commented Feb 10, 2020

sthirugn commented Feb 11, 2020

ekohl commented Feb 11, 2020

sthirugn commented Feb 11, 2020

sthirugn commented Feb 11, 2020

sthirugn commented Feb 12, 2020

ehelms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl commented Feb 19, 2020

ehelms commented Feb 19, 2020

ekohl commented Feb 19, 2020

sthirugn commented Feb 19, 2020

sthirugn commented Jan 23, 2020 •

edited

Loading

sthirugn commented Feb 6, 2020 •

edited

Loading