Skip to content

Conversation

@rcdailey
Copy link
Contributor

@rcdailey rcdailey commented Dec 6, 2018

A new Docker environment variable named PHP_MEMORY_LIMIT has been introduced
which allows the PHP memory_limit setting to be controlled without mounting
any volumes from the host. The default memory limit is still 512M if this
variable is not specified.

Tested on 14.0 apache.

@rcdailey rcdailey force-pushed the configurable-memory-limit branch 2 times, most recently from 5b0db6b to d7f4888 Compare December 11, 2018 00:13
A new Docker environment variable named `PHP_MEMORY_LIMIT` has been introduced
which allows the PHP `memory_limit` setting to be controlled without mounting
any volumes from the host. The default memory limit is still `512M` if this
variable is not specified.

Signed-off-by: Robert Dailey <[email protected]>
@rcdailey rcdailey force-pushed the configurable-memory-limit branch from d7f4888 to f988290 Compare December 12, 2018 14:57
@rcdailey
Copy link
Contributor Author

@J0WI I saw you posted on the php library github page about this. Seems like you'd prefer the generic upstream solution, which I agree with, but my concern is that may not happen for a long time. Would you be against this as a stopgap solution for now? Really all I'm doing here is allowing an existing override of a specific PHP setting to be configurable from outside of the Dockerfile.

Thoughts?

@J0WI
Copy link
Contributor

J0WI commented Dec 14, 2018

I'd prefer to use upstream recommendations: https://hub.docker.com/_/php#example
If environment variables are supported upstream, we'll support them too.

Btw. why do you need to change the memory_limit? For large instances it's recommend to use a pool of many containers and balancing the load:
https://github.com/nextcloud/documentation/blob/9919a3fd3d16099e84358f73a36988b441191301/admin_manual/installation/deployment_recommendations.rst#large-enterprises-and-service-providers

@rcdailey
Copy link
Contributor Author

I'd prefer to use upstream recommendations: https://hub.docker.com/_/php#example
If environment variables are supported upstream, we'll support them too.

My understanding is that the upstream instructions are for downstream Dockerfiles. But they don't really care how you build the php.ini. That's what I think, anyway. Someone already previously submitted a patch to specify memory_limit, I am just making that input configurable. So in my mind, the technical decision has already been made in regards to allowing a specific PHP setting to be manually specified by your Dockerfile.

Again: I fully agree, a flexible upstream solution would be ideal. But who knows how long we'll be waiting on that? It could be a year! Given that the "kitchen sink" solution takes time, thought, design, etc. I think this stop-gap solution is an ideal for now. Memory limit in PHP is arguably the most common setting people want to specify when configuring the Nextcloud instance.

Btw. why do you need to change the memory_limit? For large instances it's recommend to use a pool of many containers and balancing the load

I have Nextcloud installed in my home lab. I do not manage this for a business or anything. I literally have this thing running on my Intel NUC. The only 2 users using this thing are me and my wife, so we can each store our own files and family photos. So no, I don't want to do any load balancing or multi-instance set up. It's overkill and complex for my use case.

The reason I need to bump the memory limit is two fold:

  • occ runs out of memory when running preview generation for my family photo library, because it's got hundreds of thousands of photos in a directory structure. Bumping memory_limit to 2G has fixed this issue.
  • Nextcloud itself runs out of memory (I can see this in the Settings > Logs).

I don't think there's anything wrong with increasing the memory available to Nextcloud. This is a simple problem that needs a simple solution; not some over complicated multi-instance setup that's impractical for most homelab users.

@J0WI
Copy link
Contributor

J0WI commented Dec 14, 2018

@tilosp thoughts?

@rcdailey
Copy link
Contributor Author

Having used this change for a few days now, I think this isn't working 100%. And when I say it isn't working, I mean that even the original change, which forces memory limit to 512 via /usr/local/etc/php/conf.d/memory-limit.ini isn't working. At least, it seems to have no effect on Nextcloud itself. To add to the confusion, there's multiple places that seem to specify PHP settings:

  • /var/www/html/.htaccess: They are present here, but changing them seems to be non-permanent, because over time they seem to get replaced by some default values. Not sure what triggers this.
  • /var/www/html/.user.ini: Seems to duplicate the settings found in .htaccess, but also seem to be reset back to defaults at some point. Not sure what is modifying these files.
  • /usr/local/etc/php/conf.d/memory-limit.ini: In master, right now, this file is created and set to 512M. My change makes this configurable from docker-compose.yml, and I set mine to 2G, but this doesn't seem to affect Nextcloud itself when the 2 files above get reset back to 512M.
  • /usr/local/etc/php/php.ini: One can mount their own php.ini here (one is not provided by default). I am not sure if settings specified here take precedence over the 3 files above, but I have seen in various other issues that it has been recommended to set PHP settings globally here.

So really it's become increasingly unclear to me where one should configure PHP settings. I think the fact that you see them all over the place, as shown above, doesn't help. There's no clear, self-evident method of specifying some of these basic settings. I also have not found any official documentation on this. And sadly google search results on this issue are all over the place. The way you configure Nextcloud & PHP change drastically depending on how you are installing it, it seems. So you can't tell if certain recommendations will work with the docker container or not.

