4544: POC for using FrankenPHP behind Traefik - #59
Draft
turegjorup wants to merge 18 commits into
Draft
turegjorup wants to merge 18 commits into
turegjorup wants to merge 18 commits into
Conversation
…-poc # Conflicts: # composer.json # composer.lock # docker-compose.yml
Bring the POC compose config in line with develop: required-variable syntax, mariadb healthcheck dependency, protected /health/detail route, and the markdownlint/prettier dev services.
Replace the phpfpm and nginx pair with a single FrankenPHP container, added in the per-environment override files, and move the stack to PHP 8.5. Changes - Add the `frankenphp` service in `docker-compose.override.yml` and `docker-compose.server.override.yml`, and park `phpfpm` and `nginx` in a profile that is never enabled - Port the nginx configuration to `.docker/Caddyfile` and the PHP settings the fpm image derives from `PHP_*` variables to `.docker/php.ini` - Keep TLS termination in Traefik: `auto_https` is off and Caddy serves plain HTTP on 8080 - Build on the published `dunglas/frankenphp:1.12-php8.5` image, adding the extensions it omits: pdo_mysql, amqp and intl - Move `itkdev/php8.5-fpm`, `itkdev/supervisor-php8.5` and the composer platform requirement to PHP 8.5 - Point Taskfile, workflows, Woodpecker, the staging and redirect overrides and the docs at the `frankenphp` service Why The POC was a year behind develop and pinned to Symfony 7. Putting the service in the override files keeps the base compose files as the template ships them, so the swap is one file per environment rather than a rewrite.
API Specification - Non-breaking changesNo changelog changes |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #59 +/- ##
=============================================
+ Coverage 39.31% 43.04% +3.73%
+ Complexity 1123 1111 -12
=============================================
Files 149 149
Lines 3551 3545 -6
=============================================
+ Hits 1396 1526 +130
+ Misses 2155 2019 -136
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The populated-database job checks out the base branch before the pull request, so it sees two revisions of docker-compose.yml and cannot assume either service name.
Changes
- Expose Caddy's Prometheus endpoint at `/metrics`, behind the `ITKMetricsAuth`
middleware `/cron-metrics` used
- Log requests as JSON from Caddy and keep PHP's `error_log` on `${PHP_LOGS}`,
where php-fpm sent it
- Mirror the itkdev/php8.5-fpm ini templates in `.docker/php.ini` using the
image's own `PHP_*` variable names, defaulted in the Dockerfile
- Trust `private_ranges` instead of `172.16.0.0/16`
Why
nginx exported no metrics at all: `stub_status` is compiled into the image but
the template never enabled it, php-fpm's `pm.status_path` was never routed, and
the supercronic behind `/cron-metrics` only starts when `/app/crontab` exists,
which this project has no. Caddy has a real exporter, so the endpoint finally
has something behind it.
`172.16.0.0/16` covers neither the `frontend` network (172.18/16) nor the client
(172.22/16), so `set_real_ip_from` never matched and real-IP resolution silently
did nothing. `private_ranges` is what a `/12` in the template would have meant.
The ini file previously hardcoded values the fpm image derives from environment
variables, and invented two variable names. Mirroring the image's templates
keeps the same overrides working.
symfony/runtime has shipped FrankenPhpWorkerRunner since 7.4, selected automatically off the FRANKENPHP_WORKER=1 that FrankenPHP sets for a worker script. runtime/frankenphp-symfony is only for older Symfony, so its lack of a Symfony 8 release never blocked anything — dropping it was right, but redundancy was the reason, not incompatibility. What actually holds worker mode back is application state, not the runtime.
Changes - `PackageVersionFactory` and `ModuleVersionFactory` keep their deduplication buffers in locals threaded through the private helpers, not in properties - Key the version buffers on object identity rather than on the entity id, so they hold before Doctrine has assigned one - `LeantimeService` implements `ResetInterface`; autoconfigure tags it `kernel.reset` - Add tests for all three; the factories had none Why The contract advises statelessness over `ResetInterface` where it is possible, and for the factories it is: the buffers exist only to stand in for the repositories between `persist()` and `flush()` within one call, so their lifetime is exactly that call. Making them locals also fixes the reason they were flagged — they were cleared after `flush()` rather than in a `finally`, so a failing flush left entities from a closed EntityManager for the next call. That was a live bug in the messenger consumer, which already runs long. `LeantimeService` is the case the fallback is for. Its cache cannot become a local: `resolveUserName()` is called inside a loop over tickets, so dropping it would cost an API round trip per ticket. `reset()` restores the per-request lifetime `loadUsers()` already documents. The two `testAFailedFlushLeavesNothingForTheNextCall` tests fail against the previous implementations; the other thirteen pass either way and guard the deduplication behaviour, including a null-version quirk left deliberately intact.
Changes - Call `unsetAll()` before `setController()` in `DashboardController` and `SecurityContractCrudController` - Add `DashboardControllerTest`, and put `SecurityContractCrudController` into the admin smoke test's provider Why EasyAdmin registers `AdminUrlGenerator` as `shared: no`, so each injection point gets its own instance — but both consumers here are shared, so that instance lives as long as they do, which in a worker is longer than one request. It accumulates route parameters as it is used. `AppExtension` and `RepoAdvisoryService` already opened with `unsetAll()`; these two were the inconsistency, and inconsistency is what rots. Neither site was covered. The dashboard test asserts where the redirect lands rather than that it merely redirects, because the failure mode worth catching is silent: `unsetAll()` placed after `setController()` wipes the controller back out and produces a URL pointing somewhere else without anything throwing. Both new tests fail against that arrangement.
Worker mode needs no package and no code change — symfony/runtime has shipped FrankenPhpWorkerRunner since 7.4 and the Caddyfile already reads {$FRANKENPHP_CONFIG} — so it is documented as an environment variable, with what it measured here and the caveats on those numbers.
The statelessness rules go in claude.md as a section rather than a bullet, because messenger:consume is already long-running in production and the rules apply whether or not worker mode is on. Each rule points at the service in this codebase that follows it.
Changes - Add `igor-php/igor-php` as a dev dependency and register `IgorPhpBundle` in dev - Configure it in `igor.json`: project scope, dev environment, baseline file - Record the 33 existing findings in `igor-baseline.json`, each with a reason - Add `composer worker-state-check` and `worker-state-baseline`, and a `Worker state audit` job to the review workflow Why The statelessness rules the last few commits established are the kind that decay without enforcement, and they matter whether or not worker mode is ever switched on: `messenger:consume` is already long-running in production. igor-php audits every shared service in the compiled container rather than grepping for patterns, which is why it caught the `AdminUrlGenerator` mutations that reading `src/` for stateful properties had missed. Against that, roughly two thirds of its project findings are noise — mostly Doctrine entities returned from a repository, which it reads as shared services — so it is only usable behind a baseline. Vendor code is out of scope: it reported 341 findings there, none of them ours to fix. Every baseline entry carries a reason rather than the generated TODO, so the file documents why each is safe instead of just silencing it. Verified the gate is live: introducing a stateful property on a service fails the audit, and removing it passes.
Changes - Turn the Dockerfile into `base` → `dev` → `prod`; the override files pick a target, and a bare `docker build .` gets `prod` - `prod` drops Xdebug and sets `opcache.validate_timestamps=0` - Move the Xdebug ini to `.docker/php-dev.ini`, mounted only in development Why One image served both environments, so production loaded a debugger it never used and OPcache stat-ed every file on every request — `validate_timestamps=1` with `revalidate_freq=0` means check every time, which is the opposite of what that pair is usually meant to express. Turning timestamp validation off makes a code change need a new container. Both deployment paths already give it one: staging runs `up -d --force-recreate`, the release playbook brings the stack up again, and a fresh container starts with an empty OPcache, so it compiles what is on disk. The Xdebug ini moves rather than staying inert in production, so no file mentions settings whose extension is absent. Verified per stage: dev has the extension with timestamps validated, prod has neither, and coverage still collects in dev — `XDEBUG_MODE=coverage` reaches Xdebug even though `ini_get` reports the ini value, which the CI job depends on.
Missed from the previous commit: the script that wrote them asserted against README wording first and stopped before reaching these two.
Changes
- Create `deploy` and `runner` in the image, drop Caddy's
`cap_net_bind_service`, hand it `/data/caddy` and `/config/caddy`, and end
both stages with `USER deploy`
- Make the id a `DEPLOY_UID` build argument, defaulting to 1042
- Mirror phpfpm's `user: ${COMPOSE_USER:-deploy}` on the local override
- Add a health check on `/health/live` to both overrides
- Reorder `task site:update` to install before waiting on health
Why
Root was the one regression against the setup this replaces: phpfpm ran as
`${COMPOSE_USER:-deploy}`, and beyond the security footprint, a root container
writing `var/` through a bind mount leaves root-owned files on the host.
The id has to match whoever owns that checkout, and in devops_docker-images it
depends on the base distro — consistently across 8.3, 8.4 and 8.5, the ubuntu
tags give `deploy` 1000 and the alpine ones 1042. The servers run the alpine
tags, so 1042 is the default; a build argument because the number belongs to the
host account rather than to this image. Nothing needs a capability to bind 8080.
`up --wait` previously only waited for the process to exist, since only mariadb
had a check. Now it waits for the application to answer. That check calls into
the application, so it cannot pass before dependencies are installed — hence
starting, installing, then waiting, which is also the order that task always
meant.
Coming from the root-run container, `var/` needs handing over once:
`docker compose run --rm --user root frankenphp chown -R deploy:deploy /app/var`.
…-poc # Conflicts: # composer.lock # docker-compose.override.yml
Changes - Record the three-way measurement on `/admin` in the Caddyfile, README and changelog - Say what cloning the kernel actually does, and stop implying the reset erases worker mode's benefit Why The comment claimed the reset costs "a boot per request", which is true — `AbstractKernel::__clone()` nulls the container and clears `booted`, so the next `handle()` runs `initializeBundles()` and instantiates the compiled container again. But it was asserted rather than measured, and the conclusion drawn from it was wrong. Measured in prod, 40 seconds at 20 concurrent on `/admin`: 1319 requests per second and a 9.0 ms median with no worker, 1494 and 4.3 ms with one, 1395 and 6.1 ms with one plus the reset. The reset keeps roughly half the gain and still beats no worker on both numbers, because the PHP runtime, OPcache and autoloader stay warm across requests even when the kernel does not. That makes it a reasonable first worker-mode configuration to deploy rather than only something to compare against. Also notes that a boot is not a recompile, which the old wording invited readers to assume.
Three conflicts: - .woodpecker/stg.yml: keep the frankenphp service from this branch and take the --no-dev fix develop made in #99. This branch still carried the -no-dev typo on the frankenphp lines. - CHANGELOG.md: keep both unreleased entries, #103 then #59, followed by the 1.12.1 and 1.12.0 sections develop released. - composer.lock: regenerated with composer update against the merged composer.json rather than resolved by hand, so it carries develop's 47 package bumps and the removal of ocramius/doctrine-batch-utils alongside this branch's igor-php and php >=8.5.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link to ticket
https://leantime.itkdev.dk/tickets/showKanban?tab=ticketdetails#/tickets/showTicket/4544
Description
Serves the site from one FrankenPHP container behind Traefik instead of the phpfpm and nginx pair, and moves the stack to PHP 8.5.
Merged up to
develop, through the dependency update in #103.composer.lockwas regenerated withcomposer updateagainst the mergedcomposer.jsonrather than resolved by hand, so it now differs from develop by exactly one package,igor-php..woodpecker/stg.ymlconflicted where both sides had changed the same lines — this branch'sfrankenphpservice and develop's--no-devfix from #99 — and takes both.The parts worth reviewing:
Dockerfileis unavoidable for now.dunglas/frankenphpships withoutpdo_mysql,amqpandintl, so the application cannot boot on any published tag. Four lines add those plusgd,zip,xdebugandmsmtp. The cost is that every CI job builds instead of pulls. Publishing anitkdev/frankenphpnext toitkdev/php8.5-fpmremoves both — worth deciding before this leaves POC.phpfpmandnginxare parked in a never-enabled profile, because compose cannot delete an inherited service. The base compose files stay as the itkdev template ships them.auto_httpsis off and Caddy serves plain HTTP on 8080..docker/Caddyfilecarries every field nginx'slog_format mainhad, plusduration. The text layout needs Caddy's transform encoder, which this build lacks and which would mean compiling from source. Supercronic already logs-json, so JSON is arguably the house style — a decision worth making./metricsreplaces/cron-metrics, behind the sameITKMetricsAuthmiddleware. nginx exported nothing: the template never added astub_statuslocation, and the supercronic it proxied to never starts without an/app/crontab, so/cron-metricsalways answered 502.set_real_ip_from 172.16.0.0/16covered neither the frontend network nor the client, so nginx's real-IP resolution silently never happened. Now trustsprivate_ranges, andclient_ipresolves.Worker mode is ready but off.
symfony/runtimehas shippedFrankenPhpWorkerRunnersince 7.4, so enabling it is one uncommented line in the Caddyfile and needs no package. What held it back was application state; anigor-phpaudit found three cases, all fixed here:PackageVersionFactoryandModuleVersionFactory— dedup buffers moved from properties to locals. Already a live bug, not a worker-mode one: these run undermessenger:consume, so a failed flush left entities from a closed EntityManager for the next call.LeantimeService— implementsResetInterface. Its memoised user directory has to stay a cache, sinceresolveUserName()runs in a loop over tickets, but must not outlive the request.DashboardControllerandSecurityContractCrudController—unsetAll()beforesetController()on the injectedAdminUrlGenerator, which two other call sites already did.Measured on
/adminin prod, 40s at 20 concurrent: 1319 req/s without a worker, 1494 with one, 1395 with one plusFRANKENPHP_RESET_KERNEL=1— the reset keeps about half the gain rather than erasing it. From a laptop sharing CPU, so read it as a ranking, not absolutes.Screenshot of the result
No user interface changes.
Checklist
The container work needed no tests of its own — the point is that the existing suite passes unchanged on it. The state fixes did need them and had none; 20 added, suite now 87 tests. Each new test was run against the code it replaced and fails there, so they discriminate rather than just cover.
Additional comments or questions
Still open: the messenger worker is untouched on
itkdev/supervisor-php8.5, and whether it should also be FrankenPHP is the question. The container still runs as root — FrankenPHP wants to write/dataand/config.Left deliberately intact:
ModuleVersion::getVersion()reports'Unknown'for a null version, so a null-versioned module writes a row per occurrence. Keying it properly would change which rows get written, so it stays, pinned by a test. Worth deciding separately.Coming from the phpfpm stack, the leftover containers need removing once — a profile stops a service from starting, not one already running: