OCPBUGS-99397: remove custom MCP from mirror-set tests to prevent PDB drain deadlock#31408
OCPBUGS-99397: remove custom MCP from mirror-set tests to prevent PDB drain deadlock#31408Chandan9112 wants to merge 1 commit into
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 |
WalkthroughThe image mirror-set end-to-end tests now use the cluster-scoped ChangesWorker MCP mirror-set verification
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, 1 warning)
✅ Passed checks (13 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: |
|
@Chandan9112: 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. |
|
/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. |
…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 pool. The MCO handles worker pool rollouts sequentially (maxUnavailable=1), so nodes drain one at a time and PDB-protected pods can always reschedule. This eliminates the ~20 min MCP setup/teardown overhead while keeping test assertions identical. Bug: OCPBUGS-99397 Co-authored-by: Cursor <cursoragent@cursor.com>
46e684f to
1733cb9
Compare
|
@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. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/extended/node/node_e2e/image_mirror_set.go (2)
102-115: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse a detached, bounded context for both
DeferCleanupblocks instead ofcontext.Background(). Both cleanup closures issue deletes withcontext.Background(), dropping context values and running unbounded. Per the repo learning, detach cancellation from the specctxand apply an explicit timeout so cleanup survives spec-context cancellation while staying bounded.
test/extended/node/node_e2e/image_mirror_set.go#L102-L115: replace thecontext.Background()IDMS/ITMS deletes with actxCleanup, cancel := context.WithTimeout(context.WithoutCancel(ctx), <timeout>)derived context (defercancel()), and pass it to the delete calls.test/extended/node/node_e2e/image_mirror_set.go#L227-L255: apply the same derived context to the ICSP/ITMS/IDMSGet/Deletecalls in this cleanup block.🤖 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 102 - 115, Update both cleanup closures in test/extended/node/node_e2e/image_mirror_set.go:102-115 and test/extended/node/node_e2e/image_mirror_set.go:227-255 to derive a bounded cleanup context using context.WithTimeout(context.WithoutCancel(ctx), the repository’s appropriate timeout), defer its cancellation, and use it for all IDMS/ITMS deletes and ICSP/ITMS/IDMS Get/Delete calls. Preserve the existing cleanup behavior while ensuring it survives spec-context cancellation and remains time-bounded.Source: Learnings
42-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer context-aware polling here
wait.PollImmediateandwait.ErrWaitTimeoutare legacyapimachineryAPIs. Thread acontext.Contextthrough this helper and switch towait.PollUntilContextTimeout; usewait.Interrupted(err)(orerrors.Is(err, context.DeadlineExceeded)) for the timeout branch instead of comparing againstwait.ErrWaitTimeout.🤖 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 42 - 53, The pollMCPSpecUnchanged helper should use context-aware polling instead of legacy wait APIs. Add a context.Context parameter, call wait.PollUntilContextTimeout with that context, and recognize timeout via wait.Interrupted or context.DeadlineExceeded while preserving the existing success and spec-change error behavior.Source: Path instructions
🤖 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.
Nitpick comments:
In `@test/extended/node/node_e2e/image_mirror_set.go`:
- Around line 102-115: Update both cleanup closures in
test/extended/node/node_e2e/image_mirror_set.go:102-115 and
test/extended/node/node_e2e/image_mirror_set.go:227-255 to derive a bounded
cleanup context using context.WithTimeout(context.WithoutCancel(ctx), the
repository’s appropriate timeout), defer its cancellation, and use it for all
IDMS/ITMS deletes and ICSP/ITMS/IDMS Get/Delete calls. Preserve the existing
cleanup behavior while ensuring it survives spec-context cancellation and
remains time-bounded.
- Around line 42-53: The pollMCPSpecUnchanged helper should use context-aware
polling instead of legacy wait APIs. Add a context.Context parameter, call
wait.PollUntilContextTimeout with that context, and recognize timeout via
wait.Interrupted or context.DeadlineExceeded while preserving the existing
success and spec-change error behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8a364252-5db1-4e4e-8437-f10769d4778f
📒 Files selected for processing (1)
test/extended/node/node_e2e/image_mirror_set.go
Chandan9112
changed the title
|
/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/2fbbaf30-85da-11f1-85e7-af1285877ad2-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-azure-disruptive-longrunning-techpreview-1of2 |
|
@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/481fe2d0-85da-11f1-97c6-6d5a7b3287c4-0 |
Thanks @ngopalak-redhat ., it worked fine. Removed the custom MCP entirely — both tests now operate directly on the worker pool, which avoids the PDB drain deadlock without needing any pause/unpause logic. Tested on vSphere (2 workers), both OCP-57401 and OCP-70203 pass cleanly with cluster fully stable after cleanup. |
|
|
||
| func (p *mirrorTestPool) waitForRollout(initialSpec string) { | ||
| imagepolicy.WaitForMCPConfigSpecChangeAndUpdated(p.oc, p.PoolName, initialSpec) | ||
| func waitForWorkerRollout(oc *exutil.CLI, initialSpec string) { |
There was a problem hiding this comment.
Could you add wait and cleanup for both work and master pool?
The CRDs ICSP/IDMS/ITMS are setting cluster wide mirror configs, not for specific labeled pools.
When verifying the test, it is ok to only check work pool.
5 participants
Bug
https://redhat.atlassian.net/browse/OCPBUGS-99397
Problem
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 MachineConfigPools simultaneously. On 2-worker clusters (vSphere, AWS 1-of-2 split), this caused both the custom pool and the worker pool to drain nodes concurrently, leading to a PDB drain deadlock where PDB-protected pods (router, prometheus, etc.) had nowhere to reschedule — blocking the drain indefinitely and leaving the cluster degraded.This caused the
vsphere-disruptive-longrunning-techpreviewandaws-disruptive-longrunning-techpreviewjobs to fail consistently since 07/08/2026 when the tests were introduced via #31229.Fix
Remove the custom MCP entirely and test directly on the worker pool. The MCO handles worker pool rollouts sequentially (
maxUnavailable=1by default), so nodes drain one at a time and PDB-protected pods can always reschedule to available nodes.Key changes:
mirrorTestPoolstruct and all associated methods (create, teardown, pause/unpause)getWorkerSpec(),waitForWorkerRollout(),readRegistriesConfOnWorker()registries.confverificationTest Results (vSphere, 2 workers)
Both MCPs returned to
Updated=True, Degraded=Falseafter test completion:Summary by CodeRabbit
workerMachineConfigPool.workerpool to settle reliably.registries.confcontent, including blocked entries driven byNeverContactSource.