Wait for cluster DNS before starting nginx in the frontend - #4354
Conversation
nginx resolves the hostnames in its proxy_pass directives when it loads its configuration and exits if any of them cannot be resolved: [emerg] host not found in upstream "backend" in /etc/nginx/conf.d/default.conf:48 On a cold start the frontend container can be running before the cluster DNS can answer for the backend service, which leaves the pod crash looping until the kubelet's restart backoff happens to line up with DNS being ready. Wait for the name in a docker-entrypoint.d script, and start nginx anyway after two minutes so that a real misconfiguration is still reported rather than leaving a container that runs but never serves. Add a readiness probe to the frontend, since with the wait in place a running container is no longer necessarily a serving one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nginx exits if a proxy_pass host cannot be resolved when it loads its configuration, and the wait only knows about "backend". Say so where the upstreams are, so that a new cluster-internal one is added to the wait rather than left to fail on a cold start. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe frontend deployment adds a TCP readiness probe on port 80. nginx waits for backend DNS resolution during startup, polls for up to 120 seconds, and documents the hostname configuration in its template. ChangesReadiness checks
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🔵 Low · up to Frontend startup now waits for backend DNS before nginx begins serving, which improves cold-start behavior but can delay readiness longer than the intended two minutes if DNS resolution blocks. The PR is mergeable with owner awareness or follow-up to enforce the deadline around each DNS lookup. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4354 +/- ##
=======================================
Coverage 75.96% 75.96%
=======================================
Files 305 305
Lines 11384 11384
Branches 1411 1411
=======================================
Hits 8648 8648
Misses 2332 2332
Partials 404 404
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:
|
Delivery noteTwo of the three files here are image content rather than installer content:
So the DNS wait itself only takes effect once a release publishes a new frontend image. An installer built from master That pairing is harmless: the probe is a |
The note at the proxy_pass directives repeated the script's own account of why nginx exits on an unresolvable upstream. Keep only what is not written at the destination: that a cluster-internal upstream has to be added to the wait. Rewrap the script's header to the width of the rest of the repo. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The note at the proxy_pass directives told a maintainer to add cluster-internal upstreams to the wait, which has one hardcoded host, no list to add to, and the name in its own filename. State what it waits for instead: a host that is not "backend" is then visibly not covered, and generalizing the script can wait for a second upstream to actually exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
jasonleenaylor
left a comment
There was a problem hiding this comment.
Two nits inline. The mechanism is right and the readinessProbe is the right companion to it -- without that, a container mid-wait would take traffic.
This review was assisted by Claude Opus 5.
WAITED only tracked the sleeps, so the 120s cap did not cover the time getent spends blocking on resolver timeouts. That is exactly the cold-start case the script exists for. Use bash's SECONDS instead, which also brings the initial probe into the budget, and report actual elapsed time on timeout rather than the cap. Move the proxy_pass note above the server block. It sat above one of three proxy_pass directives and described the current state rather than what a maintainer adding an upstream needs to do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jasonleenaylor
left a comment
There was a problem hiding this comment.
LGTM
This review was assisted by Claude Opus 5.
Reviewers can ignore all the stuff below.
(Much of it is for context and details of this change within a larger effort.)
Split out of #4352, part 1 of 4 — see
#4352 (comment) for the split and how it was verified.
nginx resolves the hostnames in its
proxy_passdirectives when it loads its configuration, and exits if any of themcannot be resolved:
On a cold start the frontend container can be running before the cluster DNS is able to answer for the
backendservice, which leaves the pod crash looping until the kubelet's restart backoff happens to line up with DNS being
ready.
docker-entrypoint.dscript, and start nginx anyway after two minutes, so that a realmisconfiguration is still reported rather than leaving a container that runs but never serves.
serving one.
proxy_passdirectives that a new cluster-internal upstream has to be added to the wait. All three ofthem currently point at
backend, so the wait covers every one as it stands.Scope
Three files, none of them touched by the other parts of #4352:
nginx/init/05-wait-for-backend-dns.sh(new) —Dockerfilealready copiesnginx/init/*into/docker-entrypoint.d/, so nothing else is needed to run itnginx/templates/default.conf.templatedeploy/helm/thecombine/charts/frontend/templates/deployment-frontend.yamlIndependent of the other three parts in both directions, textually and functionally: it shares no file with them and
no behavior, so it can merge at any point in the sequence. Verified conflict-free against #4353 and the database part.
QA and prod
Affected. Both changes are in the frontend image and the shared frontend chart, with no target conditionals, so they
ship to
qa-kube.thecombine.appandthecombine.appas well as to the NUCs and desktop installs. On a server whosecluster DNS is already up, the script's first
getentsucceeds and it exits immediately, and the two-minute cap keepsit from ever hanging a deploy. The readiness probe changes when the frontend joins its Service endpoints on every
target.
Testing
Not yet exercised on hardware. The failure it fixes needs a cold cluster start to reproduce, so it wants a laptop or
NUC install rather than a QA deploy, where DNS is up before the frontend rolls.
🤖 Generated with Claude Code
This change is
Summary by CodeRabbit
Bug Fixes
Documentation