[release-4.21] OCPBUGS-100540: e2e: fix broken checks and rework netqueue tests to avoid ARM ethtool blackout flakes - #1573
Open
openshift-cherrypick-robot wants to merge 5 commits into
Conversation
…n netqueue suite
strings.ContainsAny(s, chars) reports whether ANY character in chars
appears in s -- it does not check for a substring. Since device is a NIC
name like "enP2s2f0np0", this check was true for almost any tuned output
containing common letters, regardless of whether that specific device name
actually appeared in devices_udev_regex. Replace with strings.Contains in
the 4 tests that assert tuned picked up the configured device filter:
test_id 40543, 40545, 72051, 40668.
Every netqueue test body was wrapped in a guard like:
if profile.Spec.Net.UserLevelNetworking != nil && *ULN && len(Devices) == 0 { ... }
This guard was true in the common case only because BeforeEach
unconditionally set Net whenever it was nil, and AfterEach unconditionally
reverted to the initial profile after every test. If the *initial* cluster
profile already had a non-nil Net with ULN=false or leftover Devices, the
guard evaluated false and the entire test body was skipped -- the test
reported PASS while verifying nothing. Guards removed from test_id 40308,
40543, 40545, 72051, and 40668.
Once the guard is gone, test_id 40308 and 40542 are identical bodies
(both call checkDeviceSetWithReservedCPU unconditionally). Dropping 40542
as a duplicate; 40308 covers the same case.
getReservedCPUSize re-parsed profile.Spec.CPU.Reserved into a cpuset on
every 5s poll, even though the value is static for the
whole test run. Compute it once as reservedCPUCount in the BeforeAll and
thread it through instead: checkDeviceSetWithReservedCPU's signature
changes from taking the whole *performancev2.PerformanceProfile to taking
reservedCPUCount int directly. getReservedCPUSize is removed.
AI Attribution: AIA Human-AI blend, Content edits, New content,
Human-initiated, Reviewed, opus 4.6 high v1.0
…BeforeAll/AfterAll pass - BeforeEach (enable ULN) and AfterEach (revert to initial) never isolated tests: both call profiles.UpdateWithRetry with no wait after, so the next test's own update fires before tuned reconciles the previous one. - Example: test A AfterEach reverts the profile, but test B immediately calls UpdateWithRetry with its own Devices before that revert lands -- B can still see A leftover devices_udev_regex even though "cleanup" ran in between. - No test actually needs a clean starting profile: each one fully overwrites profile.Spec.Net itself or polls until its own expected state converges. The per-test revert bought nothing. - Replaced with one BeforeAll (set baseline once) and one AfterAll (revert once, only if state changed). Side effect: total profile updates per run drop from 10-16 to 5-6.
…tead of per-test rediscovery - checkDeviceSupport was called independently by every test, re-running ethtool discovery over the tuned pod each time. checkDeviceSetWithReservedCPU then polled that same rediscovery every 5s while waiting for convergence. A slow update and a broken feature produced the same timeout -> Skip. - Split checkDeviceSupport into discoverMultiQueueNICs (per-node NIC enumeration, now via nodes.GetNodeInterfaces/node_inspector instead of tuned-pod exec) and getCombinedChannels (single NIC -> combined channel count). One does discovery, one does the ethtool read; each is reusable on its own. - discoverMultiQueueNICs runs once in BeforeAll into baselineMultiQueueNICs (map[string]map[nodes.NodeInterface]int); BeforeAll Skips the whole suite upfront if none are found. Tests read from this baseline instead of rediscovering. - checkDeviceSetWithReservedCPU -> waitForNICsToMatchReservedCPU: no longer mutates a caller map or rediscovers, just polls the baseline. Also fixes a real bug -- the old function returned success on the first NIC that matched reservedCPUCount, even for tests asserting all supported NICs converge. - getRandomNodeDevice updated for the new map type, returns a NodeInterface instead of a bare string. Drops its defensive empty-name check -- discoverMultiQueueNICs never inserts zero-value entries, unlike the old checkDeviceSupport. - 40543/40545/72051/40668 now pull their target device from the baseline instead of calling checkDeviceSupport themselves. profile updates use `var err error` explicitly to avoid re-shadowing the Describe-scoped profile var now that device is a struct, not a string, everywhere. AI Attribution: AIA Human-AI blend, Content edits, New content, Human-initiated, Reviewed, opus 4.6 high v1.0
…line
- checkDeviceSupport split into discoverMultiQueueNICs (enumeration) and
getCombinedChannels (per-NIC ethtool parsing). Discovery now runs once in
BeforeAll into baselineMultiQueueNICs, Skipping the whole suite upfront if
none are found, instead of each test re-discovering and skipping
individually.
- checkDeviceSetWithReservedCPU -> waitForNICsToMatchReservedCPU: takes a
NIC map to poll instead of rediscovering. Also fixes a bug where it
returned success on the first matching NIC instead of waiting for all of
them to converge.
- getRandomNodeDevice: updated for the new map type, returns a
NodeInterface, drops its now-unneeded empty-name guard.
- Each test builds its own scoped NIC map (targetNICs/matchedNICs/
expectedNICs) instead of polling the full baseline, so convergence checks
validate only the NICs that test actually touches. Per-test
checkDeviceSupport+Skip guards removed, BeforeAll already guarantees them.
- Skip-on-error replaced with Expect(err).ToNot(HaveOccurred()) throughout.
- profile.Spec.Net setup now only sets .Devices (UserLevelNetworking=true
comes from the BeforeAll baseline).
- var err error added before some profile fetches so profile, err = ...
assigns the Describe-scoped var instead of shadowing it.
- nodes.GetByName moved before scoped-map-building/profile update in every
test, to keep the exec call out of the post-update ethtool blackout
window.
- 40545: wildcard check now searches for the derived udev-regex form
("iface.*") instead of the literal device name, since tuned stores
wildcards as regex in devices_udev_regex.
- 72051: pick device only from nodes with >=2 NICs, a single-NIC node can't
demonstrate negation. Restored the negative assertion (pre/post
combined-channel comparison on the negated NIC) lost when
checkDeviceSupport was removed. Tuned-config check now searches for the
negated pattern ("!iface") instead of the bare device name, to avoid a
false positive from a leftover config.
- 40668: tuned-config check upgraded to a 3-way match (interface name +
vendor ID + device ID); it previously only checked the interface name.
AI Attribution: AIA Human-AI blend, Content edits, New content,
Human-initiated, Reviewed, opus 4.6 high v1.0
…ests
- Converted remaining plain `//` comments to By() calls; unified the 4
tests' tuned-config-check block (identical By text, no blank lines) --
they run the same grep, differing only in what they search for.
- Added a By() before each test's final convergence check (previously
unlabeled); all now end in "converged to reserved CPU count".
- Added testlog.Infof after each getRandomNodeDevice call, logging which
NIC/node the run picked.
- 40545/40668: added missing By("Updating the performance profile"),
matching 40543. 72051: split its stale "Enable ULN..." By into the same
two-step shape as the other tests.
- 40668: removed a redundant duplicate nodes.GetByName call.
- AfterAll now logs the profile diff before reverting, mirroring
BeforeAll's equivalent branch.
- Added a top-of-file comment on the ARM ethtool -L blackout window, and
a doc comment on getRandomNodeDevice explaining its random-pick trick.
AI Attribution: AIA Human-AI blend, Content edits, New content,
Human-initiated, Reviewed, opus 4.6 high v1.0
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
PR Summary by QodoFix netqueues e2e assertions and reduce ARM ethtool blackout flakes
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Contributor
|
@openshift-cherrypick-robot: Jira Issue OCPBUGS-99290 has been cloned as Jira Issue OCPBUGS-100540. Will retitle bug to link to clone. 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. |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
jira/valid-reference
Contributor
|
@openshift-cherrypick-robot: This pull request references Jira Issue OCPBUGS-100540, which is valid. The bug has been moved to the POST state. 7 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. |
Code Review by Qodo
1. Negated NIC check wrong
|
![qodo-for-rh-openshift[bot]](https://avatars.githubusercontent.com/u/792337?s=60&v=4){kind=small}
Comment on lines
+275
to
+276
| return strings.Contains(string(out), devicePattern) | ||
| }, cluster.ComputeTestTimeout(2*time.Minute, RunningOnSingleNode), 5*time.Second).Should(BeTrue(), "tuned config does not contain %q", devicePattern) |
There was a problem hiding this comment.
1. Negated nic check wrong 🐞 Bug ≡ Correctness
The negative-match netqueues test asserts that tuned.conf contains the literal device pattern "!<iface>", but TuneD renders negated interface filters as a negative-lookahead regex "^INTERFACE=(?!<iface>)". This makes the Eventually() condition never become true and the test will time out/fail even when the profile is correctly applied.
Agent Prompt
## Issue description
The negative interface match test builds `devicePattern := "!" + device.Name` and then checks `grep devices_udev_regex ...` output contains that literal string. However, the TuneD generator strips the `!` and emits a negative-lookahead regex like `^INTERFACE=(?!<iface>)`, so the string containment check will never match.
## Issue Context
TuneD net device regex rendering for negated interface names is implemented in the tuned component by translating `!ens5` into `^INTERFACE=(?!ens5)` (and `*` into `.*`). The e2e test should assert against this rendered form (e.g., contain `"^INTERFACE=(?!"+device.Name+")"` or at least `"(?!"+device.Name+")"`), not the raw `!` pattern.
## Fix Focus Areas
- test/e2e/performanceprofile/functests/1_performance/netqueues.go[228-276]
- pkg/performanceprofile/controller/performanceprofile/components/tuned/tuned.go[173-180]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
|
/retest |
1 similar comment
Member
|
/retest |
Contributor
|
@openshift-cherrypick-robot: 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. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: openshift-cherrypick-robot, Tal-or 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 |
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
4 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.
This is an automated cherry-pick of #1568
/assign oblau