nextcloud / helm

A community maintained helm chart for deploying Nextcloud on Kubernetes.
GNU Affero General Public License v3.0
334 stars 269 forks source link

PHP Memory Limit should be configured properly by default #582

Closed Heracles31 closed 4 months ago

Heracles31 commented 5 months ago

Describe your Issue

While running some OCC commands, Nextcloud complained that the PHP Memory Limit was lower than the recommended 512M.

I searched all over the place about how to fix that. None of the solution found was applicable : --Editing config file is useless because they are overwritten whenever the pod is moved / recreated --No changes in the /config directory did it (that one survives because it is on a PVC) --No changes in .htaccess did it (also on PVC..)

I tried to use the nextcloud.extraEnv field in my values.yaml but it either breaks the chart and does not deploy (can not convert yaml to json ; template deployment, line 104) or it get deployed but without any effect. Tried to configure both NEXTCLOUD_MEMORY_LIMIT and PHP_MEMORY_LIMIT.

It is only while reading about issue #97 here that I managed to get a solution. By creating an extra php config file with a name starting with zz and adjusting the memory limit, I now have that proper limit.

Because that limit of 512M is a minimum for every deployment and Nextcloud complains about it all the time, I think it would be important for the chart to : 1-Itself by default enforce a minimum of 512M (default is 128M as of now) 2-offer a simple way to re-configure that value (as well as maximum upload file probably...)

Logs and Errors

Warning: Failed to set memory limit to 0 bytes (Current memory usage is 2097152 bytes) in Unknown on line 0 The current PHP memory limit is below the recommended value of 512MB.

Describe your Environment

Needed to add this to get it fixed :

nextcloud:
  phpConfigs:
    zz-memory_limit.ini: |-
      upload_max_filesize=5G
      memory_limit=512M

Additional context, if any

provokateurin commented 5 months ago

This is a problem of https://github.com/nextcloud/docker though, it should be fixed in the image and not the Helm chart.

jessebot commented 5 months ago

I was actually just looking at the PHP settings in https://github.com/nextcloud/helm/issues/327 😁 ! So, yeah, as @provokateurin said, that memory limit is actually configured upstream in the docker repo they've linked here: https://github.com/nextcloud/docker/blob/064069b3060092b64f6fc1e418932fa215d57364/Dockerfile-alpine.template#L82 and also here: https://github.com/nextcloud/docker/blob/064069b3060092b64f6fc1e418932fa215d57364/Dockerfile-debian.template#L21

~You could maybe also override it nextcloud.extraEnv in your values.yaml like this (instead of how you did it above~

Update: Scratch that above statement about the env vars. I missed you explaining you tried that prior, and I just tested it and it didn't work. sorry about that!

Heracles31 commented 5 months ago

Agree that the original limit of 128 is from the docker image but still, the need to adjust that setting to different values remains. There are cases where people put it at 1,024.

As such, the second need remains : the need to fix that value from the chart to something different. And once it is an option, the chart will be able to compensate for that problem in the upstream docker image.

So 2nd point would be the reason to do it and 1st point would be an important side benefit of doing it.

jessebot commented 4 months ago

I understand your point, but I think @provokateurin's original comment still stands in that we should actually update it upstream, if its affecting everyone:

https://github.com/nextcloud/docker/blob/65138b6d22bec1ac15e2f0f125426290640bb97a/Dockerfile-alpine.template#L89-L100

We could also have some docs on updating that PHP memory limit in our README, and we'd could review that PR if you or others would like to submit that. I guess alternatively, we could have the php memory limit be an actual option in the chart like:

nextcloud:
  php:
    memory_limit: 512M

That avoids the needs to do:

nextcloud:
  phpConfigs:
    zz-memory_limit.ini: |-
      upload_max_filesize=5G
      memory_limit=512M

But then we do kinda run the risk of scope creep on this one, as there's a lot of PHP options people could need. @provokateurin and @tvories what do yall think? :) Am I overthinking this? (if the answer is yes, that's totally fine!)

Heracles31 commented 4 months ago

Agree about the fact that so many php options exist and as such, may need to be adjusted. That rabbit hole can be pretty deep...

If you choose to only document this fix, it would be important to mention that the "zz-" prefix for the file is important and is to ensure the file will be read last, so its value will overwrite whatever other values have been configured previously.

provokateurin commented 4 months ago

@jessebot I agree with you, either fix it upstream or use a config file. Adding extra options for all php options is not helpful.

jessebot commented 4 months ago

Okie dokie, submitted https://github.com/nextcloud/helm/pull/595 :)

jessebot commented 4 months ago

@Heracles31 since #595 was merged, this was auto-closed, but if you'd to adjust the wording in the docs, please feel free to let us know or submit a new PR. For actually adjusting the default memory limit for php.ini, you'll need to go to the nextcloud/docker repo, but don't worry, they are friendly :)