E2E: Minor fixes to Memory Manager and hugepages split tests - #1578
E2E: Minor fixes to Memory Manager and hugepages split tests#1578mrniranjan wants to merge 2 commits into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughThe performance profile tests now allocate hugepages per NUMA node, defer pod cleanup, and preserve existing CPU sets. CPU validation covers kernel command-line, systemd, tuned, topology, and kubelet settings. ChangesPerformance profile test updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Important 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 |
PR Summary by QodoE2E: Stabilize memory manager hugepages/NUMA tests and preserve profile CPU sets
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![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/e2e/performanceprofile/functests/2_performance_update/memorymanager.go`:
- Around line 643-645: Update the DeferCleanup callback around mm1.removePod to
inspect and handle its error instead of discarding it: tolerate only a NotFound
result after the explicit deletion, and fail cleanup for every other error using
the test framework’s established failure mechanism.
In
`@test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go`:
- Around line 324-335: Update the cmdline assertion in the “verify that isolcpus
matches the performance profile” test to match the complete whitespace-delimited
kernel argument rather than using ContainSubstring. Build a regex with
regexp.QuoteMeta(expectedIsol) and boundaries that require whitespace or the
start/end of the command line, while preserving the existing expectedIsol
construction and node iteration.
🪄 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: 53956587-b78f-4abb-8ee9-572f0bafc9e7
📒 Files selected for processing (2)
test/e2e/performanceprofile/functests/2_performance_update/memorymanager.gotest/e2e/performanceprofile/functests/2_performance_update/updating_profile.go
| DeferCleanup(func() { | ||
| _ = mm1.removePod(context.TODO(), testPod1) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle unexpected cleanup errors.
Line 644 discards every deletion error. If the test fails before Line 654, a cleanup failure can leave the pod and its NUMA resources allocated for later tests.
Ignore only a NotFound result after the explicit deletion. Fail the cleanup for every other error.
As per path instructions, **/*.go: Never ignore error returns.
🤖 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/2_performance_update/memorymanager.go`
around lines 643 - 645, Update the DeferCleanup callback around mm1.removePod to
inspect and handle its error instead of discarding it: tolerate only a NotFound
result after the explicit deletion, and fail cleanup for every other error using
the test framework’s established failure mechanism.
Source: Path instructions
Code Review by Qodo
1.
|
![qodo-for-rh-openshift[bot]](https://avatars.githubusercontent.com/u/792337?s=60&v=4){kind=small}
| expectedIsol := isolPrefix + expectedIsolated.String() | ||
| for _, node := range workerRTNodes { | ||
| cmdline, err := chkCmdLineFn(context.TODO(), &node) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(cmdline).To(ContainSubstring(expectedIsol)) |
There was a problem hiding this comment.
1. Isolcpus misses ovsdpdk cpus 🐞 Bug ≡ Correctness
The new isolcpus expectation is built from profile.Spec.CPU.Isolated only, but tuned generates isolcpus from (Isolated ∪ OvsDpdk). On profiles with OvsDpdk configured, this assertion can be wrong and can intermittently fail due to string-substring matching rather than set comparison.
Agent Prompt
### Issue description
`isolcpus` on the kernel cmdline is generated by tuned from the isolated CPU set **including** `spec.cpu.ovsDpdk`. The test currently derives `expectedIsolated` only from `spec.cpu.isolated` and then checks `ContainSubstring`, which can be incorrect for profiles with OVS-DPDK CPUs.
### Issue Context
In tuned rendering, OVS-DPDK CPUs are unioned into the isolated set before templating `isolated_cores`, and `cmdline_isolation` uses `${isolated_cores}`.
### Fix Focus Areas
- test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go[239-252]
- test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go[324-336]
### Suggested fix
1) When building `expectedIsolated`, union in `profile.Spec.CPU.OvsDpdk` when present (same semantics as tuned).
2) Prefer parsing the `isolcpus=...` value from `/proc/cmdline` and comparing CPU sets (order/format independent) instead of `ContainSubstring`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
By default profiles will not have ovsdpdk configured.
There was a problem hiding this comment.
we have separate tests for ovsdpdk where profile will be configured with ovsdpdk when those tests run
| expectedReservedCPUs := expectedReserved | ||
| if profile.Spec.CPU.OvsDpdk != nil && *profile.Spec.CPU.OvsDpdk != "" { | ||
| ovsDpdkCPUs, err := cpuset.Parse(string(*profile.Spec.CPU.OvsDpdk)) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| expectedReservedCPUs = expectedReservedCPUs.Union(ovsDpdkCPUs) | ||
| } |
There was a problem hiding this comment.
4. Reservedsystemcpus ignores shared cpus 🐞 Bug ≡ Correctness
The new ReservedSystemCPUs expectation unions OvsDpdk CPUs but does not include Shared CPUs when Mixed CPUs is enabled, even though kubeletconfig generation does. This can cause false failures on valid mixed-CPU profiles.
Agent Prompt
### Issue description
When Mixed CPUs is enabled, the controller sets `ReservedSystemCPUs = reserved ∪ ovsDpdk ∪ shared`. The test currently sets expected ReservedSystemCPUs to `reserved ∪ ovsDpdk` only.
### Issue Context
Mixed CPUs enablement is determined from the profile (`spec.cpu.shared` plus `spec.workloadHints.mixedCpus`) and feature gates; kubeletconfig generation unions shared CPUs only when mixed CPUs is enabled.
### Fix Focus Areas
- test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go[367-386]
### Suggested fix
Mirror controller logic:
- Always union OvsDpdk into expected.
- If `profile.Spec.CPU.Shared` is set and `profile.Spec.WorkloadHints.MixedCpus` is true (and any e2e-available feature-gate check indicates enabled), union Shared into expected before comparing to `kc.ReservedSystemCPUs`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…tests The restricted and single-numa-node fixtures assumed that configuring 20 x 2Mi hugepages without a NUMA Node would split evenly across both NUMA nodes (10/10). That is not how cmdline hugepages=N works: the kernel places pages system-wide, and Memory Manager can see an uneven split (for example 24Mi on NUMA 0 and 16Mi on NUMA 1). With 24Mi free on one node, a Guaranteed pod requesting 24Mi hugepages + 2 CPUs was admitted instead of failing with TopologyAffinityError. Pin 10 x 2Mi hugepages on each NUMA node so each zone has only 20Mi, restoring the rejection expectation. Also DeferCleanup test pods so flake retries do not leak hugepages from a failed attempt. AI Attribution: AIA Human-AI blend, New content, Human-initiated, Reviewed, Cursor 3.0.16 v1.0 Signed-off-by: Niranjan M.R <mniranja@redhat.com>
Hardcoding isolated=1-2 and reserved=0,3 on large-CPU nodes inflated tuned.non_isolcpus and systemd.cpu_affinity, which could drop later cmdline args such as hugepages and break test_id:34081. Keep the profile's CPU set and assert isolcpus, affinity, and ReservedSystemCPUs against those values instead. - Add FindCmdlineParam helper to nodes package for parsing /proc/cmdline parameters by splitting on whitespace instead of using regex - Compare isolcpus using exact argument match instead of ContainSubstring - Compare systemd.cpu_affinity against non-isolated CPUs (online \ isolated) instead of reserved, matching tuned's not_isolated_cores_expanded - Validate tuned.non_isolcpus mask value against expected non-isolated CPU mask instead of only checking presence - Simplify ReservedSystemCPUs to compare directly against reserved CPUs Signed-off-by: Niranjan M.R <mniranja@redhat.com> AI Attribution: AIA Human-AI blend, New content, Human-initiated, Reviewed, Cursor 3.0.16 v1.0
4a85eaf to
ba99de2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/performanceprofile/functests/2_performance_update/memorymanager.go (1)
151-158: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCheck the
WaitForConditionerror before checking the event.
err := checkPodEvent(...)shadows the outer error returned byWaitForCondition. The same pattern occurs in the second test at lines 512–514. Assert the wait error immediately and use a separate variable for the event error.🤖 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/2_performance_update/memorymanager.go` around lines 151 - 158, The WaitForCondition result is shadowed before its error is checked. In the test flow around WaitForCondition and checkPodEvent, assert or handle the returned wait error immediately, then store the event-check result in a distinct variable; apply the same change to the corresponding second-test flow.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/2_performance_update/memorymanager.go`:
- Around line 101-113: The hugepage setup currently hardcodes NUMA IDs 0 and 1,
which may not match topology IDs from lscpu. Update the surrounding
memory-manager test flow to derive and reuse the IDs returned by GetNumaNodes
for hugepage Node values and affinity assertions, and skip topologies that do
not provide the required NUMA nodes.
---
Outside diff comments:
In `@test/e2e/performanceprofile/functests/2_performance_update/memorymanager.go`:
- Around line 151-158: The WaitForCondition result is shadowed before its error
is checked. In the test flow around WaitForCondition and checkPodEvent, assert
or handle the returned wait error immediately, then store the event-check result
in a distinct variable; apply the same change to the corresponding second-test
flow.
🪄 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: 221f0e82-3ef4-492e-af0b-0a6625bdcaba
📒 Files selected for processing (4)
test/e2e/performanceprofile/functests/2_performance_update/memorymanager.gotest/e2e/performanceprofile/functests/2_performance_update/updating_profile.gotest/e2e/performanceprofile/functests/utils/nodes/nodes.gotest/e2e/performanceprofile/functests/utils/nodes/nodes_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go
mrniranjan
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Scheduling required tests: |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MarSik, mrniranjan The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
ffromani
left a comment
ffromani
left a comment
There was a problem hiding this comment.
/lgtm
assuming we want to have Expect()ations in (Custom)BeforeAll
|
/retest |
|
@mrniranjan: The following tests 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. |
4 participants
Hardcoding isolated=1-2 and reserved=0,3 on large-CPU nodes inflated tuned.non_isolcpus and systemd.cpu_affinity, which could drop later cmdline args such as hugepages and break test_id:34081. Keep the profile's CPU set and assert isolcpus, affinity, and ReservedSystemCPUs against those values instead.
The restricted and single-numa-node fixtures assumed that configuring 20 x 2Mi hugepages without a NUMA Node would split evenly across both NUMA nodes (10/10). That is not how cmdline hugepages=N works: the
kernel places pages system-wide, and Memory Manager can see an uneven split (for example 24Mi on NUMA 0 and 16Mi on NUMA 1). With 24Mi free on one node, a Guaranteed pod requesting 24Mi hugepages + 2 CPUs was admitted instead of failing with TopologyAffinityError.
Pin 10 x 2Mi hugepages on each NUMA node so each zone has only 20Mi, restoring the rejection expectation. Also DeferCleanup test pods so flake retries do not leak hugepages from a failed attempt.
Summary by CodeRabbit