ROSAENG-14114: New alert detecting when a MC cluster cannot send metrics to RHOBS or telemeter#2806
ROSAENG-14114: New alert detecting when a MC cluster cannot send metrics to RHOBS or telemeter#2806Nikokolas3270 wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@Nikokolas3270: This pull request references ROSAENG-14114 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Nikokolas3270 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughUpdates the management-cluster monitoring manifest with Prometheus access and scrape targets, then replaces stack and workload alert rules with COO-specific missing, degraded, restart, and remote-write alerts. ChangesManagement-cluster OBO monitoring
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Nikokolas3270
force-pushed
the
ROSAENG-14114
branch
from
55d3e5c to
3101d43
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
deploy/sre-prometheus/management-cluster/100-obo-stack-monitoring.yml (1)
115-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDistinguish the three
COOWorkloadDegradedSREalerts.The same alert name
COOWorkloadDegradedSREis reused for three unrelated conditions (readiness gap, restart loop, remote-write failure), all pointing to a single SOP. Prometheus permits duplicate names, but on-call SREs seeing this alert fire cannot tell which failure mode occurred without inspecting thecauselabel, and Alertmanager grouping byalertnamewill mix them. Consider giving each a distinct name (e.g.COOWorkloadNotReadySRE,COOWorkloadRestartLoopSRE,COOWorkloadRemoteWriteFailingSRE) and matching SOP links.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/sre-prometheus/management-cluster/100-obo-stack-monitoring.yml` around lines 115 - 137, The three COOWorkloadDegradedSRE entries in the alert rules are conflating different failure modes under one alertname, which makes alert routing and on-call triage ambiguous. Update each alert block in the Prometheus rules to use a distinct alert name tied to its specific condition (the readiness check, the restart-loop check, and the remote-write failure check) and keep the existing annotations/cause labels aligned with each new name. Use the COOWorkloadDegradedSRE alert blocks in this rules file as the place to split them apart so Alertmanager grouping and SOP references clearly identify the exact problem.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/sre-prometheus/management-cluster/100-obo-stack-monitoring.yml`:
- Around line 126-135: The alert text for COOWorkloadDegradedSRE is inconsistent
with the rule timing: the annotation in the monitoring alert uses a different
duration than the `for` value in the same rule. Update the `description` in the
COOWorkloadDegradedSRE alert so it matches the actual `for: 30m` behavior, and
keep the wording aligned with the `expr`/`for` settings in the alert definition
to avoid confusion.
- Around line 137-148: The COOWorkloadDegradedSRE alert expression is using the
wrong denominator and threshold, so it fires far too early. Update the PromQL in
the COOWorkloadDegradedSRE rule to calculate failure percentage using
prometheus_remote_storage_samples_failed_total divided by failed plus total, and
compare it against the intended 10% threshold. Keep the alert name, labels, and
annotations unchanged; only fix the expression inside the alert definition.
---
Nitpick comments:
In `@deploy/sre-prometheus/management-cluster/100-obo-stack-monitoring.yml`:
- Around line 115-137: The three COOWorkloadDegradedSRE entries in the alert
rules are conflating different failure modes under one alertname, which makes
alert routing and on-call triage ambiguous. Update each alert block in the
Prometheus rules to use a distinct alert name tied to its specific condition
(the readiness check, the restart-loop check, and the remote-write failure
check) and keep the existing annotations/cause labels aligned with each new
name. Use the COOWorkloadDegradedSRE alert blocks in this rules file as the
place to split them apart so Alertmanager grouping and SOP references clearly
identify the exact problem.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: affb1a89-66e0-419c-b735-5e843495191c
⛔ Files ignored due to path filters (3)
hack/00-osd-managed-cluster-config-integration.yaml.tmplis excluded by!hack/**hack/00-osd-managed-cluster-config-production.yaml.tmplis excluded by!hack/**hack/00-osd-managed-cluster-config-stage.yaml.tmplis excluded by!hack/**
📒 Files selected for processing (1)
deploy/sre-prometheus/management-cluster/100-obo-stack-monitoring.yml
…ics to RHOBS or telemeter
Nikokolas3270
force-pushed
the
ROSAENG-14114
branch
from
3101d43 to
0c2d97c
Compare
|
@Nikokolas3270: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| namespace: openshift-observability-operator | ||
| spec: | ||
| endpoints: | ||
| - bearerTokenFile: /var/run/secrets/kubernetes.io/serviceaccount/token |
There was a problem hiding this comment.
Do we really need a bearer token + TLS config? I assume that the metrics endpoint is plain HTTP without authentication.
| prometheus: sre-coo-stack | ||
| role: alert-rules | ||
| name: sre-obo-stack | ||
| name: sre-coo-stack |
There was a problem hiding this comment.
noob question but will the old sre-obo-stack resource be deleted?
| - name: sre-coo-stack | ||
| rules: | ||
| - alert: OpenshiftObservabilityOperatorStackDownSRE | ||
| - alert: COOStackMissingSRE |
There was a problem hiding this comment.
(nit) probably a follow-up but I would recommend 1 rule per resource which would provide more targeted alerts.
| absent(kube_deployment_status_replicas{deployment="observability-operator", namespace="openshift-observability-operator"}) or | ||
| absent(kube_statefulset_status_replicas{statefulset="prometheus-hypershift-monitoring-stack", namespace="openshift-observability-operator"}) or | ||
| absent(kube_statefulset_status_replicas{statefulset="alertmanager-hypershift-monitoring-stack", namespace="openshift-observability-operator"}) | ||
| absent(kube_deployment_status_replicas{deployment="obo-prometheus-operator", namespace="openshift-operators"}) or |
There was a problem hiding this comment.
namespace="openshift-operators" or namespace="openshift-observability-operator"?
| SOP: https://github.com/openshift/ops-sop/blob/master/hypershift/alerts/OpenshiftObservabilityOperatorAlertmanagerComponentsDegradedSRE.md | ||
| summary: "Some pods in the 'openshift-observability-operator' namespace cannot remote write to telemeter or RHOBS" | ||
| description: | | ||
| {{ $value | printf "%.2f" }}% of the time series cannot be be sent to {{$labels.url}} |
There was a problem hiding this comment.
or humanizePercentage without multiplying by 100
3 participants
What type of PR is this?
feature
What this PR does / why we need it?
This PR makes sure we (SREP) are alerted in case metrics cannot be remote written to RHOBS or telemeter.
It is important to be alerted as all Hypershift alerts but also the hosted cluster billing is based on those metrics
Which Jira/Github issue(s) this PR fixes?
_Fixes ROSAENG-14114
Special notes for your reviewer:
We may want to increase the severity of the other alerts from
warningtocritical.As said being able to send metrics to RHOBS & telemeter is crucial for all Hypershift alerting & billing based on the metrics sent there.
Pre-checks (if applicable):
Summary by CodeRabbit
New Features
Bug Fixes