OCPBUGS-62851: feat(e2e): add check for Service and Pod Monitors - #6965
OCPBUGS-62851: feat(e2e): add check for Service and Pod Monitors#6965sjenning wants to merge 1 commit into
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
/test e2e-aws-minimal |
openshift-ci
Bot
added
area/testing
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sjenning The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRegisters Prometheus monitoring types in the test scheme, adds a test helper that verifies Prometheus scrape targets for ServiceMonitors and PodMonitors in the hosted control plane namespace, and invokes that helper from the cluster creation e2e test. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
| // up == 1 means the target is healthy, up == 0 means it's down | ||
| query := fmt.Sprintf(`up{job="%s",namespace="%s"}`, sm.Name, hcpNamespace) | ||
|
|
||
| result, warnings, err := prometheusClient.Query(ctx, query, time.Now()) |
There was a problem hiding this comment.
An alternative would be to use the Prometheus API to query the targets endpoint, but for the sake of an e2e I don't think it would be any better or worse than using an actual query like you have. https://prometheus.io/docs/prometheus/latest/querying/api/#targets
|
b4ad460 to
6533dfe
Compare
|
/test e2e-aws-minimal |
|
/lgtm |
sjenning
changed the title
|
Because of target relabel configs, you have no guarantees that the job label == service name. As such the test feels brittle to me. |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
sjenning
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@sjenning: This pull request references Jira Issue OCPBUGS-62851, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (yli2@redhat.com), skipping review request. The bug has been updated to refer to the pull request using the external bug tracker. 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. |
| jobLabel := string(sample.Metric["job"]) | ||
|
|
||
| // Only check targets for our monitors | ||
| if monitorSet[jobLabel] { |
There was a problem hiding this comment.
Instead of looping over samples and seeing if they're in our expected monitor set, can we loop over the expected monitor set and see if they're in the collected samples? Failing if we didn't find a sample that we were expected would allow us to fail-closed and catch things like the job label not matching the ServiceMonitor name (as long as you didn't have any other monitors with that same name or someone else injecting an external monitor that generated the job label you were expecting, but those both seem like safe enough assumptions).
15e4dce to
90a0b5b
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
|
|
||
| // Use the first matching Service | ||
| service := serviceList.Items[0] |
There was a problem hiding this comment.
Bug: Non-deterministic Service matching causes flaky tests.
When multiple Services match a ServiceMonitor's selector, using the first Service from serviceList.Items[0] creates non-deterministic behavior since Kubernetes list ordering isn't guaranteed. If the matching Services have different values for the job label, the test could track the wrong job name and produce flaky results across test runs.
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
test/e2e/create_cluster_test.go(1 hunks)test/e2e/util/scheme.go(2 hunks)test/e2e/util/util.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/create_cluster_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/util/scheme.gotest/e2e/util/util.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Red Hat Konflux / hypershift-cli-mce-211-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-release-mce-211-on-pull-request
- GitHub Check: Cursor Bugbot
90a0b5b to
4179f5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/e2e/util/util.go (1)
4003-4008: Consider deterministic service selection when multiple services match.When multiple Services match a ServiceMonitor's selector, using
serviceList.Items[0]creates non-deterministic behavior since Kubernetes list ordering isn't guaranteed. If matching Services have different values for the job label, the test could derive the wrong job name and produce inconsistent results across test runs.For most Hypershift ServiceMonitors this may not be an issue in practice, but consider either:
- Asserting that exactly one Service matches (fail if multiple), or
- Sorting by name and documenting the selection strategy.
Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
test/e2e/create_cluster_test.go(1 hunks)test/e2e/util/scheme.go(2 hunks)test/e2e/util/util.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/create_cluster_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/util/util.gotest/e2e/util/scheme.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-release-mce-211-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-cli-mce-211-on-pull-request
🔇 Additional comments (7)
test/e2e/util/scheme.go (2)
19-19: LGTM: Prometheus Operator types imported.The import enables the test scheme to recognize ServiceMonitor and PodMonitor custom resources.
34-34: LGTM: Monitoring types registered with test scheme.This registration allows the e2e test client to list and work with ServiceMonitor and PodMonitor resources.
test/e2e/util/util.go (5)
73-73: LGTM: Import added for Prometheus monitoring types.This import enables the function to work with ServiceMonitor and PodMonitor lists.
4012-4020: LGTM: Job name derivation correctly implemented.The code properly handles both cases:
- When
jobLabelis specified, it reads the value from the matched Service's labels (not the ServiceMonitor's selector)- When
jobLabelis empty, it uses the Service name as the job nameThis addresses previous review feedback about jobLabel handling.
4026-4053: LGTM: PodMonitor processing correctly handles label selectors.The code properly:
- Builds selectors supporting both
matchLabelsandmatchExpressionsviametav1.LabelSelectorAsSelector- Uses the correct client list option
MatchingLabelsSelector- Handles the cluster-autoscaler PodMonitor case where pods may not exist
- Uses
namespace/nameformat for PodMonitor job names (Prometheus convention)This addresses previous feedback about selector handling.
4061-4084: LGTM: Prometheus query correctly retrieves target health metrics.The query uses the standard
upmetric to check target health in the HCP namespace. The validation ensures results are in the expected vector format and fails if no targets are found, implementing appropriate fail-closed behavior.
4086-4125: LGTM: Target aggregation implements correct fail-closed logic.The code properly implements fail-closed behavior where any down target marks the entire job as down:
- Detects down targets using
sample.Value != 1- Records all down instances per job
- Prevents a healthy instance from overriding a job that has failures
- Provides detailed failure reporting with instance information
This addresses previous feedback about failing closed when any target is down.
openshift-merge-robot
added
the
needs-rebase
4179f5b to
b722df5
Compare
openshift-merge-robot
removed
the
needs-rebase
openshift-ci-robot
added
jira/invalid-bug
|
@sjenning: This pull request references Jira Issue OCPBUGS-62851, which is invalid:
Comment 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. |
b722df5 to
105f1da
Compare
|
|
||
| // Add the PodMonitor name as a job, initially marked as down (false) | ||
| jobName := fmt.Sprintf("%s/%s", pm.Namespace, pm.Name) | ||
| jobs[jobName] = false |
There was a problem hiding this comment.
PodMonitor jobLabel field is not handled
Medium Severity
The code always derives the PodMonitor job name using the namespace/name format. However, per the PodMonitorSpec, if spec.JobLabel is set, Prometheus uses the corresponding Pod label value as the job name instead. Unlike the ServiceMonitor handling which checks sm.Spec.JobLabel, PodMonitors with jobLabel set will have a mismatched job name, causing the test to report them as down even when healthy.
Adds and e2e check to make sure that all pod and service monitors are scraping successfully.
105f1da to
3affe38
Compare
| if len(serviceMonitors.Items) == 0 && len(podMonitors.Items) == 0 { | ||
| t.Logf("No ServiceMonitors or PodMonitors found in namespace %s", hcpNamespace) | ||
| return | ||
| } |
There was a problem hiding this comment.
Empty jobs map causes test failure when all PodMonitors skipped
Medium Severity
The early return check verifies if serviceMonitors.Items and podMonitors.Items are empty, but the actual relevant condition is whether the jobs map is empty. When there are no ServiceMonitors and all PodMonitors are cluster-autoscaler ones without matching pods, the loop skips all PodMonitors, leaving jobs empty. However, the check at line 4266 sees non-empty podMonitors.Items and proceeds. Later, g.Expect(vector).NotTo(BeEmpty()) fails if there are no Prometheus targets, causing the test to timeout even though there are no expected jobs to monitor.
Additional Locations (1)
| // The jobLabel specifies which label on the matched Service contains the job name | ||
| labelValue, ok := service.GetLabels()[sm.Spec.JobLabel] | ||
| g.Expect(ok).To(BeTrue(), "ServiceMonitor %s/%s expects label %s on Service %s", sm.Namespace, sm.Name, sm.Spec.JobLabel, service.Name) | ||
| jobName = labelValue |
There was a problem hiding this comment.
ServiceMonitor JobLabel fallback to Service name not implemented
Medium Severity
According to the Prometheus Operator ServiceMonitor spec, when JobLabel is set but the specified label doesn't exist on the Service, the job name should fall back to the Service name. The test code instead fails with g.Expect(ok).To(BeTrue()) when the label is missing. This causes the test to fail in cases where Prometheus would correctly use the Service name as a fallback, creating false negative test failures for valid configurations.
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
@sjenning: The following tests 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. |
|
Are we still looking to get this PR in? |
|
Stale PRs are closed after 21d of inactivity. If this PR is still relevant, comment to refresh it or remove the stale label. If this PR is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
the
lifecycle/stale
|
Stale PRs rot after 14d of inactivity. Mark the PR as fresh by commenting If this PR is safe to close now please do so with /lifecycle rotten |
openshift-ci
Bot
added
lifecycle/rotten
|
The background agent confirmed my analysis. The report above is already complete with all the root cause details. Here's the final report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe new Root CauseTwo bugs in the new test code:
Recommendations
Evidence
|
|
Rotten PRs close after 7d of inactivity. Reopen the PR by commenting /close |
|
@openshift-ci[bot]: Closed this PR. 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 kubernetes-sigs/prow repository. |
|
@sjenning: This pull request references Jira Issue OCPBUGS-62851. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state. 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. |
9 participants
We need to catch when Service and Pod Monitor targets are down
Note
Introduces a health check to catch down metrics targets during cluster creation.
EnsureNoMetricsTargetsAreDownintest/e2e/util/util.goto listServiceMonitors/PodMonitors, derive jobs, and query Prometheusup{namespace="<hcp>"}with retries; skips autoscaler PodMonitor when no pods matchmonitoring.v1types intest/e2e/util/scheme.goTestCreateClusterafter existing validationsWritten by Cursor Bugbot for commit 3affe38. This will update automatically on new commits. Configure here.