OCPEDGE-1036: fix: latency tuning for the rt-kernel tests on AWS metal#30790
OCPEDGE-1036: fix: latency tuning for the rt-kernel tests on AWS metal#30790jeff-roche wants to merge 2 commits intoopenshift:mainfrom
Conversation
…etal instances Signed-off-by: Jeff Roche <jeroche@redhat.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@jeff-roche: This pull request references OCPEDGE-1036 which is a valid jira issue. 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. |
|
/test ? |
openshift-ci
bot
added
the
approved
|
/test e2e-gcp-ovn-rt-upgrade |
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-upgrade-from-stable-4.21-e2e-metal-ovn-single-node-rt-upgrade |
|
@jeff-roche: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/1e7a2f80-0b96-11f1-9252-e810dd3e02ff-0 |
|
Scheduling required tests: |
|
The payload job fails to upgrade but the RT Tests themselves pass. Addressing the upgrade failures on #30608 |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jeff-roche, qJkee 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 |
|
/payload-job periodic-ci-openshift-release-main-ci-4.22-upgrade-from-stable-4.21-e2e-metal-ovn-single-node-rt-upgrade-test |
|
@jeff-roche: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/61996750-0c26-11f1-8539-63c794c57c62-0 |
|
/payload-job periodic-ci-openshift-release-main-ci-4.22-upgrade-from-stable-4.21-e2e-metal-ovn-single-node-rt-upgrade-test |
|
@jeff-roche: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/9cdf16c0-11a8-11f1-9f71-c9b6ff3ae133-0 |
|
/payload-job periodic-ci-openshift-release-main-ci-4.22-upgrade-from-stable-4.21-e2e-metal-ovn-single-node-rt-upgrade-test |
|
@jeff-roche: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/4d5afc30-1253-11f1-9df8-fe79311f410f-0 |
… artifacts Signed-off-by: Jeff Roche <jeroche@redhat.com>
|
New changes are detected. LGTM label has been removed. |
|
/payload-job periodic-ci-openshift-release-main-ci-4.22-upgrade-from-stable-4.21-e2e-metal-ovn-single-node-rt-upgrade-test |
WalkthroughThe changes refactor real-time test infrastructure in the kernel test suite. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
@jeff-roche: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/44998af0-130a-11f1-9082-40e95c4d00b8-0 |
|
@jeff-roche: This pull request references OCPEDGE-1036 which is a valid jira issue. 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. |
![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/extended/kernel/common.go (1)
64-68: Side effect in getter function modifies global state.
getRealTimeWorkerNodesmodifies the globalrtTestThresholdsmap, which is unexpected for a function with a "get" prefix. This couples threshold configuration to node discovery and makes the behavior harder to reason about.Consider either:
- Renaming the function to reflect it configures thresholds (e.g.,
setupRealTimeWorkerNodes)- Returning the metal status and handling threshold adjustment at the call site
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/kernel/common.go` around lines 64 - 68, getRealTimeWorkerNodes currently mutates the global rtTestThresholds map (when nodesAreMetal is false), which is a surprising side effect for a getter; stop modifying rtTestThresholds inside getRealTimeWorkerNodes and instead either (A) rename getRealTimeWorkerNodes to setupRealTimeWorkerNodes if you intend it to configure thresholds, or (B) change getRealTimeWorkerNodes to only return the metal status (bool nodesAreMetal) and move the rtTestThresholds adjustments out to the call site so callers can set rtTestThresholds[test] = 7500 when nodesAreMetal is false; update all callers of getRealTimeWorkerNodes accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/kernel/common.go`:
- Around line 57-68: The metal-detection currently uses node.GetLabels() and
sets nodesAreMetal = false if any worker node isn't metal, which incorrectly
flags clusters where non-RT workers are non-metal; update the logic so the metal
check is only performed for nodes that match the RT kernel condition (the same
condition used to select RT nodes) — i.e., inside the RT kernel match block
iterate those nodes, call node.GetLabels(), and only then modify nodesAreMetal
and adjust rtTestThresholds; reference variables/functions: node.GetLabels(),
nodesAreMetal, rtTestThresholds, and the RT kernel match condition so the
threshold padding runs only when RT nodes are detected as non-metal.
- Line 48: Replace the incorrect capacity argument on the nodes slice
allocation: the current call uses kubeNodes.Size() (which returns protobuf
serialized size) when constructing nodes via make([]string, 0, ...); change it
to use the number of items with len(kubeNodes.Items) so nodes = make([]string,
0, len(kubeNodes.Items)). Update the allocation site that references kubeNodes
and the nodes variable in test/extended/kernel/common.go (search for the
make([]string, 0, kubeNodes.Size()) occurrence).
In `@test/extended/kernel/tools.go`:
- Around line 165-167: The error message in runCyclictest incorrectly references
"oslat test"; update the returned fmt.Errorf string in the runCyclictest
function (where cpuCount is checked) to reference "cyclictest" (or
"runCyclictest") instead and preserve the numeric cpuCount interpolation and
wording; ensure only the test name in the message is changed so the check using
cpuCount and the fmt.Errorf call remain otherwise identical.
---
Nitpick comments:
In `@test/extended/kernel/common.go`:
- Around line 64-68: getRealTimeWorkerNodes currently mutates the global
rtTestThresholds map (when nodesAreMetal is false), which is a surprising side
effect for a getter; stop modifying rtTestThresholds inside
getRealTimeWorkerNodes and instead either (A) rename getRealTimeWorkerNodes to
setupRealTimeWorkerNodes if you intend it to configure thresholds, or (B) change
getRealTimeWorkerNodes to only return the metal status (bool nodesAreMetal) and
move the rtTestThresholds adjustments out to the call site so callers can set
rtTestThresholds[test] = 7500 when nodesAreMetal is false; update all callers of
getRealTimeWorkerNodes accordingly.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
test/extended/kernel/common.gotest/extended/kernel/tools.go
| return nodes, err | ||
| } | ||
|
|
||
| nodes = make([]string, 0, kubeNodes.Size()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n test/extended/kernel/common.go | head -75Repository: openshift/origin
Length of output: 2832
🏁 Script executed:
rg "Size\(" test/extended/kernel/common.go -B 2 -A 2Repository: openshift/origin
Length of output: 132
🏁 Script executed:
rg "kubeNodes\s*:=" test/extended/kernel/common.go -B 5 -A 5Repository: openshift/origin
Length of output: 42
🏁 Script executed:
rg "NodeList" test/extended/kernel/common.goRepository: openshift/origin
Length of output: 42
🏁 Script executed:
rg "\.Size\(\)" --type go -B 2 -A 2 | head -50Repository: openshift/origin
Length of output: 3721
🌐 Web query:
Kubernetes v1.NodeList Size() method protobuf
💡 Result:
In Kubernetes’ Go API types (k8s.io/api/core/v1), NodeList.Size() (n int) is not a method from “standard” google.golang.org/protobuf—it’s a generated fast-path protobuf helper (historically via gogo/protobuf-style codegen) that returns the number of bytes the object would take when encoded in protobuf wire format, and is used by the generated Marshal* functions (e.g., MarshalToSizedBuffer). [1]
If you want the protobuf-encoding size in a generator-agnostic way, the canonical API is proto.Size(m) (which computes the wire size by walking fields). [2]
Also note: because a Size() method is generated, protobuf generators may rename any protobuf field named size (e.g., to Size_) to avoid colliding with that generated method. [3]
Sources:
[1] k8s.io/api/core/v1 package docs showing NodeList has Marshal* and Size() methods
[2] Protobuf Go “Size Semantics” (proto.Size) documentation
[3] gogo/protobuf / protoc-gen-gogo collision note (field size vs generated Size() method)
Fix: Replace kubeNodes.Size() with len(kubeNodes.Items) for correct capacity.
The Size() method on protobuf-generated types like NodeList returns the serialized wire format size in bytes, not the number of items. Use len(kubeNodes.Items) instead to allocate the slice with correct capacity.
Proposed fix
- nodes = make([]string, 0, kubeNodes.Size())
+ nodes = make([]string, 0, len(kubeNodes.Items))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| nodes = make([]string, 0, kubeNodes.Size()) | |
| nodes = make([]string, 0, len(kubeNodes.Items)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/kernel/common.go` at line 48, Replace the incorrect capacity
argument on the nodes slice allocation: the current call uses kubeNodes.Size()
(which returns protobuf serialized size) when constructing nodes via
make([]string, 0, ...); change it to use the number of items with
len(kubeNodes.Items) so nodes = make([]string, 0, len(kubeNodes.Items)). Update
the allocation site that references kubeNodes and the nodes variable in
test/extended/kernel/common.go (search for the make([]string, 0,
kubeNodes.Size()) occurrence).
| nodeLabels := node.GetLabels() | ||
| if !strings.Contains(nodeLabels["node.kubernetes.io/instance-type"], "metal") { | ||
| nodesAreMetal = false | ||
| } | ||
| } | ||
|
|
||
| // Pad the latencies for non-metal instances | ||
| if !nodesAreMetal { | ||
| e2e.Logf("One or more nodes are not a metal instance, setting all real-time test thresholds to 7500 usec") | ||
| for test := range rtTestThresholds { | ||
| rtTestThresholds[test] = 7500 // usec | ||
| } |
There was a problem hiding this comment.
Metal detection checks all worker nodes, not just RT nodes.
The metal detection logic iterates over all worker nodes and sets nodesAreMetal = false if any worker is non-metal—including non-RT workers. If a cluster has RT metal nodes alongside non-RT non-metal nodes, thresholds will be incorrectly raised to 7500µs.
Consider moving the metal check inside the RT kernel match condition.
🐛 Proposed fix to check metal status only for RT nodes
for _, node := range kubeNodes.Items {
if realTimeKernelRE.MatchString(node.Status.NodeInfo.KernelVersion) {
nodes = append(nodes, node.Name)
- }
- nodeLabels := node.GetLabels()
- if !strings.Contains(nodeLabels["node.kubernetes.io/instance-type"], "metal") {
- nodesAreMetal = false
+ nodeLabels := node.GetLabels()
+ if !strings.Contains(nodeLabels["node.kubernetes.io/instance-type"], "metal") {
+ nodesAreMetal = false
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/kernel/common.go` around lines 57 - 68, The metal-detection
currently uses node.GetLabels() and sets nodesAreMetal = false if any worker
node isn't metal, which incorrectly flags clusters where non-RT workers are
non-metal; update the logic so the metal check is only performed for nodes that
match the RT kernel condition (the same condition used to select RT nodes) —
i.e., inside the RT kernel match block iterate those nodes, call
node.GetLabels(), and only then modify nodesAreMetal and adjust
rtTestThresholds; reference variables/functions: node.GetLabels(),
nodesAreMetal, rtTestThresholds, and the RT kernel match condition so the
threshold padding runs only when RT nodes are detected as non-metal.
| if cpuCount <= 4 { | ||
| return fmt.Errorf("more than 4 cores are required to run this oslat test. Found %d cores", cpuCount) | ||
| } |
There was a problem hiding this comment.
Copy-paste error: wrong test name in error message.
The error message references "oslat test" but this is the runCyclictest function.
🐛 Proposed fix
- return fmt.Errorf("more than 4 cores are required to run this oslat test. Found %d cores", cpuCount)
+ return fmt.Errorf("more than 4 cores are required to run this cyclictest. Found %d cores", cpuCount)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if cpuCount <= 4 { | |
| return fmt.Errorf("more than 4 cores are required to run this oslat test. Found %d cores", cpuCount) | |
| } | |
| if cpuCount <= 4 { | |
| return fmt.Errorf("more than 4 cores are required to run this cyclictest. Found %d cores", cpuCount) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/kernel/tools.go` around lines 165 - 167, The error message in
runCyclictest incorrectly references "oslat test"; update the returned
fmt.Errorf string in the runCyclictest function (where cpuCount is checked) to
reference "cyclictest" (or "runCyclictest") instead and preserve the numeric
cpuCount interpolation and wording; ensure only the test name in the message is
changed so the check using cpuCount and the fmt.Errorf call remain otherwise
identical.
|
Scheduling required tests: |
3 participants
Summary by CodeRabbit