OCPBUGS-99397: remove custom MCP and pause master pool in mirror-set tests to prevent PDB drain deadlock and suite timeout#31408
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Chandan9112 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 |
|
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:
WalkthroughThe image mirror end-to-end tests now use the cluster-scoped ChangesImage mirror validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![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
🧹 Nitpick comments (1)
test/extended/node/node_e2e/image_mirror_set.go (1)
135-136: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse a bounded, detached cleanup context instead of
context.Background().Both cleanup paths discard Ginkgo context values; the new unpause PATCH also has no explicit deadline. Thread the cleanup context into teardown and derive bounded contexts with
context.WithoutCancel(ctx).
test/extended/node/node_e2e/image_mirror_set.go#L135-L136: use the detached, timeout-bounded context forunpauseWorkerMCP.test/extended/node/node_e2e/image_mirror_set.go#L279-L279: derivecleanupCtxfrom the existing spec context rather thancontext.Background().Based on learnings, deferred cleanup should preserve context values with
context.WithoutCancel(ctx)and then apply an explicit timeout. As per path instructions, Go API calls must remain cancellation/timeout bounded.🤖 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 `@test/extended/node/node_e2e/image_mirror_set.go` around lines 135 - 136, The cleanup flow in test/extended/node/node_e2e/image_mirror_set.go must use a detached, timeout-bounded context for both cleanup paths: at lines 135-136, pass the derived cleanup context to unpauseWorkerMCP; at line 279, derive cleanupCtx from the existing spec context with context.WithoutCancel(ctx) and apply an explicit timeout instead of using context.Background().Sources: Path instructions, Learnings
🤖 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/extended/node/node_e2e/image_mirror_set.go`:
- Around line 117-123: Update mirrorTestPool.unpauseWorkerMCP to retry the
worker MCP patch and verify that the resulting MachineConfigPool has
spec.paused=false. If restoration still fails, record a cleanup failure and
prevent teardown from continuing in a way that could leave the cluster-scoped
worker MCP paused; retain the success log only after verification passes.
---
Nitpick comments:
In `@test/extended/node/node_e2e/image_mirror_set.go`:
- Around line 135-136: The cleanup flow in
test/extended/node/node_e2e/image_mirror_set.go must use a detached,
timeout-bounded context for both cleanup paths: at lines 135-136, pass the
derived cleanup context to unpauseWorkerMCP; at line 279, derive cleanupCtx from
the existing spec context with context.WithoutCancel(ctx) and apply an explicit
timeout instead of using context.Background().
🪄 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: 1b5e0a66-a84d-4a2e-b23e-fad269433ea5
📒 Files selected for processing (1)
test/extended/node/node_e2e/image_mirror_set.go
Chandan9112
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@Chandan9112: This pull request references Jira Issue OCPBUGS-99397, which is invalid:
Comment 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. |
Chandan9112
changed the title
0b62ca0 to
46e684f
Compare
openshift-ci-robot
added
jira/valid-bug
|
@Chandan9112: This pull request references Jira Issue OCPBUGS-99397, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@Chandan9112: This pull request references Jira Issue OCPBUGS-99397, which is valid. 3 validation(s) were run on this bug
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: |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-vsphere-disruptive-longrunning-techpreview |
|
@BhargaviGudi: 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/c57ee790-85b1-11f1-839f-e365ebbac51d-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-azure-disruptive-longrunning-techpreview-1of2 |
|
@BhargaviGudi: 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/79743d90-85b2-11f1-837f-5e32378ade18-0 |
|
@Chandan9112 Pause is useful to prevent the MCO from changing node config, or when you want to stack up a whole bunch of changes and only take a single reboot. Have you thought of other ways to solve this? |
@ngopalak-redhat I thought of removing the custom MCP entirely and testing on the worker pool directly (sequential drain via default maxUnavailable=1), but that's a larger refactor. For now, pause is the lightest fix to unblock CI — worker nodes stay fully schedulable, only config updates are deferred, and it's cleanly reversed in Teardown. |
|
@Chandan9112 Can we try removing custom MCP for cluster level resources? Will be helpful on a long term. |
|
@Chandan9112: This pull request references Jira Issue OCPBUGS-99397, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
46e684f to
1733cb9
Compare
…dlock The mirror-set tests (OCP-57401, OCP-70203) used a custom single-node MachineConfigPool to isolate rollouts. However, cluster-scoped IDMS/ITMS/ICSP resources trigger config changes on all pools simultaneously. On 2-worker clusters, this caused both the custom pool and worker pool to drain nodes concurrently, leading to a PDB deadlock where PDB-protected pods had nowhere to reschedule. Fix: Remove the custom MCP entirely and test directly on the worker and master pools. The MCO handles pool rollouts sequentially (maxUnavailable=1), so nodes drain one at a time and PDB-protected pods can always reschedule. Bug: OCPBUGS-99397 Co-authored-by: Cursor <cursoragent@cursor.com>
c59722a to
1972030
Compare
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-disruptive-longrunning |
|
@Chandan9112: 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/8e9c0670-8658-11f1-9f79-9cc90db56f9b-0 |
|
Scheduling required tests: |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-vsphere-disruptive-longrunning-techpreview |
|
@QiWang19: 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/897ef9d0-86c7-11f1-94b7-3ddea5d78d25-0 |
@QiWang19 ., I see job is passing in CI in shard 2 |
|
/retest |
QiWang19
left a comment
QiWang19
left a comment
There was a problem hiding this comment.
From the machine-config-controller log, this failure seems to be a test case timeout; the master nodes still processed the drain from the cleanup source deletion when the test suite timed out.
We probably need to refactor the whole test case to avoid including many steps and rollouts.
To unblock us and get the required pass rate for API graduation, we may try a temporary workaround for now:
- Pause master pool
- Create/delete resources during test steps → only worker drains
- Cleanup: delete all remaining resources
- Unpause master → MCO renders config, sees it matches what masters already have → no rollout, no drain
- Wait for both pools to be Updated=True
| func (p *mirrorTestPool) waitForRollout(initialSpec string) { | ||
| imagepolicy.WaitForMCPConfigSpecChangeAndUpdated(p.oc, p.PoolName, initialSpec) | ||
| func waitForPoolsRollout(oc *exutil.CLI, workerSpec, masterSpec string) { | ||
| imagepolicy.WaitForMCPConfigSpecChangeAndUpdated(oc, "worker", workerSpec) |
There was a problem hiding this comment.
If use WaitForMCPsConfigSpecChangeAndUpdated to wait for both pool at the same time, would this save more time for this testcase?
There was a problem hiding this comment.
Thanks @QiWang19 ., Yes it reduced ~7 min of time. The latest code has been pushed. Can you have a review now?
Neeraj already pushed back on the pause approach in this PR — he suggested removing the custom MCP instead, which we've done. |
During test steps, only wait for worker pool rollout since registries.conf verification is performed exclusively on worker nodes. Master pool drains in the background concurrently. In cleanup, wait for both worker and master pools to ensure the cluster is fully stable before the next test runs. This reduces test execution time significantly (~30-40%) while maintaining correctness and cluster stability guarantees. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@Chandan9112: This pull request references Jira Issue OCPBUGS-99397, 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 (cmaurya@redhat.com), skipping review request. 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. |
|
@Chandan9112: This pull request references Jira Issue OCPBUGS-99397, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (cmaurya@redhat.com), skipping review request. 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. |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-vsphere-disruptive-longrunning-techpreview |
|
@Chandan9112: 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/2ed8b1a0-872c-11f1-98c4-ff3966c08fa4-0 |
|
Scheduling required tests: |
|
openshift/api#2844 (comment) |
|
We can also wait for openshift/release#81923 to ignoring mirror-set test on techpreview jobs. |
@QiWang19 ., I have paused the pool as fix., it will resolve deadlock issue as tested locally on vsphere cluster. |
Pause the master MCP at the start of each test so only worker nodes drain during test steps. In cleanup, after all test resources are deleted, unpause master — MCO sees the master config matches its current state (since resources were removed before unpause), so no drain is needed. This eliminates master drain time entirely from both the test body and cleanup, preventing the suite timeout that was blocking CRIOCredentialProviderConfig feature promotion (openshift/api#2844). Co-authored-by: Cursor <cursoragent@cursor.com>
c3d9e1f to
5bf65f1
Compare
Chandan9112
changed the title
|
@Chandan9112: This pull request references Jira Issue OCPBUGS-99397, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (cmaurya@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. |
|
/retest |
|
Scheduling required tests: |
|
@Chandan9112: The following test 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. |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-vsphere-disruptive-longrunning-techpreview |
|
@Chandan9112: 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/1de04700-87f9-11f1-9c4e-6f1554f631de-0 |
5 participants
Summary
Fixes PDB drain deadlock and suite timeout in
[OCP-57401]and[OCP-70203]mirror-set tests on 2-worker clusters (vSphere, AWS 1of2 split).Root Cause
The tests previously used a custom
mirrorTestPoolMCP for a single worker node. Since IDMS/ITMS/ICSP are cluster-scoped, creating them triggered rollouts on both the custom MCP and the main worker MCP simultaneously. On 2-worker clusters, MCO attempted to drain both workers at once, but PodDisruptionBudgets prevented pod rescheduling — causing a deadlock where neither node could drain and MCP rollouts stalled indefinitely.Additionally, since IDMS/ITMS are cluster-scoped, they also trigger master pool rollouts. On clusters with 3 master nodes, draining masters sequentially during cleanup adds significant time (~15-20 min), causing the overall disruptive test suite to timeout.
Fix
Remove custom MCP entirely — Tests now operate directly on the worker pool, eliminating the concurrent drain scenario that caused the PDB deadlock.
Pause master pool during test execution — Since we only verify
registries.confon worker nodes, the master pool drain is unnecessary for test validation. Pausing the master at test start and unpausing inDeferCleanupprevents master drains from contributing to suite timeout.Use worker-only waits in test body — Test steps use
waitForWorkerRollout(callsimagepolicy.WaitForMCPConfigSpecChangeAndUpdated) to wait only for the worker pool during verification steps, reducing blocking time.Use parallel pool waits in cleanup — Cleanup uses
waitForPoolsRollout(callsimagepolicy.WaitForMCPsConfigSpecChangeAndUpdated) to wait for both worker and master pools in parallel, ensuring cluster stability before the next test.Changes
mirrorTestPoolstruct, methods, and all custom MCP lifecycle codepauseMasterPool/unpauseMasterPoolhelpersgetWorkerMCPSpec/waitForWorkerRollout/waitForPoolsRollout/readRegistriesConfOnWorkerhelpersOCP-57401andOCP-70203tests updated to use new approachTesting
Bug Reference
https://redhat.atlassian.net/browse/OCPBUGS-99397