fix(velero): add startupProbe so slow startup does not crashloop the install gate - #3138
Conversation
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 crashloop issue occurring during heavy parallel platform installs where the Velero container is terminated by the kubelet before its health endpoint is ready. By delaying the initial liveness check, the container is granted sufficient time to initialize its plugins and connect to the API server, ensuring successful deployment without compromising steady-state health monitoring. 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. 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
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Velero ChangesVelero startup probe
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 |
There was a problem hiding this comment.
Code Review
This pull request configures a delayed liveness probe for Velero in values.yaml to prevent crashlooping during slow startups under heavy parallel installs, and adds corresponding assertions in velero_test.yaml. Feedback includes expanding the test assertions to cover all custom liveness probe fields (timeoutSeconds and successThreshold) and ensuring the PR description contains the required release-note block as specified in the repository guidelines.
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.
| - equal: | ||
| path: spec.template.spec.containers[0].livenessProbe.initialDelaySeconds | ||
| value: 300 | ||
| - equal: | ||
| path: spec.template.spec.containers[0].livenessProbe.periodSeconds | ||
| value: 30 | ||
| - equal: | ||
| path: spec.template.spec.containers[0].livenessProbe.failureThreshold | ||
| value: 5 | ||
| - equal: | ||
| path: spec.template.spec.containers[0].livenessProbe.httpGet.path | ||
| value: /metrics | ||
| - equal: | ||
| path: spec.template.spec.containers[0].livenessProbe.httpGet.port | ||
| value: http-monitoring |
There was a problem hiding this comment.
The test assertions for the liveness probe do not cover all the custom fields defined in values.yaml (specifically timeoutSeconds and successThreshold). Adding assertions for these fields ensures they are also protected against silent regressions.
- equal:
path: spec.template.spec.containers[0].livenessProbe.initialDelaySeconds
value: 300
- equal:
path: spec.template.spec.containers[0].livenessProbe.periodSeconds
value: 30
- equal:
path: spec.template.spec.containers[0].livenessProbe.timeoutSeconds
value: 5
- equal:
path: spec.template.spec.containers[0].livenessProbe.successThreshold
value: 1
- equal:
path: spec.template.spec.containers[0].livenessProbe.failureThreshold
value: 5
- equal:
path: spec.template.spec.containers[0].livenessProbe.httpGet.path
value: /metrics
- equal:
path: spec.template.spec.containers[0].livenessProbe.httpGet.port
value: http-monitoring| # first liveness check; keep the steady-state cadence (periodSeconds and | ||
| # failureThreshold) at the upstream defaults so a genuinely wedged server is | ||
| # still restarted promptly once it is up. | ||
| livenessProbe: |
There was a problem hiding this comment.
The PR description is missing the required release-note block. According to the repository guidelines, every PR body must contain a release note block formatted as:
```release-note
type(scope): human-readable changelog entry
Please update the PR description to include this block.
<details>
<summary>References</summary>
1. PR body must contain a release note block: ```release-note ... ``` <sup>([link](https://github.com/cozystack/cozystack/blob/main/.gemini/styleguide.md))</sup>
</details>
d9c01ec to
e2f4991
Compare
e2f4991 to
e64ccea
Compare
e64ccea to
68ec596
Compare
myasnikovdaniil
left a comment
There was a problem hiding this comment.
Correct fix for a real, recurring install-gate failure. Adding a startupProbe is exactly the right tool here — it holds liveness and readiness off while velero loads plugins, then hands over to the unchanged tight upstream defaults for steady-state crash detection, which is strictly better than loosening liveness. The vendored-patch approach (-p4, Makefile update target re-applies) follows the same convention as seaweedfs, ingress-nginx, etc. Two regression tests gate the probe budget and the metrics-off path. Three optional nits inline.
| # during backups of very large volumes. The Velero default is 4 hours. | ||
| defaultItemOperationTimeout: 24h | ||
|
|
||
| # Velero binds its metrics/health endpoint (:8085, http-monitoring) only after |
There was a problem hiding this comment.
Nit (optional): This 14-line comment block is the most verbose in the file and significantly more than the rest of the codebase's convention (cf. packages/system/kamaji/values.yaml — startupProbe: with zero comment). The same rationale lives in the PR body and the test-case comment block. Consider trimming:
# Velero binds :8085 only after loading plugins — the upstream liveness probe kills
# it mid-startup (~130s budget) before the port opens. This probe grants 300s of
# startup grace (30 * 10s) without loosening steady-state liveness.
# Upstream chart omits startupProbe; patches/add-startup-probe.patch adds it.
startupProbe:Not a blocker.
| httpGet: | ||
| path: /metrics | ||
| port: http-monitoring | ||
| scheme: HTTP |
There was a problem hiding this comment.
Nit: scheme: HTTP is the Kubernetes default for httpGet probes — it can be dropped without any behavioural change. The upstream chart's own livenessProbe values carry it too, so this is consistent, just redundant. Take or leave.
| value: http-monitoring | ||
| # No initialDelaySeconds: the first startup check fires immediately, so the | ||
| # full 300s budget is spent probing. Pin its absence so a future change | ||
| # cannot silently re-introduce a delay that eats into the grace window. |
There was a problem hiding this comment.
Nit: The comment says initialDelaySeconds would "re-introduce a delay that eats into the grace window" — but adding IDS would actually expand the total budget (first probe fires later, container lives longer before the 30th failure). The real reason to pin its absence is: without IDS, probing starts immediately at container start so none of the 300s budget is wasted idle before the first check.
Suggested rewording:
# No initialDelaySeconds (defaults to 0): probing starts at container start,
# so none of the 300s budget is spent idle before the first check fires.68ec596 to
051f73d
Compare
…install gate Velero binds its metrics/health endpoint (:8085, http-monitoring) only after the server loads its plugins and connects to the API server. Under a heavy parallel platform install that can take minutes, but the upstream liveness probe begins checking /metrics at initialDelaySeconds=10 and kills the container after failureThreshold=5 (~130s total) before :8085 is bound. The kubelet SIGTERMs velero (exitCode 0/Completed), it BackOff-restarts, and never escapes the loop inside the install-gate window. Because backupstrategy-controller hard-depends on velero, this blocks the gate. Add a startupProbe to the velero server container. While it is failing the kubelet holds off liveness and readiness entirely, granting failureThreshold * periodSeconds = 30 * 10s = 300s of startup grace, then hands over to the unchanged, tight upstream liveness (10s / 30s / 5) for steady-state crash detection. This is strictly better than loosening liveness, which would also blunt steady-state detection. The upstream velero chart (12.0.3) does not render a startupProbe, so the package patches the deployment template via patches/add-startup-probe.patch (re-applied after re-vendor by the Makefile update target, matching the seaweedfs/ingress-nginx convention) and supplies the startupProbe through the package values override. Pin the startupProbe budget and the unchanged liveness with a helm-unittest regression guard. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
051f73d to
f7271b8
Compare
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-1.5
git worktree add -d .worktree/backport-3138-to-release-1.5 origin/release-1.5
cd .worktree/backport-3138-to-release-1.5
git switch --create backport-3138-to-release-1.5
git cherry-pick -x f7271b80111d98e8772d97939fc45279a55036d3 |
…does not crashloop the install gate (#3533) ## What this PR does Hand-backport of #3138 to `release-1.5`. The `backport` label was applied at merge time but no backport PR was ever opened, so the change never reached this branch — found by auditing the branch with `cmd/backport-audit`, which reported it as `MISSING`. Like the other items in that set, it merged before #3155 fixed the bot's `conflict_resolution` input, so the conflicting cherry-pick was dropped with no draft and no failing check. Velero binds its metrics/health endpoint (`:8085`) only after loading plugins and connecting to the API server. Under a heavy parallel platform install the upstream liveness probe starts checking at `initialDelaySeconds=10` and kills the container after `failureThreshold=5`, so velero BackOff-restarts and never escapes the loop inside the install-gate window. Because `backupstrategy-controller` hard-depends on velero, this blocks the gate. The fix adds a `startupProbe`, which holds liveness and readiness off entirely while it is failing, granting `30 * 10s = 300s` of startup grace and then handing over to the unchanged upstream liveness for steady-state crash detection. ### Backport adaptations This branch vendors velero chart **11.0.0** (appVersion 1.17.0) rather than `main`'s 12.0.3 (1.18.1), so three things differ from the original. All are recorded in the commit message as well. - **The chart version is not bumped.** Only the patch re-application is added to `update`. The `helm pull` here was previously unpinned, which would have let `update` float to a chart the patch does not apply to, so it is pinned to `11.0.0` — the version already vendored. That makes the new patch step deterministic and leaves the vendored tree byte-identical. - **The `test:` target is added.** It exists on `main` but not here, so without it the shipped regression guard would never run. - **The image assertions pin this branch's own versions** (velero `v1.17.0`, `plugin-for-aws` `v1.12.1`, kubevirt plugin `v0.8.0`), and the `upgrade-crds` rationale is restated for chart 11.0.0, where that Job still runs a kubectl image — `main`'s comment about the Job running the velero image natively is true for 12.x only. The `startupProbe` hunk itself applies to chart 11.0.0's deployment template unchanged, in the same position inside the `metrics.enabled` gate. ### Verification `helm unittest .` passes 5/5 in `packages/system/velero`. The assertions are mutually corroborating rather than vacuous: the `equal` assertions prove the `startupProbe` path actually renders, and the same paths are asserted absent under `velero.metrics.enabled: false`, proving it follows the same gate as liveness and readiness so disabling metrics can never leave a `startupProbe` pointed at an unbound port. ### Screenshots Not applicable — no UI change. ### Release note ```release-note fix(velero): add a startupProbe so a slow velero server start under heavy parallel install no longer crashloops the container and blocks the install gate ```
What
Add a
startupProbeto the velero server container (and supply it through the package values), so a slow server start under a heavy parallel install no longer trips the liveness probe. Pin the probe with a helm-unittest regression guard.Why
Velero binds its metrics/health endpoint (
:8085,http-monitoring) only after the server loads its plugins and connects to the API server. Under a heavy parallel platform install that can take minutes, but the upstream liveness probe starts checking/metricsatinitialDelaySeconds=10and kills the container afterfailureThreshold=5(~130s total) — before:8085is bound. The kubelet SIGTERMs velero (exit code 0 / Completed — a graceful kill, not a crash), it BackOff-restarts, and never escapes the loop inside the install-gate window. Becausebackupstrategy-controllerhard-depends on velero, this blocks the install gate.How
While a
startupProbeis failing, the kubelet holds off the liveness and readiness probes entirely. So the startupProbe grants the server an explicit startup budget —failureThreshold * periodSeconds = 30 * 10s = 300sof grace — and then hands over to the unchanged, tight upstream liveness (10s / 30s / 5) for steady-state crash detection. This is strictly better than loosening liveness, which would also blunt steady-state detection for the lifetime of every pod.The upstream velero chart (12.0.3) does not render a
startupProbe, so the package patches the deployment template viapatches/add-startup-probe.patch, re-applied after re-vendor by the Makefileupdatetarget — the same vendored-patch convention used byseaweedfs,ingress-nginx, and the existingstartupProbepatches ingrafana-operatorandkubevirt-cdi-operator. ThestartupProbevalue itself is supplied through the package values override.Notes
Surfaced as a recurring (not flaky) install failure under heavy parallel load. Does not close a specific issue.
Summary by CodeRabbit
startupProbefor the/metricsendpoint (http-monitoring) to better accommodate slow startup.startupProbe,livenessProbe, orreadinessProbe.startupProbebehavior (including pinned timing fields) and confirm existing probe timing remains unchanged.