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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/RHsyseng/operator-utils@v1.4.13: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/go-systemd@v0.0.0-20191104093116-d3cd4ed1dbcf: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/ignition@v0.35.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/ignition/v2@v2.26.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/docker/go-units@v0.5.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-logr/stdr@v1.2.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/google/go-cmp@v0.7.0 ... [truncated 19340 characters] ... is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/legacy-cloud-providers: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/metrics: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/mount-utils: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/pod-security-admission: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-apiserver: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tgithub.com/onsi/ginkgo/v2: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" 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>
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
AI Attribution: AIA Human-AI blend, Content edits
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
Comment on lines
+44
to
49
| var err error | ||
| workerRTNodes, err = nodes.GetByLabels(testutils.NodeSelectorLabels) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
|
|
||
| _, err = nodes.MatchingOptionalSelector(workerRTNodes) | ||
| workerRTNodes, err = nodes.MatchingOptionalSelector(workerRTNodes) | ||
| Expect(err).ToNot(HaveOccurred()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate and consistently use the selected test nodes.
MatchingOptionalSelector may return an empty slice without error, making validation loops pass vacuously. The ARM check instead uses unfiltered cnfNodes, so it can skip based on a node outside the test target.
test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go#L44-L49: assertworkerRTNodesis non-empty after filtering.test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go#L155-L158: assertupdatedNodesis non-empty after filtering.test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go#L193-L201: detect ARM fromworkerRTNodes[0]rather than a separate unfiltered lookup.
📍 Affects 1 file
test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go#L44-L49(this comment)test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go#L155-L158test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go#L193-L201
🤖 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/7_performance_kubelet_node/kubelet.go`
around lines 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.
Contributor
Author
There was a problem hiding this comment.
can you check the latest commit in the PR , this should be fixed
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go"
echo "Checked-out revision:"
git log -1 --oneline
echo
echo "File structure:"
ast-grep outline "$file" --items all --type function --view compact || true
echo
echo "Affected code sections:"
nl -ba "$file" | sed -n '35,215p'
echo
echo "Relevant node-selection assertions and ARM lookup:"
rg -n -C 3 'MatchingOptionalSelector|workerRTNodes|updatedNodes|IsARM|cnfNodes' "$file"Length of output: 8151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go"
echo "Setup (lines 38-60):"
sed -n '38,60p' "$file"
echo
echo "Updated-node check (lines 148-175):"
sed -n '148,175p' "$file"
echo
echo "ARM check (lines 188-215):"
sed -n '188,215p' "$file"
echo
echo "Exact relevant occurrences:"
rg -n -C 2 'MatchingOptionalSelector|Expect\(len\((workerRTNodes|updatedNodes)\)\)|infrastructure\.IsARM|cnfNodes' "$file"Length of output: 5327
@mrniranjan, I checked the current PR revision (62eac14b). This is not fully fixed yet:
- After
MatchingOptionalSelector, neitherworkerRTNodesnorupdatedNodesis checked for non-emptiness. - The ARM check still uses a separate unfiltered
cnfNodeslookup instead ofworkerRTNodes[0].
So the original finding remains valid and should stay unresolved.
🧬 Code Graph Analysis Results
test/e2e/performanceprofile/functests/utils/nodes/nodes.go
Lines 146-148
func ExecCommand(ctx context.Context, node *corev1.Node, command []string) ([]byte, error) {
return nodeInspector.ExecCommand(ctx, node, command)
}Lines 151-164
func GetKubeletConfig(ctx context.Context, node *corev1.Node) (*kubeletconfigv1beta1.KubeletConfiguration, error) {
command := []string{"cat", path.Join("/rootfs", testutils.FilePathKubeletConfig)}
kubeletBytes, err := ExecCommand(ctx, node, command)
if err != nil {
return nil, err
}
testlog.Infof("command output: %s", string(kubeletBytes))
kubeletConfig := &kubeletconfigv1beta1.KubeletConfiguration{}
if err := yaml.Unmarshal(kubeletBytes, kubeletConfig); err != nil {
return nil, err
}
return kubeletConfig, err
}Lines 113-116
func GetByLabels(nodeLabels map[string]string) ([]corev1.Node, error) {
selector := labels.SelectorFromSet(nodeLabels)
return GetBySelector(selector)
}Lines 171-200
func MatchingOptionalSelector(toFilter []corev1.Node) ([]corev1.Node, error) {
if testutils.NodesSelector == "" {
return toFilter, nil
}
selector, err := labels.Parse(testutils.NodesSelector)
if err != nil {
return nil, fmt.Errorf("Error parsing the %s label selector, %v", testutils.NodesSelector, err)
}
toMatch, err := GetBySelector(selector)
if err != nil {
return nil, fmt.Errorf("Error in getting nodes matching the %s label selector, %v", testutils.NodesSelector, err)
}
if len(toMatch) == 0 {
return nil, fmt.Errorf("Failed to get nodes matching %s label selector", testutils.NodesSelector)
}
res := make([]corev1.Node, 0)
for _, n := range toFilter {
for _, m := range toMatch {
if n.Name == m.Name {
res = append(res, n)
break
}
}
}
return res, nil
}test/e2e/performanceprofile/functests/utils/profilesupdate/profile_update.go
Lines 89-108
func WaitForTuningUpdating(ctx context.Context, profile *performancev2.PerformanceProfile) {
GinkgoHelper()
// In case we are on OCP, we can query the MCP to determine if the update has started.
if !hypershift.IsHypershiftCluster() {
performanceMCP, err := mcps.GetByProfile(profile)
Expect(err).ToNot(HaveOccurred())
testlog.Info("waiting for MCP starting to update")
mcps.WaitForCondition(performanceMCP, mcv1.MachineConfigPoolUpdating, corev1.ConditionTrue)
return
}
// On hypershift we can check the nodepool UpdatingConfig condition to determine if the update has started.
hostedClusterName, err := hypershift.GetHostedClusterName()
Expect(err).ToNot(HaveOccurred())
np, err := nodepools.GetByClusterName(ctx, testclient.ControlPlaneClient, hostedClusterName)
Expect(err).ToNot(HaveOccurred())
testlog.Infof("wait for node pool %q transition into update config state", client.ObjectKeyFromObject(np).String())
err = nodepools.WaitForUpdatingConfig(ctx, testclient.ControlPlaneClient, np.Name, np.Namespace)
Expect(err).ToNot(HaveOccurred())
}Lines 112-131
func WaitForTuningUpdated(ctx context.Context, profile *performancev2.PerformanceProfile) {
GinkgoHelper()
// In case we are on OCP, we can query the MCP to determine if the update has completed.
if !hypershift.IsHypershiftCluster() {
performanceMCP, err := mcps.GetByProfile(profile)
Expect(err).ToNot(HaveOccurred())
testlog.Infof("waiting for MCP being updated")
mcps.WaitForCondition(performanceMCP, mcv1.MachineConfigPoolUpdated, corev1.ConditionTrue)
return
}
// On hypershift, we can check the nodepool UpdatingConfig condition to determine if the update has completed.
hostedClusterName, err := hypershift.GetHostedClusterName()
Expect(err).ToNot(HaveOccurred())
np, err := nodepools.GetByClusterName(ctx, testclient.ControlPlaneClient, hostedClusterName)
Expect(err).ToNot(HaveOccurred())
testlog.Infof("wait for node pool %q transition into config ready state", client.ObjectKeyFromObject(np).String())
err = nodepools.WaitForConfigToBeReady(ctx, testclient.ControlPlaneClient, np.Name, np.Namespace)
Expect(err).ToNot(HaveOccurred())
}test/e2e/performanceprofile/functests/utils/profiles/profiles.go
Lines 25-46
func GetByNodeLabels(nodeLabels map[string]string) (*performancev2.PerformanceProfile, error) {
profiles, err := All()
if err != nil {
return nil, err
}
var result *performancev2.PerformanceProfile
for i := 0; i < len(profiles.Items); i++ {
if reflect.DeepEqual(profiles.Items[i].Spec.NodeSelector, nodeLabels) {
if result != nil {
return nil, fmt.Errorf("found more than one performance profile with specified node selector %v", nodeLabels)
}
result = &profiles.Items[i]
}
}
if result == nil {
return nil, fmt.Errorf("failed to find performance profile with specified node selector %v", nodeLabels)
}
return result, nil
}Lines 101-117
func UpdateWithRetry(profile *performancev2.PerformanceProfile) {
EventuallyWithOffset(1, func() error {
profileFromAPIServer := &performancev2.PerformanceProfile{}
// get the current resourceVersion
if err := testclient.ControlPlaneClient.Get(context.TODO(), client.ObjectKeyFromObject(profile), profileFromAPIServer); err != nil {
return err
}
prepared := prepareForUpdate(profile, profileFromAPIServer)
if err := testclient.ControlPlaneClient.Update(context.TODO(), prepared); err != nil {
if !errors.IsConflict(err) {
testlog.Errorf("failed to update the profile %q: %v", profile.Name, err)
}
return err
}
return nil
}, time.Minute, 5*time.Second).Should(BeNil())
}test/e2e/performanceprofile/functests/utils/poolname/poolname.go
Lines 22-32
func GetByProfile(ctx context.Context, profile *performancev2.PerformanceProfile) string {
GinkgoHelper()
if !hypershift.IsHypershiftCluster() {
poolName, err := mcps.GetByProfile(profile)
Expect(err).ToNot(HaveOccurred())
return poolName
}
np, err := nodepools.GetNodePool(ctx, testclient.ControlPlaneClient)
Expect(err).ToNot(HaveOccurred(), "failed to get node pool affected by profile: %q", profile.Name)
return client.ObjectKeyFromObject(np).String()
}test/e2e/performanceprofile/functests/utils/hypershift/hypershift.go
Lines 69-71
func IsHypershiftCluster() bool {
return isHypershiftCluster
}test/e2e/performanceprofile/functests/utils/infrastructure/cpu.go
Lines 96-103
func IsARM(ctx context.Context, node *corev1.Node) (bool, error) {
architectureData, err := CPUArchitecture(ctx, node)
if err != nil {
return false, err
}
return architectureData == "aarch64", nil
}test/e2e/performanceprofile/functests/utils/label/label.go
Lines 60-60
ExperimentalAnnotations Feature = "experimental-annotations"Lines 105-105
Tier2 Tier = "tier-2"test/e2e/performanceprofile/functests/utils/client/clients.go
Lines 34-34
ControlPlaneClient client.Client
Contributor
Author
There was a problem hiding this comment.
The scope of this pr is only related to kubelet.go
There was a problem hiding this comment.
@mrniranjan, agreed — the finding is entirely scoped to kubelet.go; no changes are requested outside this file.
The earlier code-graph details were only supporting context and were unnecessary here. The remaining requested adjustments are limited to validating the filtered node slices and using workerRTNodes for the ARM check within kubelet.go.
Contributor
Author
There was a problem hiding this comment.
i am looking at the latest commit and the fix seems to be there:
https://github.com/mrniranjan/cluster-node-tuning-operator/blob/62eac14b4e956571b2fb481a99a99b01c3a19c04/test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go#L48
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
commit="62eac14b4e956571b2fb481a99a99b01c3a19c04"
file="test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go"
helper="test/e2e/performanceprofile/functests/utils/nodes/nodes.go"
git cat-file -e "${commit}^{commit}"
echo "kubelet.go relevant ranges at ${commit}:"
git show "${commit}:${file}" | nl -ba | sed -n '40,55p;150,165p;190,207p'
echo
echo "Non-empty assertions and ARM lookup at ${commit}:"
git show "${commit}:${file}" |
rg -n -C 2 'MatchingOptionalSelector|Expect\(len\((workerRTNodes|updatedNodes)\)|infrastructure\.IsARM|cnfNodes'
echo
echo "MatchingOptionalSelector implementation at ${commit}:"
git show "${commit}:${helper}" | nl -ba | sed -n '165,210p'Length of output: 1147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
commit="62eac14b4e956571b2fb481a99a99b01c3a19c04"
file="test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go"
helper="test/e2e/performanceprofile/functests/utils/nodes/nodes.go"
echo "kubelet.go relevant ranges at ${commit}:"
git show "${commit}:${file}" |
sed -n '40,55p;150,165p;190,207p'
echo
echo "Relevant occurrences with line numbers:"
git show "${commit}:${file}" |
grep -n -E 'MatchingOptionalSelector|Expect\(len\((workerRTNodes|updatedNodes)\)|infrastructure\.IsARM|cnfNodes' || true
echo
echo "MatchingOptionalSelector implementation at ${commit}:"
git show "${commit}:${helper}" |
awk '
/^func MatchingOptionalSelector\(/ { printing=1 }
printing { printf "%d: %s\n", NR, $0 }
printing && /^}/ { exit }
'Length of output: 4568
@mrniranjan, you’re right that Line 48 fixes the earlier issue of not retaining the MatchingOptionalSelector result—thanks for pointing that out.
However, the consolidated finding is still partially applicable:
- Neither filtered slice is asserted non-empty after filtering.
MatchingOptionalSelectorcan return an empty intersection without an error when the optional-selector nodes and performance-profile nodes do not overlap. - The ARM check still uses a newly fetched, unfiltered
cnfNodes[0], rather than the already filteredworkerRTNodes[0].
So I should have been more precise: the Line 48 part is fixed, but the two remaining validation/consistency points are not.
Comment on lines
+87
to
93
| autoSizingCmd := []string{"cat", "/rootfs/etc/openshift/kubelet.conf.d/20-auto-sizing.conf"} | ||
| for _, node := range workerRTNodes { | ||
| out, err := nodes.ExecCommand(context.TODO(), &node, kubeletArguments) | ||
| out, err := nodes.ExecCommand(context.TODO(), &node, autoSizingCmd) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| stdout := testutils.ToString(out) | ||
| Expect(strings.Contains(stdout, "300Mi")).To(BeTrue()) | ||
| Expect(stdout).To(ContainSubstring("300Mi")) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use bounded contexts for new node-inspector calls.
These new external calls use context.TODO(), so a stalled inspector command cannot be cancelled or timed out. Derive a context.WithTimeout context for each operation.
As per path instructions, Go code must use context.Context for cancellation and timeouts.
📍 Affects 1 file
test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go#L87-L93(this comment)test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go#L160-L162test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go#L241-L254
🤖 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/7_performance_kubelet_node/kubelet.go`
around lines 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.
Source: Path instructions
Contributor
Author
There was a problem hiding this comment.
can you check the latest commit , this should be fixed
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target='test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go'
echo '== Current HEAD =='
git log -1 --format='%H%n%s'
echo
echo '== Recent commits affecting the target file =='
git log -5 --format='%h %s' -- "$target"
echo
echo '== Target file outline =='
ast-grep outline "$target" --items all --type function
echo
echo '== ExecCommand call sites with surrounding context =='
rg -n -C 8 'nodes\.ExecCommand\s*\(' "$target"
echo
echo '== Context timeout/cancel use with surrounding context =='
rg -n -C 4 'context\.(TODO|WithTimeout|WithCancel)\s*\(|\bcancel\s*\(' "$target"Length of output: 13688
@mrniranjan, I checked the latest commit (62eac14b). This is not fixed yet: both direct node-inspector calls still pass unbounded context.TODO():
- Line 89
- Line 251
nodes.ExecCommand forwards that context directly to the inspector, and the file has no timeout/cancellation context. Please wrap each command with a context.WithTimeout(...) context and defer cancel() (or cancel per loop iteration) so a stalled command cannot block indefinitely.
🧬 Code Graph Analysis Results
Relevant snippets
test/e2e/performanceprofile/functests/utils/nodes/nodes.go (line 146-148)
func ExecCommand(ctx context.Context, node *corev1.Node, command []string) ([]byte, error) {
return nodeInspector.ExecCommand(ctx, node, command)
}test/e2e/performanceprofile/functests/utils/nodes/nodes.go (line 151-164)
func GetKubeletConfig(ctx context.Context, node *corev1.Node) (*kubeletconfigv1beta1.KubeletConfiguration, error) {
command := []string{"cat", path.Join("/rootfs", testutils.FilePathKubeletConfig)}
kubeletBytes, err := ExecCommand(ctx, node, command)
if err != nil {
return nil, err
}
testlog.Infof("command output: %s", string(kubeletBytes))
kubeletConfig := &kubeletconfigv1beta1.KubeletConfiguration{}
if err := yaml.Unmarshal(kubeletBytes, kubeletConfig); err != nil {
return nil, err
}
return kubeletConfig, err
}test/e2e/performanceprofile/functests/utils/nodes/nodes.go (line 113-116)
func GetByLabels(nodeLabels map[string]string) ([]corev1.Node, error) {
selector := labels.SelectorFromSet(nodeLabels)
return GetBySelector(selector)
}test/e2e/performanceprofile/functests/utils/nodes/nodes.go (line 171-200)
func MatchingOptionalSelector(toFilter []corev1.Node) ([]corev1.Node, error) {
if testutils.NodesSelector == "" {
return toFilter, nil
}
selector, err := labels.Parse(testutils.NodesSelector)
if err != nil {
return nil, fmt.Errorf("Error parsing the %s label selector, %v", testutils.NodesSelector, err)
}
toMatch, err := GetBySelector(selector)
if err != nil {
return nil, fmt.Errorf("Error in getting nodes matching the %s label selector, %v", testutils.NodesSelector, err)
}
if len(toMatch) == 0 {
return nil, fmt.Errorf("Failed to get nodes matching %s label selector", testutils.NodesSelector)
}
res := make([]corev1.Node, 0)
for _, n := range toFilter {
for _, m := range toMatch {
if n.Name == m.Name {
res = append(res, n)
break
}
}
}
return res, nil
}test/e2e/performanceprofile/functests/utils/profilesupdate/profile_update.go (line 89-108)
func WaitForTuningUpdating(ctx context.Context, profile *performancev2.PerformanceProfile) {
GinkgoHelper()
// In case we are on OCP, we can query the MCP to determine if the update has started.
if !hypershift.IsHypershiftCluster() {
performanceMCP, err := mcps.GetByProfile(profile)
Expect(err).ToNot(HaveOccurred())
testlog.Info("waiting for MCP starting to update")
mcps.WaitForCondition(performanceMCP, mcv1.MachineConfigPoolUpdating, corev1.ConditionTrue)
return
}
// On hypershift we can check the nodepool UpdatingConfig condition to determine if the update has started.
hostedClusterName, err := hypershift.GetHostedClusterName()
Expect(err).ToNot(HaveOccurred())
np, err := nodepools.GetByClusterName(ctx, testclient.ControlPlaneClient, hostedClusterName)
Expect(err).ToNot(HaveOccurred())
testlog.Infof("wait for node pool %q transition into update config state", client.ObjectKeyFromObject(np).String())
err = nodepools.WaitForUpdatingConfig(ctx, testclient.ControlPlaneClient, np.Name, np.Namespace)
Expect(err).ToNot(HaveOccurred())
}test/e2e/performanceprofile/functests/utils/profilesupdate/profile_update.go (line 112-131)
func WaitForTuningUpdated(ctx context.Context, profile *performancev2.PerformanceProfile) {
GinkgoHelper()
// In case we are on OCP, we can query the MCP to determine if the update has completed.
if !hypershift.IsHypershiftCluster() {
performanceMCP, err := mcps.GetByProfile(profile)
Expect(err).ToNot(HaveOccurred())
testlog.Infof("waiting for MCP being updated")
mcps.WaitForCondition(performanceMCP, mcv1.MachineConfigPoolUpdated, corev1.ConditionTrue)
return
}
// On hypershift, we can check the nodepool UpdatingConfig condition to determine if the update has completed.
hostedClusterName, err := hypershift.GetHostedClusterName()
Expect(err).ToNot(HaveOccurred())
np, err := nodepools.GetByClusterName(ctx, testclient.ControlPlaneClient, hostedClusterName)
Expect(err).ToNot(HaveOccurred())
testlog.Infof("wait for node pool %q transition into config ready state", client.ObjectKeyFromObject(np).String())
err = nodepools.WaitForConfigToBeReady(ctx, testclient.ControlPlaneClient, np.Name, np.Namespace)
Expect(err).ToNot(HaveOccurred())
}test/e2e/performanceprofile/functests/utils/profiles/profiles.go (line 25-46)
func GetByNodeLabels(nodeLabels map[string]string) (*performancev2.PerformanceProfile, error) {
profiles, err := All()
if err != nil {
return nil, err
}
var result *performancev2.PerformanceProfile
for i := 0; i < len(profiles.Items); i++ {
if reflect.DeepEqual(profiles.Items[i].Spec.NodeSelector, nodeLabels) {
if result != nil {
return nil, fmt.Errorf("found more than one performance profile with specified node selector %v", nodeLabels)
}
result = &profiles.Items[i]
}
}
if result == nil {
return nil, fmt.Errorf("failed to find performance profile with specified node selector %v", nodeLabels)
}
return result, nil
}test/e2e/performanceprofile/functests/utils/profiles/profiles.go (line 101-117)
func UpdateWithRetry(profile *performancev2.PerformanceProfile) {
EventuallyWithOffset(1, func() error {
profileFromAPIServer := &performancev2.PerformanceProfile{}
// get the current resourceVersion
if err := testclient.ControlPlaneClient.Get(context.TODO(), client.ObjectKeyFromObject(profile), profileFromAPIServer); err != nil {
return err
}
prepared := prepareForUpdate(profile, profileFromAPIServer)
if err := testclient.ControlPlaneClient.Update(context.TODO(), prepared); err != nil {
if !errors.IsConflict(err) {
testlog.Errorf("failed to update the profile %q: %v", profile.Name, err)
}
return err
}
return nil
}, time.Minute, 5*time.Second).Should(BeNil())
}test/e2e/performanceprofile/functests/utils/poolname/poolname.go (line 22-32)
func GetByProfile(ctx context.Context, profile *performancev2.PerformanceProfile) string {
GinkgoHelper()
if !hypershift.IsHypershiftCluster() {
poolName, err := mcps.GetByProfile(profile)
Expect(err).ToNot(HaveOccurred())
return poolName
}
np, err := nodepools.GetNodePool(ctx, testclient.ControlPlaneClient)
Expect(err).ToNot(HaveOccurred(), "failed to get node pool affected by profile: %q", profile.Name)
return client.ObjectKeyFromObject(np).String()
}test/e2e/performanceprofile/functests/utils/hypershift/hypershift.go (line 69-71)
func IsHypershiftCluster() bool {
return isHypershiftCluster
}test/e2e/performanceprofile/functests/utils/client/clients.go (line 34-34)
ControlPlaneClient client.Client
Contributor
Author
There was a problem hiding this comment.
As per the last commit , the latest kubelet.go doesn't have that: Please check
https://github.com/mrniranjan/cluster-node-tuning-operator/blob/62eac14b4e956571b2fb481a99a99b01c3a19c04/test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go#L48
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
commit='62eac14b4e956571b2fb481a99a99b01c3a19c04'
target='test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go'
echo '== Referenced commit =='
git show -s --format='%H%n%s' "$commit"
echo
echo '== ExecCommand and context call sites at referenced commit =='
git show "${commit}:${target}" | nl -ba | rg -C 5 'nodes\.ExecCommand\s*\(|context\.(TODO|WithTimeout|WithCancel|Background)\s*\(' || true
echo
echo '== Relevant file slice around Line 48 at referenced commit =='
git show "${commit}:${target}" | nl -ba | sed -n '40,115p'Length of output: 3072
🏁 Script executed:
#!/bin/bash
set -euo pipefail
commit='62eac14b4e956571b2fb481a99a99b01c3a19c04'
target='test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go'
content="$(git show "${commit}:${target}")"
echo '== nodes.ExecCommand call sites at referenced commit =='
printf '%s\n' "$content" | awk '
/nodes\.ExecCommand[[:space:]]*\(/ {
for (i=NR-4; i<=NR+4; i++) if (i>0) print lines[i]
print "-----"
}
{ lines[NR]=sprintf("%4d %s", NR, $0) }
' || true
echo
echo '== All context construction/use sites at referenced commit =='
printf '%s\n' "$content" | awk '
/context\.(TODO|WithTimeout|WithCancel|Background)[[:space:]]*\(/ {
for (i=NR-2; i<=NR+2; i++) if (i>0) print lines[i]
print "-----"
}
{ lines[NR]=sprintf("%4d %s", NR, $0) }
' || true
echo
echo '== Lines 40-115 at referenced commit =='
printf '%s\n' "$content" | awk 'NR >= 40 && NR <= 115 { printf "%4d %s\n", NR, $0 }'Length of output: 6170
@mrniranjan, I checked the exact commit you linked. Line 48 is indeed unrelated—it calls nodes.MatchingOptionalSelector, so it does not contain the context issue.
However, the two new direct inspector calls are still present in that same commit:
- Line 89:
nodes.ExecCommand(context.TODO(), &node, autoSizingCmd) - The later default-profile auto-sizing check (around Line 251) also calls
nodes.ExecCommandwithcontext.TODO().
Since nodes.ExecCommand forwards its context to the node inspector, the finding remains valid for those two calls.
🧬 Code Graph Analysis Results
test/e2e/performanceprofile/functests/7_performance_kubelet_node/kubelet.go
Lines 43-56 (where kubelet.go applies the optional node filtering):
testutils.CustomBeforeAll(func() {
var err error
workerRTNodes, err = nodes.GetByLabels(testutils.NodeSelectorLabels)
Expect(err).ToNot(HaveOccurred())
workerRTNodes, err = nodes.MatchingOptionalSelector(workerRTNodes)
Expect(err).ToNot(HaveOccurred())
profile, err = profiles.GetByNodeLabels(testutils.NodeSelectorLabels)
Expect(err).ToNot(HaveOccurred())
poolName = poolname.GetByProfile(ctx, profile)
initialProfile = profile.DeepCopy()
})test/e2e/performanceprofile/functests/utils/nodes/nodes.go
Lines 113-116 (Get nodes by the provided label selector map):
func GetByLabels(nodeLabels map[string]string) ([]corev1.Node, error) {
selector := labels.SelectorFromSet(nodeLabels)
return GetBySelector(selector)
}Lines 171-200 (optionally filters nodes further based on testutils.NodesSelector):
func MatchingOptionalSelector(toFilter []corev1.Node) ([]corev1.Node, error) {
if testutils.NodesSelector == "" {
return toFilter, nil
}
selector, err := labels.Parse(testutils.NodesSelector)
if err != nil {
return nil, fmt.Errorf("Error parsing the %s label selector, %v", testutils.NodesSelector, err)
}
toMatch, err := GetBySelector(selector)
if err != nil {
return nil, fmt.Errorf("Error in getting nodes matching the %s label selector, %v", testutils.NodesSelector, err)
}
if len(toMatch) == 0 {
return nil, fmt.Errorf("Failed to get nodes matching %s label selector", testutils.NodesSelector)
}
res := make([]corev1.Node, 0)
for _, n := range toFilter {
for _, m := range toMatch {
if n.Name == m.Name {
res = append(res, n)
break
}
}
}
return res, nil
}
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).