⚓ T211488 Audit and sync INI settings as needed between HHVM and PHP 7

Subject	Repo	Branch	Lines +/-
profile::mediawiki::php: tweak ini settings	operations/puppet	production	+32 -20
profile::mediawiki::php: conform error reporting levels to HHVM	operations/puppet	production	+1 -0
profile::mediawiki::php: raise mysql.connect_timeout to 3 seconds	operations/puppet	production	+1 -0
profile::mediawiki::php: disable display_errors in FPM mode	operations/puppet	production	+1 -0

Status	Subtype	Assigned	Task
Resolved		None	T176370 Migrate to PHP 7 in WMF production
Resolved	PRODUCTION ERROR	Anomie	T215126 PHP warning on some Echo email sending attempts due to mismatching PEAR file versions
Resolved		Krinkle	T211488 Audit and sync INI settings as needed between HHVM and PHP 7
Resolved		jijiki	T220681 Set `enable_dl` to 0 in php.ini

Krinkle triaged this task as Medium priority.Dec 9 2018, 12:02 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 9 2018, 12:02 AM

Krinkle moved this task from Backlog to Wikimedia production on the PHP 7.0 support board.Dec 9 2018, 12:03 AM

Krinkle mentioned this in T176370: Migrate to PHP 7 in WMF production.Jan 25 2019, 1:51 AM

Check T214248 - this is a known issue (unrelated to php7) but it is not considered a bug, just a configuration weakness (https://dev.mysql.com/doc/refman/8.0/en/load-data-local.html). Consider it enabling it on any client that doesn't use LOAD DATA LOCAL.

After some digging:

HHVM has no error_reporting INI value set, meaning we use the default value, which is, according to my tests, 16807935. Please note this value cannot be obtained by combining the various error constants php offers. This is, according to https://github.com/facebook/hhvm/blob/master/hphp/runtime/base/runtime-error.h#L64-L76 corresponds roughly to E_ALL & ~E_STRICT in php, plus FATAL_ERROR which is a HHVM-only feature.
We don't set the value in PHP 7 either, and that means we've basically settled on leaving the ini setting to the default value of 22519, which is E_ALL & ~E_DEPRECATED & ~ E_STRICT & ~ E_NOTICE

So we probably want to set

error_reporting = E_ALL & ~E_STRICT; in php 7.x

Confirmed that's the case, by running this code in CLI and in a browser

<?php
// This constant was introduced in HHVM 3.20
define('E_HHVM_FATAL_ERROR', 16777217);
if (defined('HHVM_VERSION')) {
	$hphp_all = error_reporting();
	error_reporting($hphp_all & ~E_HHVM_FATAL_ERROR);
} else {
        error_reporting(E_ALL & ~ E_STRICT);
}
var_dump(error_reporting());

I'll submit a patch shortly.

Change 486485 had a related patch set uploaded (by Giuseppe Lavagetto; owner: Giuseppe Lavagetto):
[operations/puppet@production] profile::mediawiki::php: conform error reporting levels to HHVM

https://gerrit.wikimedia.org/r/486485

gerritbot added a project: Patch-For-Review.Jan 25 2019, 3:38 PM

Change 486485 merged by Giuseppe Lavagetto:
[operations/puppet@production] profile::mediawiki::php: conform error reporting levels to HHVM

https://gerrit.wikimedia.org/r/486485

Found this old issue - T157306, should that be closed, or still something we need/want for HHVM during the migration?

• Phabricator_maintenance moved this task from Backlog to Acknowledged on the SRE board.Jan 26 2019, 10:51 PM

In T215126#4926064, @Krinkle wrote:

[..] a PHP Warning was logged directly into the HTML output stream as part of a regular response. This must never happen. Presumably, this is due to the display_errors INI settings from PHP7 being set incorrectly.

Tgr added a parent task: T215126: PHP warning on some Echo email sending attempts due to mismatching PEAR file versions.Feb 5 2019, 12:23 AM

In T211488#4926068, @Krinkle wrote:

Presumably, this is due to the display_errors INI settings from PHP7 being set incorrectly.

Yup.

tgr@mwdebug1002:~$ php7.2 -S localhost:8000 info.php 
PHP 7.2.8-1+0~20180725124257.2+stretch~1.gbp571e56 Development Server started at Wed Feb  6 03:43:07 2019
Listening on http://localhost:8000
Document root is /home/tgr
Press Ctrl-C to quit.

tgr@mwdebug1002:~$ curl -s 'http://localhost:8000' | grep display_errors
<tr><td class="e">display_errors</td><td class="v">On</td><td class="v">On</td></tr>

Change 497024 had a related patch set uploaded (by Krinkle; owner: MaxSem):
[operations/puppet@production] Disable display_errors in FPM mode

https://gerrit.wikimedia.org/r/497024

Change 497024 merged by Giuseppe Lavagetto:
[operations/puppet@production] profile::mediawiki::php: disable display_errors in FPM mode

https://gerrit.wikimedia.org/r/497024

dcausse mentioned this in T216959: Search exceptions handling prints response information on the screen.Mar 19 2019, 5:18 PM

I just finished going through all settings.

Given a lot of settings don't correspond 1:1, or have no correspondence at all between the two application servers, only a few different settings could be verified.

There are only two clear outstanding differences, of which one is intentional and the other will be fixed soon:

HHVM uses a max_execution_time of 60 seconds on the appservers, which is then overridden with set_time_limit for individual requests. PHP has a less effective way to set wall-time timeouts, and we're using Excimer to reach a similar behaviour. The hard limit (set in php-fpm) is right now at 201 seconds for the application servers. Overall, the behaviour is expected to be similar
Mysql connection timeout is 3 seconds in HHVM (hhvm.mysql.connect_timeout) and 1 second in php (mysql.connect_timeout). This needs to be fixed.

Change 499143 had a related patch set uploaded (by Giuseppe Lavagetto; owner: Giuseppe Lavagetto):
[operations/puppet@production] profile::mediawiki::php: raise mysql.connect_timeout to 3 seconds

https://gerrit.wikimedia.org/r/499143

jijiki subscribed.Mar 26 2019, 3:35 PM

Change 499143 merged by Giuseppe Lavagetto:
[operations/puppet@production] profile::mediawiki::php: raise mysql.connect_timeout to 3 seconds

https://gerrit.wikimedia.org/r/499143

Krinkle updated the task description. (Show Details)Apr 10 2019, 11:44 PM

Krinkle removed a project: Patch-For-Review.

Krinkle added a project: Performance-Team (Radar).

Per @aaron, the Redis settings don't affect us. Most of phpredis isn't INI-configurable, it's passed at run-time. The only parts of phpredis that are INI-configurable are parts we don't use (RedisArrays, RedisCluster, and a redis-based php-session handler).

In T211488#5102911, @Krinkle wrote:
Exported from mwdebug1001 in plain text and sorted. Full dumps at P8387 and P8386.

Differences

APC

This seems worth looking at. Especially because this is effectively a very major change to how APC works. On HHVM it has essentially unlimited space and no expiry or garbage collection. It's strictly TTL based. This is now going to be very different, and I would expect the defaults to be far too conservative for us.

Why is apc.enable_cli different. What do we want? Does it matter?

What did apc.stat do for us on HHVM?

Figure out good settings for segments, size, etc.
-apc.enable_cli = 1
+apc.enable_cli = 0

-apc.stat = 1

+apc.coredump_unmap = 0
+apc.entries_hint = 4096
+apc.gc_ttl = 3600
+apc.mmap_file_mask = 
+apc.preload_path = 
+apc.serializer = php
+apc.shm_segments = 1
+apc.shm_size = 32M
+apc.slam_defense = 0
+apc.smart = 0
+apc.ttl = 0
+apc.use_request_time = 1

Apc works in fundamentally different ways between HHVM and PHP. Also, we properly use APCu, not the old APC. So the only settings that actually do something are listed here
https://www.php.net/manual/en/apcu.configuration.php

The shm settings are in theory irrelevant as we use opcache for code. Ditto for apc.stat.

My approach at tuning apc is "look at the metrics". We're collecting apc metrics from PHP7 (you can see some of them in the https://grafana.wikimedia.org/d/000000550/mediawiki-application-servers dashboard and you can see all of them running php7adm on an appserver); as soon as we have more traffic we will surely be able to tune them.

As for cli, I assumed APC would be unneeded there as we don't run multi-process cli applications, and the manual strongly suggests it's a bad idea.

I would consider this checked.

Joe updated the task description. (Show Details)Apr 11 2019, 5:37 AM

In T211488#5102911, @Krinkle wrote:

Filesytem

Much lower. Don't know if it matters?
-default_socket_timeout = 60 # seconds
+default_socket_timeout = 1 # seconds

It does for places where we just open raw sockets. The number in HHVM was quite high (equal to the request timeout for GETs), but I think it's ok anyways. I'll amend the php config.

Intl

Decide whether to emit errors and/or throw exceptions when ICU functions encounter issues in PHP.

By default this is disabled, which seems typical for PHP functions. It means stuff returns null/false instead.

But, I couldn't find right away what HHVM does. Worth double-checking to see what we expect in MediaWiki and confirm that it is fine. Otherwise might need to update settings on PHP 7, or fix MW.
+intl.error_level = 0
+intl.use_exceptions = 0

I'm pretty sure this was introduced in the 7.x series, and that HHVM doesn't report ICU errors. I would assume we need to go read the HHVM source code to know more.

OPCache (docs)

Most are obvious defaults that HHVM simply did differently but in a way we don't have much choice over and seemed fine. (See bottom for details).

But, there were a few I think are worth thinking about:

opcache.validate_timestamps = 0 – Enabled by default, but disabled for us. Unsure why. The fact that this is currently disable for us, might explain why I'm editing files locally on mwdebug1001 usually does not result in it taking affect on PHP 7.2.

+opcache.force_restart_timeout = 180 – Skimmed the docs and if this controls discarding of opcache after a certain time, might be worth increasing. Especially for mwdebug.

+opcache.huge_code_pages = 0 – Allegedly a way to improve performance. Disabled by default because it is hard to detect whether the Kernel supports it. I imagine ours do. Might be useful.

+opcache.max_accelerated_files = 24,000 – No idea how many files we have in total on a multi-version deployment. Worth checking. What would happen if we were to set this to 100,000, or the max – 1,000,000?

+opcache.memory_consumption = 256 (megabytes) – Is this enough? I don't know.

There is a long ticket about opcache optimizations that includes benchmarking. We do not invalidate opcache to ensure consistency of the code and atomic releases. We do clear the opcache from scap, and you can do it locally on a machine by running php7adm /opcache-free.

A few more interesting ones:

+opcache.interned_strings_buffer = 8
+opcache.max_wasted_percentage = 10
+opcache.file_cache = 
+opcache.file_cache_consistency_checks = 1

And here again, we're collecting metrics and we will tune all opcache parameters accordingly when we get a bit more traffic. One to keep an eye on, as I didn't use it for my benchmarks, is opcache.huge_code_pages. I learned the hard way not to trust general wisdom (or Zend's reccomendations) on tuning.

Joe updated the task description. (Show Details)Apr 11 2019, 6:21 AM

Error handling was discussed in https://phabricator.wikimedia.org/T211488#4908305 and the followups. The most notable difference is HHVM has display_errors on but it doesn't really do anything - I did set it to on in php7 after looking at what we had in HHVM and we had to roll it back.

Joe updated the task description. (Show Details)Apr 11 2019, 6:34 AM

• MoritzMuehlenhoff subscribed.Apr 11 2019, 6:36 AM

Also:

PCRE JIT is enabled by default in HHVM and we definitely want it in php7 as well.
include_path in php's ini is still set to the value it had in the old times before we migrated to HHVM. I think it's irrelevant nowadays for all applications and can be removed. I think it's actually potentially harmful.
doc_root is only relevant under CGI or if you activate safe_mode, which we don't use, according to https://www.php.net/manual/en/ini.core.php#ini.doc-root

Joe updated the task description. (Show Details)Apr 11 2019, 8:33 AM

Also, the value of doc_root for HHVM doesn't seem to be set from ini settings, so I'll have to dig up where that happens.

Regarding enable_dl: i just verified dl() does work under HHVM - although I suspect it does nothing. So I would be careful in disabling it but we should definitely do it at some point.

I'm opening a subtask to do it.

Joe updated the task description. (Show Details)Apr 11 2019, 8:53 AM

In T211488#5102911, @Krinkle wrote:

File Uploads & Data Input

-upload_tmp_dir = /tmp
+upload_tmp_dir = 

+max_input_nesting_level = 64
+max_input_vars = 1000

+upload_max_filesize = 100M

+post_max_size = 100M

upload_tmp_dir uses the system's default tmp dir if not specified. That would be /tmp under linux.
The value of upload_max_filesize / post_max_size match the value of hhvm.server.max_post_size which is 100MB

Session

We don't use the default session storage in PHP. But, we do use some of its functions and legacy variables like $_SESSION. I don't know whether some of those might vary their behaviour through these.

I'll be honest, I had no idea either, at least for some of these settings. But from a read of the docs, I don't see how the security ones might have an impact.

What does have an impact is

session.upload_progress.enabled = 1

which will indeed interact with $_SESSION by adding a variable named

session.upload_progress.name = PHP_SESSION_UPLOAD_PROGRESS

in the session to track file uploads progress.

Do we want this to be disabled? I'm not sure, maybe it's potentially useful?

Joe updated the task description. (Show Details)Apr 11 2019, 9:06 AM

Joe updated the task description. (Show Details)Apr 11 2019, 9:16 AM

Regarding mail: both paths actually link to /usr/sbin/exim4 on our systems.

As far as serialisation goes:

Igbinary is installed because the memcached extension and (IIRC) the apcu extension have it as a requirement. I'm not sure if we use it anywhere
the compact_strings setting is wrong in our puppet code, thanks for catching that, I'm fixing it.
memcached serializer is set to php as that's the value that makes memcached interoperable between HHVM and php7.2 (after we patched HHVM already)
serialize_precision is AIUI 14 in our version of HHVM as well, see https://github.com/facebook/hhvm/blob/HHVM-3.18/hphp/runtime/base/variable-serializer.cpp#L264
precision matches as well but has nothing to do with serialization, and we don't work much with high-precision math, right?

Joe updated the task description. (Show Details)Apr 11 2019, 9:54 AM

Looking at memcached settings with @elukey the only one that really seems potentially problematic is

memcached.store_retry_count = 2

which we want to set to 1 instead. I'm not sure it matters for us as we don't use the multiserver capabilities of the memcached extension anyways.

Re mysqli, we should care about:

+mysqlnd.net_cmd_buffer_size = 4096
+mysqlnd.net_read_buffer_size = 32768
+mysqlnd.net_read_timeout = 86400

I don't think we care about the others (although I remember in the past changing some long-running query timeouts?)

We should also introduce mysqli.allow_local_infile = 0, but I've been asked to do that on a separate task.

Note I am not sure if mysqli uses mysqlnd for this installation, so maybe those do nothing (needs checking on how it is being compiled).

In T211488#5103689, @jcrespo wrote:
Re mysqli, we should care about:
+mysqlnd.net_cmd_buffer_size = 4096
+mysqlnd.net_read_buffer_size = 32768
+mysqlnd.net_read_timeout = 86400
I don't think we care about the others (although I remember in the past changing some long-running query timeouts?)

We should also introduce mysqli.allow_local_infile = 0, but I've been asked to do that on a separate task.

We already have that as the default setting, so I can just explicitly add it without the need of a separate task I guess.

Note I am not sure if mysqli uses mysqlnd for this installation, so maybe those do nothing (needs checking on how it is being compiled).

It does indeed. For those settings, I'd frankly use the defaults for now, and we can iteratively improve doing some benchmarking later, unless you see something insane in the default settings.

Joe updated the task description. (Show Details)Apr 11 2019, 12:26 PM

For those settings, I'd frankly use the defaults for now, and we can iteratively improve doing some benchmarking later, unless you see something insane in the default settings.

If the intention was to do a sweep to see grave issues first, those are indeed not problematic- normally you wouldn't change them. But I have been here enough time to see huge (not sane) query sizes, so I cannot discard they may need changes on edge cases. :-)

Change 502986 had a related patch set uploaded (by Giuseppe Lavagetto; owner: Giuseppe Lavagetto):
[operations/puppet@production] profile::mediawiki::php: tweak ini settings

https://gerrit.wikimedia.org/r/502986

gerritbot added a project: Patch-For-Review.Apr 11 2019, 12:35 PM

In T211488#5103435, @Joe wrote:

Session

We don't use the default session storage in PHP. But, we do use some of its functions and legacy variables like $_SESSION. I don't know whether some of those might vary their behaviour through these.

I'll be honest, I had no idea either, at least for some of these settings. But from a read of the docs, I don't see how the security ones might have an impact.

What does have an impact is

session.upload_progress.enabled = 1

which will indeed interact with $_SESSION by adding a variable named

session.upload_progress.name = PHP_SESSION_UPLOAD_PROGRESS

in the session to track file uploads progress.

Do we want this to be disabled? I'm not sure, maybe it's potentially useful?

Upload progress should just be disabled since we don't use it.

The other session INI changes look harmless to me.

In T211488#5103305, @Joe wrote:

include_path in php's ini is still set to the value it had in the old times before we migrated to HHVM. I think it's irrelevant nowadays for all applications and can be removed. I think it's actually potentially harmful.

Yeah, PEAR relies on include_path and a non-empty value can result in surprises. See T215224: PEAR PHP classes are loaded from system packages instead of Composer packages in WMF production.

Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.Apr 12 2019, 1:10 PM

@Joe Regarding opcache, I'm not sure why this change for manual reloading is applied now. Could we do that after the migration? The way it is with HHVM is that it checks at run-time via stat/mtimes. I assume that's the default on PHP 7 as well, when validate_timestamps is enabled (as it is by default).

The reason I'm concerned is because it will be confusing for a while for people's debugging workflows (and things are already confusing due to the migration). Also, I'm not entirely sure it will be an adequate substitute for T104352 because disabling validation does not mean we disable parsing of new files (it's not like HHVM RepoAuth). It just means we won't eagerly re-parse files we still have in cache. As such, we can still get Frankenstein-states during a deployment.

In any event, I think it's cool, worth considering, and you're probably right :) - but perhaps we can do that at a later time?

In T211488#5107374, @Krinkle wrote:

@Joe Regarding opcache, I'm not sure why this change for manual reloading is applied now. Could we do that after the migration? The way it is with HHVM is that it checks at run-time via stat/mtimes. I assume that's the default on PHP 7 as well, when validate_timestamps is enabled (as it is by default).

The reason I'm concerned is because it will be confusing for a while for people's debugging workflows (and things are already confusing due to the migration). Also, I'm not entirely sure it will be an adequate substitute for T104352 because disabling validation does not mean we disable parsing of new files (it's not like HHVM RepoAuth). It just means we won't eagerly re-parse files we still have in cache. As such, we can still get Frankenstein-states during a deployment.

In any event, I think it's cool, worth considering, and you're probably right :) - but perhaps we can do that at a later time?

While I commented on this on IRC, it's worth reporting it here:

If we leave a revalidate_interval different than zero, we could end up with unpredictable results during a code deploy as scap deploys are not atomic.
scap thus now invalidates opcache once a release is done, and no other revalidation is done.
always revalidating doesn't really work and wouldn't scale. It's also a sizeable performance hit.

The best alternative I can think of is adding a relatively long revalidation period (like 5 minutes), and/or controlling revalidation externally running opcache_free() from a periodic job that could check if scap is running before doing anything.

regarding enable_dl, I did some investigating:

The documentation on php.net says it's removed since php 7.0
dl() always returns false under hhvm as far as I can say.
It works under php 7.2, with no warning whatsoever. At least it does if one uses our debian package.
It returns an error under php7.3 in buster

So I'll definitely disable enable_dl now.

Krinkle edited projects, added PHP 7.2 support; removed PHP 7.0 support.Apr 16 2019, 12:34 AM

Krinkle moved this task from Untriaged to Wikimedia production on the PHP 7.2 support board.

Change 502986 merged by Giuseppe Lavagetto:
[operations/puppet@production] profile::mediawiki::php: tweak ini settings

https://gerrit.wikimedia.org/r/502986

FWIW, the doc_root being set was causing severe issues under php7.2. I removed it from the list of ini settings we want to use, and from the actual config as well.

jijiki added a project: User-jijiki.Apr 24 2019, 10:10 AM

Mentioned in SAL (#wikimedia-operations) [2019-04-24T10:23:08Z] <jijiki> Restarting php-fpm on mw1238 for 505383 and T211488

Mentioned in SAL (#wikimedia-operations) [2019-04-24T11:25:12Z] <jijiki> Restarting php7.2-fpm on mw-canary for 505383 and T211488

Mentioned in SAL (#wikimedia-operations) [2019-04-24T11:59:36Z] <jijiki> Restarting php7.2-fpm on mw12* for 505383 and T211488

Mentioned in SAL (#wikimedia-operations) [2019-04-24T13:01:21Z] <jijiki> Restarting php7.2-fpm on mw13* for 505383 and T211488

Mentioned in SAL (#wikimedia-operations) [2019-04-24T15:32:03Z] <jijiki> Restarting php7.2-fpm on mw2* in codfw for 505383 and T211488

jijiki closed subtask T220681: Set `enable_dl` to 0 in php.ini as Resolved.Apr 24 2019, 10:13 PM

jijiki updated the task description. (Show Details)

We have pushed https://gerrit.wikimedia.org/r/502986 and (its update) https://gerrit.wikimedia.org/r/505383/ to production. Are we going to proceed with session and mysql settings or should we mark this task as resolved?

For sessions.

From T211488#5102911:

-session.save_path = 
+session.save_path = /tmp

From T211488#5102911:

-session.entropy_file = 
-session.entropy_length = 0
-session.hash_bits_per_character = 4
-session.hash_function = 0
+session.sid_bits_per_character = 4
+session.sid_length = 32
+session.trans_sid_hosts = 
+session.trans_sid_tags = a=href,area=href,frame=src,form=

These relate to the creation of new session IDs as create by session_start or session_regenerate_id, and returned by session_id.

The session.entropy_* settings that HHVM and PHP 5 had were empty which means using the default of /dev/urandom. These configs have been removed in PHP 7.1 and use whatever random-entropy source is appropriate on a given platform.

session.hash_bits_per_character effectively became session.sid_bits_per_character. And the others are all new options in PHP 7 that were previously not configurable but behave the same at least as far as PHP code can observe. (Possibly more secure or scalable defaults now).

In any event, I realized that MediaWiki's SessionManager has its own algorithm for generating session IDs, which we then inform PHP of, and use instead. This code lives in MediaWiki\SessionManager::generateSessionId().

From T211488#5102911:
+session.lazy_write = 1

Doesn't affect us afaik. Our custom SessionHandler seems to write all data from what I can see. Adding support for the lazy-write method as this PHP 7.0 feature does seems like a good idea, but setting or not setting this doesn't affect us.

From T211488#5102911:

+session.upload_progress.cleanup = 1
+session.upload_progress.enabled = 1
+session.upload_progress.freq = 1%
+session.upload_progress.min_freq = 1
+session.upload_progress.name = PHP_SESSION_UPLOAD_PROGRESS
+session.upload_progress.prefix = upload_progress_

The patch @Joe wrote did the one thing we cared about, which is disabling session.upload_progress.enabled. The rest seems fine.

From T211488#5102911:
+session.use_strict_mode = 0

Interesting (ini docs), and something to consider for later. But doesn't matter for right now. This is a new option in PHP 7 that allows changing a behaviour that previously was not configurable. The default is effectively what PHP5/HHVM did.

The mysql settings have been checked by aaron, jcrespo and joe.

Krinkle mentioned this in T223180: Monitoring PHP 7 APC usage.May 13 2019, 10:49 PM

Audit and sync INI settings as needed between HHVM and PHP 7
Closed, ResolvedPublic
Actions

Description

Details

Related Objects
Search...

Event Timeline

Differences

Differences

Audit and sync INI settings as needed between HHVM and PHP 7 Closed, ResolvedPublicActions

Description

Details

Related ObjectsSearch...

Event Timeline

Differences

Differences

Audit and sync INI settings as needed between HHVM and PHP 7
Closed, ResolvedPublic
Actions

Related Objects
Search...