cloudfoundry / php-buildpack

A Cloud Foundry Buildpack for PHP.
Apache License 2.0
142 stars 346 forks source link

Adapt PHP configuration to fit memory limit of an application automatically #232

Closed dmikusa closed 5 years ago

dmikusa commented 6 years ago

Currently, the PHP buildpack really makes no attempt to configure HTTPD, Nginx and PHP-FPM such that resource usage does not go above the memory limit assigned to a given application.

If you look at PHP & PHP-FPM, this is where 90%+ of the assigned memory will be used. Right now, we default the memory_limit to 128M in php.ini [1] and we set the max number of workers in php-fpm.conf to 5 [2]. This leaves us with a default memory liability of roughly 128M * 5 + HTTPD/Nginx + PHP-FPM itself. From what I've seen, this is far higher than most memory limits (128 - 256M) being assigned to PHP workloads, which means given the current default configurations there's a potential for PHP to consume more memory than expected and crash the container.

Currently, we put the responsibility of adjusting PHP's configuration on the user. It's easy enough with the PHP buildpack to override these default PHP & PHP-FPM settings so that you can scale the app to use as much memory as is required for your workload. Having said that, it's unlikely new users or casual users of the buildpack will go through this process and will just rely on the buildpack defaults.

This issue is to open a discussion about how we can improve the buildpack so that it will do a better job adapting the PHP configuration to make use of the memory limit assigned to an application by default.

cf-gitbot commented 6 years ago

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/152486505

The labels on this github issue will be updated when the story is started.

sclevine commented 6 years ago

@idoru note ^

dmikusa commented 6 years ago

Some challenges I see with this:

dgodd commented 6 years ago

This seems very sensible to me, a few thoughts

Luckily @sclevine already has plans for how to pass memory requirements for supply buildpacks (and thus APM agents in the future) through to memory calculations

Re: splitting memory limits and num workers, the one thing I think we shouldn't do is compilcated analysis similar to the java buildpack. I'm inclined to run the most complicated php app we can find (eg. wordpress) and make it a proxy for a default memory per worker. Assuming that this would change the current memory_limit, we should also make that VERY clear to people during staging for several versions of the buildpack.

dmikusa commented 6 years ago

Re: splitting memory limits and num workers, the one thing I think we shouldn't do is compilcated analysis similar to the java buildpack.

I'm on board with this. Simple is good, plus we offer ways easy ways to override and customize so users can make it do what they want. This will just let us focus on a better out-of-the-box experience.

I'm inclined to run the most complicated php app we can find (eg. wordpress) and make it a proxy for a default memory per worker. Assuming that this would change the current memory_limit, we should also make that VERY clear to people during staging for several versions of the buildpack.

This sounds reasonable to me as well.

Here's what I found with a little searching for memory recommendations on Wordpress & Drupal.

https://www.drupal.org/docs/7/system-requirements/php#memory https://codex.wordpress.org/Editing_wp-config.php#Increasing_memory_allocated_to_PHP

It seems like 64M might be a good, conservative choice. That's half of the default memory limit we have now which is 128M.


Additional question, how do you feel we should handle the case where the memory limit is insufficient for the defaults? For example, if we assign a 128M memory limit to the app and we use 32M as overhead for HTTPD/Nginx, that leaves 96M for PHP. If we use a default PHP memory limit of 64M, that leaves us with a worker pool of one worker which doesn't seem very useful.

I can think of a couple ways we could handle this:

With the auto correct option, I do think there would still need to be some minimum acceptable memory limit for a PHP application. Probably 96M. In that case, you'd have 32M for HTTPD/Nginx + 64M for PHP which would give you a worker pool of 4 w/16M per process memory limit or 5 w/12M per process limit. That is probably about as low as we'd want the buildpack to go. At that point, we'd probably need to log an error and tell the customer to manually configure if they want to proceed.

@dgodd - Thoughts?

dgodd commented 6 years ago

That all sounds very reasonable. My only query is on whether we choose workers over memory or memory over workers? My inclination would have been the opposite of your suggestion and to choose to have two 48M workers, rather than 5 12M workers. However I do not have enough knowledge of PHP to in any way trust my gut on these choices.

@sclevine @idoru - Thoughts?

dmikusa commented 6 years ago

For what it's worth, I opted for more workers because that allows you to process more simultaneous requests. My understanding is that one worker will process one request (to a PHP file, HTTPD/Nginx directly handle requests for static files w/out involving PHP), then when it's done it will move on to the next request. If you have two workers, I feel like that would be pretty limiting. I'm not sure there's a good choice though, 12M per PHP process is also pretty limiting.

Perhaps the deciding factor here could be how things breakdown? A test might be to push with the two configurations and see what happens if you exceed the memory limit for PHP or if you try to process a lot of simultaneous requests. If we have to configure it in an way that's not ideal, I guess the best case would be for the limitations of that configuration to be obvious. That way the user will know they need to adjust things or increase the memory limit.

astrieanna commented 6 years ago

Is this still under active consideration? It hasn't been commented on in most of a year, and it doesn't appear ready to be worked on as a story int he backlog.

dmikusa commented 6 years ago

@astrieanna - I opened this to get feedback mostly. Right now what the buildpack does is pretty simple. There are more complicated things that we could be doing (see brainstorming above ^^).

I haven't heard a lot of feedback on this though. It'd be nice if we could get this in front of a wider audience to perhaps hear what others thinks. Are there problems with what we do now? Does it work for most cases? Are there things people routinely change, customize or override with how the buildpack handles this now? Are there any concrete use cases that people could share? etc..

I will post something to the mailing list and see if that draws any additional feedback.

dwillist commented 5 years ago

Closing this due to inactivity, it seems like this discussion could be continued with relation to the php-web-cnb here. But please feel free to re-open.