Skip to content

Conversation

@mbec-printify
Copy link

@mbec-printify mbec-printify commented Jan 10, 2024

PR Description

Already done in those 2 PRs for legacy repo & current loki Helm chart. I've migrated the above solution to the loki-distributed Helm chart and tested locally with prom-stack.


What this PR does:

PR adds a sidecar container to the Ruler deployment to fetch alerting rules defined in separate ConfigMaps and Secrets (based on k8s labels).

The goal of this update is to enable the creation of AlertingRules independently for other services deployments. With this setup, services can manage their own alerting rules in a self-contained manner, through individual ConfigMaps or Secrets, without directly interfacing with the primary ruler deployment.

Checklist

  • DCO signed
  • Version bumped
  • Documentation added

@CLAassistant
Copy link

CLAassistant commented Jan 10, 2024

CLA assistant check
All committers have signed the CLA.

@sebv004
Copy link

sebv004 commented Mar 7, 2024

Hi guys, any news on this feature?

@zanhsieh
Copy link
Contributor

@zalegrala Can you help with this PR please?

@zanhsieh
Copy link
Contributor

@Sheikh-Abubaker Do you have permission to approve this PR?

@Sheikh-Abubaker
Copy link
Collaborator

@Sheikh-Abubaker Do you have permission to approve this PR?

Yes I have, is this feature tested properly ?

@Diontsoumas
Copy link

Can we somehow unblock this please? It's a pity that Loki supports pull based alerts but we need to patch things manually because of an open PR :)

@gacopl
Copy link

gacopl commented Feb 21, 2025

+1 it's getting silly now

@sharovmerk
Copy link

any news?

@zalegrala
Copy link
Contributor

I've not reviewed the change change, but I would offer that, considering the lack of attention on the loki-distirbuted chart, it may be prudent to use a chart which gets more support, updates, and attention. As mentioned in the OP, the change has been merged in the loki repo.
https://github.com/grafana/loki/tree/main/production/helm/loki
https://github.com/grafana/loki/commits/main/production/helm/loki

@gacopl
Copy link

gacopl commented Apr 8, 2025

there's a reason this chart is being used, in you would be surprised how complex places... this change has also been battle tested for over a year now, we just have to jump through hoops every upgrade which is annoying, considering the effort PR requestor made here

@zalegrala
Copy link
Contributor

Understood @gacopl. Please don't shoot the messenger. I imagine switching charts to be something of a chore considering potential complexity. What I'm offering by way of suggestion, is that some charts get more review from the folks who know best.

@gacopl
Copy link

gacopl commented Apr 8, 2025

Sorry didn't mean to be mean :D it's just someone made the legwork to add this in multiple charts and for some reason we're have issue only with the one we care the most :D

# -- Label value that the configmaps/secrets with rules will be set to.
labelValue: ""
# -- Folder into which the rules will be placed.
folder: /etc/loki/sc-rules
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this reference the variable?

type: local
local:
directory: /etc/loki/rules
directory: {{ if .Values.ruler.sidecar.enabled }}/etc/loki/sc-rules{{ else }}/etc/loki/rules{{ end }}
Copy link
Contributor

Choose a reason for hiding this comment

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

If the sidecar is enabled, could it just mount at the exiting path instead of a new path?

Copy link
Collaborator

@Sheikh-Abubaker Sheikh-Abubaker left a comment

Choose a reason for hiding this comment

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

Closing in favor of #3975 Thanks for your contribution! feel free to explore the issues and contribute to charts which are actively being maintained.

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.

10 participants