Skip to content

Conversation

@DebakelOrakel
Copy link
Contributor

@DebakelOrakel DebakelOrakel commented Sep 22, 2021

Move prometheus rules from openshift4-monitoring component to logging component.

Checklist

  • Keep pull requests small so they can be easily reviewed.
  • Update the documentation.
  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog
  • Link this PR to related issues.

@DebakelOrakel DebakelOrakel added the enhancement New feature or request label Sep 22, 2021
@DebakelOrakel DebakelOrakel force-pushed the add-prometheus-rules branch 2 times, most recently from 28acf17 to 16fd07b Compare September 22, 2021 07:32
Copy link
Member

@simu simu left a comment

Choose a reason for hiding this comment

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

LGTM overall, with one big caveat: we lose the functionality of adding custom annotations to openshift-logging alerts and removing individual openshift-logging alerts via parameters.openshift4_monitoring.alerts with this change.

See https://github.com/projectsyn/component-rook-ceph/blob/master/component/alertrules.libsonnet for an example which implements custom annotations and removal of rules. The idea there was that we'd move the core logic for adding custom annotations and removing rules based on the contents of parameters.openshift4_monitoring.alerts to a component library in component openshift4-monitoring, once more components start having a need to make their alert rules ready for OCP4.

Move prometheus rules from openshift4-monitoring component to logging component.
Copy link
Member

@simu simu left a comment

Choose a reason for hiding this comment

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

LGTM, see inline comments for some cleanup and a note regarding recording rules.

Comment on lines +92 to +94
// only create duplicates of alert rules, we can use the recording
// rules which are deployed anyway when we enable monitoring on the
// CephCluster resource.
Copy link
Member

Choose a reason for hiding this comment

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

Comment is just copy-pasted. We should check if the recording rules (if there are any) are actually deployed by the cluster-logging operator, and adjust the filter function if we need to deploy the recording rules ourselves.

Co-authored-by: Simon Gerber <simon.gerber@vshn.ch>
@DebakelOrakel DebakelOrakel merged commit ce95c31 into master Sep 22, 2021
@DebakelOrakel DebakelOrakel deleted the add-prometheus-rules branch September 22, 2021 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants