feat(nginx): add an opt-in patch that re-resolves the upstream addresses - #4498
feat(nginx): add an opt-in patch that re-resolves the upstream addresses#4498spawnia wants to merge 9 commits into
Conversation
nginx resolves upstream names once at startup and caches the result for the life of a worker. When a container is recreated onto a different address, nginx keeps using the old one and every request fails until nginx is restarted. Ingest fails silently in that state: relay serves /api/store/, /api/<id>/ and /api/0/relays/, so events are dropped while the UI keeps answering. Ship this as an opt-in patch rather than a change to nginx.conf, per getsentry#3894 (comment) — the caching is intentional and the suggested remedy was to apply it locally. The resolver address is rendered at container start from the container's own /etc/resolv.conf via the nginx image entrypoint, so the patch does not hardcode 127.0.0.11 and stays engine-agnostic. nginx.conf keeps its existing mount target and pulls the generated snippet in with an include, so a custom nginx.conf is neither overwritten nor made unwritable. Measured cost with 32 workers over 70s: 14 A queries total, no AAAA — one per upstream per valid=10s window, independent of worker count and traffic, because the shared zone means a single worker performs the lookup. Verified recovery after a forced address change (filler container holding the old address): patched recovers at t=8s, unpatched still 502 at t=30s. The README records the tradeoffs, notably that resolve replaces a loud startup failure on an unresolvable upstream with a running container serving 502s.
The first version explained the mechanism at a length that only repeated what the patch files show. Keep the problem, the apply order and the tradeoffs a reader cannot derive from the diff.
The hunk carried image: "nginx:1.31.4-alpine" as a context line, so every dependabot bump broke it. That tag moved four times in the last twelve months, which gave the patch a shelf life of about one quarter. Insert the mount and the environment key after the volumes block instead. The tag is no longer in any hunk's context, and the patch applies cleanly through a simulated bump to 1.32.0.
The three patches are interdependent, and applying them as separate commands in the previous order put the dangerous state one upstream change away: nginx.conf gained an include for a snippet that docker-compose.yml was never patched to render, so nginx refused to start. Reproduced by adding a volume to the nginx service, which is enough to reject the compose hunk. Chain them with && and patch nginx.conf last, so a rejected hunk stops the sequence before anything depends on it. Verified both directions: a failing compose patch now leaves the tree untouched, and the compose and template patches without the nginx.conf one boot fine.
Keep the commands and the constraints; drop the prose that explained mechanics the patch files already show.
Short sentences, one per line, so edits produce single-line diffs.
Name the actor in each sentence and drop the hedged phrasing.
One directive, one trailing clause, one line saved.
State that restart: true already covers Compose operations, so a reader can tell whether the patch applies to them, and link the prior attempts.
aminvakil
left a comment
aminvakil
left a comment
There was a problem hiding this comment.
Someone should maintain the patch to be correct, I don't want something unmaintainable with little value to be shipped here.
I still think this should be documented for users who want to try this and patch it locally without keeping it in self-hosted.
It's totally fine to have diff with self-hosted locally, I have access_log off; in nginx.conf and maxmemory 16gb in redis.conf for years, previously I had a custom docker-compose-override.yml for specific needs, I had kafka custom configurations locally.
|
Thanks for the feedback. I was planning on cleaning up the description and code here before marking ready for review btw, but seems like the notification reached you already. From what the repo establishes, I thought patches were the way to go for such customizations. They do seem to strike a nice middle ground between pure docs prose and the complexity and maintenance weight of a built-in solution. Yes, it can break - but so can docs. At least with the patch, it is documented very precisely and able to be verified somewhat mechanically. Are you open to continuing work on this approach, or do think we should leave it here? Agree on the local diff. We do maintain a few files that differ from upstream already and it has worked fine, but copying over the full nginx.conf seemed too much for me - which is why I tried to upstream a fix. Getting a second or third opinion on something I tried locally was already valuable for me, so I thank you for that. |
2 participants
Adds an opt-in patch that makes nginx re-resolve
relayandweb, underoptional-modifications/patches/next toexternal-kafka. Nothing changes for anyone who does not apply it.@aminvakil, this is the shape you offered to review in #4295 (comment):
A patch avoids the install-script env check you disliked, and it is how this repository already handles opt-in edits to git-tracked files.
#4492 changed the default configuration and you rejected that. It stays closed. Its branch now carries a cleaned-up single commit for anyone who wants the same change without the patch indirection.
Scope, corrected
I previously argued on #4295 that #3914 does not close the gap because our own outage came from a host-level Docker restart. That was wrong, and I have corrected it there. Our
docker inspectevidence shows a plaindocker compose down/up -d, which is exactly the caserestart: truecovers. We hit it only because our pinned version predates #3914.So this patch is not for the case #3914 already handles. It is for address changes that no Compose operation causes: a crash restart, or a Docker daemon restart. The README says so, so a reader can tell whether it applies to them.
We also took an unclean VM hard reset on unpatched nginx and the failure did not reproduce, because a reboot starts containers rather than recreating them. I am not claiming evidence for the remaining gap beyond the mechanism.
Answering the earlier objections
valid=10scosts about six queries per minute per upstream against the engine's embedded DNS. A per-upstream timer drives it, so worker count and request volume do not affect it. Measured with 32 workers: 14 A queries in 70 s, zero AAAA.127.0.0.11. Dropped. It is Docker-only; Podman answers on the network gateway. The entrypoint reads the address from the container's own/etc/resolv.confinstead.512k, not64k. nginx requires at least eight system pages, so64krefuses to start on 64 KiB-page hosts such as RHEL 9 on arm64.nginx.conf. The resolver lands in a separate include, so the/etc/nginx/nginx.confmount target is unchanged and existing overrides keep working.Test plan
Applied against this branch on current
master: all three hunks apply with no.rej,docker compose configpasses, and the entrypoint rendersresolver <address> ipv6=off valid=10sandnginx -tsucceeds.Behaviour verified on Docker with the patch applied: baseline 200; recreating
relayonto a new address recovers in 4 s without touching nginx;nginx -s reloadkeeps serving; an upstream absent at startup yields 502 until it appears rather than refusing to start; two nameservers render and validate; no AAAA queries and no unreachable-peer errors.Not tested on Podman. The patch reads the resolver from the container rather than assuming Docker's address, so it should work, but I have no Podman host to confirm on.
Credit to @ilias-115, whose #4295 established the mechanism.