e2e: Fix broken assertions - #1571
Conversation
PR Summary by QodoE2E: Fix no-op Gomega assertions in performance profile tests
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
WalkthroughPerformance profile end-to-end tests now use explicit Gomega assertions, direct PPC output checks, dynamic reserved-CPU expectations, corrected mixed-CPU state handling, and detailed offline CPU-set diagnostics. ChangesPerformance test updates
Estimated code review effort: 3 (Moderate) | ~25 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: oblau 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 |
Code Review by Qodo
1.
|
![qodo-for-rh-openshift[bot]](https://avatars.githubusercontent.com/u/792337?s=60&v=4){kind=small}
![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/10_performance_ppc/ppc.go`:
- Around line 164-167: Update the error matching in the test around errString
and regexp.MatchString to escape the literal CPU range before regex evaluation,
using regexp.QuoteMeta or an equivalent approach. Preserve the existing expected
message and assertions while ensuring the output comparison matches the literal
“[1,3]” text.
🪄 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: ff43d9d3-90fb-489e-be11-f312db4d9f15
📒 Files selected for processing (3)
test/e2e/performanceprofile/functests/10_performance_ppc/ppc.gotest/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.gotest/e2e/performanceprofile/functests/2_performance_update/updating_profile.go
|
/retest |
| errString := "Error: failed to obtain data from flags not appropriate to split reserved CPUs in case of topology-manager-policy: single-numa-node" | ||
| ok, err := regexp.MatchString(errString, string(output)) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| if ok { |
There was a problem hiding this comment.
That's a change in the behavior. it makes the test more restrict. is that what we want here?
There was a problem hiding this comment.
This is changing behavior - but isn't this the intention here?
If we are not asserting on ok then i fail to see why we need the errString
and regexp.MatchString(errString, string(output)) to begin with.
Maybe im missing something here.
Is the goal to make sure the correct error message appears, or that PPC script fails in general?
There was a problem hiding this comment.
After reevaluation I think this change is ok.
the only change I would add is in the message to make it more clear:
Expect(ok).To(BeTrue(), "expected error %q to be found in output: %s", errString, output)
b138108 to
9c24e1d
Compare
|
/retest |
|
@oblau: The following test 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. |
Expect(bool) without .To() is a Gomega no-op; these tests never fail on these assertions.
…pand fix - Add .To(Succeed()) / err checks on three ControlPlaneClient.Get() calls. Bare Expect(Get(...)) is a Gomega no-op, so API errors were ignored. - Change scheduling Guaranteed pods from cpu:1 to cpu:2, and remove the setup() path that set full-pcpus-only=false on small nodes. With default SMT alignment, cpu:1 cannot schedule; CI has enough CPUs for cpu:2, so the cancel-SMT workaround is unnecessary. - When exec-cpu-affinity expands Shared from 1 CPU to a new 2-CPU set, Union the previous shared CPU into Isolated. Replacing Shared without that left the old shared CPU in neither set and broke later assertions.
The `if ok { testlog.Info(...) }` pattern after regexp.MatchString
never failed the test when the expected error was absent from output.
Replace with Wait + gexec.Exit and ContainSubstring (not regexp) so a
missing error message actually fails the test case.
Match leaf error text only; outer wraps have changed and are not the
intent of these cases:
- 41405: https://github.com/openshift/cluster-node-tuning-operator/blob/a9d25d502ca894272f88753f1bd7ecef82fb188e/pkg/performanceprofile/profilecreator/cmd/root.go#L221
- 41419: https://github.com/openshift/cluster-node-tuning-operator/blob/a9d25d502ca894272f88753f1bd7ecef82fb188e/pkg/performanceprofile/profilecreator/profilecreator.go#L363
- 41420: https://github.com/openshift/cluster-node-tuning-operator/blob/a9d25d502ca894272f88753f1bd7ecef82fb188e/pkg/performanceprofile/profilecreator/profilecreator.go#L128
([1,%d] is TotalThreads-1; format at runtime via
maxReservedCPUCountFromMustGather from the must-gather MCP node)
Also move gexec.Exit checks to right after Wait (before content
asserts), and fix success-path "marshal" -> "unmarshal" messages.
9c24e1d to
3cafd46
Compare
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/11_mixedcpus/mixedcpus.go`:
- Line 149: Update setup in
test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go to verify at
least two isolated CPUs are available when mixed CPUs are enabled, and skip
these workloads when they are not; alternatively provision that capacity before
creation. Apply the protection to the two-CPU workload definitions at lines 149,
181, 201, 218, 249, 325, and 437, ensuring shared-CPU resources are not treated
as isolated CPU capacity.
🪄 Autofix
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: 5d8b0ebb-5852-4f4a-b8ca-3ef3c534df2e
📒 Files selected for processing (2)
test/e2e/performanceprofile/functests/10_performance_ppc/ppc.gotest/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go
| It("verify cpu load balancing still works with mixed cpus", func() { | ||
| rl := &corev1.ResourceList{ | ||
| corev1.ResourceCPU: resource.MustParse("1"), | ||
| corev1.ResourceCPU: resource.MustParse("2"), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go --items all
rg -n -C 5 \
'ResourceCPU:\s+resource\.MustParse\("2"\)|func setup\(|CPU\.Isolated|isolated.*Size\(\)|createPod\(' \
test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go
fd -e go . test/e2e/performanceprofile | while IFS= read -r file; do
rg -n -C 3 'MixedCPUs|CPU\.Isolated|isolated.*cpus|isolated.*CPU' "$file" || true
doneRepository: openshift/cluster-node-tuning-operator
Length of output: 50394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- test profile and suite setup ---'
sed -n '55,155p' test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go
sed -n '810,875p' test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go
printf '%s\n' '--- all direct uses of the seven workloads ---'
sed -n '140,270p' test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go
sed -n '286,355p' test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go
sed -n '428,470p' test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go
printf '%s\n' '--- resource and scheduling helpers ---'
sed -n '900,1035p' test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go
rg -n -C 4 'enable-shared-cpus|sharedCpusResource|MixedCpus|mixed cpus|mixed CPUs|ResourceCPU' \
pkg test/e2e/performanceprofile/functests/utils | head -n 300Repository: openshift/cluster-node-tuning-operator
Length of output: 50394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- profile construction and test entry points ---'
sed -n '95,155p' test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go
rg -n -C 5 'testProfile\(\)|MixedCPUs|NodeSelectorLabels|ProfileNotFound' \
test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go \
test/e2e/performanceprofile/functests/utils
printf '%s\n' '--- mixed-CPU resource implementation ---'
rg -n -C 8 --glob '*.go' \
'enable-shared-cpus|shared_cpus|shared cpus|SharedCPUs|MixedCpus|MixedCPUs' \
. | head -n 500
printf '%s\n' '--- current change context for mixedcpus.go ---'
git diff --stat -- test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go
git diff --unified=12 -- test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go | sed -n '1,260p'Repository: openshift/cluster-node-tuning-operator
Length of output: 50394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- files defining the shared CPU resource ---'
git ls-files | rg -i 'mixed|shared|cpu.*manager|kubelet|crio'
printf '%s\n' '--- exact resource-name references ---'
rg -n --glob '*.go' --glob '*.yaml' --glob '*.yml' --glob '*.json' \
'workload\.openshift\.io/enable-shared-cpus' . || true
printf '%s\n' '--- test profile defaults and discovery behavior ---'
rg -n -C 8 --glob '*.go' \
'IsolatedCPUs|SharedCPUs|NewPerformanceProfile|GetDiscoveryPerformanceProfile|PerformanceProfileName' \
pkg test e2e | head -n 400
printf '%s\n' '--- concise static invariant check ---'
python3 - <<'PY'
from pathlib import Path
import re
p = Path("test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go")
s = p.read_text()
resource_blocks = re.findall(
r'rl := &corev1\.ResourceList\{(.*?)\n\s*\}',
s,
re.S,
)
target = []
for i, block in enumerate(resource_blocks, 1):
if 'resource.MustParse("2")' in block and 'sharedCpusResource' in block:
target.append(i)
setup = s[s.index('func setup('):s.index('func cpuSetToPerformanceCPUSet(')]
print(f"target resource-list blocks: {len(target)}")
print(f"target blocks contain CPU=2 and shared resource: {target}")
print("setup changes isolated CPUs only under !profileutil.IsMixedCPUsEnabled(profile):",
'!profileutil.IsMixedCPUsEnabled(profile)' in setup)
print("setup has no isolated-size guard:",
'Size()' not in setup and 'List()' not in setup)
PYRepository: openshift/cluster-node-tuning-operator
Length of output: 50394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- generated mixed-CPU configuration ---'
cat assets/performanceprofile/configs/openshift-workload-mixed-cpus
sed -n '1,220p' docs/performanceprofile/kubelet-config-snippets.md | rg -n -C 8 \
'mixed|shared|reservedSystemCPUs|cpu-shared'
printf '%s\n' '--- E2E profile defaults and discovery ---'
rg -n -C 6 --glob '*.go' \
'IsolatedCPUs|SharedCPUs|NewPerformanceProfile|GetDiscoveryPerformanceProfile|PerformanceProfileName' \
pkg test | head -n 300
printf '%s\n' '--- static invariant check ---'
python3 - <<'PY'
from pathlib import Path
import re
p = Path("test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go")
s = p.read_text()
blocks = re.findall(r'rl := &corev1\.ResourceList\{(.*?)\n\s*\}', s, re.S)
targets = [
(i, 'CPU=2' in b, 'shared=1' in b)
for i, b in enumerate(blocks, 1)
if 'resource.MustParse("2")' in b and 'sharedCpusResource' in b
]
setup = s[s.index('func setup('):s.index('func cpuSetToPerformanceCPUSet(')]
print("target resource-list blocks:", len(targets))
print("all target blocks request CPU=2 and shared resource=1:",
all(cpu and shared for _, cpu, shared in targets))
print("setup changes the profile only when mixed CPUs are disabled:",
'!profileutil.IsMixedCPUsEnabled(profile)' in setup)
print("setup checks isolated CPU count:", 'Size()' in setup)
print("setup checks isolated CPU list length:", 'List()' in setup)
PYRepository: openshift/cluster-node-tuning-operator
Length of output: 371
🏁 Script executed:
#!/bin/bash
set -u
python3 - <<'PY'
from pathlib import Path
import re
p = Path("test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go")
s = p.read_text()
blocks = re.findall(r'rl := &corev1\.ResourceList\{(.*?)\n\s*\}', s, re.S)
targets = [
b for b in blocks
if 'resource.MustParse("2")' in b and 'sharedCpusResource' in b
]
setup = s[s.index('func setup('):s.index('func cpuSetToPerformanceCPUSet(')]
print("target resource-list blocks:", len(targets))
print("all target blocks request CPU=2:", all('corev1.ResourceCPU' in b for b in targets))
print("all target blocks request shared resource=1:",
all('sharedCpusResource' in b and 'resource.MustParse("1")' in b for b in targets))
print("setup changes the profile only when mixed CPUs are disabled:",
'!profileutil.IsMixedCPUsEnabled(profile)' in setup)
print("setup checks isolated CPU count:", 'Size()' in setup or 'len(isolated.List())' in setup)
PYRepository: openshift/cluster-node-tuning-operator
Length of output: 400
🌐 Web query:
OpenShift mixed CPUs allocation workload.openshift.io/enable-shared-cpus ordinary CPU request isolated CPUs
💡 Result:
The annotation workload.openshift.io/enable-shared-cpus is part of the Mixed-CPUs feature in OpenShift, which allows Guaranteed Quality of Service (QoS) pods to utilize both isolated and shared CPU pools [1][2][3]. Typically, Guaranteed QoS pods with integer CPU requests are assigned exclusive (isolated) CPUs, meaning they cannot access the shared CPU pool [4]. The Mixed-CPUs feature changes this behavior by permitting such pods to request access to the shared CPU pool via this specific resource annotation [1][2]. Key aspects of this functionality include: 1. Admission Validation: To use this feature, a pod must be in the Guaranteed QoS class [1][2][3]. The admission controller also verifies that the pod is deployed in a namespace explicitly allowed to use mixed-CPU workloads, typically indicated by the annotation workload.mixedcpus.openshift.io/allowed [2][3]. 2. Runtime Hinting: Because container runtimes (like CRI-O) are not natively aware of extended OpenShift resources, the admission controller injects an annotation (e.g., cpu-shared.crio.io/) into the pod definition [2][3]. This annotation informs the runtime that the specific container has been granted access to shared CPUs [2]. 3. Constraint: A pod can request no more than a single instance of the workload.openshift.io/enable-shared-cpus resource [1][3]. This feature is distinct from standard workload partitioning (enabled via cpuPartitioningMode: AllNodes), which separates cluster management pods into a reserved CPU pool while isolating customer workloads on separate cores [5][6][7]. The Mixed-CPUs feature specifically addresses the flexibility for customer workloads that require both isolation and access to shared resources within a Guaranteed QoS context [1][3].
Citations:
- 1: CNF-8809: admission: add new admission for handling shared cpus request kubernetes#1799
- 2: openshift/kubernetes@0032151
- 3: openshift/kubernetes@ac7efb4
- 4: https://andreaskaris.github.io/blog/openshift/cpu-isolation-in-openshift/
- 5: https://docs.okd.io/latest/scalability_and_performance/enabling-workload-partitioning.html
- 6: https://docs.redhat.com/en/documentation/openshift_container_platform/4.21/html/scalability_and_performance/enabling-workload-partitioning
- 7: https://docs.redhat.com/en/documentation/openshift_container_platform/4.17/html/scalability_and_performance/enabling-workload-partitioning
Guard the two-isolated-CPU requirement.
These workloads request two whole CPUs. The shared-CPU resource only grants shared-CPU access. setup does not check isolated CPU capacity when mixed CPUs are already enabled. Skip the workloads or provision at least two isolated CPUs before creating them.
📍 Affects 1 file
test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go#L149-L149(this comment)test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go#L181-L181test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go#L201-L201test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go#L218-L218test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go#L249-L249test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go#L325-L325test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go#L437-L437
🤖 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/11_mixedcpus/mixedcpus.go` at line 149,
Update setup in test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go
to verify at least two isolated CPUs are available when mixed CPUs are enabled,
and skip these workloads when they are not; alternatively provision that
capacity before creation. Apply the protection to the two-CPU workload
definitions at lines 149, 181, 201, 218, 249, 325, and 437, ensuring shared-CPU
resources are not treated as isolated CPU capacity.
|
|
||
| coreSiblings, err := nodes.GetCoreSiblings(ctx, workerRTNode) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| // When Shared already has 1 CPU and we need 2, we replace Shared with a new pair from |
There was a problem hiding this comment.
But with the new logic we always have 2 shared CPUs by default, so why the comment says
"When Shared already has 1 CPU and we need 2"?
Besides the wrong comment, we don't even need to change anything, since now 2 CPUs is the default.
2 participants
Split into 3 commits by bug pattern — each fixes a distinct class of broken assertion across different test files.
Expect(cpuSet.Equals(other))without a matcher is a Gomega no-op — always passes.Added
.To(BeTrue())with expected/got CPU sets in the failure message.Expect(client.Get(...))without a matcher silently swallows errors →Expect(...Get...).To(Succeed())(3 sites).cpu:1→cpu:2, and remove the setup path that setfull-pcpus-only=falseon small nodes. With default SMT alignment,cpu:1cannot schedule; CI has enough CPUs forcpu:2, so the cancel-SMT workaround is unnecessary.if ok { testlog.Info(...) }afterregexp.MatchStringnever failed when the expected error was missing.Replaced with
gexec.ExitafterWait, thenContainSubstringon leaf error text (not outer wraps / not regex).For reserved-count-too-high, assert dynamic
[1,%d]viamaxReservedCPUCountFromMustGather(TotalThreads-1).Also fix success-path "marshal" → "unmarshal" messages.
Summary by CodeRabbit