[release-4.21] OCPBUGS-104586: e2e: fix: clear hugepages before switching kernelPageSize to 4k - #1576
Conversation
BeforeAll set kernelPageSize=4k without clearing hugepages (512M is default on arm ci cluster), causing the performance admission webhook to reject the incompatible combination on aarch64. Extract common 3-step profile update pattern (UpdateWithRetry + WaitForTuningUpdating + WaitForTuningUpdated) into ApplyProfileAndWait helper in profilesupdate package. - Declarative baseline in BeforeAll/AfterAll with DeepEqual guard - Fix error check ordering for GetByNodeLabels - Remove unused imports and variables Ref: CNF-25175
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
@openshift-cherrypick-robot: Jira Issue OCPBUGS-92011 has been cloned as Jira Issue OCPBUGS-104586. Will retitle bug to link to clone. 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. |
PR Summary by Qodoe2e: clear hugepages before enforcing 4k kernelPageSize on ARM
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
|
@openshift-cherrypick-robot: This pull request references Jira Issue OCPBUGS-104586, 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. |
Code Review by Qodo
1. Waits on no-op update
|
| perfProfile.Spec.HugePages = nil | ||
| perfProfile.Spec.KernelPageSize = ptr.To(performancev2.KernelPageSize(kernelPageSize4k)) | ||
| perfProfile.Spec.RealTimeKernel = &performancev2.RealTimeKernel{ | ||
| Enabled: ptr.To(false), |
There was a problem hiding this comment.
1. Waits on no-op update 🐞 Bug ☼ Reliability
BeforeAll forces default-equivalent spec fields (e.g., KernelPageSize=4k, RealTimeKernel.Enabled=false, HugePages=nil) and then may call ApplyProfileAndWait whenever the raw Spec differs from the original, even if the rendered MachineConfig would be unchanged. In that case WaitForTuningUpdating can wait until mcps.WaitForCondition times out (40 min/node) because the MCP may never enter Updating=True.
Agent Prompt
### Issue description
`kernelpagesize.go` normalizes the PerformanceProfile by setting default-equivalent fields (KernelPageSize=4k, RealTimeKernel.Enabled=false, HugePages=nil) and then calls `profilesupdate.ApplyProfileAndWait(...)` based on a raw `Spec` DeepEqual comparison. Some of these raw-spec changes can be semantically no-ops for rendered node configuration (e.g., KernelPageSize defaults to 4k when unset; RT is disabled when RealTimeKernel is nil), so no MCP rollout may start and the test can block until the MCP wait times out.
### Issue Context
- `ApplyProfileAndWait` always waits for the tuning update to start and complete.
- `mcps.WaitForCondition` uses a large timeout (40 minutes per targeted node).
### Fix Focus Areas
- test/e2e/performanceprofile/functests/14_arm/kernelpagesize.go[59-68]
- test/e2e/performanceprofile/functests/utils/profilesupdate/profile_update.go[133-144]
### Suggested fix approaches
Pick one:
1) **Only mutate/apply fields that actually require a rollout**:
- Don’t force `RealTimeKernel` from `nil` to `{Enabled:false}`; only disable it if it is explicitly enabled.
- Don’t force `KernelPageSize` to explicit `4k` unless it is explicitly `64k` (or you detect the node is currently 64k).
- Keep clearing `HugePages` (the PR’s intent) but only call `ApplyProfileAndWait` if `HugePages` was non-nil / non-empty or other truly roll-out-affecting fields changed.
2) **Make `ApplyProfileAndWait` tolerant to no-rollout updates**:
- Capture MCP/nodepool “update state” (e.g., current generation / config source / Updating status) before update.
- After `UpdateWithRetry`, if the pool never transitions into Updating within a short window, skip the rollout wait and proceed (or assert expected state directly).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: openshift-cherrypick-robot 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 |
|
@openshift-cherrypick-robot: 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. |
|
/jira refresh |
|
@oblau: This pull request references Jira Issue OCPBUGS-104586, which is valid. The bug has been moved to the POST state. 7 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. |
This is an automated cherry-pick of #1554
/assign oblau