Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configure the pm.max_children with env var #1766

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tclavier
Copy link
Contributor

@tclavier tclavier commented Jun 7, 2022

No description provided.

@tclavier tclavier force-pushed the patch-2 branch 2 times, most recently from b4ca46a to bcd3994 Compare July 23, 2022 16:36
@PrivatePuffin
Copy link

We also got some user(s) requesting this... would be great if it can be added :)

@PrivatePuffin
Copy link

@J0WI This is a pretty simple fix, can we please expedite this?

@J0WI
Copy link
Contributor

J0WI commented Aug 30, 2022

I think it's easier if you just mount your custom configuration to /usr/local/etc/php-fpm.d/. This is more generic and works for any config.

@tclavier
Copy link
Contributor Author

@jowi not very easy in kube environment

@PrivatePuffin
Copy link

I think it's easier if you just mount your custom configuration to /usr/local/etc/php-fpm.d/. This is more generic and works for any config.

That can be done, but at this rate we would also need to completely maintain this as well.
On top of that, the max_children setting does not even seem to be in-line with what is adviced by Nextcloud themselves in the first place...

So at the very least this setting needs to be increased to be in-sync with the nextcloud docs, at which point we can just as well just merge this and have it done in a more flexible maner right away

@J0WI
Copy link
Contributor

J0WI commented Sep 1, 2022

not very easy in kube environment

I'm not sure what you mean. For Nextcloud and the other server configs you need a persistent filesystem anyway.
Isn't the concept of k8s that you should rather use multiple container/machine instances to balance the load than a single big one?

On top of that, the max_children setting does not even seem to be in-line with what is adviced by Nextcloud themselves in the first place...

Nextcloud advises to use a value depending on your hardware. There is no meaningful default.

@PrivatePuffin
Copy link

I'm not sure what you mean. For Nextcloud and the other server configs you need a persistent filesystem anyway.

Well, normally this would be done in a configmap, so it can be done. But to be frank, even Nextcloud doesn't offer this option in their Helm chart, so it offloads more work onto upstreams.

Isn't the concept of k8s that you should rather use multiple container/machine instances to balance the load than a single big one?

Yes this can ofcoarse be done, but it's rather heavy-handed to scale the complete container just for this setting. and it depends a bit on the quality of the Helm chart on how they support this, but that is indeed not your problem to worry about ;-)

Nextcloud advises to use a value depending on your hardware. There is no meaningful default.

Shoot, I might've based that on a older resource.
Still, as Nextcloud advices it to be tweaked (just as the other variables exposed here), I think all these settings should be threated the same and exposed, in line with the Nextcloud advice to tweak them as users see fit.

To be clear: I don't expect all "convienient" settings to be exposed this way, as that would be weird. But at least exposing the settings nextcloud advices to tweak seems like the best way. It also prevents users from asking "how can I tweak this, because Nextcloud advices me to tweak it"....

@PrivatePuffin
Copy link

PrivatePuffin commented Oct 28, 2022

@J0WI It's a shame that php-docker points to upstream php though (which is also not 100% correct, as they don't mind adding other settings).
However, we've multiple users reporting timeouts on the nextcloud fpm version due to the low default of 5 by now...

Imho this should be solved for as many users as possible, so that's why i'm hesitent to make it a downstream modification

@stavros-k
Copy link
Contributor

Since upstream php-docker won't accept the change, can we revisit this PR please?

@J0WI
Copy link
Contributor

J0WI commented Nov 25, 2022

Does this even work? The FPM config usually lives in /usr/local/etc/php-fpm.d not /usr/local/etc/php/conf.d.

@a3linux
Copy link

a3linux commented Feb 27, 2023

Could I know if I want to add my override config for pm.max_children, what should I mount to the nextcloud:fpm docker container?
For example, -v /path-to/my.ini:/usr/loca/etc/php-fpm.d/my.ini,
is this docker bind volume will load my configuration and override the pm.max_children = 5 in /usr/loca/etc/php-fpm.d/www.conf, which pm.max_children = 5, 5 is too small for production deployment.

@J0WI
Copy link
Contributor

J0WI commented Mar 4, 2023

They are applied alphabetically. You can prefix your file with zz or something.

@PrivatePuffin
Copy link

They are applied alphabetically. You can prefix your file with zz or something.

Awesome info!

@@ -106,6 +107,7 @@ RUN { \
echo 'memory_limit=${PHP_MEMORY_LIMIT}'; \
echo 'upload_max_filesize=${PHP_UPLOAD_LIMIT}'; \
echo 'post_max_size=${PHP_UPLOAD_LIMIT}'; \
echo 'pm.max_children = ${PHP_PM_MAX_CHILDREN}'; \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FPM parameters don't belong in the php.ini path (PHP_INI_DIR). FPM stuff needs to go into the php-fpm.d/ path.

We don't currently customize anything FPM related in the Dockerfile but it can probably be modeled after the PHP_INI_DIR stuff. Perhaps /usr/local/etc/php-fpm.d/nextcloud.ini or something like that

As currently coded, at best this parameter gets ignored.

@escoand
Copy link

escoand commented May 25, 2023

As a workaround I use this in a docker-compose file:

  nextcloud:
    image: library/nextcloud:26.0.1-fpm-alpine
    ...
    command: >-
      sh -c '
        {
          echo pm=dynamic;
          echo pm.max_children=120;
          echo pm.start_servers=12;
          echo pm.min_spare_servers=6;
          echo pm.max_spare_servers=18;
        } >> /usr/local/etc/php-fpm.d/www.conf;
        exec php-fpm
      '

@rugk
Copy link
Contributor

rugk commented Jul 21, 2023

@escoand @joshtrichards Don't do this please. This may cause upgrade issues like #2005.

Longer: You are overriding the entrypoint/command of Nextcloud. As such, on any upgrade the upgrade script may not run/trigger anymore and you may get errors, because you try to start your NC without upgrading to the new version.
One error can be the PHP version does not match anymore as linked above, because you have a newer php version

      # workaround for php-fpm issue https://github.com/nextcloud/docker/pull/1766
      - type: bind
        source: ./php-fpm-config/zz-prevent-servererror-onload.conf
        target: /usr/local/etc/php-fpm.d/zz-prevent-servererror-onload.conf:z   

(:z is just relevant for SELinux systems)

…and add the config lines from above in the corresponding file.

Better mount the config file in there.

I also just noticed this as I fall into the same trap. 😜

@escoand
Copy link

escoand commented Aug 21, 2023

@rugk I try to avoid additional files and just have it in the docker-compose. To circumvent this problem you could use:

  nextcloud:
    image: library/nextcloud:26.0.1-fpm-alpine
    ...
    command: >-
      sh -c '
        {
          echo pm=dynamic;
          echo pm.max_children=120;
          echo pm.start_servers=12;
          echo pm.min_spare_servers=6;
          echo pm.max_spare_servers=18;
        } >> /usr/local/etc/php-fpm.d/www.conf;
        exec /entrypoint.sh php-fpm
      '

@tclavier
Copy link
Contributor Author

Waiting docker-library/php#1452 to move configuration entries in good directory

@tclavier tclavier force-pushed the patch-2 branch 2 times, most recently from 04bd05c to 7dc3c98 Compare October 26, 2023 11:29
Signed-off-by: Thomas Clavier <tom@tcweb.org>
This reverts commit 4ce703e.

Signed-off-by: Thomas Clavier <tom@tcweb.org>
Signed-off-by: Thomas Clavier <tom@tcweb.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement feature: auto config (environment variables) Auto configuring via environment variables
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants