cloudControl / buildpack-php

cloudControl PHP buildpack
Apache License 2.0
13 stars 18 forks source link

[custom-buildpack] Container memory limit not respected when using specific PHP version #37

Closed holtkamp closed 8 years ago

holtkamp commented 9 years ago

With a composer.json with:

"require": {
        "php-64bit": "5.6.10"
    }

and a container configuration set to 256M of available memory per container, phpinfo() shows:

memory_limit 128M

This can be manipulated by using a .buildpack/php/conf/php.ini file with a setting like:

memory_limit = 256M

Not sure whether this last configuration should be allowed to be set by a 'user'.

TooAngel commented 9 years ago

The memory limitations are done on the container level, so if you configure a too high php memory_limit you still get only the memory you have configured via your cli and will run into a Out Of Memory error.

Vertical scaling of your container also provides you with more php processes. So it still can make sense to keep the memory_limit = 128M while you scale up. Up to now I want to stick to the default php configuration values, mainly because it is an easy rule 'You get the default php configuration and can change whatever you want'.

But I also get your point to have it dependent on the container size. My best idea so far is to have it at the beginning of your application, which gives you the best flexibility.

holtkamp commented 9 years ago

Yeah indeed, the out-of-memory error was the cause of me looking into this.

I understand the approach, but this might confuse users who solely use the dashboard / do not use the .buildpack capabilities and therefore do not override php settings. Should not be a problem as long as the custom-buildpack remains the non-default approach.

TooAngel commented 9 years ago

After thinking about it and discussing it internally, we will stick to the php default configuration. Any value or dynamic configuration can run into trouble:

memory_limit can be adapted via PHP_INI_ALL, so it can be easily adapted via the .user.ini without additional buildpack knowledge.

holtkamp commented 9 years ago

Ok, but maybe the dashboard should mention this as a notice. When a "less technical" colleague ramps up a deployment by adding some memory and instances, to deal with expected extra load / install a CMS that needs > 128M / whatever reason, this can lead to confusion, right?

pst commented 8 years ago

Right now we think it's best to enforce specific decisions from customers on the memory settings.