At this point, I'm not sure if I should decline my PR, or try to make some sense of this and correct the problem. Would love some help from the developers. I'd love to help make a useful change here to improve the issue, so let me know if I can do anything to help. I've hit a wall on trying to figure this out myself.

@J0WI
Copy link
Contributor

J0WI commented Dec 16, 2018

memory_limit can be set anywhere, so your php.ini may get overwritten by .htaccess (apache) or .user.ini (fpm).

@rcdailey
Copy link
Contributor Author

Unfortunately that link doesn't explain the precedence of these files, but with some searching I found this. If that's correct, then using php.ini doesn't make a difference, since php_value in apache takes precedence over those settings. Furthermore, since .htaccess gets reverted at non-deterministic points in time, editing this file is not an option either.

So maybe we have a much bigger issue here: Inability to override PHP settings. Granted, I have never done PHP development before, so it could very well be there just needs to be more documentation.

What is the official way to override PHP settings via the docker container?

@rcdailey
Copy link
Contributor Author

Looking further, I see this comment: nextcloud/server#4769 (comment)

Basically they say not to modify .htaccess or .user.ini for modifying PHP settings. And instead to make them part of "apache config", whatever that means (not that familiar with Apache). Given that all of this is isolated in a docker container, it looks like some extra functionality is needed to support overriding these settings in a way idiomatic to Docker itself.

Perhaps now we've come full circle. Looks like this isn't going to be solved by asking the upstream PHP image maintainers to add support for this, because even if they did, those PHP settings would be overridden by Nextcloud's .htaccess and .user.ini files, by design.

So instead, I think this Docker image needs a specialized solution to modifying these files post-upgrade path. After an upgrade, apply user-specified PHP settings that overwrite the default ones that Nextcloud inserts into those files.

Am I on the right track? @J0WI @tilosp

@J0WI
Copy link
Contributor

J0WI commented Dec 16, 2018

Thanks for sharing your analysis.
All files except the ones specified in upgrade.exclude are updated (overwritten) by NextCloud sources.
Since memory_limit is already defined in the upstream .htaccess and .user.ini I'd consider this as a duplicate of nextcloud/server#121.

@rcdailey
Copy link
Contributor Author

I guess it's a duplicate if you prefer the solution to be within the server project itself. However, there's still some (IMHO better) solutions that make things more docker-friendly, such as environment variables that result in those files being modified programmatically by the entrypoint script.

My immediate concern is that issue has been open since 2016. 2 years, no fix. Will it be another 2 years? Could get a working docker-only solution in the meantime, right?

@rcdailey
Copy link
Contributor Author

@J0WI Hey, just following up. It seems you've hit a dead end on convincing the PHP library developers.

Given that, I think we have two choices:

  1. Convince the Nextcloud server guys to remove memory_limit from .htaccess, and instead rely on php.ini configuration.
  2. Have some container-specific logic to programmatically modify the .htaccess and user.ini settings to adjust memory limits.

The long-term, most correct option is probably the first one. However, it's also the most difficult. Because they probably have those settings there for a good reason. Given that, and the short-term demand for configurability of memory limit by many users over the past few years, I recommend the second option as, at the very least, a stopgap solution until the first solution can be implemented.

At this point, this issue has slowed down, but I want to work on a solution to this problem with urgency if possible. I'm willing to contribute any changes needed. Who's doing the work isn't the issue here. I think it's just getting the developers engaged and in agreement on the ideal short-term and long-term solutions.

I'm not able to contribute any meaningful implementation to support this without it being rejected at this point. Can we come to a consensus on this issue so I can stop manually building Nextcloud docker images from forks?

@J0WI J0WI added the upstream label Jan 21, 2019
@J0WI
Copy link
Contributor

J0WI commented Jan 21, 2019

+1 for fixing this upstream.
Do you have a concept to implement the second one without patching the upstream sources?

@rcdailey
Copy link
Contributor Author

@J0WI for option two, I'd probably use a primitive approach where I grep the memory limit value in the existing .htaccess and user.ini files to modify it.

Another option is to replace those two files with built in versions that have those settings removed and instead either require users to mount their own php.ini or define a system to procedurally generate it based on environment variables. I'm open to other ideas.

@J0WI
Copy link
Contributor

J0WI commented Jan 21, 2019

Modifying these files will cause the integrity check to fail:
https://docs.nextcloud.com/server/15/admin_manual/issues/code_signing.html

@rcdailey
Copy link
Contributor Author

I've modified them locally and haven't run into issues. What am I missing?

@J0WI
Copy link
Contributor

J0WI commented Jan 21, 2019

@rcdailey
Copy link
Contributor Author

I haven't done any manual scans, so maybe that's why? I did remove the settings in my fork of Nextcloud server and had no errors upgrading from 15.0.0 to 15.0.2

@J0WI
Copy link
Contributor

J0WI commented Feb 2, 2019

nextcloud/server#13990 (comment): If you are using the Apache variant, you can just insert a custom memory_limit on the very end of your .htaccess.

@rcdailey
Copy link
Contributor Author

rcdailey commented Feb 28, 2019

@J0WI Looks like we can close this in favor of nextcloud/server#14430

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants