fix: log and count opcache restarts - #2634
Conversation
|
Pushed 88313a6: the same event is now also exposed as a |
alexandre-daubois
left a comment
There was a problem hiding this comment.
I can see how this would be useful to have such metric making the configuration/debugging easier.
opcache schedules a restart of its shared memory on exhaustion or hash overflow, then carries it out at the next request init on any thread. Under ZTS it does that while other threads are still running, because the deferral gate (accel_is_inactive()) probes for a conflicting lock with fcntl F_GETLK, and POSIX fcntl locks belong to the process, so the probe never sees the threads of the process holding them. Workers are the worst case: they hold shared memory references for their whole life rather than for a single request. The result is a crash or a slowdown with nothing in the logs pointing at opcache. opcache does report it, but only at opcache.log_verbosity_level=4 and in its own log. zend_accel_schedule_restart_hook is the only in-process signal for this, so it is used to emit one warning naming the restart reason and the two settings that make restarts less likely. Nothing else is done with it: the threads are not rebooted, which is what php#2564 removed.
The log line alone cannot be alerted on. Expose the same event as a counter labelled by reason, pre-populated at zero for the known reasons so a rate or an alert works from the first restart on.
88313a6 to
4486a7c
Compare
|
I'm not convinced of the need to log these as metrics, too. Wouldn't it be enough to properly log them as warnings? |
|
Given that a possible outcome of opcache restarts is a segfault, I'd rather monitor them in a programmatic way rather than by sniffing logs. That's my rationale for proposing it. |
|
The usual metric for this should optimally always be zero. I feel like it being a metric normalises opcache restarts as if they were a normal thing during sapi execution, but they aren't, they cause segfaults in fpm too, just with a smaller blast radius. |
|
Yes, that's a should-always-be-zero metric. I think it's important to give visibility here. This should come with docs saying that - if not zero there's an issue with your config. APM exist because these happen IRL and need proper monitoring, so the metric would be valuable IMHO. |
|
Why not but marked as experimental, and deleted when this will be fixed upstream. |
Should always be zero, to be removed once opcache handles restarts safely under ZTS.
henderkes
left a comment
There was a problem hiding this comment.
As experimental this is okay, but please note that we will probably delete the metric when/if the root issue is fixed upstream.
|
Done in 81b2916: marked experimental in the docs, the help string and the code, with a note that it should always be zero and that it will be removed once opcache handles restarts safely under ZTS. |
Split out of #2621, which is closed: this keeps only the reporting, not the thread reboot.
opcache schedules a restart of its shared memory on exhaustion or hash overflow, then carries it out at the next request init on any thread. Under ZTS that happens while other threads are still running: the deferral gate
accel_is_inactive()probes for a conflicting lock withfcntl F_GETLK, and POSIX fcntl locks belong to the process, so the probe never sees the threads of the process holding them. Workers are the worst case, they hold shared memory references for their whole life instead of for one request.What an operator sees today is a segfault or a sudden slowdown with nothing in the logs pointing at opcache. opcache does report the event, but only at
opcache.log_verbosity_level=4and in its own log.This PR makes the event visible and does nothing else. As a log line:
And as a counter, since a log line cannot be alerted on:
zend_accel_schedule_restart_hookis the only in-process signal for this, so it is set again, but this time it only logs and counts. No thread reboot, so nothing here can loop the way #2564 was worried about.Same
#if defined(ZTS) && PHP_VERSION_ID >= 80400guard as before.reasoncomes fromzend_accel_restart_reason:out of memory,hash overflow,user.usershould be unreachable sinceopcache_reset()is overridden, so seeing it means the override did not take, which is worth knowing too.On the metric: the
Metricsinterface gets anOpcacheRestart(reason)method (no-op onnullMetrics), backed by afrankenphp_opcache_restartscounter registered up front like the thread gauges. The known reasons are pre-populated at zero so a rate or an alert on the series works from the first restart on, instead of missing it for lack of a previous sample. The counter is incremented before the log-level gate, so the event is recorded even when warnings are silenced.The counter has a unit test. No test for the hook itself: forcing a real restart is exactly what takes the process down, so a test for this would be a test that crashes.