-
Notifications
You must be signed in to change notification settings - Fork 45
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
docs: Add release
label to prometheusRule example
#3346
docs: Add release
label to prometheusRule example
#3346
Conversation
This label is mandatory as it is use by Prometheus to find the rules. Refs: #3293
Hello alexandre-allard-scality,My role is to assist you with the merge of this Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
/approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue None. Goodbye alexandre-allard-scality. |
All the labels in the example above are mandatory for Prometheus | ||
to take the new rules into account. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like an issue, no? The app deploys the rule, so app.kubernetes.io/name
and app
must be related to the app creating the rule, totally unrelated to prometheus-operator
. Similarly, release
should not be part of any (required) set of labels, and should not be prometheus-release
.
In essence, we need to change the set of labels our prometheus-operator
uses to filter out rules (and other objects) it should take into account, e.g., turn the selector into metalk8s.scality.com/monitor: "true"
or something along those lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can easily be overridden using prometheus.prometheusSpec.ruleSelector
in the charts/kube-prometheus-stack.yaml
file:
prometheus:
prometheusSpec:
ruleSelector:
matchLabels:
metalk8s.scality.com/monitor: "true"
And we should probably do the same for ServiceMonitor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, that's what we recommend in #2199
This PR was more about making sure our docs are accurate about the current state of the product (although it's debatable whether it's a useful change 😜)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oneline fix, does not cost much, at least now it's accurate even if it's ugly (which was already the case).
Component: docs
Context:
In the documentation we were missing the
release
label for PrometheusRules, so rules were not taken into account by Prometheus.Summary:
Add the label in the example and a note to explain the labels are mandatory.
Acceptance criteria: n/a
Closes: #3293