OCPQE-32064: Reduce monitortest sensitivity for upcoming spot check jobs, use etcd-scaling job as early example - #31212
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces a new SpotCheck cluster stability mode that enables lightweight monitoring runs without influencing test results via optional JUnit suppression. It migrates etcd leader-changes verification from Ginkgo tests to the monitor framework, updates multiple monitor tests to handle flaking and skipping flags, and integrates the mode throughout the test infrastructure. ChangesSpotCheck Cluster Stability Mode
Sequence DiagramsequenceDiagram
participant CLI as Test CLI
participant Framework as Monitor Framework
participant Registry as Registry Builder
participant EtcdMonitor as Etcd Leader-Changes
participant Prometheus
CLI->>Framework: NewMonitorTestsFor(SpotCheck)
Framework->>Registry: newSpotCheckMonitorTests(info with SkipJunits=true)
Registry->>EtcdMonitor: Register etcd leader-changes monitor
Registry->>Framework: Return curated monitor set
Framework->>EtcdMonitor: StartCollection
EtcdMonitor->>EtcdMonitor: Record start time
Framework->>EtcdMonitor: CollectData
EtcdMonitor->>Prometheus: Query etcd_server_leader_changes_seen_total
Prometheus-->>EtcdMonitor: Return sampled value
EtcdMonitor->>EtcdMonitor: Compare vs allowed threshold
EtcdMonitor-->>Framework: JUnit test case (pass/fail)
Framework-->>CLI: Test artifacts (suppressed if SkipJunits)
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-gcp-ovn-etcd-scaling |
|
@dgoodwin: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/87a97980-5843-11f1-8824-e7855c3d2249-0 |
9f8b39b to
7525d8a
Compare
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-etcd-scaling |
|
@dgoodwin: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/81856720-5844-11f1-805b-5abfdac98a10-0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/monitortests/etcd/leaderchanges/monitortest.go`:
- Around line 83-85: The code currently only treats
configv1.ExternalTopologyMode as external by setting etcdNamespace =
"clusters-.*" in monitortest.go; update the conditional around
infra.Status.ControlPlaneTopology to also treat the DualReplica topology as
external (or explicitly map it) so etcdNamespace is set appropriately for
dual-replica clusters and avoid querying in-cluster metrics when etcd is
external; modify the branch that sets etcdNamespace (referencing
infra.Status.ControlPlaneTopology, configv1.ExternalTopologyMode, and the
etcdNamespace variable) to include configv1.DualReplicaTopologyMode (or the
correct DualReplica constant) as an additional case.
🪄 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: b9a3a38a-08e7-4578-92da-2d3d995a9356
📒 Files selected for processing (11)
pkg/defaultmonitortests/types.gopkg/monitortestframework/types.gopkg/monitortests/clusterversionoperator/clusterversionchecker/monitortest.gopkg/monitortests/etcd/etcdloganalyzer/monitortest.gopkg/monitortests/etcd/leaderchanges/monitortest.gopkg/monitortests/node/watchnodes/monitortest.gopkg/monitortests/testframework/legacytestframeworkmonitortests/alerts_monitortest.gopkg/test/ginkgo/cmd_runsuite.gopkg/test/ginkgo/test_suite.gopkg/testsuites/standard_suites.gotest/extended/etcd/leader_changes.go
💤 Files with no reviewable changes (1)
- test/extended/etcd/leader_changes.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/monitortests/etcd/leaderchanges/monitortest.go`:
- Around line 119-123: The code truncates Prometheus leader-change counts by
converting leaderChanges (vec[0].Value, a model.SampleValue/float) to int before
comparing against allowedLeaderChanges; change the comparison to use
floating-point so no truncation occurs (e.g., compare leaderChanges as a float64
to float64(allowedLeaderChanges)), update any fmt.Sprintf/return messages to
format the original float value (leaderChanges) rather than the truncated int,
and keep the existing symbols (leaderChanges, allowedLeaderChanges,
numberOfRevisions, vec, failJunit) to locate and apply the change.
🪄 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: a97e84c4-7b22-4803-85fb-5ad06593327b
📒 Files selected for processing (4)
pkg/defaultmonitortests/types.gopkg/monitortests/etcd/leaderchanges/monitortest.gopkg/testsuites/standard_suites.gotest/extended/etcd/leader_changes.go
💤 Files with no reviewable changes (1)
- test/extended/etcd/leader_changes.go
|
Scheduling required tests: |
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 7525d8a
|
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-gcp-ovn-etcd-scaling |
|
@dgoodwin: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/58637900-58f2-11f1-9c98-f9959cc00a15-0 |
dgoodwin
changed the title
|
/hold Still exploring. Need confirmation from etcd team they like the change, and need to confirm that ported etcd test which will now run in serial jobs, is not going to cause problems. |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-gcp-ovn-etcd-scaling |
|
@dgoodwin: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/cf4a3d50-6672-11f1-8dc3-85130dfe8ae7-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-gcp-ovn-etcd-scaling |
|
@dgoodwin: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/9a6a2ff0-68ac-11f1-9d7a-3f72da01643f-0 |
7525d8a to
37c6910
Compare
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-gcp-ovn-etcd-scaling Earlier runs picked up an actual regression, that was since fixed. Now rebased for another try. |
|
@dgoodwin: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/b3fba4a0-698b-11f1-9cda-b477f0c42256-0 |
|
Scheduling required tests: |
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 37c6910
|
…t not fail junits
|
Scheduling required tests: |
There was a problem hiding this comment.
🤖 AI-generated by Opus 4.8 — posted on behalf of @mkowalski.
Re-review at 2e9b3d28 — thanks for the quick turnaround, most of the previous round is resolved. I re-checked each item against the new revision:
- ✅
termination-message-policyrestored to the Stable set only (not Disruptive/SpotCheck). Matches your intent given the test's content. - ✅ SpotCheck doc/inline comments no longer reference a flaked
cluster-version-checker; the prose now matches the registry (etcd-log-analyzer, alerts). - ✅
clusterversioncheckerno-op edit dropped from the PR. - ✅
JUnitsToFlakesnow returns a fresh slice, and the newTestJUnitsToFlakes_DoesNotExtendOriginalSlicecovers exactly the backing-array aliasing case — nice. - ✅ etcd-scaling qualifier wrapped in parens, so the
&& !name.contains("leader changes")now binds to the whole OR. Reads unambiguously now.
One minor doc nit still open (left a note in the existing thread, non-blocking): the Disruptive doc comment still uses node-lifecycle as the "still hard-fails" example, but node-lifecycle isn't registered in the Disruptive set — so the example points at a monitor that isn't there.
For my part this all looks good; the remaining item is cosmetic. (Also confirmed the leader-changes Ginkgo test is intentionally just excluded here rather than migrated — the description/CodeRabbit walkthrough still describe the older migration approach, so you may want to refresh the PR body before merge.)
🤖 End AI-generated content.
|
Comment discrepancies now fixed. |
|
/lgtm |
|
/verified by @dgoodwin |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin, mkowalski 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-robot
added
the
verified
|
@mkowalski: This PR has been marked as verified by 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. |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@dgoodwin: This pull request references OCPQE-32064 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. |
|
Scheduling required tests: |
|
Unrelated and enough is enough. /override ci/prow/e2e-gcp-csi |
|
@dgoodwin: Overrode contexts on behalf of dgoodwin: ci/prow/e2e-gcp-csi 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. |
|
@dgoodwin: all tests passed! 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. |
This work progresses towards the concept of spot check jobs as defined by the next gen QE working group. These will be jobs run very rarely, roughly once a month. The jobs must have an owning component and capability/feature. If the job passes, all is good. If it fails, we will retry with backoff measured in days. After a couple failures, the component will be marked regressed. The job must pass once every 30 days.
These jobs must be much less sensitive, they are looking for verify obscure features or configurations and do not need the rigor our normal test jobs we run thousands of times a week do. As such, we need to calm down monitortests signficantly.
To do this I added a new clusterstability level for spot check jobs so they can control which monitortests are enabled.
A couple monitortests are left in for information gathering purposes, but their junit generation can now be set to flake only, so they will not fail jobs but we can track flake rates and see flakes in spyglass if needed.
I moved the etcd-scaling suite to use this new stability level and confirmed it corrects the monitortests that typically fail this never passing job, as they validate functionality that is inherently going to happen in jobs like this. etcd team has reviewed and approved the change. (though they want to keep their job running weekly, which still works fine with this plan as we don't have to create something to schedule retries)
One ginko etcd leader change e2e test is ported from an e2e to a monitortest to take advantage of this functionality, it was at it's core a monitortest, and is not applicable to a job where we intentionally are scaling etcd.
The disruptive stability level also now has an explicit list of monitortests, I removed the concept of universal tests as they were no longer universal and it was getting too confusing to view what stability level uses what tests. It's now an explicit list for each.