WIP test increase intervals for image policy update event buffers#31031
WIP test increase intervals for image policy update event buffers#31031QiWang19 wants to merge 1 commit intoopenshift:mainfrom
Conversation
Only extend the "To" field to +30 minutes for AddSigtermProtection
and RemoveSigtermProtection event matchers, while keeping the "From"
field at ±30 seconds. This avoids unnecessarily long time windows
before the test run while still allowing for the MCP rollout duration
which can take upwards of 30 minutes.
Signed-off-by: Peter Hunt <pehunt@redhat.com>
Signed-off-by: Qi Wang <qiwan@redhat.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
Skipping CI for Draft Pull Request. |
WalkthroughThree interval-adjustment loops in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 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: QiWang19 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 |
|
/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-azure-ovn-techpreview-serial |
|
@QiWang19: 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/9cdbcdc0-3aad-11f1-88e0-eb078fc32a0d-0 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@pkg/monitortestlibrary/pathologicaleventlibrary/duplicated_event_patterns.go`:
- Around line 1275-1278: The loop that expands
configDriftMonitorStoppedIntervals is applying the +30m and -30s window to all
ConfigDriftMonitorStopped entries but the PR should only extend windows for
AddSigtermProtection and RemoveSigtermProtection; update the code in
duplicated_event_patterns.go so you no longer unconditionally modify
configDriftMonitorStoppedIntervals (variable name) — either remove this loop or
guard it to only adjust entries whose associated pattern/type equals
AddSigtermProtection or RemoveSigtermProtection (use the same identifying field
or slice index logic used where those patterns are created), leaving all other
ConfigDriftMonitorStopped intervals unchanged.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 58815797-7481-40af-be67-a5b515fbd798
📒 Files selected for processing (1)
pkg/monitortestlibrary/pathologicaleventlibrary/duplicated_event_patterns.go
| for i := range configDriftMonitorStoppedIntervals { | ||
| configDriftMonitorStoppedIntervals[i].To = configDriftMonitorStoppedIntervals[i].To.Add(time.Second * 30) | ||
| configDriftMonitorStoppedIntervals[i].To = configDriftMonitorStoppedIntervals[i].To.Add(time.Minute * 30) | ||
| configDriftMonitorStoppedIntervals[i].From = configDriftMonitorStoppedIntervals[i].From.Add(time.Second * -30) | ||
| } |
There was a problem hiding this comment.
ConfigDrift window expansion looks unintended for this PR scope.
Line 1276 also expands ConfigDriftMonitorStopped to +30m, but the PR objective says only AddSigtermProtection and RemoveSigtermProtection should get that extension. This broadens suppression behavior beyond the stated intent.
Suggested scope-aligned fix
for i := range configDriftMonitorStoppedIntervals {
- configDriftMonitorStoppedIntervals[i].To = configDriftMonitorStoppedIntervals[i].To.Add(time.Minute * 30)
+ configDriftMonitorStoppedIntervals[i].To = configDriftMonitorStoppedIntervals[i].To.Add(time.Second * 30)
configDriftMonitorStoppedIntervals[i].From = configDriftMonitorStoppedIntervals[i].From.Add(time.Second * -30)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for i := range configDriftMonitorStoppedIntervals { | |
| configDriftMonitorStoppedIntervals[i].To = configDriftMonitorStoppedIntervals[i].To.Add(time.Second * 30) | |
| configDriftMonitorStoppedIntervals[i].To = configDriftMonitorStoppedIntervals[i].To.Add(time.Minute * 30) | |
| configDriftMonitorStoppedIntervals[i].From = configDriftMonitorStoppedIntervals[i].From.Add(time.Second * -30) | |
| } | |
| for i := range configDriftMonitorStoppedIntervals { | |
| configDriftMonitorStoppedIntervals[i].To = configDriftMonitorStoppedIntervals[i].To.Add(time.Second * 30) | |
| configDriftMonitorStoppedIntervals[i].From = configDriftMonitorStoppedIntervals[i].From.Add(time.Second * -30) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/monitortestlibrary/pathologicaleventlibrary/duplicated_event_patterns.go`
around lines 1275 - 1278, The loop that expands
configDriftMonitorStoppedIntervals is applying the +30m and -30s window to all
ConfigDriftMonitorStopped entries but the PR should only extend windows for
AddSigtermProtection and RemoveSigtermProtection; update the code in
duplicated_event_patterns.go so you no longer unconditionally modify
configDriftMonitorStoppedIntervals (variable name) — either remove this loop or
guard it to only adjust entries whose associated pattern/type equals
AddSigtermProtection or RemoveSigtermProtection (use the same identifying field
or slice index logic used where those patterns are created), leaving all other
ConfigDriftMonitorStopped intervals unchanged.
Only extend the "To" field to +30 minutes for AddSigtermProtection
and RemoveSigtermProtection event matchers, while keeping the "From"
field at ±30 seconds. This avoids unnecessarily long time windows
before the test run while still allowing for the MCP rollout duration
which can take upwards of 30 minutes.
Summary by CodeRabbit