Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix malformed types in Prometheus rules #1767

Merged
merged 1 commit into from
Aug 16, 2022

Conversation

rzetelskik
Copy link
Member

@rzetelskik rzetelskik commented Jul 15, 2022

At the moment, trying to install prometheus-community/kube-prometheus-stack Helm chart with additional Prometheus rules defined by Scylla Monitoring results in the following validation error:

Error: PrometheusRule.monitoring.coreos.com "kube-prometheus-stack-prometheus.rules" is invalid: spec.groups.rules.labels.status: Invalid value: "integer": spec.groups.rules.labels.status in body must be of type string: "integer"

This pull request changes the ill-formed values from integers to strings in order to pass validation.

To give you some context: using the prometheus-community/kube-prometheus-stack Helm chart is a recommended way of deploying Scylla Monitoring stack in K8s with Scylla Operator (see Setting up Monitoring). Introducing this change lets us avoid formatting these rules before deploying them.

Required by scylladb/scylla-operator#1001.

@amnonh
Copy link
Collaborator

amnonh commented Aug 10, 2022

overall it looks ok, but have you tested it?

@rzetelskik
Copy link
Member Author

rzetelskik commented Aug 10, 2022

overall it looks ok, but have you tested it?

What exactly do you mean by testing it? With these changes I was able to set these rules up with prometheus-community/kube-prometheus-stack helm chart (see scylladb/scylla-operator#1001). I can't vouch for all of the rules working correctly. If you're able to tell me how to check for any breaking changes, I'll be happy to.

@amnonh
Copy link
Collaborator

amnonh commented Aug 10, 2022

I mean you made a change, have you check that it does not break current functionality?

@rzetelskik
Copy link
Member Author

I mean you made a change, have you check that it does not break current functionality?

No, I haven't. Again, if you're able to give me some guidance on how to check for any breaking changes, I'll be happy to. I didn't find any tests or contributing guidelines.

@amnonh
Copy link
Collaborator

amnonh commented Aug 14, 2022

for some reason I see that my reply is not here,
you can simply perform select with allow filter, or select without using paging

@rzetelskik
Copy link
Member Author

rzetelskik commented Aug 16, 2022

you can simply perform select with allow filter, or select without using paging

I ran a select with allow filtering and it looks fine to me.
Screenshot from 2022-08-16 15-21-15

@amnonh
Copy link
Collaborator

amnonh commented Aug 16, 2022

The question, did it reach the dasbhoard? under the Advisor section (in the overview dashboard) there is a warning table, you should see the warning there, if so, it's ok and I'll merge

@rzetelskik
Copy link
Member Author

Yup, it reached the dashboard.
image

@amnonh amnonh merged commit 85a6c74 into scylladb:master Aug 16, 2022
@rzetelskik rzetelskik deleted the fix-prom-rules branch August 16, 2022 13:56
@rzetelskik
Copy link
Member Author

@amnonh thanks. Can we also have this backported to 4.0.0? I can send a PR if you don't mind backporting.

@amnonh
Copy link
Collaborator

amnonh commented Aug 16, 2022

no need, I'll backport

@rzetelskik
Copy link
Member Author

Perfect. Are you planning on releasing a patch/minor anytime soon?

@amnonh
Copy link
Collaborator

amnonh commented Aug 16, 2022

Not realy, but it would work for anyone that work with the branch directly

@rzetelskik
Copy link
Member Author

@amnonh the thing is in our docs we are instructing the users to download the scylla-monitoring release (see https://operator.docs.scylladb.com/stable/monitoring.html). If they were to work with the branch directly, they would have to build the dashboards themselves. It would be much nicer to have them download the released version.

@amnonh
Copy link
Collaborator

amnonh commented Aug 17, 2022

@rzetelskik the branch contains everything, we instruct users to use the release, but in practice many do use the branch that contains everything.

When possible, i prefer to release minor releases over patch releases, 4.1 will probably be released in about a month

@amnonh
Copy link
Collaborator

amnonh commented Oct 11, 2022 via email

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

Successfully merging this pull request may close these issues.

2 participants