Skip to content

[Bugfix][Helm] skip lmcache dashboard when cache server is disabled#987

Open
bitnik wants to merge 1 commit into
vllm-project:mainfrom
bitnik:lmcache-dashboard
Open

[Bugfix][Helm] skip lmcache dashboard when cache server is disabled#987
bitnik wants to merge 1 commit into
vllm-project:mainfrom
bitnik:lmcache-dashboard

Conversation

@bitnik

@bitnik bitnik commented Jul 1, 2026

Copy link
Copy Markdown
  • Make sure the code changes pass the pre-commit checks.
  • Sign-off your commit by using -s when doing git commit
  • Try to classify PRs for easy understanding of the type of changes, such as [Bugfix], [Feat], and [CI].

When grafanaDashboards.enabled is true, the chart renders a ConfigMap for every JSON file in dashboards/*.json, including lmcache-dashboard.json. This happened regardless of whether the cache server was actually deployed, so clusters running with cacheserverSpec.enabled: false ended up with an LMCache Grafana dashboard for a component that isn't running and it showes panels with no-data.

This PR makes the dashboard rendering conditional: lmcache-dashboard.json is now skipped when cacheserverSpec.enabled is false. All other dashboards continue to render as before.

Signed-off-by: Kenan Erdogan <kenanerdogan@gmail.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces conditional rendering for the lmcache-dashboard.json Grafana dashboard, ensuring it is only deployed when cacheserverSpec.enabled is set to true. It also adds and updates Helm unit tests to validate this behavior. The reviewer provided valuable feedback pointing out a potential nil pointer evaluation error if cacheserverSpec is omitted or null in the values, suggesting a safer traversal using the dig function.

{{- if .Values.grafanaDashboards.enabled }}
{{- range $path, $bytes := .Files.Glob "dashboards/*.json" }}
{{- $filename := base $path }}
{{- if and (eq $filename "lmcache-dashboard.json") (not $.Values.cacheserverSpec.enabled) }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If a user overrides .Values and completely omits cacheserverSpec or sets it to null, accessing $.Values.cacheserverSpec.enabled will cause a template rendering error (nil pointer evaluation). To make this check robust and prevent potential rendering failures, we can use the Sprig dig function to safely traverse the nested map with a default fallback value.

{{- if and (eq $filename "lmcache-dashboard.json") (not (dig "cacheserverSpec" "enabled" false $.Values)) }}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant