Repository navigation
[monitoring] Scope infrastructure dashboards to tenant-root only - #2197
Andrei Kvapil (kvaps) merged 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds a new infrastructure dashboards list and updates the dashboards template to conditionally emit those dashboards only when the release namespace is "tenant-root"; also prunes many infra-related entries from the main dashboards list. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
size/M
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 the problem of all tenants receiving infrastructure-specific monitoring dashboards that are primarily relevant to platform operators. By separating these dashboards into a dedicated list and conditionally deploying them only to the 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. Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 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 effectively splits the dashboards into tenant-facing and infrastructure-specific lists, and correctly scopes the infrastructure dashboards to the tenant-root namespace. The changes are clear and align with the stated goal. I've suggested a refactoring in dashboards.yaml to improve maintainability by removing duplicated code, which should make future updates easier.
| {{- if eq .Release.Namespace "tenant-root" }} | ||
| {{- range (split "\n" (.Files.Get "dashboards-infra.list")) }} | ||
| {{- $parts := split "/" . }} | ||
| {{- if eq (len $parts) 2 }} | ||
| --- | ||
| apiVersion: grafana.integreatly.org/v1beta1 | ||
| kind: GrafanaDashboard | ||
| metadata: | ||
| name: {{ $parts._0 }}-{{ $parts._1 }} | ||
| spec: | ||
| folder: {{ $parts._0 }} | ||
| instanceSelector: | ||
| matchLabels: | ||
| dashboards: grafana | ||
| url: http://grafana-dashboards.cozy-grafana-operator.svc/{{ . }}.json | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
The newly added block from line 17 to 34 duplicates the logic from lines 1-16. To improve maintainability and avoid code duplication, you could refactor this by defining a named template.
Also, consider using .Files.Lines instead of split "\n" for more robustly parsing the list files.
Here is an example of how the refactored file could look:
{{- define "monitoring.renderDashboard" . -}}
{{- $parts := split "/" . -}}
{{- if and . (eq (len $parts) 2) -}}
---
apiVersion: grafana.integreatly.org/v1beta1
kind: GrafanaDashboard
metadata:
name: {{ $parts._0 }}-{{ $parts._1 }}
spec:
folder: {{ $parts._0 }}
instanceSelector:
matchLabels:
dashboards: grafana
url: http://grafana-dashboards.cozy-grafana-operator.svc/{{ . }}.json
{{- end -}}
{{- end -}}
{{- range .Files.Lines "dashboards.list" -}}
{{- template "monitoring.renderDashboard" . -}}
{{- end -}}
{{- if eq .Release.Namespace "tenant-root" -}}
{{- range .Files.Lines "dashboards-infra.list" -}}
{{- template "monitoring.renderDashboard" . -}}
{{- end -}}
{{- end -}}There was a problem hiding this comment.
Good suggestion for future improvement. The duplication is minimal (two similar loops iterating different list files) and refactoring into a named template would add complexity for little benefit at this stage. We can revisit if more dashboard list files are added.
Signed-off-by: Mattia Eleuteri <[email protected]> Signed-off-by: mattia-eleuteri <[email protected]>
185ca03 to
e08c895
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/system/monitoring/templates/dashboards.yaml (1)
17-34: Consider extracting a reusable template to reduce duplication.The new block is nearly identical to lines 1-16, differing only in the source file. A
defineblock could DRY this up.♻️ Optional refactor using a helper template
+{{- define "monitoring.dashboards" -}} +{{- range (split "\n" .) }} +{{- $parts := split "/" . }} +{{- if eq (len $parts) 2 }} +--- +apiVersion: grafana.integreatly.org/v1beta1 +kind: GrafanaDashboard +metadata: + name: {{ $parts._0 }}-{{ $parts._1 }} +spec: + folder: {{ $parts._0 }} + instanceSelector: + matchLabels: + dashboards: grafana + url: http://grafana-dashboards.cozy-grafana-operator.svc/{{ . }}.json +{{- end }} +{{- end }} +{{- end }} + +{{- include "monitoring.dashboards" (.Files.Get "dashboards.list") }} +{{- if eq .Release.Namespace "tenant-root" }} +{{- include "monitoring.dashboards" (.Files.Get "dashboards-infra.list") }} +{{- end }} -{{- range (split "\n" (.Files.Get "dashboards.list")) }} -{{- $parts := split "/" . }} -{{- if eq (len $parts) 2 }} ---- -apiVersion: grafana.integreatly.org/v1beta1 -kind: GrafanaDashboard -metadata: - name: {{ $parts._0 }}-{{ $parts._1 }} -spec: - folder: {{ $parts._0 }} - instanceSelector: - matchLabels: - dashboards: grafana - url: http://grafana-dashboards.cozy-grafana-operator.svc/{{ . }}.json -{{- end }} -{{- end }} -{{- if eq .Release.Namespace "tenant-root" }} -{{- range (split "\n" (.Files.Get "dashboards-infra.list")) }} -{{- $parts := split "/" . }} -{{- if eq (len $parts) 2 }} ---- -apiVersion: grafana.integreatly.org/v1beta1 -kind: GrafanaDashboard -metadata: - name: {{ $parts._0 }}-{{ $parts._1 }} -spec: - folder: {{ $parts._0 }} - instanceSelector: - matchLabels: - dashboards: grafana - url: http://grafana-dashboards.cozy-grafana-operator.svc/{{ . }}.json -{{- end }} -{{- end }} -{{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/monitoring/templates/dashboards.yaml` around lines 17 - 34, The new GrafanaDashboard block duplicates the earlier block; extract the repeated manifest into a reusable Helm template (e.g., define "renderDashboardFromList") that accepts the list filename as a parameter, moves the shared logic (range (split "\n" (.Files.Get ...)), $parts parsing, metadata/spec fields, and url construction) into that helper, and then call that template twice passing "dashboards.list" and "dashboards-infra.list" (or their existing names) instead of duplicating the block; update references to $parts and the file input inside the helper so the behavior of name, folder and url (including {{ . }} and $parts._0/$parts._1) is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/system/monitoring/templates/dashboards.yaml`:
- Around line 17-34: The new GrafanaDashboard block duplicates the earlier
block; extract the repeated manifest into a reusable Helm template (e.g., define
"renderDashboardFromList") that accepts the list filename as a parameter, moves
the shared logic (range (split "\n" (.Files.Get ...)), $parts parsing,
metadata/spec fields, and url construction) into that helper, and then call that
template twice passing "dashboards.list" and "dashboards-infra.list" (or their
existing names) instead of duplicating the block; update references to $parts
and the file input inside the helper so the behavior of name, folder and url
(including {{ . }} and $parts._0/$parts._1) is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12ab3bec-578e-4d4f-aa80-39741deeffee
📒 Files selected for processing (3)
packages/system/monitoring/dashboards-infra.listpackages/system/monitoring/dashboards.listpackages/system/monitoring/templates/dashboards.yaml
💤 Files with no reviewable changes (1)
- packages/system/monitoring/dashboards.list
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/system/monitoring/dashboards-infra.list
Andrei Kvapil (kvaps)
left a comment
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
LGTM, thank you for this contribution!
|
Now cozy-monitoring's grafana doesn't get all the infra dashboards. Was it intended? |
|
Igor Cherkaev (@igcherkaev) The monitoring HelmRelease (which includes Grafana and dashboard resources) is deployed in tenant-root, not in cozy-monitoring. The cozy-monitoring namespace only contains collection agents (monitoring-agents, metrics-server) and has no Grafana or GrafanaDashboard resources. Could you clarify which namespace/Grafana instance you're seeing this in? If it's in tenant-root, the infra dashboards should be present. |
|
mattia-eleuteri For what it's worth, when I installed cozystack (v1.1.3) from scratch with an empty bundle and default variants of pretty much everything, and after installing packages related to monitoring, I ended up with these: When I checked both, I found only the grafana in cozy-monitoring showing metrics related to the control plane and other infra. The one in the root tenant showed no data for them. So I assumed that the main (system) one is the one in the cozy-monitoring namespace and configured everything there (things like oidc/okta auth and roles and access). Then when I upgraded to cozystack 1.2.1 I discovered that the dashboards were gone other than a few related to managed apps. This is when I found this PR and raised the question whether it was intended or not. |
|
Igor Cherkaev (@igcherkaev) You're right, sorry about that! The default variant deploys monitoring to Fix PR: #2365 |
## What this PR does - Adds `cozy-monitoring` namespace to the infra dashboards rendering condition in `dashboards.yaml` - The default variant deploys the monitoring chart to `cozy-monitoring`, but the condition from #2197 only checked for `tenant-root`, causing all infrastructure dashboards to be missing - This is consistent with the existing pattern in `vmagent.yaml` which already handles both namespaces ## Release note ```release-note [monitoring] Fix infrastructure dashboards not rendering in default variant where monitoring is deployed to cozy-monitoring namespace ``` Fixes #2349 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Infrastructure & Monitoring** * Expanded Grafana dashboard availability across additional deployment environments for infrastructure monitoring. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Add per-tenant KubeVirt VM dashboards (resource metrics + serial console logs) and register them in dashboards.list. Rebased onto main: dashboards.list now adds only the two vm/* entries — the infra dashboards moved to dashboards-infra.list by cozystack#2197 are no longer re-added. CPU panels plotting rate(kubevirt_vmi_cpu_*_usage_ seconds_total) now use the `short` unit with "(cores)" titles instead of `percentunit`, so multi-vCPU VMs are no longer rendered as >100%; the percentage-style color thresholds are dropped (absolute core thresholds would be arbitrary across heterogeneous VM sizes). Signed-off-by: mattia-eleuteri <[email protected]>
Add per-tenant KubeVirt VM dashboards (resource metrics + serial console logs) and register them in dashboards.list. Rebased onto main: dashboards.list now adds only the two vm/* entries — the infra dashboards moved to dashboards-infra.list by cozystack#2197 are no longer re-added. CPU panels plotting rate(kubevirt_vmi_cpu_*_usage_ seconds_total) now use the `short` unit with "(cores)" titles instead of `percentunit`, so multi-vCPU VMs are no longer rendered as >100%; the percentage-style color thresholds are dropped (absolute core thresholds would be arbitrary across heterogeneous VM sizes). Signed-off-by: mattia-eleuteri <[email protected]>
3 participants
Summary
dashboards.listinto tenant-facing dashboards and infrastructure dashboards (dashboards-infra.list)tenant-rootProblem
All tenants currently receive infrastructure dashboards (VictoriaMetrics, Hubble, LINSTOR, Flux, control-plane, etc.) that are only relevant to platform operators.
Relates to #2194
Test plan
helm templatemonitoring intenant-rootnamespace — both lists renderedhelm templatemonitoring in a child tenant namespace — onlydashboards.listrendered🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores