Skip to content

chore(helm): Add comments, clarifying ruler setup #18607

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

eranikid
Copy link

@eranikid eranikid commented Jul 28, 2025

What this PR does / why we need it:

This PR addresses issue #13027 by adding clarifying comments to the Helm chart's values.yaml.

Problem

When using the Helm chart, if a user defines rules via .Values.ruler.directories,

directories: {}
# tenant_foo:
# rules1.txt: |
# groups:
# - name: should_fire
# rules:
# - alert: HighPercentageError
# expr: |
# sum(rate({app="foo", env="production"} |= "error" [5m])) by (job)
# /
# sum(rate({app="foo", env="production"}[5m])) by (job)
# > 0.05
# for: 10m
# labels:
# severity: warning
# annotations:
# summary: High error rate

the chart correctly creates ConfigMaps

{{- range $dir, $files := .Values.ruler.directories }}

and mounts them

{{- range $dir, $_ := .Values.ruler.directories }}
- name: {{ include "loki.rulerRulesDirName" $dir }}
mountPath: /etc/loki/rules/{{ $dir }}
{{- end }}

to the ruler's filesystem at /etc/loki/rules.

However, if .Values.loki.storage.type is set to a cloud storage provider (e.g., s3, gcs), the ruler is also implicitly configured

{{- define "loki.rulerStorageConfig" -}}
{{- if .Values.minio.enabled -}}
type: "s3"
s3:
bucketnames: ruler
{{- else if eq .Values.loki.storage.type "s3" -}}
{{- with .Values.loki.storage.s3 }}
type: "s3"

to use that same provider for its rule storage. This causes the ruler to ignore the local rules mounted from the ConfigMaps, as it's looking for them in the cloud backend instead of the local filesystem.


Workaround

To resolve this, the user must manually override the ruler's storage configuration to point to the local directory:

ruler:
  config:
    storage:
      # This override is necessary for the ruler to read rules
      # defined in .Values.ruler.directories.
      type: local
      local:
        directory: /etc/loki/rules

This solution is not intuitive and is fragile because it requires the user to hardcode the /etc/loki/rules path, which is an internal implementation detail of the chart.


Proposed Solution

While modifying the chart's templates to automatically configure local storage for the ruler, I deemed it was potentially too intrusive and a possible breaking change.

Instead, this PR adds detailed comments to the values.yaml file. These comments explain the behavior and provide the required configuration override, improving the user experience by preventing this common pitfall without introducing breaking changes.

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@eranikid eranikid requested a review from a team as a code owner July 28, 2025 09:54
@CLAassistant
Copy link

CLAassistant commented Jul 28, 2025

CLA assistant check
All committers have signed the CLA.

@@ -515,6 +515,11 @@ loki:
rulerConfig:
wal:
dir: /var/loki/ruler-wal
# -- Storage for the ruler. If defining rules in `ruler.directories`, this must be configured to use local storage as shown below.
Copy link
Contributor

Choose a reason for hiding this comment

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

As I know, this is not possible unless #18510 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants