Repository navigation
fix(external-dns): add startupProbe to survive slow first start under load - #3461
Draft
IvanHunters wants to merge 3 commits into
Draft
IvanHunters wants to merge 3 commits into
IvanHunters wants to merge 3 commits into
Conversation
… load The external-dns container binds its /healthz server on :7979 only after the informer source-cache sync completes. The upstream chart templates no startupProbe, so a slow first start has no grace period distinct from the tight livenessProbe (initialDelaySeconds 10 + periodSeconds 10 * failureThreshold 2 = ~30s). Under CI-cluster load the sync outlasts that budget, kubelet SIGKILLs a healthy-but-slow container, and the restart loop cascades into the wrapping HelmRelease timing out and the external-dns-inmemory E2E case failing. Render a values-driven startupProbe on /healthz:7979 (budget 5s * 45 = 225s, well inside the operator's 10m HelmRelease install timeout) so liveness only begins evaluating after the slow-start window. Carried as a patch on the vendored chart and re-applied on 'make update'; the version pin guards the patch against an upstream layout shift. Signed-off-by: IvanHunters <[email protected]>
Guards the vendored external-dns startupProbe patch against silent loss: a 'make update' bump that re-pulls upstream and drops the local diff, or an upstream refactor that renames the Deployment or probe endpoint, fails these asserts before the E2E flake can return. Also pins the 5s * 45 budget as a regression sentinel and checks liveness/readiness survive the patch unchanged. Signed-off-by: IvanHunters <[email protected]>
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
📝 WalkthroughWalkthroughExternalDNS now defines an HTTP startup probe, conditionally renders it in the deployment, tests its configuration, and refreshes the vendored chart using a pinned upstream version plus a local patch. ChangesExternalDNS startup probe
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
The explanatory comment sat as literal YAML '#' lines inside the {{- with }}
block, so it rendered into 'helm template' output and onto the live Deployment
object. Move it into a Helm {{/* */}} template comment: the rationale stays in
the chart source for maintainers but no longer leaks into rendered manifests.
Patch regenerated against pristine upstream 1.20.0; forward-apply, reverse-apply
byte-consistency, and helm-unittest all re-verified.
Signed-off-by: IvanHunters <[email protected]>
IvanHunters
marked this pull request as ready for review
July 27, 2026 19:08
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
dosubot
Bot
added
area/platform
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/system/external-dns/charts/external-dns/templates/deployment.yaml`:
- Around line 160-176: Move the startupProbe change out of the vendored
deployment template and into the repository-owned external-dns chart patch,
restoring the generated template to upstream form. Ensure the referenced patch
exists and run the project’s make update workflow to regenerate the chart; if
checked-in generated changes are intentional, update the project convention
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2450a5c2-6045-43ab-9e1f-aae30556a87d
📒 Files selected for processing (5)
packages/system/external-dns/Makefilepackages/system/external-dns/charts/external-dns/templates/deployment.yamlpackages/system/external-dns/patches/deployment-startup-probe.diffpackages/system/external-dns/tests/deployment_startup_probe_test.yamlpackages/system/external-dns/values.yaml
Comment on lines
+160
to
+176
| {{- /* | ||
| cozystack (patches/deployment-startup-probe.diff): the upstream | ||
| external-dns chart templates only liveness/readiness and renders no | ||
| startupProbe, so a slow first start has no grace period distinct | ||
| from liveness. Under E2E-sandbox load the informer source-cache sync | ||
| outlasts the liveness budget (initialDelaySeconds 10 + periodSeconds | ||
| 10 * failureThreshold 2 = ~30s), kubelet SIGKILLs a healthy-but-slow | ||
| container before it binds /healthz on :7979, and the restart loop | ||
| cascades into the wrapping HelmRelease timing out. A values-driven | ||
| startupProbe gates liveness on the slow-start window instead. Rendered | ||
| with `with` so an unset value is a no-op (upstream-compatible). | ||
| See cozystack/cozystack#3459. | ||
| */}} | ||
| {{- with .Values.startupProbe }} | ||
| startupProbe: | ||
| {{- toYaml . | nindent 12 }} | ||
| {{- end }} |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files around target:"
git ls-files packages/system/external-dns/charts/external-dns/templates/deployment.yaml patches/deployment-startup-probe.diff 2>/dev/null || true
echo
echo "Git status/stat summary:"
git diff --stat -- packages/system/external-dns/charts/external-dns/templates/deployment.yaml patches/deployment-startup-probe.diff 2>/dev/null || true
echo
echo "Target diff context:"
git diff -- packages/system/external-dns/charts/external-dns/templates/deployment.yaml patches/deployment-startup-probe.diff 2>/dev/null | sed -n '1,220p'
echo
echo "README/docs references to chart vendoring/update/patches:"
rg -n --hidden --glob '!*.png' --glob '!*.jpg' --glob '!*.jpeg' \
'packages/[^/]*/charts|vendored|make update|update|deployment-startup-probe.diff|patches/' \
README.md docs Makefile .github 2>/dev/null || trueRepository: cozystack/cozystack
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Patch file existence/content:"
if [ -f patches/deployment-startup-probe.diff ]; then
wc -l patches/deployment-startup-probe.diff
sed -n '1,120p' patches/deployment-startup-probe.diff
else
echo "missing patches/deployment-startup-probe.diff"
fi
echo
echo "Target template startupProbe context:"
sed -n '130,190p' packages/system/external-dns/charts/external-dns/templates/deployment.yamlRepository: cozystack/cozystack
Length of output: 2954
Do not hand-edit the vendored chart template.
packages/system/external-dns/charts/external-dns/templates/deployment.yaml is in the protected packages/**/charts/** tree, and the referenced repo patch file is missing. Keep changes in the repository-owned patch and regenerate the chart template through make update, or update the project convention if checked-in generated diffs are intentional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/system/external-dns/charts/external-dns/templates/deployment.yaml`
around lines 160 - 176, Move the startupProbe change out of the vendored
deployment template and into the repository-owned external-dns chart patch,
restoring the generated template to upstream form. Ensure the referenced patch
exists and run the project’s make update workflow to regenerate the chart; if
checked-in generated changes are intentional, update the project convention
accordingly.
Source: Coding guidelines
github-actions
Bot
added
area/uncategorized
IvanHunters
marked this pull request as draft
July 27, 2026 19:58
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
1 participant
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.
What this PR does
Fixes the
external-dns/external-dns-inmemoryChainsaw E2E flake (#3459).The
external-dnscontainer binds its/healthzserver on:7979only after the informer source-cache sync completes. The upstream chart templates nostartupProbe, so a slow first start has no grace period distinct from the tightlivenessProbe(initialDelaySeconds 10 + periodSeconds 10 × failureThreshold 2 ≈ 30s). Under CI-cluster load the sync outlasts that budget, kubelet SIGKILLs a healthy-but-slow container (exitCode 137), and the restart loop cascades: Deployment never Ready → HRcozy-external-dnsInstallFailed → HRexternal-dnsInstallFailed → Chainsaw case FAIL.This is not a regression of any feature PR: in the same run
kubernetes-previousandkubernetes-latestpass; the external-dns chart is orthogonal.Fix: render a values-driven
startupProbeon/healthz:7979solivenessProbeonly begins evaluating after the slow-start window. Budget = periodSeconds 5 × failureThreshold 45 = 225s, comfortably clearing the single-digit-second healthy bind time while staying well inside the operator's default 10m HelmRelease install timeout. Same class of fix as #3204 (snapshot-controller), applied to a different component.The template stanza is rendered with
{{- with .Values.startupProbe }}, so an unset value is a no-op (upstream-compatible). Because the chart is vendored viahelm pull, the template change is carried aspatches/deployment-startup-probe.diffand re-applied bymake update; the--version 1.20.0pin guards the patch against an upstream layout shift. A helm-unittest locks the patch in and pins the budget as a regression sentinel.Downstream repositories
Walked the trigger map file-by-file against the diff. This is internal probe tuning on a vendored system chart: no API/CRD change, no user-facing values schema, no docs surface.
Release note
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Closes #3459