Make /ping of php-fpm work again #13980
Conversation
|
Your changes makes sense but seems to qualify as bug fix. Might be possible to target PHP-8.2 branch instead. |
|
The problem with this is that the ping can be currently used for readiness and identifying if FPM is responsive. This change might change it. I need to think about this. |
Wilhansen
requested review from
kocsismate,
dstogov,
arnaud-lb,
iluuu1994,
alexdowad,
nielsdos,
devnexen,
Girgias,
derickr,
adoy and
TimWolla
as code owners
got it, will do so and rebase to php-8.2 branch. (update: done, but i could not figure out how to add the |
The ping feature of php-fpm monitoring was previously not working due to the configuration variables ping.path and ping.response not being copied over to the worker when forked. This results in the ping code path being disabled because the worker detects that ping.path is not configured.
Wilhansen
force-pushed
the
php-fpm-ping.path-fix
branch
from
210b346 to
23190c9
Compare
we were planning to use the ping feature for this, but it kept on returning a 404 despite |
|
@Wilhansen Can you actually create an issue and describe the whole issue with all info including all config files. It's useful for linking the fix with that. |
|
And mainly useful for me as a reminder to look into and see the exact problem that you have. |
posted a bug report here: #14037 |
|
The patch looks good to me.
If the end user plans to use the /ping route for readiness checks, they should use the regular endpoint. This way, if all children are busy, FPM will not be considered ready to serve new requests, and the readiness check will fail appropriately. However, if the end user plans to use the /ping route for liveness checks, they should use the status_listen endpoint. This ensures that the process is not incorrectly killed for being unresponsive. |
There was a problem hiding this comment.
I thought about this and I can see the use case. It's something between feature and a bug (omission) as obviously status_listen was meant just for status but considering that adding the functionality cannot cause any breaking issue so I'm happy to accept it as a bug and merge it to 8.2
The ping feature of php-fpm monitoring was previously not working (but the status is) due to the configuration variables
ping.pathandping.responsenot being copied over to the worker when forked (butpm.status_pathdoes). This results in the ping code path being disabled because the worker detects thatping.pathis not configured.