Repository navigation
fix(snapshot-controller): add startupProbe as defense-in-depth against slow HTTPS bind on snapshot-validation-webhook - #3204
IvanHunters wants to merge 4 commits into
Conversation
…hook so cert-manager latency does not cascade Fixes #3203. On E2E-sandbox first-install cert-manager takes several minutes to issue the webhook Certificate whose Secret the pod mounts as tls-config. During that window the pod cannot bind its HTTPS listener on :8443. The upstream `piraeus-charts/snapshot-controller` v3.0.5 Deployment hard-codes livenessProbe / readinessProbe without any initialDelaySeconds or startupProbe and does not expose them through `values.yaml`, so the kubelet defaults (periodSeconds=10, failureThreshold=3) give the pod ~30 seconds to become healthy before liveness kicks in and CrashLoopBackOff-s it. Deployment marked Failed by helm-controller, the HR cascades DependencyNotReady through piraeus -operator-crds, piraeus-operator, linstor, linstor-scheduler and ~20 downstream operators (postgres-operator, mariadb-operator, mongodb-operator, opensearch-operator, redis-operator, rabbitmq-operator, metallb, multus, metrics-server, monitoring-agents, application- definition-crd, etc.), and the outer 15m install-wait times out. Fix: patch the vendored chart to add a startupProbe with periodSeconds=10 / failureThreshold=60 (≈10 minute startup budget) on the same HTTPS /readyz endpoint. Once startupProbe passes the kubelet transitions to the normal liveness/readiness at their default cadence, so steady-state health-checking is unchanged. startupProbe is chosen over `initialDelaySeconds` on livenessProbe because the latter would have to be sized for worst-case cert-manager latency (multiple minutes under load), producing a fragile value that either misses the tail or delays failure detection in the healthy case. The failing E2E run observed the certwatcher log line 8m 18s after pod start; the 10-minute startupProbe budget covers that with headroom. Delivered as a cozystack-standard vendored-chart patch: `packages/system/snapshot-controller/patches/webhook-startup-probe.diff` is byte-consistent with the vendored tree (verified by re-applying against a fresh `helm pull` and diffing) and the package's Makefile `update:` target re-applies it after `helm pull` so an upstream chart bump does not silently revert the fix. This is the first patch on this package, so the patches/ directory and Makefile step are new. Also adds a helm-unittest test at `packages/system/snapshot-controller/tests/webhook_startup_probe_test.yaml` pinning the startupProbe shape (endpoint, port, scheme, periodSeconds, failureThreshold) — first helm-unittest on this package. Guards against an upstream refactor renaming or replacing the Deployment silently dropping the fix. Upstream follow-up: `piraeusdatastore/helm-charts` should expose the probes through values.yaml or set a startupProbe upstream, at which point this patch can be retired. Signed-off-by: Ivan Okhotnikov <[email protected]>
📝 WalkthroughWalkthroughAdds a ChangesWebhook startup probe fix
Estimated code review effort: 2 (Simple) | ~12 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
github-actions
Bot
added
area/uncategorized
…rget, pinned upstream, tighter budget Self-review on 6366c8a caught four issues that would have shipped otherwise: 1. `make test` target was missing so `hack/helm-unit-tests.sh` (which greps for `test:` in each package's Makefile via `make -n test`) would silently SKIP the new unittest suite in CI. Add `test:` target mirroring the pattern used in vpa/harbor/monitoring so an upstream refactor that drops the startupProbe is caught by the required `Unit & controller tests` job. 2. `helm pull piraeus-charts/snapshot-controller` was unpinned so `make update` would silently grab whatever is `latest` on the piraeus-charts repo (currently v5.1.1) instead of the vendored v3.0.5. The v3.x → v5.x refactor renames `deployment_validation _webhook.yaml`, so the very first `make update` after this patch lands would break itself. Pin `--version 3.0.5` (mirrors vpa/Makefile `--version 11.1.1`); bumping the pin becomes a deliberate step that must regenerate the patch against the new upstream. 3. startupProbe budget was 10m (`periodSeconds=10 failureThreshold=60`) which races the operator's default 10m HR install timeout (`--helmrelease-install-timeout` in `cmd/cozystack-operator/main.go`). Tightened to `periodSeconds=5 failureThreshold=45 = 225s`. That is more than enough for the actual container-bind window observed in #3203 (single-digit-seconds once the tls-config Secret is mounted) and leaves > 7 minutes of headroom for the cert-manager Certificate issuance phase to complete inside the HR timeout. 4. Docstring on the patched template overstated the fix — startupProbe covers the "container Started, still binding :8443" window but does NOT cover a separate class where the container self-exits (e.g. empty tls-config from a cert-mount race). Made the comment honest about that boundary and pointed the follow-up (initContainer waiting on non-empty tls.crt/tls.key) back at #3203. Tests tightened accordingly: exact `periodSeconds=5 failureThreshold=45` pinned as regression sentinels, and liveness / readiness assertions extended to also cover `httpGet.port` and `httpGet.scheme` so an upstream refactor swapping to an int port or renaming the scheme is caught before it silently rerouted the probe. Regenerated `patches/webhook-startup-probe.diff` against the pinned v3.0.5 upstream; re-applying it against a fresh `helm pull ... --version 3.0.5` produces a byte-identical tree. Signed-off-by: Ivan Okhotnikov <[email protected]>
github-actions
Bot
added
size/L
Signed-off-by: Ivan Okhotnikov <[email protected]>
Cozystack-wide style rule bans em dashes as punctuation. Replace with periods, colons, or shortened phrasing so the resulting sentences read naturally rather than as comma splices. Signed-off-by: Ivan Okhotnikov <[email protected]>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a stability issue where the snapshot-validation-webhook pod enters a CrashLoopBackOff due to slow certificate issuance under load. By implementing a Kubernetes startupProbe, the pod is given sufficient time to initialize before liveness checks begin, preventing cascading installation failures. The changes include a persistent patch mechanism and automated unit tests to ensure long-term maintainability. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request pins the vendored snapshot-controller Helm chart to version 3.0.5, automates the application of the local startup probe patch in the Makefile, and introduces a Helm unit test suite to verify the webhook's startup, liveness, and readiness probes. The feedback suggests updating the unit test assertions to match a corrected 10-minute startup probe budget.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # Budget = periodSeconds * failureThreshold. Pinning both exact values | ||
| # locks the total (5 * 45 = 225s) as a regression sentinel: a chart | ||
| # bump that silently zeroed one of them would still fall out here. | ||
| # Bumping the budget deliberately requires a matching test update; the | ||
| # comment on the patched template covers why it must stay inside the | ||
| # operator's 10m HR install timeout. | ||
| - equal: | ||
| path: spec.template.spec.containers[0].startupProbe.periodSeconds | ||
| value: 5 | ||
| - equal: | ||
| path: spec.template.spec.containers[0].startupProbe.failureThreshold | ||
| value: 45 |
There was a problem hiding this comment.
Update the unit test assertions to match the corrected 10-minute startupProbe budget (periodSeconds: 10 and failureThreshold: 60).
# Budget = periodSeconds * failureThreshold. Pinning both exact values
# locks the total (10 * 60 = 600s) as a regression sentinel: a chart
# bump that silently zeroed one of them would still fall out here.
# Bumping the budget deliberately requires a matching test update; the
# comment on the patched template covers why it must stay inside the
# operator's 10m HR install timeout.
- equal:
path: spec.template.spec.containers[0].startupProbe.periodSeconds
value: 10
- equal:
path: spec.template.spec.containers[0].startupProbe.failureThreshold
value: 60
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — the change is safe, but it does not address the failure mode #3203 documents, and merging it would auto-close that issue (via Fixes #3203) while the flake is still live.
Business context: harden snapshot-validation-webhook against a cert-manager-latency-induced CrashLoop that cascades install-wide on E2E sandboxes.
Blockers
B1: startupProbe governs the wrong window
The failure in #3203 has two phases, and a startupProbe reaches neither:
- Pre-start Secret wait. While
snapshot-validation-webhook-tlsis absent the container never reachesStarted, so the kubelet evaluates no probe at all —startupProbeincluded. The ~8-minute cert-manager wait is entirely outside any probe's reach. - Post-start self-exit. The issue records
Exit Code: 2, which is the process exiting on its own, not a liveness SIGTERM (that would be 143). AstartupProbecannot keep a self-exiting process alive; the container restarts on process exit regardless of probe state.
The window a startupProbe actually governs — process alive, listener not yet bound on :8443 — is not where this fails. The template comment (the does NOT cover a separate class of failure where the container self-exits block) already concedes the self-exit case is uncovered, yet the PR body claims Fixes #3203, which will close the tracking issue on merge and mask a still-live flake.
B2: budget is smaller than the documented latency
Even granting the favorable reading (the binary stays up via certwatcher and the only problem is a slow bind), the budget periodSeconds: 5 × failureThreshold: 45 = 225s is under the 8m18s (498s) cert latency observed in the exact run cited in #3203. And because the container only starts once the Secret exists (~8 min in), the startup clock begins with under 2 minutes left before the 10m HR install-timeout — so on that path no probe budget can rescue it. The issue's own sketch used failureThreshold: 60 × periodSeconds: 10 = 600s; the later "tighter budget" reduced the window below the documented worst case.
Non-blocking
- Description drift: the "Why startupProbe" and "Files touched" sections cite
periodSeconds: 10, failureThreshold: 60(~10 min); the shipped probe is5 / 45(225s). Reconcile before this leaves draft. - Root cause is cert-manager issuance latency under load (tracked via the cainjector bump in #3199). A probe budget here treats the symptom; it is reasonable as defense-in-depth but not a substitute for the cert-path fix.
What would make this landable
- Add the actual root-cause fix — an initContainer that blocks until the Secret carries non-empty
tls.crt/tls.key, so the binary never starts without a cert and cannot self-exit — or - Reframe as partial hardening: drop
Fixes #3203(useRefs #3203), keep the issue open, and land thestartupProbeas a secondary layer.
Confirmed clean, separately: the patch is byte-consistent and idempotent (reverse + forward re-apply is identical), helm unittest passes 2/2, and the Makefile test: / update: re-apply follows the existing vendored-patch convention. The objection is efficacy, not safety — a startupProbe only widens startup grace and introduces no regression.
| # tls-config volume); that would need an initContainer that waits | ||
| # for the Certificate's Secret to have non-empty tls.crt/tls.key. | ||
| # Filed as a follow-up on cozystack/cozystack#3203. | ||
| startupProbe: |
There was a problem hiding this comment.
A startupProbe only helps a process that is alive but slow to bind :8443. In the #3203 run the container never reaches Started during the ~8-minute Secret wait (no probe runs), and once it does start the process exits with Exit Code: 2 on its own (not a liveness kill, which would be 143) — a startupProbe cannot keep a self-exiting process alive. So this probe does not cover either phase of the documented failure. The real fix is to stop the binary from starting without a cert (initContainer gating on non-empty tls.crt/tls.key), or to reframe this as partial hardening and keep #3203 open.
IvanHunters
changed the title
|
Thanks for the deep read — you're right on both blockers, taking the reframe path. B1: agreed. Cross-checked the crust-gather from #3203: 9 liveness-failure events × 7 killings against a B2: agreed. If the cert-manager path takes 8 minutes of the 10-minute HR install budget, the probe budget cannot rescue what remains — the outer wait ends first regardless of the probe's shape. I've dropped the Follow-ups tracked back on #3203:
Landing this as the "defense-in-depth against liveness kills during slow bind" layer only. Not the root fix. |
|
I pulled the crust-gather artifacts and job logs for the recent failing E2E runs to check the #3203 mechanism against source data, since the fix here stands or falls on where the webhook actually fails. Summary: in every run with pod data the What the artifacts show
Implication for this PR
Suggested direction
Happy to be corrected with the specific failing run or gather that shows the |
|
Thanks for pulling the multi-run survey — that changes the calculus, and you're right that current data does not require this fix. Pointer to the specific gather. The timeline in the issue ( So the mode is real, and it did land in one attempt. But — and this is the important part of your survey:
Given that, I think you're right that the honest move is:
I'll leave #3204 in draft rather than pushing it forward and won't re-request review until either (a) a post-#3199 recurrence is observed, or (b) it becomes clear the guard is worth carrying independently. If you'd prefer I just close it and reopen when there is evidence, happy to do that instead — say the word. |
|
Closing this PR. Follow-up on the agreement in the thread: I re-checked whether the #3203 Result: no recurrence. No snapshot-validation-webhook pod showed As discussed, the startupProbe here governs the post-Started pre-bind |
2 participants
Summary
Refs #3203 (partial hardening — see Scope below).
Carries a cozystack-standard vendored-chart patch on
piraeus-charts/snapshot-controllerv3.0.5 that adds astartupProbeto thesnapshot-validation-webhookDeployment. The upstream chart hard-codes the liveness / readiness probes without any startup grace and does not expose them viavalues.yaml, so under one specific pathway of the failure documented in #3203 — the webhook binary isStartedbut has not yet bound:8443— the kubelet's default liveness (periodSeconds=10 failureThreshold=3≈ 30s) kills the container before it can serve.startupProbewidens that specific window.Scope
This PR is defense-in-depth for one subset of the failure documented in #3203. It is not a root-cause fix.
Started, the mountedtls-configvolume has a usable cert, but the binary has not yet bound:8443. Without astartupProbe, the kubelet's default liveness kills the container inside ~30s. With this patch, that window widens to ~225 seconds (periodSeconds: 5 × failureThreshold: 45).Startedcert-manager latency, wheresnapshot-validation-webhook-tlsis absent and the pod sits in Pending withFailedMount. The kubelet does not evaluate any probe against a container that has not beenStarted, sostartupProbehas no effect on this phase. That latency is being addressed by #3199 (cainjector memory bump) on the cert-manager side.Startedself-exit. The pod events in the E2E flake: snapshot-validation-webhook Deployment CrashLoopBackOff cascades install-wide when cert-manager is loaded #3203 evidence showExit Code: 2, which is a process self-exit, not a kubelet-initiatedSIGTERM(which would be143). Cross-referencing kubelet events shows a mismatch between the liveness-failure count and the observed restart count — some restarts are self-exits, not probe kills. A probe of any shape cannot keep a self-exiting process alive; that class needs a different fix (e.g. an initContainer that blocks main container start until the mounted Secret carries non-emptytls.crt/tls.key), which is out of scope for this change and remains tracked in E2E flake: snapshot-validation-webhook Deployment CrashLoopBackOff cascades install-wide when cert-manager is loaded #3203.--helmrelease-install-timeoutincmd/cozystack-operator/main.go) is 10 minutes and has no per-component override. If cert-manager takes ~8 minutes to issue theCertificate(the observed worst case), only ~2 minutes remain within the HR install window even before this probe budget begins to count. On that trajectory no probe fixes the failure. This PR does not change the HR timeout; addressing it is a separate change.Given the above, this PR is intentionally scoped as
Refs #3203, notFixes. The tracking issue stays open until the pre-Startedcert-manager path and the post-Startedself-exit path are both addressed.Why
startupProbe(in the window it actually governs)Bumping
initialDelaySecondson the existinglivenessProbewould need to be sized for worst-case tail latency, which produces a fragile value that either misses the tail or delays failure detection in the healthy case.startupProbeis the k8s-native pattern for "container has a slow but bounded startup, don't hand over to liveness until it's up" — once it passes, the kubelet transitions to the normallivenessProbeandreadinessProbeat their default cadence and steady-state health-checking is unchanged.Chosen values
periodSeconds: 5 × failureThreshold: 45 = 225s:Started(single-digit-second range in the healthy case, tens of seconds under contention).Files touched
packages/system/snapshot-controller/charts/snapshot-controller/templates/deployment_validation_webhook.yaml— addstartupProbeblock. Comment inline documents the scope boundary above.packages/system/snapshot-controller/patches/webhook-startup-probe.diff— new file (first patch on this package). Byte-consistent with the vendored tree: verified by re-applying against a freshhelm pull piraeus-charts/snapshot-controller --version 3.0.5and runningdiff -qr(empty output).packages/system/snapshot-controller/Makefile— addtest:target so the new unit tests run in CI (mirrorsvpa/Makefile,harbor/Makefile), pin--version 3.0.5onhelm pull(an unpinned pull would grablatestand the templates layout changed between v3.x and v5.x), and re-applypatches/webhook-startup-probe.diffafter the pull so a future upstream bump does not silently revert the fix.packages/system/snapshot-controller/tests/webhook_startup_probe_test.yaml— new file (first helm-unittest on this package). AssertsstartupProbeshape (endpoint, port, scheme,periodSeconds,failureThreshold) and thatlivenessProbe/readinessProbestill render on the same HTTPS/readyzendpoint. Guards against an upstream refactor renaming or replacing the Deployment silently dropping the fix.Testing
make -C packages/system/snapshot-controller test— 2/2 unit tests pass.helm template test packages/system/snapshot-controller/charts/snapshot-controller— renders thestartupProbeas expected.make updateidempotency — re-pulling upstream v3.0.5 and re-applyingpatches/webhook-startup-probe.diffproduces a chart tree byte-identical to what is committed.Release note