E2E: Fix ovs dynamic pinning, kubelet and mustgather tests - #1565
Open
mrniranjan wants to merge 4 commits into
Open
E2E: Fix ovs dynamic pinning, kubelet and mustgather tests #1565mrniranjan wants to merge 4 commits into
mrniranjan wants to merge 4 commits into
Conversation
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mrniranjan 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPerformance profile end-to-end tests now derive must-gather filenames dynamically, validate activation-file persistence after node reboots, handle ARM-specific kubelet behavior, refresh profiles before tests, and restore profiles when specs or annotations change. ChangesPerformance profile end-to-end tests
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
mrniranjan
force-pushed
the
fix_cgroups_test
branch
from
July 18, 2026 11:05
86c8107 to
f3b67b3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/7_performance_kubelet_node/cgroups.go`:
- Line 23: Update the reboot flow in the test’s main function and the referenced
lines to stop using MachineConfigPool state: remove the machineconfigv1 import,
performanceMCP definition, and MCP lookup. Handle the reboot command’s returned
error, then synchronize by waiting for the node to transition from Ready to
NotReady and back to Ready, using context.Context for cancellation and timeouts
as established by the other reboot tests in this file.
In `@test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go`:
- Around line 184-187: Validate that cnfNodes contains at least one node
immediately after nodes.GetByLabels succeeds and before the infrastructure.IsARM
call. Use the test’s existing assertion style to fail clearly when the slice is
empty, while preserving the current first-node behavior for non-empty results.
- Around line 246-249: The comparison before reverting the profile ignores
json.Marshal errors and unnecessarily serializes annotations. In the revert
condition, replace the marshaled annotation values with a direct
equality.Semantic.DeepEqual comparison of profile.Annotations and
initialProfile.Annotations, and remove the currentAnnotations and
initialAnnotations declarations.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8cbc0db0-31d5-45a3-bd63-109721523e37
📒 Files selected for processing (3)
test/e2e/performanceprofile/functests/6_mustgather_testing/mustgather.gotest/e2e/performanceprofile/functests/7_performance_kubelet_node/cgroups.gotest/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go
mrniranjan
force-pushed
the
fix_cgroups_test
branch
from
July 19, 2026 00:57
f3b67b3 to
55276e4
Compare
Contributor
Author
|
/test e2e-gcp-pao-updating-profile |
1 similar comment
Contributor
Author
|
/test e2e-gcp-pao-updating-profile |
Tal-or
reviewed
Jul 20, 2026
mrniranjan
force-pushed
the
fix_cgroups_test
branch
from
July 20, 2026 10:41
55276e4 to
44ecf76
Compare
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/7_performance_kubelet_node/cgroups.go`:
- Around line 160-166: Update the reboot sequence around nodes.ExecCommand to
derive a bounded timeout context from ctx, execute the reboot command directly
without sh -c, capture the returned error, and log it as a warning because
connection loss is expected. Replace context.TODO() in the activation-file check
with the existing ctx variable, while preserving the readiness waits and
subsequent validation.
In `@test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go`:
- Around line 234-237: Update the comparison in the profile-revert condition
near currentSpec and spec to use equality.Semantic.DeepEqual directly on
profile.Spec and initialProfile.Spec, matching the existing annotation
comparison. Remove the json.Marshal calls and their ignored error returns while
preserving the revert decision behavior.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 85b1bd70-b3d8-416c-9f97-480d369c1ee5
📒 Files selected for processing (3)
test/e2e/performanceprofile/functests/6_mustgather_testing/mustgather.gotest/e2e/performanceprofile/functests/7_performance_kubelet_node/cgroups.gotest/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/performanceprofile/functests/6_mustgather_testing/mustgather.go
mrniranjan
force-pushed
the
fix_cgroups_test
branch
from
July 21, 2026 05:21
44ecf76 to
2c6e84d
Compare
Earlier we used to modify performance profile to trigger reboot i.e change the topology manager to best-effort. This is problem when ran on ARM where changing topology manager to best-effort or any other value other than none causes ARM worker node to get stuck So instead we now use systemctl reboot to reboot the node Signed-off-by: Niranjan M.R <mniranja@redhat.com>
Earlier when kubelet experimental annotions e2e tests ran , it used to overwrite any existing annotations. When running on clusters deployed using ztp, the existing annotations need to be honoured. Modify the existing tests to honor the existing annotations. Also skip the tests if architecture is ARM where the annotations set kublet topology manager to any value other than none Signed-off-by: Niranjan M.R <mniranja@redhat.com>
Instead of using hardcoded performance profile name, fetch the performance profile name Signed-off-by: Niranjan M.R <mniranja@redhat.com>
mrniranjan
force-pushed
the
fix_cgroups_test
branch
from
July 23, 2026 06:04
2c6e84d to
62eac14
Compare
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/7_performance_kubelet_node/kubelet.go`:
- Around line 87-93: Replace context.TODO() with bounded context.WithTimeout
contexts for every nodes.ExecCommand call in kubelet.go: the autoSizingCmd loop
at lines 87-93, the sibling call at lines 160-162, and the call at lines
241-254. Ensure each operation’s context is cancelled appropriately and
preserves existing command execution and error assertions.
- Around line 44-49: Ensure selected-node validation cannot pass on empty
results: after filtering in kubelet.go lines 44-49, assert workerRTNodes is
non-empty; after filtering in lines 155-158, assert updatedNodes is non-empty;
and in lines 193-201, determine ARM status from workerRTNodes[0] instead of an
unfiltered cnfNodes lookup.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fbda7144-43dd-4e92-9fb2-8b1b4742b61c
📒 Files selected for processing (3)
test/e2e/performanceprofile/functests/6_mustgather_testing/mustgather.gotest/e2e/performanceprofile/functests/7_performance_kubelet_node/cgroups.gotest/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/performanceprofile/functests/6_mustgather_testing/mustgather.go
- test/e2e/performanceprofile/functests/7_performance_kubelet_node/cgroups.go
Contributor
Author
|
/retest |
Test 45488 — Multiple kubelet settings
1. Removed broken ps -ef | grep kubelet block —
systemReserved is in the drop-in config (/etc/openshift/kubelet.conf.d/20-auto-sizing.conf),
not in kubelet CLI flags or kubelet.conf
2. Added check via drop-in config file —
reads 20-auto-sizing.conf from the node and verifies it contains 300Mi
Test 45493 — Should not override PAO values
3. Changed annotation values to differ from PAO defaults —
was setting cpuManagerPolicy: "static" and cpuManagerReconcilePeriod: "5s" (same as what PAO sets, proving nothing).
Now sets "none" and "10s", and verifies PAO overrides them back to "static" and 5s
Test 45490 — Memory reservation changes
4. Re-fetch nodes after tuning update —
workerRTNodes was populated in CustomBeforeAll
with stale node.Status values. Now re-fetches nodes so
Capacity and Allocatable reflect the updated reservations
5. Account for hugepages in allocatable calculation —
pre-allocated hugepages are subtracted from allocatable
memory by the kubelet but included in node capacity.
Added loop to sum hugepages from node.Status.Capacity and
subtract from calculated allocatable
Test 45489 — Verify settings are reverted
6. Added WaitForTuningUpdating / WaitForTuningUpdated —
was missing the wait after reverting the profile,
causing assertions to run before rollback completed
7. Replaced systemReserved check —
same as 45488: removed ps -ef approach,
added drop-in config file check verifying 300Mi is no longer present
8. Fixed nil vs empty slice assertion —
changed Equal(nil) to BeEmpty() for AllowedUnsafeSysctls,
since after reverting it's an empty slice ([]string{}), not nil
9. Replaced context.TODO() with ctx —
all ExecCommand, GetKubeletConfig, and ControlPlaneClient.Get calls
now use the existing ctx (context.Background()) variable for
consistent context usage
10. Added non-emptiness checks for node slices —
workerRTNodes and updatedNodes are now validated after
MatchingOptionalSelector to prevent out-of-bounds panics
11. Use workerRTNodes for ARM check —
replaced separate cnfNodes lookup with workerRTNodes[0] to
ensure the ARM check targets the same filtered node set
used by the tests
AI Attribution: AIA Human-AI blend, Content edits
Signed-off-by: Niranjan M.R <mniranja@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mrniranjan
force-pushed
the
fix_cgroups_test
branch
from
August 5, 2026 07:03
62eac14 to
d032bc4
Compare
Tal-or
reviewed
Aug 5, 2026
| Expect(err).ToNot(HaveOccurred()) | ||
| currentSpec, _ := json.Marshal(profile.Spec) | ||
| spec, _ := json.Marshal(initialProfile.Spec) | ||
| // revert only if the profile changes. |
Contributor
There was a problem hiding this comment.
if this comment meant to emphasis that only if the spec changes, it's no longer true.
Contributor
There was a problem hiding this comment.
The commit signature already contains AIA, we don't need the:
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| totalKubeMemory := systemReservedResource.MilliValue() + kubeReservedMemoryResource.MilliValue() + evictionMemoryResource.MilliValue() | ||
| calculatedAllocatable := totalCapactity - totalKubeMemory | ||
|
|
||
| // Pre-allocated hugepages are subtracted from allocatable memory by the |
Contributor
There was a problem hiding this comment.
There is no way to reserve hugepages on kubelet:
kubernetes/kubernetes#140544
Contributor
Author
There was a problem hiding this comment.
We are not reserving hugepages through kubelet, Hugepages are being set using Kernel from the parameters defined in Performance profile.
The hugepages here are pre-allocated by the kernel (via the performance profile's hugepages spec, which sets kernel boot parameters like hugepagesz=2M hugepages=N). When the kernel pre-allocates hugepages,
Contributor
|
@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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mustgather: Fetch performance profile instead of hard coding
kubelet/annotations:
E2E: annotations. modify existing tests to preserve existing annotations
E2E: ovs:
Summary by CodeRabbit
-*.yaml).