Repository navigation
fix(kubevirt-operator): move PrometheusRule to Helm-rendered template - #2543
Conversation
Move alerts/PrometheusRule.yaml from raw-file loading (via Files.Glob)
to templates/prometheusrule.yaml as a proper Helm template. Escape
Prometheus template variables with {{ "{{" }} to prevent Helm from
evaluating them during chart rendering.
Remove templates/alerts.yaml which is now dead code since the alerts/
directory is empty.
Signed-off-by: IvanHunters <[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 improves the maintainability and reliability of the KubeVirt operator's monitoring configuration. By transitioning from raw file loading to native Helm templates, the deployment process is now more robust, ensuring that template variables are correctly handled and preventing potential rendering issues during installation. Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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 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 counter productive. 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
|
dosubot
Bot
added
the
size/XS
|
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 due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR refactors KubeVirt alert definitions by consolidating them from a separate ChangesAlert Definition Consolidation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
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)
Tip 💬 Introducing Slack Agent: Turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
github-actions
Bot
added
area/uncategorized
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 migrates Prometheus alert rules from a dynamic loading system to a dedicated Helm template in packages/system/kubevirt-operator/templates/prometheusrule.yaml. The review feedback identifies that the current alert expressions using max_over_time are overly sensitive and suggests using a for: 10m duration instead to ensure the alerts only trigger for persistent issues. The reviewer also recommends formatting improvements for the alert annotations, such as using block scalars for descriptions and consistent quoting.
| - alert: VMNotRunningFor10Minutes | ||
| expr: | | ||
| max_over_time( | ||
| kubevirt_vm_info{ | ||
| status!="running", | ||
| exported_namespace=~".+", | ||
| name=~".+" | ||
| }[10m] | ||
| ) == 1 | ||
| labels: | ||
| severity: critical | ||
| namespace: '{{ "{{" }} $labels.exported_namespace {{ "}}" }}' | ||
| annotations: | ||
| summary: 'VM {{ "{{" }} $labels.name {{ "}}" }} in namespace {{ "{{" }} $labels.exported_namespace {{ "}}" }} is not Running for more than 10 minutes' | ||
| description: 'The VM {{ "{{" }} $labels.name {{ "}}" }} in namespace {{ "{{" }} $labels.exported_namespace {{ "}}" }} has been in a non-Running state for more than 10 minutes. Please investigate the issue.' |
There was a problem hiding this comment.
The current expression max_over_time(...)[10m] == 1 triggers the alert if the VM was in a non-running state at any point during the last 10 minutes. For an alert named VMNotRunningFor10Minutes, it is more appropriate to use a persistent condition with a for: 10m duration. Additionally, I've restored the single quotes in the annotations for consistency with the original version and improved the description readability using a block scalar.
- alert: VMNotRunningFor10Minutes
expr: kubevirt_vm_info{status!="running",exported_namespace=~".+",name=~".+"} == 1
for: 10m
labels:
severity: critical
namespace: '{{ "{{" }} $labels.exported_namespace {{ "}}" }}'
annotations:
summary: "VM '{{ "{{" }} $labels.name {{ "}}" }}' in namespace '{{ "{{" }} $labels.exported_namespace {{ "}}" }}' is not Running for more than 10 minutes"
description: >-
The VM '{{ "{{" }} $labels.name {{ "}}" }}' in namespace '{{ "{{" }} $labels.exported_namespace {{ "}}" }}'
has been in a non-Running state for more than 10 minutes. Please investigate the issue.| - alert: VMINotRunningFor10Minutes | ||
| expr: | | ||
| max_over_time( | ||
| kubevirt_vmi_info{ | ||
| phase!="running", | ||
| exported_namespace=~".+", | ||
| name=~".+" | ||
| }[10m] | ||
| ) == 1 | ||
| labels: | ||
| severity: critical | ||
| namespace: '{{ "{{" }} $labels.exported_namespace {{ "}}" }}' | ||
| annotations: | ||
| summary: 'VMI {{ "{{" }} $labels.name {{ "}}" }} in namespace {{ "{{" }} $labels.exported_namespace {{ "}}" }} is not Running for more than 10 minutes' | ||
| description: 'The VMI {{ "{{" }} $labels.name {{ "}}" }} in namespace {{ "{{" }} $labels.exported_namespace {{ "}}" }} has been in a non-Running state for more than 10 minutes. Please investigate the issue.' |
There was a problem hiding this comment.
Similar to the VM alert, the expression max_over_time(...)[10m] == 1 is likely too sensitive for a '10 minutes' alert. Using for: 10m ensures the VMI has been in a non-running phase for the entire duration. I've also restored the single quotes in the annotations and used a block scalar for the description.
- alert: VMINotRunningFor10Minutes
expr: kubevirt_vmi_info{phase!="running",exported_namespace=~".+",name=~".+"} == 1
for: 10m
labels:
severity: critical
namespace: '{{ "{{" }} $labels.exported_namespace {{ "}}" }}'
annotations:
summary: "VMI '{{ "{{" }} $labels.name {{ "}}" }}' in namespace '{{ "{{" }} $labels.exported_namespace {{ "}}" }}' is not Running for more than 10 minutes"
description: >-
The VMI '{{ "{{" }} $labels.name {{ "}}" }}' in namespace '{{ "{{" }} $labels.exported_namespace {{ "}}" }}'
has been in a non-Running state for more than 10 minutes. Please investigate the issue.
📝 WalkthroughWalkthroughKubeVirt monitoring alerts are reorganized by removing a standalone PrometheusRule manifest and its loader template, replacing them with a new Helm template that defines equivalent VM/VMI "not running for 10 minutes" alerting rules. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![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 the current code and only fix it if needed.
Inline comments:
In `@packages/system/kubevirt-operator/templates/prometheusrule.yaml`:
- Around line 9-38: The rules VMNotRunningFor10Minutes and
VMINotRunningFor10Minutes currently use max_over_time(...[10m]) == 1 which only
detects a single non-running sample; replace each max_over_time expression
targeting kubevirt_vm_info and kubevirt_vmi_info with an equivalent instant
vector selector (e.g.
kubevirt_vm_info{status!="running",exported_namespace=~".+",name=~".+"} and
kubevirt_vmi_info{phase!="running",exported_namespace=~".+",name=~".+"}) and add
a top-level for: 10m to each alert block so the alert only fires if the
non-running condition is sustained for 10 minutes.
🪄 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: 074c50d1-bc9a-4c2d-b5f1-ca2a38c96a55
📒 Files selected for processing (3)
packages/system/kubevirt-operator/alerts/PrometheusRule.yamlpackages/system/kubevirt-operator/templates/alerts.yamlpackages/system/kubevirt-operator/templates/prometheusrule.yaml
💤 Files with no reviewable changes (2)
- packages/system/kubevirt-operator/templates/alerts.yaml
- packages/system/kubevirt-operator/alerts/PrometheusRule.yaml
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — the structural refactor (raw-glob loader → Helm-rendered template with {{ "{{" }} escape) is correct, but both Gemini and CodeRabbit flagged a real PromQL-semantic bug in the alert expressions that this PR moves verbatim. Per the rule that pre-existing issues in the neighborhood of the diff are owned by the PR (the touched lines themselves contain the bug), this should be addressed before merge — either fixed inline or explicitly punted to a tracked follow-up issue.
Business context: Move kubevirt-operator's VM/VMI alert rules from raw-file glob loading (templates/alerts.yaml + alerts/PrometheusRule.yaml) to a standard Helm-rendered template, escaping Prometheus's {{ $labels.X }} syntax so Helm doesn't try to evaluate it.
Blocker
B1: alert expression doesn't match its name — fires on any non-running event in the last 10 min, not "non-running for 10 min"
File: packages/system/kubevirt-operator/templates/prometheusrule.yaml:13-21 (VMNotRunningFor10Minutes) and 28-36 (VMINotRunningFor10Minutes)
Issue: The expression max_over_time(kubevirt_vm_info{status!="running"}[10m]) == 1 returns 1 whenever the metric was 1 at any point in the last 10 minutes. So a VM that was briefly non-running 9 minutes ago but is currently running still triggers the alert. The alert name promises "non-running for 10 minutes", which means continuous non-running for the full window.
Evidence: Prometheus max_over_time semantics return the maximum value over the lookback window, not the minimum or the duration of true. To express "the condition has held for the entire window", either use min_over_time(metric{...}[10m]) == 1 (the metric was always 1 in the window) or change the expression to kubevirt_vm_info{status!="running"} == 1 and add a for: 10m PromQL clause to the alert. Both bots flagged this; CodeRabbit cited the upstream Prometheus docs explicitly. The expression is preserved verbatim from the deleted alerts/PrometheusRule.yaml — this PR didn't introduce the bug, but it's now in the diff and the touched lines own it.
Impact: False positives on transient VM/VMI restarts (e.g. brief reschedules, node maintenance). Operators get paged for events that resolved themselves minutes ago.
Fix: One of:
(a) Change to kubevirt_vm_info{status!="running"} == 1 with for: 10m (idiomatic Prometheus alert-fires-after-stable-condition pattern).
(b) Change to min_over_time(kubevirt_vm_info{status!="running"}[10m]) == 1 (alert if metric was non-running for the entire 10m window).
(c) Punt explicitly: open a tracking issue for the alert tuning, reference it in the PR body, and add a comment in the YAML noting the known issue is tracked separately. This keeps the refactor scope minimal but doesn't leave the bot findings unanswered.
…ax_over_time Replace max_over_time([10m]) == 1 with an instant vector selector and a top-level for: 10m clause. The previous expression fired as long as the metric was non-running at any point in the last 10 minutes; the new expression fires only when the condition is sustained continuously for the full 10-minute window, matching the alert name semantics. Signed-off-by: IvanHunters <[email protected]>
dosubot
Bot
added
size:XS
and removed
size/XS
github-actions
Bot
added
size/M
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM. The PromQL semantic blocker from the prior review is addressed in 000157f9:
- VMNotRunningFor10Minutes / VMINotRunningFor10Minutes now use instant vector selectors (
kubevirt_vm_info{status!="running", …} == 1,kubevirt_vmi_info{phase!="running", …} == 1) withfor: 10mat the alert level. - This is the idiomatic Prometheus pattern for "condition has held continuously for the duration" — the alert fires only after the metric has stayed at 1 for the full 10-minute window, which matches the alert name and resolves the false-positive class on transient VM/VMI restarts.
Verification:
- Both alerts now match shape: instant vector +
for: 10m. The exact form recommended as option (a) in the prior review. - Diff is minimal: 12 insertions, 14 deletions in
prometheusrule.yaml— no other rule logic touched. - Helm-template escaping (
{{ "{{" }} $labels.X {{ "}}" }}) survives the rewrite intact, so the original PR's structural goal (raw-glob loader → Helm-rendered template) still holds. - CI green: pre-commit, Build, E2E Tests, CodeRabbit.
3 participants
What this PR does
Move
alerts/PrometheusRule.yamlfrom raw-file loading (via.Files.Globintemplates/alerts.yaml) to a proper Helm-rendered templatetemplates/prometheusrule.yaml.The raw-file approach silently passed unescaped
{{ $labels.xxx }}toKubernetes unchanged, which worked by accident. As a Helm template, those
expressions must be escaped with
{{ "{{" }}to prevent Helm from tryingto evaluate them as Go template variables.
Also removes
templates/alerts.yamlwhich became dead code once thealerts/directory is empty.Release note
```release-note
fix(kubevirt-operator): move PrometheusRule alerts to Helm-rendered template and escape Prometheus template variables
```
Summary by CodeRabbit