Pause worker MCP for image mirror tests#31425
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. |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-vsphere-disruptive-longrunning-techpreview |
|
@JoelSpeed: 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/affe8a20-874a-11f1-8216-6da3073dc66c-0 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: JoelSpeed 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 |
WalkthroughImage mirror set tests now pause the worker MachineConfigPool before each test and restore it afterward. Shared node utilities retrieve and update the pool only when its pause state differs from the requested state, logging setup or update failures. ChangesWorker pool isolation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 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/extended/node/node_e2e/image_mirror_set.go`:
- Around line 214-218: Update the worker-pool setup around
EnsureWorkerPoolPaused and the g.AfterEach cleanup to capture the original pause
state, register cleanup with g.DeferCleanup after pausing succeeds, and restore
that exact state instead of always unpausing; preserve the existing setup
behavior for both initially paused and unpaused suites.
In `@test/extended/node/node_utils.go`:
- Around line 997-1023: Update ensureWorkerPoolPaused and its
EnsureWorkerPoolPaused/EnsureWorkerPoolUnpaused callers to return and propagate
errors from NewForConfig, MachineConfigPools().Get, and Update instead of only
logging them. Make the callers fail when the worker-pool operation fails, while
preserving the existing already-paused early return.
🪄 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: 35a80689-3a0c-43c0-b08e-430de7993e3c
📒 Files selected for processing (2)
test/extended/node/node_e2e/image_mirror_set.gotest/extended/node/node_utils.go
| nodeutils.EnsureWorkerPoolPaused(ctx, oc) | ||
| }) | ||
|
|
||
| g.AfterEach(func(ctx context.Context) { | ||
| nodeutils.EnsureWorkerPoolUnpaused(ctx, oc) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the test file around the cited lines.
sed -n '180,250p' test/extended/node/node_e2e/image_mirror_set.go
# Find the pause/unpause helpers to understand whether they preserve original state.
rg -n "EnsureWorkerPool(Paused|Unpaused)|Paused" test/extended -g '*.go'Repository: openshift/origin
Length of output: 4340
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the worker-pool helper implementation.
sed -n '990,1035p' test/extended/node/node_utils.goRepository: openshift/origin
Length of output: 1498
Restore the worker pool’s original pause state.
AfterEach always unpauses the worker pool, so a suite that starts with Spec.Paused=true will leave the cluster in a different state. Capture the original value and restore that exact state, ideally with g.DeferCleanup after the pause succeeds.
🤖 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 214 - 218,
Update the worker-pool setup around EnsureWorkerPoolPaused and the g.AfterEach
cleanup to capture the original pause state, register cleanup with
g.DeferCleanup after pausing succeeds, and restore that exact state instead of
always unpausing; preserve the existing setup behavior for both initially paused
and unpaused suites.
| func EnsureWorkerPoolPaused(ctx context.Context, oc *exutil.CLI) { | ||
| ensureWorkerPoolPaused(ctx, oc, true) | ||
| } | ||
|
|
||
| func EnsureWorkerPoolUnpaused(ctx context.Context, oc *exutil.CLI) { | ||
| ensureWorkerPoolPaused(ctx, oc, false) | ||
| } | ||
|
|
||
| func ensureWorkerPoolPaused(ctx context.Context, oc *exutil.CLI, paused bool) { | ||
| mcClient, err := machineconfigclient.NewForConfig(oc.AdminConfig()) | ||
| if err != nil { | ||
| framework.Logf("Warning: failed to create MC client: %v", err) | ||
| } | ||
| workerPool, err := mcClient.MachineconfigurationV1().MachineConfigPools().Get(ctx, "worker", metav1.GetOptions{}) | ||
| if err != nil { | ||
| framework.Logf("Warning: failed to get worker pool: %v", err) | ||
| } | ||
| if workerPool.Spec.Paused == paused { | ||
| framework.Logf("Worker pool is already paused=%v", paused) | ||
| return | ||
| } | ||
|
|
||
| workerPool.Spec.Paused = paused | ||
| _, err = mcClient.MachineconfigurationV1().MachineConfigPools().Update(ctx, workerPool, metav1.UpdateOptions{}) | ||
| if err != nil { | ||
| framework.Logf("Warning: failed to update worker pool: %v", err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the target region with line numbers
sed -n '960,1045p' test/extended/node/node_utils.go | cat -n
echo
echo '--- usages ---'
rg -n "EnsureWorkerPoolPaused|EnsureWorkerPoolUnpaused|ensureWorkerPoolPaused" -S .Repository: openshift/origin
Length of output: 3702
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '960,1045p' test/extended/node/node_utils.go | cat -n
printf '\n--- usages ---\n'
rg -n "EnsureWorkerPoolPaused|EnsureWorkerPoolUnpaused|ensureWorkerPoolPaused" -S .Repository: openshift/origin
Length of output: 3702
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pwdRepository: openshift/origin
Length of output: 173
Propagate worker-pool API errors
ensureWorkerPoolPaused logs NewForConfig, Get, and Update failures but still returns as if the pool was updated, so setup/cleanup can continue with the worker pool in the wrong state. Return an error and fail the caller instead.
🤖 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_utils.go` around lines 997 - 1023, Update
ensureWorkerPoolPaused and its EnsureWorkerPoolPaused/EnsureWorkerPoolUnpaused
callers to return and propagate errors from NewForConfig,
MachineConfigPools().Get, and Update instead of only logging them. Make the
callers fail when the worker-pool operation fails, while preserving the existing
already-paused early return.
Source: Path instructions
1 participant
These tests create IMDS which affects all MCPs. The worker pool rolling out is hindering the progress on this test when a cluster is created with just two workers.
Summary by CodeRabbit