From 3d6743639cce925be3fcb26421e9b640e28b045c Mon Sep 17 00:00:00 2001 From: David Zuelke Date: Tue, 16 Jan 2024 16:56:51 -0500 Subject: [PATCH 1/8] handle invalid memory_limit suffix --- bin/util/autotune.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/bin/util/autotune.php b/bin/util/autotune.php index b4b8f4a2f1..85f48cc0b4 100755 --- a/bin/util/autotune.php +++ b/bin/util/autotune.php @@ -3,13 +3,16 @@ function stringtobytes($amount) { // convert "256M" etc to bytes - switch(strtolower(substr($amount, -1))) { + switch($suffix = strtolower(substr($amount, -1))) { case 'g': $amount = (int)$amount * 1024; case 'm': $amount = (int)$amount * 1024; case 'k': $amount = (int)$amount * 1024; + break; + case !is_numeric($suffix): + fprintf(STDERR, "WARNING: ignoring invalid suffix '%s' in 'memory_limit' value '%s'\n", $suffix, $amount); default: $amount = (int)$amount; } From d9a3f5dad8a81a31735d3f7cf9c88af553fa8e52 Mon Sep 17 00:00:00 2001 From: David Zuelke Date: Tue, 16 Jan 2024 16:59:07 -0500 Subject: [PATCH 2/8] fix null deprecation warning if fpm config missing in autotune.php --- bin/util/autotune.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/bin/util/autotune.php b/bin/util/autotune.php index 85f48cc0b4..26a6099817 100755 --- a/bin/util/autotune.php +++ b/bin/util/autotune.php @@ -23,6 +23,7 @@ function stringtobytes($amount) { // (it may have been set in the FPM config or config include) function get_fpm_memory_limit($fpmconf, $startsection = "global") { if(!is_readable($fpmconf)) { + // an include may point to a file that does not exist, that's fine return array(); } $fpm = parse_ini_string("[$startsection]\n".file_get_contents($fpmconf), true); // prepend section from parent so stuff is parsed correctly in includes that lack a leading section marker @@ -54,7 +55,10 @@ function get_fpm_memory_limit($fpmconf, $startsection = "global") { $opts = getopt("y:t:"); -$limits = get_fpm_memory_limit(isset($opts["y"]) ? $opts["y"] : null); +$limits = array(); +if(isset($opts["y"])) { + $limits = get_fpm_memory_limit($opts["y"]); +} if( !$limits /* .user.ini memory limit is ignored if one is set via FPM */ && From cecfe2aabd23a7362a4c1411afed72b2c3f2caad Mon Sep 17 00:00:00 2001 From: David Zuelke Date: Tue, 16 Jan 2024 17:00:08 -0500 Subject: [PATCH 3/8] make RAM_LIMIT arg optional in autotune.php --- bin/util/autotune.php | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/bin/util/autotune.php b/bin/util/autotune.php index 26a6099817..f87f3e82cb 100755 --- a/bin/util/autotune.php +++ b/bin/util/autotune.php @@ -53,7 +53,22 @@ function get_fpm_memory_limit($fpmconf, $startsection = "global") { return $retval; } -$opts = getopt("y:t:"); +$opts = getopt("y:t:", array(), $rest_index); +$argv = array_slice($argv, $rest_index); +$argc = count($argv); +if($argc < 1 || $argc > 2) { + fprintf(STDERR, + "Usage:\n". + " %s [options] []\n\n", + basename(__FILE__) + ); + fputs(STDERR, + "Options:\n". + " -y PHP-FPM config file to read 'memory_limit' settings from\n". + " -t Dir to read '.user.ini' with 'memory_limit' settings from\n\n" + ); + exit(2); +} $limits = array(); if(isset($opts["y"])) { @@ -79,13 +94,16 @@ function get_fpm_memory_limit($fpmconf, $startsection = "global") { } $limit = ini_get('memory_limit'); -$ram = stringtobytes($argv[$argc-2]); // second to last arg is the available memory -$max_ram_string = $argv[$argc-1]; -$max_ram = stringtobytes($max_ram_string); // last arg is the maximum RAM we're allowed +$ram = stringtobytes($argv[0]); // first arg is the available memory + +if(isset($argv[1])) { // optional second arg is the maximum RAM we're allowed + $max_ram_string = $argv[1]; + $max_ram = stringtobytes($max_ram_string); -if($ram > $max_ram) { - $ram = $max_ram; - file_put_contents("php://stderr", "Limiting to $max_ram_string Bytes of RAM usage\n"); + if($ram > $max_ram) { + $ram = $max_ram; + file_put_contents("php://stderr", "Limiting to $max_ram_string Bytes of RAM usage\n"); + } } // assume 64 MB base overhead for web server and FPM, and 1 MB overhead for each worker From e676235902853f88fc05138e27392750a82b89d9 Mon Sep 17 00:00:00 2001 From: David Zuelke Date: Tue, 30 Jan 2024 20:26:59 +0100 Subject: [PATCH 4/8] Human-readable RAM limit output --- bin/util/autotune.php | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/bin/util/autotune.php b/bin/util/autotune.php index f87f3e82cb..7c17cdcec6 100755 --- a/bin/util/autotune.php +++ b/bin/util/autotune.php @@ -19,6 +19,16 @@ function stringtobytes($amount) { return $amount; } +function bytestostring($amount) { + $suffixes = array('K', 'M', 'G', 'T', 'P', 'E'); + $suffix = ''; + while($suffixes && $amount % 1024 == 0) { + $amount /= 1024; + $suffix = array_shift($suffixes); + } + return sprintf("%d%s", $amount, $suffix); +} + // if given, parse FPM configs as well to figure out the memory limit // (it may have been set in the FPM config or config include) function get_fpm_memory_limit($fpmconf, $startsection = "global") { @@ -102,7 +112,7 @@ function get_fpm_memory_limit($fpmconf, $startsection = "global") { if($ram > $max_ram) { $ram = $max_ram; - file_put_contents("php://stderr", "Limiting to $max_ram_string Bytes of RAM usage\n"); + fprintf(STDERR, "Limiting to %s Bytes of RAM usage\n", bytestostring($ram)); } } From e640a6e0d84dd54cce50f648a65a2c44df4b0791 Mon Sep 17 00:00:00 2001 From: David Zuelke Date: Fri, 26 Jan 2024 11:26:43 +0100 Subject: [PATCH 5/8] refactor -t option handling ahead of related changes We need to dump the FPM config for WEB_CONCURRENCY calculation, and so we can re-use a bunch of the logic and make the code a bit neater --- bin/heroku-php-apache2 | 87 ++++++++++++++++++++++-------------------- bin/heroku-php-nginx | 87 ++++++++++++++++++++++-------------------- 2 files changed, 92 insertions(+), 82 deletions(-) diff --git a/bin/heroku-php-apache2 b/bin/heroku-php-apache2 index 98b9c5ac69..7275c06b39 100755 --- a/bin/heroku-php-apache2 +++ b/bin/heroku-php-apache2 @@ -366,6 +366,26 @@ if [[ -n ${httpd_config:-} || ( ${httpd_config:=$(findconfig "$httpd_version" "$ fi httpd_config=$(php_passthrough "$httpd_config") +fpm_pidfile=$(mktemp -t "heroku.php-fpm.pid-$PORT.XXXXXX" -u) +httpd_pidfile=$(httpd -t -D DUMP_RUN_CFG 2> /dev/null | sed -n -E 's/PidFile: "(.+)"/\1/p') # get PidFile location + +# we need to dump the FPM config to fetch possible values for memory_limit in php_value and php_admin_value pool declarations +# parsing this by hand is cumbersome - there might be recursive includes in the config, glob patterns, etc +# also, the FPM behavior isn't quite correct (https://github.com/php/php-src/issues/13249), but might be fixed at some point +# so we use php-fpm -tt to dump the config, and extract the values from there +# we need to ensure the log level is NOTICE, since FPM uses its logging for dumping - but we default to WARNING +# that's why we're making a temporary config with the log level overridden +# (the same temporary file is also used for our own -tt and -tttt config test modes) +fpm_config_tmp=$(mktemp "$fpm_config.XXXXX") +cp "$fpm_config" "$fpm_config_tmp" +trap 'trap - EXIT; rm -f "$fpm_config_tmp"' EXIT +echo -e "\n[global]\nlog_level = notice" >> "$fpm_config_tmp" + +# build command string arrays for PHP-FPM and HTTPD +# we're using an array, because we need to correctly preserve quoting, spaces, etc +fpm_command=( php-fpm --pid "$fpm_pidfile" --nodaemonize -y "$fpm_config_tmp" ${php_config:+-c "$php_config"} ) +httpd_command=( httpd -D NO_DETACH -c "Include $httpd_config" ) + mlib="/sys/fs/cgroup/memory/memory.limit_in_bytes" if [[ -f "$mlib" ]]; then ram="$(cat "$mlib")" @@ -392,52 +412,37 @@ else echo '$WEB_CONCURRENCY env var is set, skipping automatic calculation' >&2 fi -fpm_conftest= -httpd_conftest= -fpm_config_tmp=$fpm_config -case $conftest in - [24]) - # to dump the FPM config, we need to ensure the log level is NOTICE - fpm_config_tmp=$(mktemp "$fpm_config.XXXXX") - cp "$fpm_config" "$fpm_config_tmp" - trap 'trap - EXIT; rm "$fpm_config_tmp"' EXIT - echo -e "\n[global]\nlog_level = notice" >> "$fpm_config_tmp" - ;;& # resume - 1) - fpm_conftest="-t" - httpd_conftest="-t" - ;; - 2) - fpm_conftest="-tt" - httpd_conftest="-t" - ;; - 3) - fpm_conftest="-t" - httpd_conftest="-S" - ;; - 4) - fpm_conftest="-tt" - httpd_conftest="-S" - ;; -esac - -fpm_pidfile=$(mktemp -t "heroku.php-fpm.pid-$PORT.XXXXXX" -u) -httpd_pidfile=$(httpd -t -D DUMP_RUN_CFG 2> /dev/null | sed -n -E 's/PidFile: "(.+)"/\1/p') # get PidFile location - -# build command string arrays for PHP-FPM and HTTPD -# we're using an array, because we need to correctly preserve quoting, spaces, etc -fpm_command=( php-fpm ${fpm_conftest} --pid "$fpm_pidfile" --nodaemonize -y "$fpm_config_tmp" ${php_config:+-c "$php_config"} ) -httpd_command=( httpd ${httpd_conftest} -D NO_DETACH -c "Include $httpd_config" ) - if [[ $conftest ]]; then - echo -e "\n$(basename "$0"): Config testing php-fpm using ${fpm_command[@]}:" >&2 - "${fpm_command[@]}" - echo -e "\n$(basename "$0"): Config testing httpd using ${httpd_command[@]}:" >&2 - "${httpd_command[@]}" + case $conftest in + 1) + fpm_conftest="-t" + httpd_conftest="-t" + ;; + 2) + fpm_conftest="-tt" + httpd_conftest="-t" + ;; + 3) + fpm_conftest="-t" + httpd_conftest="-S" + ;; + *) + fpm_conftest="-tt" + httpd_conftest="-S" + ;; + esac + + echo -e "\n$(basename "$0"): Config testing php-fpm using ${fpm_command[@]} ${fpm_conftest}:" >&2 + "${fpm_command[@]}" ${fpm_conftest} + echo -e "\n$(basename "$0"): Config testing httpd using ${httpd_command[@]} ${httpd_conftest}:" >&2 + "${httpd_command[@]}" ${httpd_conftest} echo -e "\n$(basename "$0"): All configs okay." >&2 exit 0 fi +# we're done dumping stuff; restore original FPM config +cp "$fpm_config" "$fpm_config_tmp" + # make a shared pipe; we'll write the name of the process that exits to it once that happens, and wait for that event below # this particular call works on Linux and Mac OS (will create a literal ".XXXXXX" on Mac, but that doesn't matter). wait_pipe=$(mktemp -t "heroku.waitpipe-$PORT.XXXXXX" -u) diff --git a/bin/heroku-php-nginx b/bin/heroku-php-nginx index a9113307b7..fd656caa9b 100755 --- a/bin/heroku-php-nginx +++ b/bin/heroku-php-nginx @@ -366,6 +366,26 @@ nginx_config=$(php_passthrough "$nginx_config") nginx_main=$(findconfig "$nginx_version" "$bp_dir/conf/nginx/main.conf") +fpm_pidfile=$(mktemp -t "heroku.php-fpm.pid-$PORT.XXXXXX" -u) +nginx_pidfile=$(mktemp -t "heroku.nginx.pid-$PORT.XXXXXX" -u) + +# we need to dump the FPM config to fetch possible values for memory_limit in php_value and php_admin_value pool declarations +# parsing this by hand is cumbersome - there might be recursive includes in the config, glob patterns, etc +# also, the FPM behavior isn't quite correct (https://github.com/php/php-src/issues/13249), but might be fixed at some point +# so we use php-fpm -tt to dump the config, and extract the values from there +# we need to ensure the log level is NOTICE, since FPM uses its logging for dumping - but we default to WARNING +# that's why we're making a temporary config with the log level overridden +# (the same temporary file is also used for our own -tt and -tttt config test modes) +fpm_config_tmp=$(mktemp "$fpm_config.XXXXX") +cp "$fpm_config" "$fpm_config_tmp" +trap 'trap - EXIT; rm -f "$fpm_config_tmp"' EXIT +echo -e "\n[global]\nlog_level = notice" >> "$fpm_config_tmp" + +# build command string arrays for PHP-FPM and Nginx +# we're using an array, because we need to correctly preserve quoting, spaces, etc +fpm_command=( php-fpm --pid "$fpm_pidfile" --nodaemonize -y "$fpm_config_tmp" ${php_config:+-c "$php_config"} ) +nginx_command=( nginx -c "$nginx_main" -g "pid $nginx_pidfile; include $nginx_config;" ) + mlib="/sys/fs/cgroup/memory/memory.limit_in_bytes" if [[ -f "$mlib" ]]; then ram="$(cat "$mlib")" @@ -392,52 +412,37 @@ else echo '$WEB_CONCURRENCY env var is set, skipping automatic calculation' >&2 fi -fpm_conftest= -nginx_conftest= -fpm_config_tmp=$fpm_config -case $conftest in - [24]) - # to dump the FPM config, we need to ensure the log level is NOTICE - fpm_config_tmp=$(mktemp "$fpm_config.XXXXX") - cp "$fpm_config" "$fpm_config_tmp" - trap 'trap - EXIT; rm "$fpm_config_tmp"' EXIT - echo -e "\n[global]\nlog_level = notice" >> "$fpm_config_tmp" - ;;& # resume - 1) - fpm_conftest="-t" - nginx_conftest="-t" - ;; - 2) - fpm_conftest="-tt" - nginx_conftest="-t" - ;; - 3) - fpm_conftest="-t" - nginx_conftest="-T" - ;; - 4) - fpm_conftest="-tt" - nginx_conftest="-T" - ;; -esac - -fpm_pidfile=$(mktemp -t "heroku.php-fpm.pid-$PORT.XXXXXX" -u) -nginx_pidfile=$(mktemp -t "heroku.nginx.pid-$PORT.XXXXXX" -u) - -# build command string arrays for PHP-FPM and Nginx -# we're using an array, because we need to correctly preserve quoting, spaces, etc -fpm_command=( php-fpm ${fpm_conftest} --pid "$fpm_pidfile" --nodaemonize -y "$fpm_config_tmp" ${php_config:+-c "$php_config"} ) -nginx_command=( nginx ${nginx_conftest} -c "$nginx_main" -g "pid $nginx_pidfile; include $nginx_config;" ) - if [[ $conftest ]]; then - echo -e "\n$(basename "$0"): Config testing php-fpm using ${fpm_command[@]}:" >&2 - "${fpm_command[@]}" - echo -e "\n$(basename "$0"): Config testing nginx using ${nginx_command[@]}:" >&2 - "${nginx_command[@]}" + case $conftest in + 1) + fpm_conftest="-t" + nginx_conftest="-t" + ;; + 2) + fpm_conftest="-tt" + nginx_conftest="-t" + ;; + 3) + fpm_conftest="-t" + nginx_conftest="-T" + ;; + *) + fpm_conftest="-tt" + nginx_conftest="-T" + ;; + esac + + echo -e "\n$(basename "$0"): Config testing php-fpm using ${fpm_command[@]} ${fpm_conftest}:" >&2 + "${fpm_command[@]}" ${fpm_conftest} + echo -e "\n$(basename "$0"): Config testing nginx using ${nginx_command[@]} ${nginx_conftest}:" >&2 + "${nginx_command[@]}" ${nginx_conftest} echo -e "\n$(basename "$0"): All configs okay." >&2 exit 0 fi +# we're done dumping stuff; restore original FPM config +cp "$fpm_config" "$fpm_config_tmp" + # make a shared pipe; we'll write the name of the process that exits to it once that happens, and wait for that event below # this particular call works on Linux and Mac OS (will create a literal ".XXXXXX" on Mac, but that doesn't matter). wait_pipe=$(mktemp -t "heroku.waitpipe-$PORT.XXXXXX" -u) From deb4e72d6e071f99c4c06fc01266f131274bd530 Mon Sep 17 00:00:00 2001 From: David Zuelke Date: Fri, 26 Jan 2024 12:15:53 +0100 Subject: [PATCH 6/8] Improve memory_limit detection Turns out that bug which would not allow overriding of FPM config php_value settings in .user.ini was fixed in... PHP 7.2: https://bugs.php.net/bug.php?id=75212 Also, that behavior where a later declaration does not override a newer one is probably a bug: https://github.com/php/php-src/issues/13249 We now use php-fpm -tt to dump the config, since that will have stuff printed in evaluation order - if the bug above ever gets fixed, we have to do nothing. This also removes the need for us to handle the PHP-FPM config include directives ourselves, and as a result, variable expansion and globbing now work in these! GUS-W-14901875 --- CHANGELOG.md | 4 ++ bin/heroku-php-apache2 | 20 +++++- bin/heroku-php-nginx | 20 +++++- bin/util/autotune.php | 67 ++++++------------- test/fixtures/bootopts/conf/fpm.admin.conf | 1 + .../fixtures/bootopts/conf/fpm.include.broken | 2 +- test/fixtures/bootopts/conf/fpm.include.conf | 2 +- test/spec/composer_spec.rb | 2 +- test/spec/php_concurrency_shared.rb | 19 ++++-- 9 files changed, 80 insertions(+), 57 deletions(-) create mode 100644 test/fixtures/bootopts/conf/fpm.admin.conf diff --git a/CHANGELOG.md b/CHANGELOG.md index a8fbe6872f..6c263208be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ - Boot scripts now have a `--test`/`-t` option to test PHP-FPM and web server configs and then exit. Can be repeated to dump configs for either or both, see `--help` for details. [David Zuelke] +### FIX + +- PHP `memory_limit` in `.user.ini` does not override PHP-FPM `php_value` for `$WEB_CONCURRENCY` calculation [David Zuelke] + ## v244 (2024-01-24) ### ADD diff --git a/bin/heroku-php-apache2 b/bin/heroku-php-apache2 index 7275c06b39..f9acfff3e8 100755 --- a/bin/heroku-php-apache2 +++ b/bin/heroku-php-apache2 @@ -402,8 +402,26 @@ if [[ -z ${WEB_CONCURRENCY:-} ]]; then max_ram="6G" fi + # dump the FPM config and parse out memory_limit declarations + # for that to work, we need the WEB_CONCURRENCY env var set, as the FPM config references it + export WEB_CONCURRENCY=1 + # inside this subshell, we want pipefail on to ensure that a failing FPM command causes an exit + # grep might not match anything, so if it doesn't (status 1), we carry on, since that's fine (but other exit statuses will bubble up) + fpm_limits=$(set -o pipefail; "${fpm_command[@]}" -tt 2>&1 | { grep -E $'NOTICE: \tphp(_admin)?_value\[memory_limit\]' || test $? = 1; } | cut -f2) || { + # on failure, we should output something meaningful + echo "PHP-FPM config test failed with status $?:" >&2 + # restore the original config to avoid any potential for confusion + cp "$fpm_config" "$fpm_config_tmp" + # single -t is enough this time + "${fpm_command[@]}" -t + exit 1 + } + # determine number of FPM processes to run - read WEB_CONCURRENCY php_memory_limit <<<$(php ${php_config:+-c "$php_config"} "$bp_dir/bin/util/autotune.php" -y "$fpm_config" -t "$DOCUMENT_ROOT" "$ram" "$max_ram") + # we feed it the PHP config we found much earlier (it must be the one FPM loads, not the CLI one!), docroot (for .user.ini), detected RAM, and RAM cap + # on STDIN, we also pass it any php_value or php_admin_value lines from the FPM config dump that may have a memory_limit (via a herestring, easier than echo) + # the output is then passed into read via a herestring; read creates our variables (we cannot pipe into read, since pipelines are a subshell, and then variables would be gone straight after) + read WEB_CONCURRENCY php_memory_limit <<<$(php ${php_config:+-c "$php_config"} "$bp_dir/bin/util/autotune.php" -t "$DOCUMENT_ROOT" "$ram" "$max_ram" <<<"$fpm_limits") [[ $WEB_CONCURRENCY -lt 1 ]] && WEB_CONCURRENCY=1 export WEB_CONCURRENCY diff --git a/bin/heroku-php-nginx b/bin/heroku-php-nginx index fd656caa9b..7b4e70e3a9 100755 --- a/bin/heroku-php-nginx +++ b/bin/heroku-php-nginx @@ -402,8 +402,26 @@ if [[ -z ${WEB_CONCURRENCY:-} ]]; then max_ram="6G" fi + # dump the FPM config and parse out memory_limit declarations + # for that to work, we need the WEB_CONCURRENCY env var set, as the FPM config references it + export WEB_CONCURRENCY=1 + # inside this subshell, we want pipefail on to ensure that a failing FPM command causes an exit + # grep might not match anything, so if it doesn't (status 1), we carry on, since that's fine (but other exit statuses will bubble up) + fpm_limits=$(set -o pipefail; "${fpm_command[@]}" -tt 2>&1 | { grep -E $'NOTICE: \tphp(_admin)?_value\[memory_limit\]' || test $? = 1; } | cut -f2) || { + # on failure, we should output something meaningful + echo "PHP-FPM config test failed with status $?:" >&2 + # restore the original config to avoid any potential for confusion + cp "$fpm_config" "$fpm_config_tmp" + # single -t is enough this time + "${fpm_command[@]}" -t + exit 1 + } + # determine number of FPM processes to run - read WEB_CONCURRENCY php_memory_limit <<<$(php ${php_config:+-c "$php_config"} "$bp_dir/bin/util/autotune.php" -y "$fpm_config" -t "$DOCUMENT_ROOT" "$ram" "$max_ram") + # we feed it the PHP config we found much earlier (it must be the one FPM loads, not the CLI one!), docroot (for .user.ini), detected RAM, and RAM cap + # on STDIN, we also pass it any php_value or php_admin_value lines from the FPM config dump that may have a memory_limit (via a herestring, easier than echo) + # the output is then passed into read via a herestring; read creates our variables (we cannot pipe into read, since pipelines are a subshell, and then variables would be gone straight after) + read WEB_CONCURRENCY php_memory_limit <<<$(php ${php_config:+-c "$php_config"} "$bp_dir/bin/util/autotune.php" -t "$DOCUMENT_ROOT" "$ram" "$max_ram" <<<"$fpm_limits") [[ $WEB_CONCURRENCY -lt 1 ]] && WEB_CONCURRENCY=1 export WEB_CONCURRENCY diff --git a/bin/util/autotune.php b/bin/util/autotune.php index 7c17cdcec6..afe65d8516 100755 --- a/bin/util/autotune.php +++ b/bin/util/autotune.php @@ -29,41 +29,7 @@ function bytestostring($amount) { return sprintf("%d%s", $amount, $suffix); } -// if given, parse FPM configs as well to figure out the memory limit -// (it may have been set in the FPM config or config include) -function get_fpm_memory_limit($fpmconf, $startsection = "global") { - if(!is_readable($fpmconf)) { - // an include may point to a file that does not exist, that's fine - return array(); - } - $fpm = parse_ini_string("[$startsection]\n".file_get_contents($fpmconf), true); // prepend section from parent so stuff is parsed correctly in includes that lack a leading section marker - - $retval = array(); - - foreach($fpm as $section => $directives) { - foreach($directives as $key => $value) { - if($section == "www" && $key == "php_admin_value" && isset($value['memory_limit'])) { - $retval['php_admin_value'] = $value['memory_limit']; - } elseif($section == "www" && $key == "php_value" && isset($value['memory_limit']) && !isset($retval['php_value'])) { - // an existing value takes precedence - // we can only emulate that for includes; within the same file, the INI parser overwrites earlier values :( - $retval['php_value'] = $value['memory_limit']; - } elseif($key == "include") { - // values from the include don't overwrite existing values - $retval = array_merge(get_fpm_memory_limit($value, $section), $retval); - } - - if(isset($retval['php_admin_value'])) { - // done for good as nothing can change this anymore, bubble upwards - return $retval; - } - } - } - - return $retval; -} - -$opts = getopt("y:t:", array(), $rest_index); +$opts = getopt("t:", array(), $rest_index); $argv = array_slice($argv, $rest_index); $argc = count($argv); if($argc < 1 || $argc > 2) { @@ -74,33 +40,42 @@ function get_fpm_memory_limit($fpmconf, $startsection = "global") { ); fputs(STDERR, "Options:\n". - " -y PHP-FPM config file to read 'memory_limit' settings from\n". - " -t Dir to read '.user.ini' with 'memory_limit' settings from\n\n" + " -t Dir to read '.user.ini' with 'memory_limit' settings from\n\n". + "php_value or php_admin_value lines from a PHP-FPM config can be fed via STDIN.\n\n" ); exit(2); } -$limits = array(); -if(isset($opts["y"])) { - $limits = get_fpm_memory_limit($opts["y"]); +// first, parse potential php_value and php_admin_value data from STDIN +// the expected format is lines like the following: +// php_value[memory_limit] = 128M +// php_admin_value[memory_limit] = 128M +$limits = (stream_isatty(STDIN) ? [] : parse_ini_string(stream_get_contents(STDIN))); +if($limits === false) { + fputs(STDERR, "ERROR: Malformed FPM php_value/php_admin_value directives on STDIN.\n"); + exit(1); } if( - !$limits /* .user.ini memory limit is ignored if one is set via FPM */ && isset($opts['t']) && is_readable($opts['t'].'/.user.ini') ) { // we only read the topmost .user.ini inside document root $userini = parse_ini_file($opts['t'].'/.user.ini'); + if($userini === false) { + fputs(STDERR, "ERROR: Malformed .user.ini in document root.\n"); + exit(1); + } if(isset($userini['memory_limit'])) { - $limits['php_value'] = $userini['memory_limit']; + // if .user.ini has a limit set, it will overwrite an FPM config php_value, but not a php_admin_value + $limits['php_value']['memory_limit'] = $userini['memory_limit']; } } -if(isset($limits['php_admin_value'])) { - ini_set('memory_limit', $limits['php_admin_value']); -} elseif(isset($limits['php_value'])) { - ini_set('memory_limit', $limits['php_value']); +if(isset($limits['php_admin_value']['memory_limit'])) { + ini_set('memory_limit', $limits['php_admin_value']['memory_limit']); +} elseif(isset($limits['php_value']['memory_limit'])) { + ini_set('memory_limit', $limits['php_value']['memory_limit']); } $limit = ini_get('memory_limit'); diff --git a/test/fixtures/bootopts/conf/fpm.admin.conf b/test/fixtures/bootopts/conf/fpm.admin.conf new file mode 100644 index 0000000000..39f571ee13 --- /dev/null +++ b/test/fixtures/bootopts/conf/fpm.admin.conf @@ -0,0 +1 @@ +php_admin_value[memory_limit] = 24M diff --git a/test/fixtures/bootopts/conf/fpm.include.broken b/test/fixtures/bootopts/conf/fpm.include.broken index b518f35b07..96ed661f67 100644 --- a/test/fixtures/bootopts/conf/fpm.include.broken +++ b/test/fixtures/bootopts/conf/fpm.include.broken @@ -1 +1 @@ -foobar = 0 +listen = 256.256.256.256:${PORT} diff --git a/test/fixtures/bootopts/conf/fpm.include.conf b/test/fixtures/bootopts/conf/fpm.include.conf index 739005d6c5..ba2a69f7d4 100644 --- a/test/fixtures/bootopts/conf/fpm.include.conf +++ b/test/fixtures/bootopts/conf/fpm.include.conf @@ -1,3 +1,3 @@ request_slowlog_timeout = 1 -php_value[memory_limit] = 32M +php_value[memory_limit] = 16M diff --git a/test/spec/composer_spec.rb b/test/spec/composer_spec.rb index de0be89fed..bdc1ad2884 100644 --- a/test/spec/composer_spec.rb +++ b/test/spec/composer_spec.rb @@ -71,7 +71,7 @@ ['heroku-php-apache2', 'heroku-php-nginx'].each do |script| retry_until retry: 3, sleep: 5 do out = app.run("#{script} -F composer.lock", :heroku => {:env => "COMPOSER_AUTH=malformed"}) # prevent FPM from starting up using an invalid config, that way we don't have to wrap the server start in a `timeout` call - expect(out).to match(/Starting php-fpm/) # we got far enough (until FPM spits out an error) + expect(out).to match(/FPM initialization failed/) # we got far enough (until FPM spits out an error) end end end diff --git a/test/spec/php_concurrency_shared.rb b/test/spec/php_concurrency_shared.rb index 6b3a4f4bbd..b475b45ef4 100644 --- a/test/spec/php_concurrency_shared.rb +++ b/test/spec/php_concurrency_shared.rb @@ -30,6 +30,13 @@ .and match("pm.max_children = 1") end end + it "takes precedence over a PHP-FPM memory_limit" do + retry_until retry: 3, sleep: 5 do + expect(expect_exit(code: 0) { @app.run("heroku-php-#{server} -tt -F conf/fpm.include.conf docroot/", :return_obj => true) }.output) + .to match("PHP memory_limit is 32M Bytes") + .and match("pm.max_children = 16") + end + end it "is only done for a .user.ini directly in the document root" do retry_until retry: 3, sleep: 5 do expect(expect_exit(code: 0) { @app.run("heroku-php-#{server} -tt", :return_obj => true) }.output) @@ -43,8 +50,8 @@ it "calculates concurrency correctly" do retry_until retry: 3, sleep: 5 do expect(expect_exit(code: 0) { @app.run("heroku-php-#{server} -tt -F conf/fpm.include.conf", :return_obj => true) }.output) - .to match("PHP memory_limit is 32M Bytes") - .and match("pm.max_children = 16") + .to match("PHP memory_limit is 16M Bytes") + .and match("pm.max_children = 32") end end it "always launches at least one worker" do @@ -54,11 +61,11 @@ .and match("pm.max_children = 1") end end - it "takes precedence over a .user.ini memory_limit" do + it "takes precedence over a .user.ini memory_limit if it's a php_admin_value" do retry_until retry: 3, sleep: 5 do - expect(expect_exit(code: 0) { @app.run("heroku-php-#{server} -tt -F conf/fpm.include.conf docroot/onegig/", :return_obj => true) }.output) - .to match("PHP memory_limit is 32M Bytes") - .and match("pm.max_children = 16") + expect(expect_exit(code: 0) { @app.run("heroku-php-#{server} -tt -F conf/fpm.admin.conf docroot/onegig/", :return_obj => true) }.output) + .to match("PHP memory_limit is 24M Bytes") + .and match("pm.max_children = 21") end end end From 2d96005375254442e06a4194b7c1869a73b1410a Mon Sep 17 00:00:00 2001 From: David Zuelke Date: Fri, 26 Jan 2024 13:20:20 +0100 Subject: [PATCH 7/8] Streamline WEB_CONCURRENCY related output It is now all handled by autotune.php, and available RAM is in human readable format. The removal of the read command also makes errors from autotune.php bubble up. GUS-W-14901894 --- bin/heroku-php-apache2 | 8 ++------ bin/heroku-php-nginx | 8 ++------ bin/util/autotune.php | 12 +++++++----- test/spec/php_concurrency_shared.rb | 6 +++--- 4 files changed, 14 insertions(+), 20 deletions(-) diff --git a/bin/heroku-php-apache2 b/bin/heroku-php-apache2 index f9acfff3e8..9f17faf173 100755 --- a/bin/heroku-php-apache2 +++ b/bin/heroku-php-apache2 @@ -389,10 +389,9 @@ httpd_command=( httpd -D NO_DETACH -c "Include $httpd_config" ) mlib="/sys/fs/cgroup/memory/memory.limit_in_bytes" if [[ -f "$mlib" ]]; then ram="$(cat "$mlib")" - echo "Detected ${ram} Bytes of RAM" >&2 else ram="512M" - echo "Assuming ${ram}B of RAM" >&2 + echo "Assuming RAM to be ${ram} Bytes" >&2 fi if [[ -z ${WEB_CONCURRENCY:-} ]]; then max_ram="16G" # blanket upper limit for how much RAM we will use for PHP, unless the machine has less anyway @@ -420,12 +419,9 @@ if [[ -z ${WEB_CONCURRENCY:-} ]]; then # determine number of FPM processes to run # we feed it the PHP config we found much earlier (it must be the one FPM loads, not the CLI one!), docroot (for .user.ini), detected RAM, and RAM cap # on STDIN, we also pass it any php_value or php_admin_value lines from the FPM config dump that may have a memory_limit (via a herestring, easier than echo) - # the output is then passed into read via a herestring; read creates our variables (we cannot pipe into read, since pipelines are a subshell, and then variables would be gone straight after) - read WEB_CONCURRENCY php_memory_limit <<<$(php ${php_config:+-c "$php_config"} "$bp_dir/bin/util/autotune.php" -t "$DOCUMENT_ROOT" "$ram" "$max_ram" <<<"$fpm_limits") + WEB_CONCURRENCY=$(php ${php_config:+-c "$php_config"} "$bp_dir/bin/util/autotune.php" -t "$DOCUMENT_ROOT" "$ram" "$max_ram" <<<"$fpm_limits") [[ $WEB_CONCURRENCY -lt 1 ]] && WEB_CONCURRENCY=1 export WEB_CONCURRENCY - - echo "PHP memory_limit is ${php_memory_limit} Bytes" >&2 else echo '$WEB_CONCURRENCY env var is set, skipping automatic calculation' >&2 fi diff --git a/bin/heroku-php-nginx b/bin/heroku-php-nginx index 7b4e70e3a9..0d4dc92058 100755 --- a/bin/heroku-php-nginx +++ b/bin/heroku-php-nginx @@ -389,10 +389,9 @@ nginx_command=( nginx -c "$nginx_main" -g "pid $nginx_pidfile; include $nginx_co mlib="/sys/fs/cgroup/memory/memory.limit_in_bytes" if [[ -f "$mlib" ]]; then ram="$(cat "$mlib")" - echo "Detected ${ram} Bytes of RAM" >&2 else ram="512M" - echo "Assuming ${ram}B of RAM" >&2 + echo "Assuming RAM to be ${ram} Bytes" >&2 fi if [[ -z ${WEB_CONCURRENCY:-} ]]; then max_ram="16G" # blanket upper limit for how much RAM we will use for PHP, unless the machine has less anyway @@ -420,12 +419,9 @@ if [[ -z ${WEB_CONCURRENCY:-} ]]; then # determine number of FPM processes to run # we feed it the PHP config we found much earlier (it must be the one FPM loads, not the CLI one!), docroot (for .user.ini), detected RAM, and RAM cap # on STDIN, we also pass it any php_value or php_admin_value lines from the FPM config dump that may have a memory_limit (via a herestring, easier than echo) - # the output is then passed into read via a herestring; read creates our variables (we cannot pipe into read, since pipelines are a subshell, and then variables would be gone straight after) - read WEB_CONCURRENCY php_memory_limit <<<$(php ${php_config:+-c "$php_config"} "$bp_dir/bin/util/autotune.php" -t "$DOCUMENT_ROOT" "$ram" "$max_ram" <<<"$fpm_limits") + WEB_CONCURRENCY=$(php ${php_config:+-c "$php_config"} "$bp_dir/bin/util/autotune.php" -t "$DOCUMENT_ROOT" "$ram" "$max_ram" <<<"$fpm_limits") [[ $WEB_CONCURRENCY -lt 1 ]] && WEB_CONCURRENCY=1 export WEB_CONCURRENCY - - echo "PHP memory_limit is ${php_memory_limit} Bytes" >&2 else echo '$WEB_CONCURRENCY env var is set, skipping automatic calculation' >&2 fi diff --git a/bin/util/autotune.php b/bin/util/autotune.php index afe65d8516..a065e223ba 100755 --- a/bin/util/autotune.php +++ b/bin/util/autotune.php @@ -78,19 +78,21 @@ function bytestostring($amount) { ini_set('memory_limit', $limits['php_value']['memory_limit']); } -$limit = ini_get('memory_limit'); $ram = stringtobytes($argv[0]); // first arg is the available memory +fprintf(STDERR, "Available RAM is %s Bytes\n", bytestostring($ram)); + if(isset($argv[1])) { // optional second arg is the maximum RAM we're allowed $max_ram_string = $argv[1]; $max_ram = stringtobytes($max_ram_string); if($ram > $max_ram) { $ram = $max_ram; - fprintf(STDERR, "Limiting to %s Bytes of RAM usage\n", bytestostring($ram)); + fprintf(STDERR, "Limiting RAM usage to %s Bytes\n", bytestostring($ram)); } } -// assume 64 MB base overhead for web server and FPM, and 1 MB overhead for each worker -// echo floor(($ram-stringtobytes('64M'))/(stringtobytes($limit)+stringtobytes('1M'))) . " " . $limit; -echo floor($ram / (stringtobytes($limit)?:-1)) . " " . $limit; +$limit = ini_get('memory_limit'); +fprintf(STDERR, "PHP memory_limit is %s Bytes\n", $limit); // we output the original value here, since it's user supplied + +echo floor($ram / (stringtobytes($limit)?:-1)); diff --git a/test/spec/php_concurrency_shared.rb b/test/spec/php_concurrency_shared.rb index b475b45ef4..b296e18895 100644 --- a/test/spec/php_concurrency_shared.rb +++ b/test/spec/php_concurrency_shared.rb @@ -98,8 +98,8 @@ it "restricts the app to 6 GB of RAM", :if => series < "7.4" do retry_until retry: 3, sleep: 5 do expect(expect_exit(code: 0) { @app.run("heroku-php-#{server} -tt", :return_obj => true, :heroku => {:size => "Performance-L"}) }.output) - .to match("Detected 15032385536 Bytes of RAM") - .and match("Limiting to 6G Bytes of RAM usage") + .to match("Available RAM is 14G Bytes") + .and match("Limiting RAM usage to 6G Bytes") .and match("pm.max_children = 48") end end @@ -107,7 +107,7 @@ it "uses all available RAM for PHP-FPM workers", :unless => series < "7.4" do retry_until retry: 3, sleep: 5 do expect(expect_exit(code: 0) { @app.run("heroku-php-#{server} -tt", :return_obj => true, :heroku => {:size => "Performance-L"}) }.output) - .to match("Detected 15032385536 Bytes of RAM") + .to match("Available RAM is 14G Bytes") .and match("pm.max_children = 112") end end From 3866053e0f89d46eabd406ffbe8443a9f4d48466 Mon Sep 17 00:00:00 2001 From: David Zuelke Date: Fri, 26 Jan 2024 13:51:20 +0100 Subject: [PATCH 8/8] verbose mode for autotune.php and RAM detection GUS-W-14901894 --- bin/heroku-php-apache2 | 9 ++++++-- bin/heroku-php-nginx | 9 ++++++-- bin/util/autotune.php | 52 +++++++++++++++++++++++++++--------------- 3 files changed, 47 insertions(+), 23 deletions(-) diff --git a/bin/heroku-php-apache2 b/bin/heroku-php-apache2 index 9f17faf173..70bd4fde47 100755 --- a/bin/heroku-php-apache2 +++ b/bin/heroku-php-apache2 @@ -388,8 +388,10 @@ httpd_command=( httpd -D NO_DETACH -c "Include $httpd_config" ) mlib="/sys/fs/cgroup/memory/memory.limit_in_bytes" if [[ -f "$mlib" ]]; then + [[ $verbose ]] && echo "Reading available RAM from '$mlib'" >&2 ram="$(cat "$mlib")" else + [[ $verbose ]] && echo "No '$mlib' with RAM info found" >&2 ram="512M" echo "Assuming RAM to be ${ram} Bytes" >&2 fi @@ -401,6 +403,8 @@ if [[ -z ${WEB_CONCURRENCY:-} ]]; then max_ram="6G" fi + [[ $verbose ]] && echo "Calculating WEB_CONCURRENCY..." >&2 + # dump the FPM config and parse out memory_limit declarations # for that to work, we need the WEB_CONCURRENCY env var set, as the FPM config references it export WEB_CONCURRENCY=1 @@ -419,8 +423,9 @@ if [[ -z ${WEB_CONCURRENCY:-} ]]; then # determine number of FPM processes to run # we feed it the PHP config we found much earlier (it must be the one FPM loads, not the CLI one!), docroot (for .user.ini), detected RAM, and RAM cap # on STDIN, we also pass it any php_value or php_admin_value lines from the FPM config dump that may have a memory_limit (via a herestring, easier than echo) - WEB_CONCURRENCY=$(php ${php_config:+-c "$php_config"} "$bp_dir/bin/util/autotune.php" -t "$DOCUMENT_ROOT" "$ram" "$max_ram" <<<"$fpm_limits") - [[ $WEB_CONCURRENCY -lt 1 ]] && WEB_CONCURRENCY=1 + WEB_CONCURRENCY=$(php ${php_config:+-c "$php_config"} -f "$bp_dir/bin/util/autotune.php" -- ${verbose:+-v} -t "$DOCUMENT_ROOT" "$ram" "$max_ram" <<<"$fpm_limits") + [[ $verbose ]] && echo "WEB_CONCURRENCY=${WEB_CONCURRENCY} (RAM / memory_limit)" >&2 + [[ $WEB_CONCURRENCY -lt 1 ]] && { WEB_CONCURRENCY=1; [[ $verbose ]] && echo "WEB_CONCURRENCY=1 (was below mininum)" >&2; } export WEB_CONCURRENCY else echo '$WEB_CONCURRENCY env var is set, skipping automatic calculation' >&2 diff --git a/bin/heroku-php-nginx b/bin/heroku-php-nginx index 0d4dc92058..8423fa0444 100755 --- a/bin/heroku-php-nginx +++ b/bin/heroku-php-nginx @@ -388,8 +388,10 @@ nginx_command=( nginx -c "$nginx_main" -g "pid $nginx_pidfile; include $nginx_co mlib="/sys/fs/cgroup/memory/memory.limit_in_bytes" if [[ -f "$mlib" ]]; then + [[ $verbose ]] && echo "Reading available RAM from '$mlib'" >&2 ram="$(cat "$mlib")" else + [[ $verbose ]] && echo "No '$mlib' with RAM info found" >&2 ram="512M" echo "Assuming RAM to be ${ram} Bytes" >&2 fi @@ -401,6 +403,8 @@ if [[ -z ${WEB_CONCURRENCY:-} ]]; then max_ram="6G" fi + [[ $verbose ]] && echo "Calculating WEB_CONCURRENCY..." >&2 + # dump the FPM config and parse out memory_limit declarations # for that to work, we need the WEB_CONCURRENCY env var set, as the FPM config references it export WEB_CONCURRENCY=1 @@ -419,8 +423,9 @@ if [[ -z ${WEB_CONCURRENCY:-} ]]; then # determine number of FPM processes to run # we feed it the PHP config we found much earlier (it must be the one FPM loads, not the CLI one!), docroot (for .user.ini), detected RAM, and RAM cap # on STDIN, we also pass it any php_value or php_admin_value lines from the FPM config dump that may have a memory_limit (via a herestring, easier than echo) - WEB_CONCURRENCY=$(php ${php_config:+-c "$php_config"} "$bp_dir/bin/util/autotune.php" -t "$DOCUMENT_ROOT" "$ram" "$max_ram" <<<"$fpm_limits") - [[ $WEB_CONCURRENCY -lt 1 ]] && WEB_CONCURRENCY=1 + WEB_CONCURRENCY=$(php ${php_config:+-c "$php_config"} -f "$bp_dir/bin/util/autotune.php" -- ${verbose:+-v} -t "$DOCUMENT_ROOT" "$ram" "$max_ram" <<<"$fpm_limits") + [[ $verbose ]] && echo "WEB_CONCURRENCY=${WEB_CONCURRENCY} (RAM / memory_limit)" >&2 + [[ $WEB_CONCURRENCY -lt 1 ]] && { WEB_CONCURRENCY=1; [[ $verbose ]] && echo "WEB_CONCURRENCY=1 (was below mininum)" >&2; } export WEB_CONCURRENCY else echo '$WEB_CONCURRENCY env var is set, skipping automatic calculation' >&2 diff --git a/bin/util/autotune.php b/bin/util/autotune.php index a065e223ba..4a0394c2ea 100755 --- a/bin/util/autotune.php +++ b/bin/util/autotune.php @@ -29,7 +29,7 @@ function bytestostring($amount) { return sprintf("%d%s", $amount, $suffix); } -$opts = getopt("t:", array(), $rest_index); +$opts = getopt("vt:", array(), $rest_index); $argv = array_slice($argv, $rest_index); $argc = count($argv); if($argc < 1 || $argc > 2) { @@ -40,13 +40,28 @@ function bytestostring($amount) { ); fputs(STDERR, "Options:\n". + " -v Verbose mode\n". " -t Dir to read '.user.ini' with 'memory_limit' settings from\n\n". "php_value or php_admin_value lines from a PHP-FPM config can be fed via STDIN.\n\n" ); exit(2); } -// first, parse potential php_value and php_admin_value data from STDIN +$ram = stringtobytes($argv[0]); // first arg is the available memory + +fprintf(STDERR, "Available RAM is %s Bytes\n", bytestostring($ram)); + +if(isset($argv[1])) { // optional second arg is the maximum RAM we're allowed + $max_ram_string = $argv[1]; + $max_ram = stringtobytes($max_ram_string); + + if($ram > $max_ram) { + $ram = $max_ram; + fprintf(STDERR, "Limiting RAM usage to %s Bytes\n", bytestostring($ram)); + } +} + +// parse potential php_value and php_admin_value data from STDIN // the expected format is lines like the following: // php_value[memory_limit] = 128M // php_admin_value[memory_limit] = 128M @@ -56,42 +71,41 @@ function bytestostring($amount) { exit(1); } +if(isset($opts['v'])) { + if(isset($limits['php_value'])) { + fputs(STDERR, "memory_limit changed by php_value in PHP-FPM configuration\n"); + } +} + if( isset($opts['t']) && - is_readable($opts['t'].'/.user.ini') + is_readable($userini_path = $opts['t'].'/.user.ini') ) { // we only read the topmost .user.ini inside document root - $userini = parse_ini_file($opts['t'].'/.user.ini'); + $userini = parse_ini_file($userini_path); if($userini === false) { - fputs(STDERR, "ERROR: Malformed .user.ini in document root.\n"); + fprintf(STDERR, "ERROR: Malformed %s.\n", $userini_path); exit(1); } if(isset($userini['memory_limit'])) { + if(isset($opts['v'])) { + fprintf(STDERR, "memory_limit changed by %s\n", $userini_path); + } // if .user.ini has a limit set, it will overwrite an FPM config php_value, but not a php_admin_value $limits['php_value']['memory_limit'] = $userini['memory_limit']; } } if(isset($limits['php_admin_value']['memory_limit'])) { + // these take precedence and cannot be overridden later + if(isset($opts['v'])) { + fputs(STDERR, "memory_limit overridden by php_admin_value in PHP-FPM configuration\n"); + } ini_set('memory_limit', $limits['php_admin_value']['memory_limit']); } elseif(isset($limits['php_value']['memory_limit'])) { ini_set('memory_limit', $limits['php_value']['memory_limit']); } -$ram = stringtobytes($argv[0]); // first arg is the available memory - -fprintf(STDERR, "Available RAM is %s Bytes\n", bytestostring($ram)); - -if(isset($argv[1])) { // optional second arg is the maximum RAM we're allowed - $max_ram_string = $argv[1]; - $max_ram = stringtobytes($max_ram_string); - - if($ram > $max_ram) { - $ram = $max_ram; - fprintf(STDERR, "Limiting RAM usage to %s Bytes\n", bytestostring($ram)); - } -} - $limit = ini_get('memory_limit'); fprintf(STDERR, "PHP memory_limit is %s Bytes\n", $limit); // we output the original value here, since it's user supplied