Fixes #30789 - Set DB pool size dynamically #882
Conversation
|
Note I didn't test this at all. This is here mostly for review and discussion about the approach. Also makes it easy to test. |
| @@ -35,6 +35,12 @@ | |||
| mode => '0640', | |||
| } | |||
|
|
|||
| if $foreman::use_foreman_service { | |||
| $db_pool = max($foreman::db_pool, $foreman::foreman_service_puma_threads_max) | |||
There was a problem hiding this comment.
I think to be fully safe, this has to also consider dynflow_pool_size if for some reason that was set higher than the max puma threads we would need to set the db_pool to match it.
There was a problem hiding this comment.
should that be max(db_pool, puma, dynflow) or max(db_pool, puma+dynflow)?
There was a problem hiding this comment.
What I've understood from @adamruzicka is that dynflow uses sequel and thus its own pool. However, it copies the config from Rails. Perhaps also the pool but I'm sure he can correct me.
I think the pool size was actually unused btw.
There was a problem hiding this comment.
dynflow_pool_size shouldn't be used for anything since we moved sidekiq and even when it was previously used, dynflow knew how to increase the number of connections in the pool to work no matter which value was configured there. We can safely ignore it here
There was a problem hiding this comment.
Right now, dynflow_pool_size determines the concurrency for workers (https://github.com/theforeman/puppet-foreman/blob/master/manifests/dynflow/worker.pp#L25). If this concurrency value was set higher than the db_pool configured in the Rails database.yml, would that imply that a worker could need more connections than we configure it for @adamruzicka ?
There was a problem hiding this comment.
Ah, forgot it is being used for that.
With Dynflow/dynflow#362 the worker calculates how many connections it should need in the worst case and then it reconfigures its pool to have max($calculated, $db_pool_size_from_database.yml) connections available
There was a problem hiding this comment.
Ahh, so behind the scenes the worker could calculate a value that exceeds the db_pool configured through Rails, but the workers are using Sequel anyway so they are not bound by the db_pool defined in Rails?
There was a problem hiding this comment.
but the workers are using Sequel anyway so they are not bound by the db_pool defined in Rails?
Not exactly. They are not bound by db_pool, but not because they are using Sequel.
The worker boots, takes AR configuration (what is set in db_pool), calculates how much connections it thinks will need, picks the bigger number, overrides the original AR configuration with the new value and uses that. Then it takes the new configuration for AR and derives configuration for Sequel from it.
Every thread in the Puma worker can open a database connection. This means it needs to be taken into account to avoid exhasuting the pool.
ekohl
force-pushed
the
30789-set-db-pool
branch
from
6cf95c6 to
5e65343
Compare
Every thread in the Puma worker can open a database connection. This means it needs to be taken into account to avoid exhasuting the pool.