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

[bitnami/thanos] Sync prometheus alerting rules with upstream #23381

Closed
wants to merge 11 commits into from

Conversation

antonblr
Copy link

@antonblr antonblr commented Feb 9, 2024

Description of the change

Inspired by prometheus-community/kube-prometheus-stack/hack.

This change adds a script to sync-up Thanos chart prometheus alerting rules with upstream https://github.com/thanos-io/thanos/tree/main/mixin library and updates existing rules with the generated ones.

In particular the sync resulted in the following changes:

  • Removed ThanosReceiveTrafficBelowThreshold (see #5824)
  • Added ThanosReceiveLimitsConfigReloadFailure (see #6466
  • Added ThanosReceiveLimitsHighMetaMonitoringQueriesFailureRate (see #6466)
  • Added ThanosReceiveTenantLimitedByHeadSeries (see #6467)

The remaining diff are mostly formatting changes.

Benefits

Automatically generates up to date rules.

Possible drawbacks

Some python knowledge may be needed in order to add new functionality to the script and / or update the existing one.

Applicable issues

N/A

Additional information

Checklist

  • Chart version bumped in Chart.yaml according to semver. This is not necessary when the changes only affect README.md files.
  • Variables are documented in the values.yaml and added to the README.md using readme-generator-for-helm
  • Title of the pull request follows this pattern [bitnami/<name_of_the_chart>] Descriptive title
  • All commits signed off and in agreement of Developer Certificate of Origin (DCO)

@github-actions github-actions bot added thanos triage Triage is needed labels Feb 9, 2024
@antonblr antonblr force-pushed the thanos-sync-rules branch 2 times, most recently from 89935a3 to 06c7b48 Compare February 9, 2024 18:53
*/ -}}
{{- if and .Values.metrics.enabled (or .Values.metrics.prometheusRule.default.create .Values.metrics.prometheusRule.default.replicate ) }}
apiVersion: monitoring.coreos.com/v1
kind: PrometheusRule
metadata:
name: {{ template "common.names.fullname" . }}-replicate
name: thanos-bucket-replicate
Copy link
Author

Choose a reason for hiding this comment

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

Name comes from the upstream - let me know if you think script should generated it as {{ template "common.names.fullname" . }}-<suffix>
We would need to hardcode suffixes then, in order to accommodate things like -ruler, -store-gateway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,
Sorry for the long delay.
Yes, the name of the all kubernetes objects created by the chart should keep the naming. This would allow to have multiple instances with different names, and be coherent with how we handle the naming across all our charts.

Copy link
Author

Choose a reason for hiding this comment

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

Kept PrometheusRule as it is. @rafariossaa let me know if there is anything else.

@carrodher carrodher added verify Execute verification workflow for these changes in-progress labels Feb 9, 2024
@github-actions github-actions bot removed the triage Triage is needed label Feb 9, 2024
@github-actions github-actions bot removed the request for review from carrodher February 9, 2024 19:46
@antonblr
Copy link
Author

Resolved conflicts due to changes in https://github.com/bitnami/charts/pull/22687/files

@antonblr antonblr force-pushed the thanos-sync-rules branch 3 times, most recently from 15d2ae4 to f86dae6 Compare February 15, 2024 01:47
@carrodher
Copy link
Member

Sorry for the inconvenience, there was a new minor version of this chart that requires a rebase from main and a new bump of the version

@antonblr
Copy link
Author

antonblr commented Feb 22, 2024

Hi @carrodher, should we sync-up somehow on when you're ready to merge it (if there is no outstanding questions/concerns) and I'll do the final bump in the Chrat.yaml. Otherwise I feel like in a never ending race 😄 due to continuous changes in it. Thank you!

*/ -}}
{{- if and .Values.metrics.enabled (or .Values.metrics.prometheusRule.default.create .Values.metrics.prometheusRule.default.replicate ) }}
apiVersion: monitoring.coreos.com/v1
kind: PrometheusRule
metadata:
name: {{ template "common.names.fullname" . }}-replicate
name: thanos-bucket-replicate
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,
Sorry for the long delay.
Yes, the name of the all kubernetes objects created by the chart should keep the naming. This would allow to have multiple instances with different names, and be coherent with how we handle the naming across all our charts.

@antonblr antonblr closed this Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solved thanos verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants