Skip to content

NO-ISSUE: fixes flaky config drift monitor E2E tests - #6373

Draft
cheesesashimi wants to merge 1 commit into
openshift:mainfrom
cheesesashimi:zzlotnik/fix-flaky-gcp-job
Draft

NO-ISSUE: fixes flaky config drift monitor E2E tests#6373
cheesesashimi wants to merge 1 commit into
openshift:mainfrom
cheesesashimi:zzlotnik/fix-flaky-gcp-job

Conversation

@cheesesashimi

@cheesesashimi cheesesashimi commented Aug 5, 2026

Copy link
Copy Markdown
Member

- What I did

This raises the timeout for all config drift monitor E2E test cases which expect the node to reboot as part of the test process. Setting this to 15 minutes makes this more consistent with the rest of the MCO E2E test suite.

- How to verify it

Run any of the test jobs which run the config drift monitor E2E tests. They should pass.

- Description for the changelog
Fixes flaky config drift monitor E2E tests

Summary by CodeRabbit

  • Tests
    • Improved recovery test reliability by supporting configurable timeouts.
    • Added longer recovery windows for scenarios requiring a node reboot.
    • Preserved the existing default timeout for standard recovery checks.

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@openshift-ci

openshift-ci Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 5, 2026
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 87ee6798-a066-40d8-bc21-2fb48da5cd97

📥 Commits

Reviewing files that changed from the base of the PR and between 13ea39a and 8a53c85.

📒 Files selected for processing (2)
  • test/e2e-shared-tests/helpers.go
  • test/e2e-shared-tests/mcd_config_drift.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • test/e2e-shared-tests/helpers.go
  • test/e2e-shared-tests/mcd_config_drift.go

Walkthrough

Recovery helpers now accept configurable timeouts for node and MachineConfigPool checks. Config drift tests use a longer timeout when recovery requires a node reboot, while standard recovery retains the five-minute default.

Changes

Config Drift Recovery

Layer / File(s) Summary
Recovery assertion timeouts
test/e2e-shared-tests/helpers.go, test/e2e-shared-tests/mcd_config_drift.go
Recovery helpers use five-minute defaults and a fifteen-minute reboot-specific timeout. Node and MachineConfigPool polling passes the active context to resource checks.
Recovery test wiring
test/e2e-shared-tests/mcd_config_drift.go
Config drift recovery uses shared logic and selects the extended timeout when a node reboot is expected. Forcefile recovery uses the reboot-specific helper.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 15
✅ Passed checks (15 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: fixing flaky config drift monitor E2E tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed The modified files (helpers.go and mcd_config_drift.go) contain no Ginkgo test definitions. All test case names in mcd_config_drift.go are static strings without dynamic values.
Test Structure And Quality ✅ Passed Reboot recovery uses a bounded 15-minute poll, normal recovery keeps 5 minutes, cleanup runs via the shared teardown, and failure messages identify the node or MachineConfigPool.
Microshift Test Compatibility ✅ Passed The PR does not add new Ginkgo e2e tests. It only modifies existing test infrastructure by adding helper functions and adjusting timeouts for recovery assertions in config drift tests.
Single Node Openshift (Sno) Test Compatibility ✅ Passed The patch adds no Ginkgo declarations or new tests; it only changes recovery helpers and routes the existing forcefile case through a longer timeout, with no multi-node assumptions.
Topology-Aware Scheduling Compatibility ✅ Passed The PR modifies only config-drift E2E helpers and tests. It adds timeout logic and no deployments, controllers, replicas, affinity, selectors, tolerations, or other scheduling constraints.
Ote Binary Stdout Contract ✅ Passed The PR changes only recovery helpers and polling callbacks. Neither changed file has main, init, suite setup, or stdout/log API writes; the OTE main also has no direct stdout writes.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed The PR does not add new Ginkgo e2e tests. It modifies reusable test helper functions in a utility package. The changes only adjust timeouts (from 5 to 15 minutes for reboot scenarios) and contain n...
No-Weak-Crypto ✅ Passed The patch adds only timeout, polling, and recovery-state logic; touched-file scans found no weak crypto APIs, custom crypto, or secret/token comparisons.
Container-Privileges ✅ Passed The PR changes only two Go test files and adds no Kubernetes/container manifests or privilege settings such as privileged, hostPID, hostNetwork, hostIPC, SYS_ADMIN, or allowPrivilegeEscalation.
No-Sensitive-Data-In-Logs ✅ Passed The commit adds no logging statements. Existing logs contain only synthetic test contents, resource names, paths, and durations; no passwords, tokens, API keys, PII, or customer data are introduced.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@openshift-ci

openshift-ci Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheesesashimi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2026
@cheesesashimi

Copy link
Copy Markdown
Member Author

/test e2e-gcp-op-part1 e2e-gcp-op-single-node

@cheesesashimi cheesesashimi changed the title fixes flaky config drift monitor E2E tests NO-ISSUE: fixes flaky config drift monitor E2E tests Aug 5, 2026
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 5, 2026
@openshift-ci-robot

Copy link
Copy Markdown
Contributor

@cheesesashimi: This pull request explicitly references no jira issue.

Details

In response to this:

- What I did

This raises the timeout for all config drift monitor E2E test cases which expect the node to reboot as part of the test process. Setting this to 15 minutes makes this more consistent with the rest of the MCO E2E test suite.

- How to verify it

Run any of the test jobs which run the config drift monitor E2E tests. They should pass.

- Description for the changelog
Fixes flaky config drift monitor E2E tests

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@test/e2e-shared-tests/mcd_config_drift.go`:
- Around line 421-422: Update assertNodeReachesStateWithTimeout and
assertPoolReachesStateWithTimeout to accept or receive the polling callback
context from pollForResourceState, and pass that context to Nodes().Get and
MachineConfigPools().Get instead of context.TODO(), so both requests are
cancelled when the polling deadline expires.
- Around line 421-422: Update assertNodeReachesStateWithTimeout and the
corresponding MachineConfigPool callback to check the Get API error before
dereferencing the returned resource or invoking stateFunc. Pass the error first
to pollForResourceState so nil resources during outages trigger retries rather
than panics.
🪄 Autofix

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: ba3623fe-2540-49ad-83f4-eac553b9fddc

📥 Commits

Reviewing files that changed from the base of the PR and between 2117ea3 and d090bd8.

📒 Files selected for processing (2)
  • test/e2e-shared-tests/helpers.go
  • test/e2e-shared-tests/mcd_config_drift.go

Comment thread test/e2e-shared-tests/mcd_config_drift.go
Assisted-By: Claude Sonnet 4.6

This raises the timeout for all config drift monitor test cases which
expect the node to reboot as part of the test process. Setting this to
15 minutes makes this more consistent with the rest of the MCO E2E test
suite.
@cheesesashimi
cheesesashimi force-pushed the zzlotnik/fix-flaky-gcp-job branch from d090bd8 to 8a53c85 Compare August 6, 2026 14:14
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@cheesesashimi

Copy link
Copy Markdown
Member Author

/test e2e-gcp-op-part1 e2e-gcp-op-single-node

@openshift-ci

openshift-ci Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@cheesesashimi: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-gcp-op-part1 8a53c85 link true /test e2e-gcp-op-part1

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants