E2E: Fix tuned active profile check for wrapped performance profiles#1549
E2E: Fix tuned active profile check for wrapped performance profiles#1549mrniranjan wants to merge 1 commit into
Conversation
Test 37127 read /etc/tuned/active_profile on the node and expected the performance profile name. That fails when a higher-priority Tuned CR wraps the PP via include=<pp-profile> (e.g. ran-du-performance), because the active profile name is the wrapper, not the PP name. Detect wrapper profiles by scanning Tuned CRs for include=<pp-profile> and assert Profile.status.tunedProfile via the API instead of pod exec. Signed-off-by: Niranjan M.R <mniranja@redhat.com>
WalkthroughA new helper ChangesTuned Profile Verification Rewrite
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 1430-1439: The loop that returns the first profile containing
include=<performanceProfileName> does not account for Tuned recommendation
semantics and can return an incorrect profile when multiple Tuned CRs include
the performance profile. Instead of returning on the first include= match found
via strings.Contains, resolve the wrapper profile by consulting Tuned's
recommendation selection logic (priority and match criteria) to identify the
active/effective recommended profile. Alternatively, detect when multiple
candidate profiles exist that contain the include match and fail fast with a
clear error rather than returning an ambiguous result.
🪄 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: ee8fdbda-b6f7-416d-a49b-3d1cfb261e56
📒 Files selected for processing (1)
test/e2e/performanceprofile/functests/1_performance/performance.go
| for _, tuned := range tunedList.Items { | ||
| for _, profile := range tuned.Spec.Profile { | ||
| if profile.Data == nil || profile.Name == nil { | ||
| continue | ||
| } | ||
|
|
||
| if strings.Contains(*profile.Data, fmt.Sprintf("include=%s", performanceProfileName)) { | ||
| testlog.Warningf("performance profile is wrapped by Tuned CR profile %q (profile: %q)", tuned.Name, *profile.Name) | ||
| return *profile.Name | ||
| } |
There was a problem hiding this comment.
Resolve active wrapper via recommendation semantics, not first include= hit.
Line 1430-Line 1439 returns the first profile whose data contains include=<performanceProfileName>. This can select the wrong expected profile when multiple Tuned CRs include the performance profile (or include it but are not the effective/highest-priority recommended profile), causing nondeterministic test outcomes.
Please resolve against Tuned recommendation selection (priority/match) before returning a wrapper profile name, or fail fast when multiple include-matches exist and cannot be disambiguated.
🤖 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 1430 - 1439, The loop that returns the first profile containing
include=<performanceProfileName> does not account for Tuned recommendation
semantics and can return an incorrect profile when multiple Tuned CRs include
the performance profile. Instead of returning on the first include= match found
via strings.Contains, resolve the wrapper profile by consulting Tuned's
recommendation selection logic (priority and match criteria) to identify the
active/effective recommended profile. Alternatively, detect when multiple
candidate profiles exist that contain the include match and fail fast with a
clear error rather than returning an ambiguous result.
|
/retest-required |
|
@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. |
1 participant
Test 37127 read /etc/tuned/active_profile on the node and expected the performance profile name. That fails when a higher-priority Tuned CR wraps the PP via include= (e.g. ran-du-performance), because the active profile name is the wrapper, not the PP name.
Detect wrapper profiles by scanning Tuned CRs for include= and assert Profile.status.tunedProfile via the API instead of pod exec.
Summary by CodeRabbit