fix(kubernetes): add spec.timeout to tenant CSI HelmRelease - #2727
Conversation
github-actions
Bot
added
size/XS
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 updates the kubevirt-csi-node HelmRelease configuration by increasing the timeout to 10 minutes. This change addresses issues where slow control-plane bringups caused premature remediation cycles, ensuring that the CSI component has sufficient time to reach a healthy state. 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
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a 10-minute timeout configuration to the CSI HelmRelease template. The timeout is applied conditionally when the etcd namespace is configured, ensuring that CSI deployment operations complete within a bounded time window. ChangesCSI Timeout Configuration
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 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 |
![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 increases the timeout for the CSI HelmRelease to 10 minutes within the Kubernetes app package. The reviewer pointed out that the PR body's release note does not follow the required format specified in the repository style guide and suggested using a more descriptive entry.
| sharding.fluxcd.io/key: tenants | ||
| spec: | ||
| interval: 5m | ||
| timeout: 10m |
There was a problem hiding this comment.
The release note in the PR body is set to NONE, which does not follow the required format specified in the repository style guide. Since this change fixes a bug that can cause wedged volume mounts, it should have a descriptive release note following the type(scope): human-readable changelog entry format (e.g., fix(kubernetes): increase timeout for CSI HelmRelease).
References
- PR body must contain a release note block in the format
type(scope): human-readable changelog entry. Flag any PR whose body has no release-note block or lacks the correct format. (link)
bfa923b to
39b8af5
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — one-line change aligns the tenant CSI HelmRelease with every workload-bearing sibling; the fix is correct and minimal.
Business context: the tenant csi HelmRelease runs with remediation.retries: -1 and the default 5m Helm timeout, so a slow tenant control-plane bringup trips an Install→Uninstall→Install remediation cycle that wedges in-flight volume mounts (surfaces as the E2E NFS test timeout).
Verified: the csi HelmRelease does carry remediation.retries: -1; 16 of 19 HelmReleases in packages/apps/kubernetes/templates/helmreleases/ already set timeout: 10m; commit 9f9d8f85 ("Allign timeouts for HelmReleases") is real and missed this template.
Non-blocking follow-ups
- The description says
csi.yaml"was the only one missing" the timeout, butprometheus-operator-crds.yamlandvertical-pod-autoscaler-crds.yamlin the same directory still lacktimeout(they fall back to 5m). They are CRD-only installs (fast, unlikely to hit the wall), so low priority — but for full consistency with9f9d8f85's intent they could taketimeout: 10mtoo. - The
release-noteblock isNONE; a bug fix that prevents wedged volume mounts arguably warrants afix(kubernetes): …changelog line. Style-guide nit only.
39b8af5 to
921ecc2
Compare
Aligns kubevirt-csi-node HR with every other tenant addon HR in packages/apps/kubernetes/templates/helmreleases/ (cilium, coredns, ingress-nginx, etc.) — all set timeout: 10m, csi.yaml was the only exception. 9f9d8f8 ("Allign timeouts for HelmReleases") missed it. Symptom: e2e NFS test (hack/e2e-apps/run-kubernetes.sh) times out waiting for nfs-test-pod to reach Succeeded. Cozyreport events show the tenant csi HR running Install -> Uninstall -> Install remediation cycles. With install.remediation.retries: -1 and the default 5m Helm timeout, kstatus marks the kubevirt-csi-node DaemonSet 'InProgress' past 5m on a slow tenant control-plane bringup (Kamaji APIs flapping, VM image pulls), helm-controller times out, the remediation uninstalls the DaemonSet, then reinstalls. Any volume mount in flight at that moment is wedged. Observed on PR #2619 CI run 26142986969: csi HR remediation at ~07:07 happened ~10 min into the nfs-test-pod wait, killing the mount and timing out the pod-Succeeded condition. 10m matches the surrounding HRs and gives the DaemonSet headroom without changing remediation semantics on truly-broken installs. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> (cherry picked from commit 866588f)
921ecc2 to
d8ba954
Compare
|
Successfully created backport PR for |
2 participants
What this PR does
Adds
spec.timeout: 10mto thekubevirt-csi-nodeHelmRelease template inpackages/apps/kubernetes/templates/helmreleases/csi.yaml, aligning it with every other tenant addon HR in the same directory (cilium, coredns, ingress-nginx, etc. — all settimeout: 10m).csi.yamlwas the only one missing it; commit 9f9d8f8 ("Align timeouts for HelmReleases") missed this template.Symptom
E2E NFS test times out waiting for
nfs-test-podto reachSucceeded. Cozyreport events show the tenantcsiHR running an Install → Uninstall → Install remediation cycle. Withinstall.remediation.retries: -1and the default 5m Helm timeout, kstatus marks thekubevirt-csi-nodeDaemonSetInProgresspast 5m on a slow tenant control-plane bringup (Kamaji APIs flapping, VM image pulls), helm-controller times out, the remediation uninstalls the DaemonSet, then reinstalls. Any volume mount in flight at that moment is wedged.Origin
Single commit lifted unchanged from #2619.
Release note
Summary by CodeRabbit
spec.timeoutof 10 minutes when the etcd namespace value is set (keeping the existingspec.intervalat 5 minutes).