NO-JIRA: e2e: fix: restart tuned pod after irqbalance test to prevent cpuset pollution - #1575
NO-JIRA: e2e: fix: restart tuned pod after irqbalance test to prevent cpuset pollution#1575Tal-or wants to merge 1 commit into
Conversation
WalkthroughThe IRQ balance test cleanup now marks itself as a Ginkgo helper, deletes and recreates the tuned pod, waits through ChangesIRQ balance cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoE2E: Restart tuned pod in IRQBalance cleanup to avoid stale CPU affinity
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Tal-or 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 |
Tal-or
changed the title
![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
🧹 Nitpick comments (1)
test/e2e/performanceprofile/functests/1_performance/irqbalance.go (1)
246-246: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPass a bounded context to the cleanup API call.
context.TODO()has no cancellation or deadline. A blocked Kubernetes delete request can outlive the test timeout. Use the test context or create a finite cleanup context. Pass the same context throughnodes.TunedForNodeinstead of creating anothercontext.TODO()in that helper.As per path instructions: Go code must use
context.Contextfor cancellation and timeouts.🤖 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/e2e/performanceprofile/functests/1_performance/irqbalance.go` at line 246, Update the cleanup flow around pods.DeleteAndSync to use the test’s bounded context instead of context.TODO(), and propagate that same context through nodes.TunedForNode rather than creating another TODO context in the helper. Ensure the Kubernetes deletion and tuned-pod lookup honor cancellation and deadlines via context.Context.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.
Inline comments:
In `@test/e2e/performanceprofile/functests/1_performance/irqbalance.go`:
- Around line 247-248: The call to nodes.TunedForNode can return a pod before
its containers are actually ready, causing the testlog.Infof at line 248 to log
false success when Status.ContainerStatuses is still empty. Add an explicit
readiness check (such as a PodReady wait) after the TunedForNode call completes
and before the testlog.Infof call to verify that the pod's container statuses
are populated and the containers are ready, ensuring the restart success is only
logged when TuneD is truly running.
- Around line 240-248: The TuneD restart currently runs in the normal control
flow and can be skipped when final-state assertions fail. Move the deletion and
synchronization logic for the tuned pod identified by TunedForNode into an
independent cleanup handler that executes after test-pod deletion, so it still
runs when Ginkgo Fail/Expect aborts the main flow; preserve the existing restart
and clean-affinity behavior.
---
Nitpick comments:
In `@test/e2e/performanceprofile/functests/1_performance/irqbalance.go`:
- Line 246: Update the cleanup flow around pods.DeleteAndSync to use the test’s
bounded context instead of context.TODO(), and propagate that same context
through nodes.TunedForNode rather than creating another TODO context in the
helper. Ensure the Kubernetes deletion and tuned-pod lookup honor cancellation
and deadlines via context.Context.
🪄 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: Enterprise
Run ID: e6c3fed8-cdad-4eb6-be6a-e26ba28d68c3
📒 Files selected for processing (1)
test/e2e/performanceprofile/functests/1_performance/irqbalance.go
| nodes.TunedForNode(targetNode, RunningOnSingleNode) | ||
| testlog.Infof("tuned pod restarted on node %q with clean CPU affinity", targetNode.Name) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Require a real readiness check before logging the restart.
At Line 247, nodes.TunedForNode can return a pod while Status.ContainerStatuses is empty. Its polling loop then succeeds without checking readiness. A newly recreated pod can reach this state before TuneD is ready. Line 248 can log false success, and the next test can start too early.
Update TunedForNode to require populated container statuses and ready containers, or use an explicit PodReady wait here.
Suggested helper guard
if len(tunedList.Items) == 0 {
return false
}
+ if len(tunedList.Items[0].Status.ContainerStatuses) == 0 {
+ return false
+ }
for _, s := range tunedList.Items[0].Status.ContainerStatuses {🤖 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/e2e/performanceprofile/functests/1_performance/irqbalance.go` around
lines 247 - 248, The call to nodes.TunedForNode can return a pod before its
containers are actually ready, causing the testlog.Infof at line 248 to log
false success when Status.ContainerStatuses is still empty. Add an explicit
readiness check (such as a PodReady wait) after the TunedForNode call completes
and before the testlog.Infof call to verify that the pod's container statuses
are populated and the containers are ready, ensuring the restart success is only
logged when TuneD is truly running.
Code Review by Qodo
1. Cleanup assertions obscure failures
|
![qodo-for-rh-openshift[bot]](https://avatars.githubusercontent.com/u/792337?s=60&v=4){kind=small}
| By(fmt.Sprintf("restarting tuned pod on %s to restore clean CPU affinity", targetNode.Name)) | ||
| tunedPod := nodes.TunedForNode(targetNode, RunningOnSingleNode) | ||
| Expect(pods.DeleteAndSync(context.TODO(), testclient.DataPlaneClient, tunedPod)).To(Succeed(), "failed to delete tuned pod on node %q", targetNode.Name) | ||
| nodes.TunedForNode(targetNode, RunningOnSingleNode) |
There was a problem hiding this comment.
1. Cleanup assertions obscure failures 🐞 Bug ☼ Reliability
The new tuned restart runs inside a Go defer and uses Expect(...) plus nodes.TunedForNode(...) (which asserts via Eventually(...).Should(...)), so a cleanup problem can add secondary failures that obscure the original test failure context. This makes diagnosing the primary failure harder when the spec is already failing and the cleanup path hits transient tuned/API issues.
Agent Prompt
## Issue description
The tuned restart logic was added inside a Go `defer` and contains multiple assertions (`Expect(...)` and the assertion inside `nodes.TunedForNode`). If the spec already failed, a cleanup failure can add additional failures and make the original failure harder to interpret.
## Issue Context
`nodes.TunedForNode` performs an `Eventually(...).Should(...)` assertion internally and can wait for a long time before failing. Executing this in a Go `defer` means failures are not clearly separated as cleanup failures.
## Fix Focus Areas
- test/e2e/performanceprofile/functests/1_performance/irqbalance.go[225-249]
## Suggested fix
Convert the Go `defer func() { ... }()` cleanup to Ginkgo `DeferCleanup(...)` so failures are attributed to cleanup rather than confusing the primary assertion failure. Use the `cleanupCtx` parameter (or a derived context with timeout) for API calls, and consider recording/logging errors in cleanup separately from the main test expectations if you want to preserve the original failure signal.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| tunedPod := nodes.TunedForNode(targetNode, RunningOnSingleNode) | ||
| Expect(pods.DeleteAndSync(context.TODO(), testclient.DataPlaneClient, tunedPod)).To(Succeed(), "failed to delete tuned pod on node %q", targetNode.Name) | ||
| nodes.TunedForNode(targetNode, RunningOnSingleNode) | ||
| testlog.Infof("tuned pod restarted on node %q with clean CPU affinity", targetNode.Name) |
There was a problem hiding this comment.
2. Cleanup can block too long 🐞 Bug ➹ Performance
The added tuned restart cleanup does two nodes.TunedForNode(...) waits (up to 480s each) plus a pod deletion wait (up to 120s), so a single spec’s cleanup can be delayed by many minutes when tuned is slow/unhealthy. This can significantly slow feedback in failure scenarios and make unrelated failures take much longer to complete.
Agent Prompt
## Issue description
The new cleanup performs:
- `nodes.TunedForNode(...)` (waits up to `testTimeout=480s`)
- `pods.DeleteAndSync(...)` (waits up to `DefaultDeletionTimeout=120s`)
- another `nodes.TunedForNode(...)` (another up to 480s)
In failure scenarios, this can delay cleanup completion by a large amount.
## Issue Context
`nodes.TunedForNode` is implemented with `Eventually(..., cluster.ComputeTestTimeout(testTimeout*time.Second, sno), ...)` and `testTimeout` is 480 seconds.
## Fix Focus Areas
- test/e2e/performanceprofile/functests/1_performance/irqbalance.go[240-248]
- test/e2e/performanceprofile/functests/utils/nodes/nodes.go[35-38]
- test/e2e/performanceprofile/functests/utils/nodes/nodes.go[379-405]
## Suggested fix
In the cleanup, avoid the first long readiness wait before deletion:
- List the tuned pods for the node once (no `Eventually`), delete what you find (handling NotFound / empty list gracefully), then do a single `nodes.TunedForNode(...)` wait to ensure tuned is back.
- Alternatively, add a new helper like `nodes.TunedForNodeWithTimeout(node, sno, timeout)` and use a shorter timeout specifically for cleanup.
Also consider running the cleanup under a bounded context (`cleanupCtx` from `DeferCleanup`, optionally wrapped with `context.WithTimeout`) so the cleanup can’t stall indefinitely if the API server is degraded.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
The irqbalance test "Should not overwrite the banned CPU set on tuned restart" creates a guaranteed pod (exclusive CPUs), then restarts the tuned pod while that guaranteed pod is still running. The new tuned process starts with a narrowed CPU affinity mask (missing the exclusive CPUs). When the guaranteed pod is later cleaned up, the cgroup cpuset expands but the tuned process's sched_setaffinity mask stays narrow. If the cpu_management.go Ordered block runs after irqbalance.go, test test_id:87722 reads the tuned pod's stale process affinity via taskset and fails because it doesn't match the full online CPU set. Fix: restart the tuned pod in the irqbalance test's defer cleanup after the guaranteed pod is deleted, so the tuned process picks up the current (full) default cpuset. AIA Human-AI blend, Content edits, Human-initiated, Reviewed, Claude Opus 4.6 v1.0 Signed-off-by: titzhak <titzhak@redhat.com>
a741c3e to
ff1c580
Compare
|
/lgtm |
|
/retest |
Tal-or
changed the title
|
@Tal-or: 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. |
Tal-or
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@Tal-or: This pull request explicitly references no jira issue. 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. |
|
/verified by @Tal-or |
openshift-ci-robot
added
the
verified
|
@Tal-or: This PR has been marked as verified by 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. |
|
Hmm, shouldn't the real fix be in the place where tuned detects the available cpus? Because if Tuned pod starts after a guaranteed pod on a real cluster deployment and this happens then we will have a misconfigured system too, right? |
4 participants
The irqbalance test
Should not overwrite the banned CPU set on tuned restartcreates a guaranteed pod (exclusive CPUs), then restarts the tuned pod while that guaranteed pod is still running.The new tuned process starts with a narrowed CPU affinity mask (missing the exclusive CPUs).
When the guaranteed pod is later cleaned up, the cgroup cpuset expands but the tuned process's sched_setaffinity mask stays narrow.
If the
cpu_management.goOrderedblock runs afterirqbalance.go, testtest_id:87722reads the tuned pod's stale process affinity via taskset and fails because it doesn't match the full online CPU set.Fix: restart the tuned pod in the irqbalance test's defer cleanup after the guaranteed pod is deleted, so the tuned process picks up the current (full) default cpuset.
Summary by CodeRabbit