Repository navigation
fix: startupProbes for kubevirt-cdi-operator and grafana-operator - #2725
Conversation
…lection cdi-operator's controller-runtime starts the health probe HTTP server (:8444) only after the leader election lease is acquired. On a fresh cluster the lease can take 15–20 s, exceeding the liveness probe's 5s + 3×10s grace under load. Kubelet kills the container before the probe can succeed; the pod loops in CrashLoopBackOff, CDI never reconciles its CR, datavolumes.cdi.kubevirt.io stays unavailable, and every downstream test that creates a DataVolume (tenant kubernetes worker VMs, vm-instance) fails. Add a startupProbe that hits /healthz with 30×5s grace (150 s total). Once it succeeds once, liveness/readiness take over with their existing strict config. No effect on healthy clusters; only relaxes the ramp-up window. Implemented as a patches/*.diff applied from the Makefile update target, mirroring the fluxcd-operator pattern. Survives the next upstream regen; if upstream restructures the area around the readinessProbe/resources boundary the patch will fail loudly so the maintainer notices. Co-Authored-By: Claude Opus 4.7 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]> (cherry picked from commit 8f5d45e)
… load
Install Cozystack failed because grafana-operator pod looped in
CrashLoopBackOff under concurrent helm-install load (Flux v2.8
helm-controller dispatching every system HR in parallel + kstatus
walking all applied resources). The chain that fell over:
grafana-operator pod liveness probe :8081/healthz fails 3x in 30s
-> monitoring-rd dependency 'cozy-grafana-operator/grafana-operator'
-> tenant-rd dependency 'cozy-system/monitoring-rd'
-> cozystack-basics dependency 'cozy-system/tenant-rd'
(cozystack-basics creates the cozy-public ns)
-> kubevirt-cdi RoleBinding cdi-clone-dv: namespace
"cozy-public" not found
-> vm-disk-rd, dashboard, flux-plunger all wait, install times out
Pod log shows the manager starts at T+0, controllers spin up workers,
folder/dashboard/datasource sync completes by T+10s — then SIGTERM at
T+27s. Liveness probe is the upstream chart default (delay=0s,
timeout=1s, period=10s, failureThreshold=3) which gives ~30s grace,
not enough on a fresh cluster where controller-runtime's HTTP probe
server is still binding while kubelet starts polling immediately.
Same shape as the kubevirt-cdi-operator commit already in this branch
(37b06e2). Mirror the fix:
- Edit deployment.yaml on the vendored chart to add a startupProbe
(30 x 5s = 150s grace, /healthz on :8081). Once it passes once,
liveness/readiness keep the existing strict config.
- Stash the change as patches/startupProbe.diff and re-apply it from
the Makefile update target after the curl|tar upstream fetch, so
the next chart bump does not silently regress.
The patch uses package-relative paths (a/charts/...) so `patch -p1`
from the package dir resolves correctly — repo-relative paths would
need patch's fuzzy fallback which is unreliable across distros.
Signed-off-by: Myasnikov Daniil <[email protected]>
(cherry picked from commit 40949c8)
github-actions
Bot
added
area/uncategorized
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 issues where operator pods were failing readiness checks during cold cluster installs due to slow initialization or high load. By introducing startup probes, the operators are granted a sufficient grace period to reach a steady state before the more aggressive readiness probes begin monitoring them, thereby preventing unnecessary service disruptions and blocking dependencies. 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
|
📝 WalkthroughWalkthroughThis PR adds Kubernetes startup probes to two operators: grafana-operator and kubevirt-cdi-operator. Each operator receives an HTTP-based startup probe configured to hit its existing ChangesOperator Startup Probes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
dosubot
Bot
added
area/monitoring
![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 adds startupProbe configurations to the grafana-operator and kubevirt-cdi-operator packages by modifying their Makefiles to apply patches. The review identifies that the patch command for kubevirt-cdi-operator uses an incorrect path depth which will lead to failure. Additionally, the reviewer suggests switching the probe endpoint from /healthz to /readyz to properly handle leader election during the startup phase.
| wget https://github.com/kubevirt/containerized-data-importer/releases/download/$$VERSION/cdi-operator.yaml -O templates/cdi-operator.yaml | ||
| sed -i 's/namespace: cdi/namespace: cozy-kubevirt-cdi/g' templates/cdi-operator.yaml | ||
| awk -i inplace -v RS="---" '!/kind: Namespace/{printf "%s", $$0 RS}' templates/cdi-operator.yaml | ||
| patch --no-backup-if-mismatch -p1 < patches/startupProbe.diff |
There was a problem hiding this comment.
The patch command uses -p1, but the patch file patches/startupProbe.diff contains paths relative to the repository root (e.g., a/packages/system/kubevirt-cdi-operator/...). When running make update from the package directory, this will cause the patch to fail as it will look for a packages/ subdirectory. Please recreate the patch file relative to the package root (similar to the one in grafana-operator) or adjust the -p level to -p4.
References
- Structural changes to vendored or downloaded manifests should be applied via patch files in the package root, and the Makefile must correctly apply them.
| timeoutSeconds: 10 | ||
| startupProbe: | ||
| httpGet: | ||
| path: /healthz |
There was a problem hiding this comment.
The startupProbe is configured to check /healthz, but the PR description indicates that the primary issue is with the readinessProbe failing during leader election. In controller-runtime based operators, /healthz often becomes healthy as soon as the process starts, while /readyz waits for leader election. To effectively suppress readiness probe noise and prevent dependants from blocking prematurely, the startupProbe should target the /readyz endpoint.
path: /readyz| mkdir -p charts | ||
| curl -sSL https://github.com/grafana-operator/grafana-operator/archive/refs/heads/master.tar.gz | \ | ||
| tar xzvf - --strip 3 -C charts grafana-operator-master/deploy/helm/grafana-operator | ||
| patch --no-backup-if-mismatch -p1 < patches/startupProbe.diff |
There was a problem hiding this comment.
The patch patches/startupProbe.diff being applied here uses the /healthz endpoint for the startupProbe, while the readinessProbe in the upstream chart uses /readyz. If the goal is to prevent readiness probe failures during a long bootstrap or reconcile loop, the startupProbe should likely target the /readyz endpoint to ensure the tight readiness checks only begin once the operator is truly ready.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/kubevirt-cdi-operator/patches/startupProbe.diff`:
- Around line 1-4: The patch header in startupProbe.diff uses repo-root-relative
paths (packages/system/kubevirt-cdi-operator/...) which don't match the
Makefile's patch -p1 context; update the diff headers so paths are package-local
(e.g., change
a/packages/system/kubevirt-cdi-operator/templates/cdi-operator.yaml and b/... to
a/templates/cdi-operator.yaml and b/templates/cdi-operator.yaml) so the patch
applies correctly when run from the package directory; verify the hunk paths in
startupProbe.diff and any other patch files in this package follow the same
package-local convention.
🪄 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
Run ID: 9c0d2e3e-4854-4e28-a37a-c84d29009568
📒 Files selected for processing (6)
packages/system/grafana-operator/Makefilepackages/system/grafana-operator/charts/grafana-operator/templates/deployment.yamlpackages/system/grafana-operator/patches/startupProbe.diffpackages/system/kubevirt-cdi-operator/Makefilepackages/system/kubevirt-cdi-operator/patches/startupProbe.diffpackages/system/kubevirt-cdi-operator/templates/cdi-operator.yaml
| diff --git a/packages/system/kubevirt-cdi-operator/templates/cdi-operator.yaml b/packages/system/kubevirt-cdi-operator/templates/cdi-operator.yaml | ||
| index f46ba09e6..03ac3b3a2 100644 | ||
| --- a/packages/system/kubevirt-cdi-operator/templates/cdi-operator.yaml | ||
| +++ b/packages/system/kubevirt-cdi-operator/templates/cdi-operator.yaml |
There was a problem hiding this comment.
Patch header path is incompatible with the current patch -p1 invocation.
Line 1 uses repo-root-relative paths (packages/system/kubevirt-cdi-operator/...), but this package Makefile applies the patch from packages/system/kubevirt-cdi-operator with -p1. That combination can fail to locate the target file and break update.
💡 Suggested fix (normalize patch paths to package-local)
-diff --git a/packages/system/kubevirt-cdi-operator/templates/cdi-operator.yaml b/packages/system/kubevirt-cdi-operator/templates/cdi-operator.yaml
+diff --git a/templates/cdi-operator.yaml b/templates/cdi-operator.yaml
index f46ba09e6..03ac3b3a2 100644
---- a/packages/system/kubevirt-cdi-operator/templates/cdi-operator.yaml
-+++ b/packages/system/kubevirt-cdi-operator/templates/cdi-operator.yaml
+--- a/templates/cdi-operator.yaml
++++ b/templates/cdi-operator.yaml📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| diff --git a/packages/system/kubevirt-cdi-operator/templates/cdi-operator.yaml b/packages/system/kubevirt-cdi-operator/templates/cdi-operator.yaml | |
| index f46ba09e6..03ac3b3a2 100644 | |
| --- a/packages/system/kubevirt-cdi-operator/templates/cdi-operator.yaml | |
| +++ b/packages/system/kubevirt-cdi-operator/templates/cdi-operator.yaml | |
| diff --git a/templates/cdi-operator.yaml b/templates/cdi-operator.yaml | |
| index f46ba09e6..03ac3b3a2 100644 | |
| ++ b/templates/cdi-operator.yaml |
🤖 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/kubevirt-cdi-operator/patches/startupProbe.diff` around lines
1 - 4, The patch header in startupProbe.diff uses repo-root-relative paths
(packages/system/kubevirt-cdi-operator/...) which don't match the Makefile's
patch -p1 context; update the diff headers so paths are package-local (e.g.,
change a/packages/system/kubevirt-cdi-operator/templates/cdi-operator.yaml and
b/... to a/templates/cdi-operator.yaml and b/templates/cdi-operator.yaml) so the
patch applies correctly when run from the package directory; verify the hunk
paths in startupProbe.diff and any other patch files in this package follow the
same package-local convention.
Aleksei Sviridkin (lexfrei)
removed
the
area/uncategorized
3 participants
What this PR does
Adds
startupProbeto two operator deployments whose readiness probes fail repeatedly during a cold cluster install, generating noise and (for CDI) blocking dependants on slow leader-election:kubevirt-cdi-operator— leader-election against a slowkube-apiserver(Kamaji control plane bringup) consistently takes >30s on a cold install. The default readinessProbe with 1speriodSeconds/ 3failureThresholdfires the first time election times out, the operator pod gets marked NotReady, and dependants block on it.grafana-operator— bootstrap CRD discovery + Helm-managed Dashboard reconcile loop spikes startup duration past readinessProbe's default budget under load.startupProbegives each operator a longer one-time window to reach steady state; the regularreadinessProbethen takes over with its tight per-second cadence to detect genuine ongoing failures.Origin
Both commits lifted unchanged from #2619.
Release note
Summary by CodeRabbit