Skip to content
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

Reorganize the job rules and the management-cluster-certificate alerts #1213

Merged

Conversation

QuentinBisson
Copy link
Contributor

Before adding a new alerting rule into this repository you should consider creating an SLO rules instead.
SLO helps you both increase the quality of your monitoring and reduce the alert noise.


This PR reorganizes the job and management-cluster certificate alerts to the correct teams.
This is a part of #1211

Checklist

@QuentinBisson QuentinBisson requested a review from a team June 5, 2024 20:36
@QuentinBisson QuentinBisson self-assigned this Jun 5, 2024
@QuentinBisson QuentinBisson requested a review from a team as a code owner June 5, 2024 20:36
Copy link
Contributor

@AndiDog AndiDog left a comment

Choose a reason for hiding this comment

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

What's up with the strange PR title / commit message format?

I'm not sure what is done here – the description says "reorganize", but this adds rules. Did you plan to move/rename existing rules, maybe (there's an existing file helm/prometheus-rules/templates/kaas/phoenix/alerting-rules/job.rules.yml, for example)?

@QuentinBisson
Copy link
Contributor Author

Sorry @AndiDog you're right this was less than poorly explained :(

I edited the changelog and removed the old job file I forgot to remove so now the job.rules was moved to shared and not just created. Let me know if you have other questions :)

@QuentinBisson QuentinBisson changed the title reorganize-job-and-management-cluster-certificage Reorganize the job rules and the management-cluster-certificate alerts Jun 6, 2024
Copy link
Contributor

@AndiDog AndiDog left a comment

Choose a reason for hiding this comment

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

Minor comment, not sure if important. Rest looks good, thx!

@@ -0,0 +1,29 @@
## TODO Remove with vintage
# This rule applies to vintage aws management clusters
{{- if eq .Values.managementCluster.provider.flavor "vintage" }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need {{- if eq .Values.managementCluster.provider.kind "aws" }} as before? Or we're fine that cronjob="route53-manager" won't find anything on other MCs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're fine with it, this is just to make sure we do not deploy the rules on capa and other mcs because we do not need it. But it's UP to you. Let me know what you prefer :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, i misunderstood, we're fine with it because aws is the only remaining vintage provider (because KVM alerts are coming from a custom branch and azure is gone)

@QuentinBisson QuentinBisson merged commit da92a86 into main Jun 9, 2024
6 of 7 checks passed
@QuentinBisson QuentinBisson deleted the move-job-and-management-cluster-certificate-to-shared branch June 9, 2024 20:42
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.

2 participants