CNTRLPLANE-3238: pathological events: bump KMS ScalingReplicaSet threshold to 150#31209
CNTRLPLANE-3238: pathological events: bump KMS ScalingReplicaSet threshold to 150#31209gangwgr wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
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 selected for processing (1)
WalkthroughThis pull request increases the repeat threshold tolerance for ChangesKMS Encryption ScalingReplicaSet Matcher
🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested labels: Suggested reviewers:
🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gangwgr 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
openshift-ci
Bot
added
the
ready-for-human-review
|
Scheduling required tests: |
|
/retest-required |
|
/test e2e-vsphere-ovn-upi |
|
/verified by ci runs |
openshift-ci-robot
added
the
verified
|
@gangwgr: 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. |
gangwgr
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@gangwgr: This pull request explicitly references no jira issue. 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. |
gangwgr
changed the title
|
@gangwgr: This pull request references CNTRLPLANE-3238 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. |
| }, | ||
| messageReasonRegex: regexp.MustCompile(`^ScalingReplicaSet$`), | ||
| repeatThresholdOverride: 100, | ||
| repeatThresholdOverride: 150, |
There was a problem hiding this comment.
IMO, 150 is too high. If 106 is expected count, maybe we should update this to 110.
There was a problem hiding this comment.
Added 120 since 110 is very close to the current threshold.
There was a problem hiding this comment.
I think, in that scenario, it is better to close to the threshold. I'd like to hear opinions from @p0lyn0mial
gangwgr
force-pushed
the
fix-kms-scaling-threshold
branch
from
e5cce4a to
aa7c66b
Compare
openshift-ci-robot
removed
the
verified
The KMS encryption tests trigger cascading rollouts across openshift-apiserver and openshift-oauth-apiserver. The previous threshold of 100 was exceeded in CI (observed 106 events), causing spurious pathological event failures. Bump to 150 to provide adequate headroom.
gangwgr
force-pushed
the
fix-kms-scaling-threshold
branch
from
aa7c66b to
cf9ccf9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/monitortestlibrary/pathologicaleventlibrary/duplicated_event_patterns.go`:
- Line 1403: The value assigned to repeatThresholdOverride in
duplicated_event_patterns.go (repeatThresholdOverride: 120) contradicts the PR
description that says the threshold was increased to 150 and ignores prior
feedback suggesting 110; fix by making the intent consistent: either update the
code to set repeatThresholdOverride to 150 to match the PR title/description, or
if 120 (or 110) is preferred, update the PR description and justification to
state that value and why (e.g., observed max = 106 with chosen headroom). Locate
the repeatThresholdOverride assignment in duplicated_event_patterns.go and
adjust the numeric constant or the PR text accordingly so code and PR messaging
align.
- Line 1394: The comment above the repeated-event threshold is inconsistent with
the code: update the comment to match the implemented value used by
repeatThresholdOverride (currently 120) or change repeatThresholdOverride to 150
if the intended threshold is 150; locate the occurrence of
repeatThresholdOverride in duplicated_event_patterns.go and either (A) edit the
comment "threshold set to 150 with headroom." to "threshold set to 120 with
headroom." or (B) change the constant/assignment for repeatThresholdOverride
from 120 to 150 so code and comment match.
🪄 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: 22625b1d-902e-4f29-8146-5e76de51a887
📒 Files selected for processing (1)
pkg/monitortestlibrary/pathologicaleventlibrary/duplicated_event_patterns.go
| }, | ||
| messageReasonRegex: regexp.MustCompile(`^ScalingReplicaSet$`), | ||
| repeatThresholdOverride: 100, | ||
| repeatThresholdOverride: 120, |
There was a problem hiding this comment.
Threshold value (120) contradicts PR description (150) and ignores prior feedback.
The repeatThresholdOverride is set to 120, but the PR title and description state the threshold is being increased to 150. Additionally, a past reviewer suggested that 110 would be more appropriate given the observed maximum of 106 events.
The current value of 120 provides reasonable headroom (~13% above the observed max), but the PR messaging is inconsistent and prior feedback appears unaddressed.
🔍 Suggested clarification
Either:
- Update the PR description to reflect the actual threshold of 120, or
- Change the code to 150 if that's the intended value, or
- Address the past review feedback suggesting 110 is sufficient
🤖 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 `@pkg/monitortestlibrary/pathologicaleventlibrary/duplicated_event_patterns.go`
at line 1403, The value assigned to repeatThresholdOverride in
duplicated_event_patterns.go (repeatThresholdOverride: 120) contradicts the PR
description that says the threshold was increased to 150 and ignores prior
feedback suggesting 110; fix by making the intent consistent: either update the
code to set repeatThresholdOverride to 150 to match the PR title/description, or
if 120 (or 110) is preferred, update the PR description and justification to
state that value and why (e.g., observed max = 106 with chosen headroom). Locate
the repeatThresholdOverride assignment in duplicated_event_patterns.go and
adjust the numeric constant or the PR text accordingly so code and PR messaging
align.
|
Scheduling required tests: |
|
@gangwgr: 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. |
3 participants
The KMS encryption tests trigger cascading rollouts across openshift-apiserver and openshift-oauth-apiserver. The previous threshold of 100 was exceeded in CI (observed 106 events), causing spurious pathological event failures. Bump to 150 to provide adequate headroom.
Summary by CodeRabbit