OCPBUGS-98399: E2E: Add functional test cases checking GOMAXPROCS#1556
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughThe PR adds e2e coverage for GOMAXPROCS behavior on performance-profile nodes. It verifies CRI-O configuration on workers and checks pod-visible GOMAXPROCS outcomes across multiple QoS, runtime-class, and init-container cases. ChangesGOMAXPROCS configuration coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (11 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 |
Following tests were added: 1. Verify GOMAXPROCS configuration is injected when Performance Profile is applied 2. Create a go-based container with below scenarios BestEffort - No Requests/Limits Burstable low - cpu:100m requests, no limits Burstable high - cpu:8 requests, no limits (2x overcommit) Guaranteed - cpu:1 request + limit Guaranteed with runtime class - cpu:1 request + limit Init + app container - cpu:100m on both, no limits Burstable with limit - cpu:2 request, cpu:4 limit Signed-off-by: Niranjan M.R <mniranja@redhat.com>
mrniranjan
force-pushed
the
gomaxprocs_e2e_test
branch
from
961314a to
3dee669
Compare
mrniranjan
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![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
🧹 Nitpick comments (1)
test/e2e/performanceprofile/functests/1_performance/performance.go (1)
1238-1242: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDerive the runtime class name instead of hard-coding it.
This file derives performance-profile-owned names elsewhere. Hard-coding
"performance-performance"makes this entry fail when the configured performance profile name differs.Proposed change
Entry("Guaranteed with runtime class - cpu:1 request + limit", "Guaranteed-RuntimeClass", ptr.To(resource.MustParse("1")), ptr.To(resource.MustParse("1")), - false, ptr.To("performance-performance"), + false, ptr.To(components.GetComponentName(testutils.PerformanceProfileName, components.ProfileNamePerformance)), "", true, ),🤖 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/performance.go` around lines 1238 - 1242, The runtime class name in this table-driven case is hard-coded, which breaks when the performance profile name changes. Update the “Guaranteed-RuntimeClass” entry in performance.go to derive the runtime class name the same way the other performance-profile-owned names are built in this file, using the existing helper/derived profile name logic instead of the literal "performance-performance".
🤖 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/performance.go`:
- Around line 1168-1172: The cleanup path in DeferCleanup is using an unbounded
context, and the pod exec flow behind pods.ExecCommandOnPod also lacks
cancellation control. Replace the context.Background() cleanupCtx with a fresh
timeout-derived context tied to the spec/test scope, and update or add a
context-aware pod exec helper so exec calls do not rely on context.TODO(). Apply
the same bounded-context pattern to the related cleanup block that deletes the
test pod and any exec paths in the performance test helpers.
- Around line 1124-1127: The “Burstable-High” test setup unconditionally pins
the pod to workerRTNodes[0], which can leave the pod Pending on small clusters
with insufficient CPU. Update the test flow around pods.GetTestPod(),
testpod.Spec.NodeSelector, and runtimeClassName to first verify the selected
node can satisfy the 8-CPU request, and if not, skip or guard this case rather
than waiting for the full timeout. Apply the same protection in the later
duplicated setup around the other Burstable-High block as well.
- Around line 1135-1146: The Guaranteed pod setup in the performance test only
assigns CPU resources, so the pod can still remain Burstable. Update the
container resource setup in the code that configures
testpod.Spec.Containers[0].Resources to also set matching memory request and
limit whenever the Guaranteed path is being exercised, and make the same change
in the runtime-class variant so both paths stay aligned.
---
Nitpick comments:
In `@test/e2e/performanceprofile/functests/1_performance/performance.go`:
- Around line 1238-1242: The runtime class name in this table-driven case is
hard-coded, which breaks when the performance profile name changes. Update the
“Guaranteed-RuntimeClass” entry in performance.go to derive the runtime class
name the same way the other performance-profile-owned names are built in this
file, using the existing helper/derived profile name logic instead of the
literal "performance-performance".
🪄 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: ea2e2504-5787-4660-968c-1b9b29f4e931
📒 Files selected for processing (1)
test/e2e/performanceprofile/functests/1_performance/performance.go
| DeferCleanup(func() { | ||
| testlog.Infof("[%s] Cleaning up test pod: %s", testCase, testpod.Name) | ||
| cleanupCtx := context.Background() | ||
| err := testclient.DataPlaneClient.Delete(cleanupCtx, testpod) | ||
| if err != nil { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Keep cleanup and pod exec calls bounded by context timeouts.
Line 1170 uses an unbounded context.Background(), and these new exec paths call pods.ExecCommandOnPod, which uses context.TODO() internally. A stalled API delete or SPDY exec can outlive the spec context. Use a fresh timeout for cleanup and add/use a context-aware pod exec helper. As per path instructions, **/*.go: context.Context for cancellation and timeouts.
Proposed cleanup-side fix
DeferCleanup(func() {
testlog.Infof("[%s] Cleaning up test pod: %s", testCase, testpod.Name)
- cleanupCtx := context.Background()
+ cleanupCtx, cancel := context.WithTimeout(context.Background(), time.Minute)
+ defer cancel()
err := testclient.DataPlaneClient.Delete(cleanupCtx, testpod)Also applies to: 1182-1191
🤖 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/performance.go` around
lines 1168 - 1172, The cleanup path in DeferCleanup is using an unbounded
context, and the pod exec flow behind pods.ExecCommandOnPod also lacks
cancellation control. Replace the context.Background() cleanupCtx with a fresh
timeout-derived context tied to the spec/test scope, and update or add a
context-aware pod exec helper so exec calls do not rely on context.TODO(). Apply
the same bounded-context pattern to the related cleanup block that deletes the
test pod and any exec paths in the performance test helpers.
Source: Path instructions
There was a problem hiding this comment.
check again on the latest commit
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate candidate helper files =="
fd -i 'pods*.go' test/e2e/performanceprofile/functests/utils || true
echo
echo "== Outline performance.go around GOMAXPROCS additions =="
ast-grep outline test/e2e/performanceprofile/functests/1_performance/performance.go --items all --type function
echo
echo "== Show current GOMAXPROCS block in performance.go =="
sed -n '1095,1235p' test/e2e/performanceprofile/functests/1_performance/performance.go
echo
echo "== Outline pods helper files =="
for f in $(fd -i 'pods*.go' test/e2e/performanceprofile/functests/utils); do
echo "-- $f --"
ast-grep outline "$f" --items all --type function || true
done
echo
echo "== Search for ExecCommandOnPod definitions and calls =="
rg -n -C3 '\bExecCommandOnPod\s*\(' test/e2e/performanceprofile/functests
echo
echo "== Read pods helper implementation slices =="
for f in $(fd -i 'pods*.go' test/e2e/performanceprofile/functests/utils); do
if rg -n '\bExecCommandOnPod\s*\(' "$f" >/dev/null; then
echo "-- $f --"
ln=$(rg -n '\bExecCommandOnPod\s*\(' "$f" | cut -d: -f1 | head -n1)
start=$((ln>25 ? ln-25 : 1))
end=$((ln+80))
sed -n "${start},${end}p" "$f"
fi
doneLength of output: 32036
@mrniranjan, re-checked on the latest commit:
DeferCleanupis now properly bounded withcontext.WithTimeout(...), so that part is fixed.- The remaining concern is still valid: the new exec calls at Line 1198 and Line 1206 still go through
test/e2e/performanceprofile/functests/utils/pods/pods.go, whereExecCommandOnPod()usesexec.StreamWithContext(context.TODO(), ...).
So I’d consider this partially addressed, but I wouldn’t withdraw the thread yet until the pod exec helper is made context-aware (or a context-aware variant is introduced and used here).
There was a problem hiding this comment.
The cleanup path has been fixed to use context.WithTimeout() for the pod deletion. If making the exec helper context-aware is needed, that should be addressed in a separate PR to avoid impacting other tests that depend on this shared utility
There was a problem hiding this comment.
@mrniranjan, that's a reasonable call — the DeferCleanup fix addresses the immediate risk in this PR, and changing the shared ExecCommandOnPod() helper's context handling in test/e2e/performanceprofile/functests/utils/pods/pods.go does carry a wider blast radius across other tests that depend on it. Deferring that to a separate, focused PR makes sense.
Would you like me to open a follow-up issue to track making ExecCommandOnPod() context-aware? If so, let me know who should be assigned (defaulting to you if unspecified).
There was a problem hiding this comment.
okay assign it to me
Signed-off-by: Niranjan M.R <mniranja@redhat.com>
Signed-off-by: Niranjan M.R <mniranja@redhat.com>
|
/test e2e-gcp-pao |
|
@mrniranjan: 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. |
|
[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
mrniranjan
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@mrniranjan: This pull request references Jira Issue OCPBUGS-98399, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
/verified by @mrniranjan |
openshift-ci-robot
added
the
verified
|
@mrniranjan: 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. |
|
@mrniranjan: Jira Issue Verification Checks: Jira Issue OCPBUGS-98399 Jira Issue OCPBUGS-98399 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
/cherry-pick release-4.22 |
|
@mrniranjan: new pull request created: #1561 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 kubernetes-sigs/prow repository. |
4 participants
E2E: Add functional test cases checking GOMAXPROCS
Following tests were added:
BestEffort - No Requests/Limits
Burstable low - cpu:100m requests, no limits
Burstable high - cpu:8 requests, no limits (2x overcommit)
Guaranteed - cpu:1 request + limit
Guaranteed with runtime class - cpu:1 request + limit
Init + app container - cpu:100m on both, no limits
Burstable with limit - cpu:2 request, cpu:4 limit
Summary by CodeRabbit
Summary by CodeRabbit