search

geerlingguy / ansible-role-php

Ansible Role - PHP
https://galaxy.ansible.com/geerlingguy/php/
MIT License
496 stars 441 forks source link

Configure extra fpm pools #302

Closed rsicart closed 3 years ago

rsicart commented 4 years ago

Allow to configure extra fpm pools with a generic template.

You can find an example of php_fpm_extra_pools usage in default vars. pool_name and pool_listen are mandatory.

rsicart commented 4 years ago

What do you think about that PR @geerlingguy ?

ncharlot commented 4 years ago

Hi @rsicart ! We follow definitely the same features :)

I think it would be better to use a common var for all pools with default to the www basic one as it exists now. And we could share the same template too.

The default vars should be like:

php_fpm_pools:
  - name: www
    listen: 127.0.0.1
    user: www-data
    ...

And maybe a template attribute as @geerlingguy do with Nginx role:

php_fpm_pools:
  - name: www
    template: "{{ php_fpm_pool_template }}"
    listen: 127.0.0.1
    user: www-data
    ...
rsicart commented 4 years ago

Hahah I see @ncharlot ! I have often cases where I need several pools :) And well, I implemented fpm extra pools separated from default www pool because, in case that festure is not interesting for @geerlingguy , I can easily keep my fork up to date with this repo without conflicts. It’s less intrusive. Nevertheless, I agree with you: fpm pool setup should be the same logic for 1 and for N. And of course, allowing the user to choose the template!

ncharlot commented 4 years ago

Yes for performance and readability purpose we for use to split apps parts to differents pools.

rsicart commented 4 years ago

I rewrote the PR to use a single list of pools, as @ncharlot proposed: php_fpm_pools.

You can find an example of php_fpm_pools usage in default vars, and the documentation in README.md

Nevertheless, I kept variables php_fpm_pool_user and php_fpm_pool_group as they were.

What do you think about that @geerlingguy ?

ncharlot commented 4 years ago

Ahah good job @rsicart!

rsicart commented 4 years ago

Hi there @geerlingguy , do you have some feedback about this PR please? Thanks in advance

rsicart commented 4 years ago

Rebase on master

JKetelaar commented 4 years ago

Would love to see this feature introduced as well!

stale[bot] commented 3 years ago

This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the PR will be closed in another 30 days. Thank you for your contribution!

Please read this blog post to see the reasons why I mark pull requests as stale.

JKetelaar commented 3 years ago

Could you review this @geerlingguy?

stale[bot] commented 3 years ago

This issue is no longer marked for closure.

EdinTC commented 3 years ago

This feature would be an great addition!

ncharlot commented 3 years ago

@geerlingguy I'm sorry to insist but adding such a common feature in the main repo seems to be nice rather in a fork.

geerlingguy commented 3 years ago

One thing I definitely would not like to do is break backwards compatibility. Probably 99.9% (maybe 100%) of this roles current users are happy with the single defined www pool, as the use case for this role is most frequently "one server, one webserver, one site" (or maybe a few / dozen / more, but still behind one pool).

This PR breaks that backwards compatibility. At minimum I'd rather have this be opt-in functionality, and have it be able to use the normal / standard way it uses now, and you could add additional pools (or named pools and disable the standard pool the role currently sips with) if desired through the new variable.

rsicart commented 3 years ago

Or we could release a major version, for example 5.0.0, including a changelog if needed :)

geerlingguy commented 3 years ago

@rsicart - The problem is, this is a feature that I would guess relatively few people (of all this role's users) use, and I don't want to change the default way the entire role works to cater to the minority of the role's users.

More specifically, I have never used more than the single default node pool, and that's why this PR has sat for a long time. I would rather not merge something into a role I use a lot that will require me to change dozens of playbooks just to keep my own role working with my own playbooks ;)

So it's less of an ask, and more of a demand.

rsicart commented 3 years ago

No worries, I completely understand your point of view. I just hope you do not refuse only because you'll have to update your excellent book Ansible for Devops ! ;)

@all What do you think about something more like this commit, when this branch was less intrusive (before PR rewrite on June 26th)? https://github.com/geerlingguy/ansible-role-php/commit/e732bafeac010874003a7512458b4f46f5b815a5 Would that be acceptable for all of us?

ncharlot commented 3 years ago

To preserve BC, maybe we should just keep the current vars for values of the 1st pool:

php_fpm_listen: "127.0.0.1:9000"
php_fpm_listen_allowed_clients: "127.0.0.1"
php_fpm_pm_max_children: 50
php_fpm_pm_start_servers: 5
php_fpm_pm_min_spare_servers: 5
php_fpm_pm_max_spare_servers: 5

# PHP-FPM pool configuration.
php_fpm_pools:
 - pool_name: www
    pool_template: www.conf.j2
    pool_listen: "{{ php_fpm_listen }}"
    pool_listen_allowed_clients: "{{ php_fpm_listen_allowed_clients }}"
    pool_pm: dynamic
    pool_pm_max_children: {{ php_fpm_pm_max_children }}
    pool_pm_start_servers: {{ php_fpm_pm_start_servers }}
    pool_pm_min_spare_servers: {{ php_fpm_pm_min_spare_servers }}
    pool_pm_max_spare_servers: {{ php_fpm_pm_max_spare_servers }}
    pool_pm_max_requests: 500
rsicart commented 3 years ago

Good point! That seems a good compromise.

mcdir commented 3 years ago

MR https://github.com/geerlingguy/ansible-role-php/pull/321 with a good compromise ;)

geerlingguy commented 3 years ago

This has been merged and will be in the 4.6.0 release of the role. Thanks for your patience!

rsicart commented 3 years ago

Thanks to you and all people who collaborated along this PR :)

ncharlot commented 3 years ago

Congrats @rsicart!

tjoelsson commented 3 years ago

I'm a little confused as to why this PR was merged. As mentioned in comments, it's not backwards compatible. All FPM settings are set to default values unless you update configuration.

geerlingguy commented 3 years ago

@tjoelsson - Shoot! I didn't even think about that when I merged... can you open a new issue so we can figure out a better way of handling backwards compatibility for those upgrading this role?

tjoelsson commented 3 years ago

@geerlingguy Sure, here is the issue https://github.com/geerlingguy/ansible-role-php/issues/329

geerlingguy commented 3 years ago

@tjoelsson - Thanks; if nobody can help with getting the backwards compatibility sorted soon, I'm willing to revert this PR and release a new version of the role, then delay adding it back in until a compatibility layer is added back in.

rsicart commented 3 years ago

What about what @ncharlot proposed in this comment ? https://github.com/geerlingguy/ansible-role-php/pull/302#issuecomment-741609370