OCPBUGS-98060 BUG-FIX Latency Test - #1566
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tavital 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 |
|
Hi @tavital. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe latency e2e tests now use centralized environment configuration, performance-profile and worker-node helpers, and a shared pod runner. Setup-delay configuration is documented and covered by valid and invalid environment tests. ChangesLatency test execution and configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LatencyTest
participant Config
participant Cluster
participant TestPod
participant Kubernetes
LatencyTest->>Config: Load latency environment
LatencyTest->>Cluster: Load profile and select worker
Cluster-->>LatencyTest: Compatible profile and node
LatencyTest->>TestPod: Define runner pod
TestPod->>Kubernetes: Create and monitor pod
Kubernetes-->>TestPod: Running and Succeeded phases
TestPod-->>LatencyTest: Logs and latency result
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 4 warnings)
✅ Passed checks (9 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 |
tavital
changed the title
tavital
changed the title
openshift-ci-robot
added
jira/severity-moderate
|
@tavital: This pull request references Jira Issue OCPBUGS-98060, which is invalid:
Comment 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. |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/performanceprofile/functests/4_latency/latency.go (1)
524-537: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWait 2/4 predicate ignores terminal phases; can spin the full pod-start budget on fast or crashing pods.
The predicate only returns
trueforPodRunning. If the pod transitions straight through toSucceeded/Failedbetween 1s polls (shortLATENCY_TEST_RUN_TIMEOUT, or an immediate container crash), this wait never succeeds and burns the entirelatencyTestPodStartTimeoutbudget before failing with a misleading "did not reach Running" message, rather than detecting the terminal state promptly.🐛 Proposed fix to also short-circuit on terminal phases
currentPod, err := pods.WaitForPredicate(context.TODO(), client.ObjectKeyFromObject(testPod), latencyTestPodStartTimeout, func(pod *corev1.Pod) (bool, error) { - if pod.Status.Phase == corev1.PodRunning { - return true, nil - } - return false, nil + switch pod.Status.Phase { + case corev1.PodRunning, corev1.PodSucceeded, corev1.PodFailed: + return true, nil + default: + return false, nil + } })🤖 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/4_latency/latency.go` around lines 524 - 537, Update the WaitForPredicate callback in the wait 2/4 flow to return promptly with an appropriate error when pod.Status.Phase is corev1.PodSucceeded or corev1.PodFailed, while retaining successful completion for corev1.PodRunning and continued polling for non-terminal phases. Ensure the resulting failure identifies the observed terminal phase instead of waiting for the full latencyTestPodStartTimeout.
🧹 Nitpick comments (1)
test/e2e/performanceprofile/functests/4_latency/latency.go (1)
587-641: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared helper for these four near-identical timeout parsers.
getLatencyTestImagePullTimeout,getLatencyTestPodStartTimeout,getLatencyTestStartupTimeout, andgetLatencyTestExitTimeoutdiffer only by env var name and default duration — same lookup,Atoi, bounds check, and error text pattern repeated four times.♻️ Proposed helper
func getPositiveDurationEnv(envName string, defaultVal time.Duration) (time.Duration, error) { v, ok := os.LookupEnv(envName) if !ok { return defaultVal, nil } sec, err := strconv.Atoi(v) if err != nil { return 0, fmt.Errorf("the environment variable %s has incorrect value %q, it must be a positive integer: %w", envName, v, err) } if sec < 1 || sec > math.MaxInt32 { return 0, fmt.Errorf("the environment variable %s has an invalid number %q, it must be a positive integer", envName, v) } return time.Duration(sec) * time.Second, nil }Each of the four functions then becomes a one-liner, e.g.
return getPositiveDurationEnv("LATENCY_TEST_IMAGE_PULL_TIMEOUT", 2*time.Minute).🤖 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/4_latency/latency.go` around lines 587 - 641, Extract the shared parsing logic from getLatencyTestImagePullTimeout, getLatencyTestPodStartTimeout, getLatencyTestStartupTimeout, and getLatencyTestExitTimeout into a getPositiveDurationEnv helper accepting the environment variable name and default duration. Preserve the existing lookup, integer parsing, positive/range validation, error wording, and seconds-to-duration conversion; reduce each timeout function to delegating with its current environment key and default.
🤖 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/4_latency/latency.go`:
- Around line 539-547: Update the latency test flow around the existing
latencyTestRunTimeout parsing to call strconv.Atoi once, retain and validate its
error, and reuse the parsed runtime value for the CPU-check condition and later
logic. Remove the duplicate parse and any discarded error handling while
preserving the current behavior for valid timeout values.
---
Outside diff comments:
In `@test/e2e/performanceprofile/functests/4_latency/latency.go`:
- Around line 524-537: Update the WaitForPredicate callback in the wait 2/4 flow
to return promptly with an appropriate error when pod.Status.Phase is
corev1.PodSucceeded or corev1.PodFailed, while retaining successful completion
for corev1.PodRunning and continued polling for non-terminal phases. Ensure the
resulting failure identifies the observed terminal phase instead of waiting for
the full latencyTestPodStartTimeout.
---
Nitpick comments:
In `@test/e2e/performanceprofile/functests/4_latency/latency.go`:
- Around line 587-641: Extract the shared parsing logic from
getLatencyTestImagePullTimeout, getLatencyTestPodStartTimeout,
getLatencyTestStartupTimeout, and getLatencyTestExitTimeout into a
getPositiveDurationEnv helper accepting the environment variable name and
default duration. Preserve the existing lookup, integer parsing, positive/range
validation, error wording, and seconds-to-duration conversion; reduce each
timeout function to delegating with its current environment key and default.
🪄 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: 179ead0f-85b9-40e0-82f0-68c4aa5a7fa3
📒 Files selected for processing (3)
docs/performanceprofile/performance_controller.mdtest/e2e/performanceprofile/functests/4_latency/latency.gotest/e2e/performanceprofile/functests/5_latency_testing/latency_testing.go
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 `@docs/performanceprofile/performance_controller.md`:
- Around line 115-119: Expand the environment-variable list in the performance
controller documentation to include LATENCY_TEST_STARTUP_TIMEOUT,
LATENCY_TEST_EXIT_TIMEOUT, and the image-pull timeout using its exact supported
variable name. Briefly describe each variable’s purpose and timeout units,
preserving the existing entries and documenting all staged readiness controls.
🪄 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: 786814fc-cd24-4058-b734-1a7fe88b4fbc
📒 Files selected for processing (3)
docs/performanceprofile/performance_controller.mdtest/e2e/performanceprofile/functests/4_latency/latency.gotest/e2e/performanceprofile/functests/5_latency_testing/latency_testing.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/performanceprofile/functests/5_latency_testing/latency_testing.go
- test/e2e/performanceprofile/functests/4_latency/latency.go
| You can run the container with different ENV variables, but the bare minimum is to pass | ||
| `KUBECONFIG` mount and ENV variable, to give to the test access to the cluster. | ||
|
|
||
| - `LATENCY_TEST_DELAY` indicates an (optional) delay in seconds to be used between the container is created and the tests actually start. Default is zero (start immediately). | ||
| - `LATENCY_TEST_RUNTIME` the amount of time in seconds that the latency test should run. | ||
| - `LATENCY_TEST_DELAY_TIMEOUT` indicates an (optional) delay in seconds to be used between the container is created and the tests actually start. Default is zero (start immediately). | ||
| - `LATENCY_TEST_RUN_TIMEOUT` the amount of time in seconds that the latency test should run. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the other latency timeout controls.
This list now covers only LATENCY_TEST_DELAY_TIMEOUT and LATENCY_TEST_RUN_TIMEOUT, but the test harness also supports LATENCY_TEST_STARTUP_TIMEOUT and LATENCY_TEST_EXIT_TIMEOUT; the new image-pull timeout should be documented as well using its exact environment-variable name. Otherwise, operators cannot discover or configure all stages of the staged readiness workflow.
🤖 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 `@docs/performanceprofile/performance_controller.md` around lines 115 - 119,
Expand the environment-variable list in the performance controller documentation
to include LATENCY_TEST_STARTUP_TIMEOUT, LATENCY_TEST_EXIT_TIMEOUT, and the
image-pull timeout using its exact supported variable name. Briefly describe
each variable’s purpose and timeout units, preserving the existing entries and
documenting all staged readiness controls.
There was a problem hiding this comment.
Nice work.
I left comments inside, but as a general rule we cannot remove existing API, and exposing few more fields will make the UX (which is already not great) more complex.
Maybe it worth consider to run the pod in a high level object such as deployment/cronjob? maybe those object will provide richer status about the pod progress that we can expose to the user? I'm genuinely asking, I never tried this approach before.
| // LATENCY_TEST_IMAGE_PULL_TIMEOUT: seconds to wait for the image to be ready (Pulled); default 120 | ||
| // LATENCY_TEST_POD_START_TIMEOUT: seconds to wait for schedule + container start → Running (after image is ready); default 120 | ||
| // LATENCY_TEST_STARTUP_TIMEOUT: seconds after Running (excluding LATENCY_TEST_DELAY_TIMEOUT) to wait for the tool to start (default 600) | ||
| // LATENCY_TEST_EXIT_TIMEOUT: seconds beyond LATENCY_TEST_RUN_TIMEOUT to wait for Succeeded (default 120) |
There was a problem hiding this comment.
This bunch of new ENV variables will complicated the API we're exposing to the users.
We're striving to simplify, i.e minimizing the learning curve for the users for them to run the tests properly.
There was a problem hiding this comment.
I have a solution for that I do understand that those four new timeout ENVs is too much surface for customers.
I have an idea:
I’ll keep the four separate waits (that’s what fixes the shared-120s bug), but drop the four public ENVs:
LATENCY_TEST_IMAGE_PULL_TIMEOUT
LATENCY_TEST_POD_START_TIMEOUT
LATENCY_TEST_STARTUP_TIMEOUT
LATENCY_TEST_EXIT_TIMEOUT
Instead I can hardcode default per-phase bases in code (used when no buffer is set):
| Phase | Default base |
|---|---|
| Image pull (wait 1) | 120s |
| Pod start → Running (wait 2) | 120s |
| Startup / init / CPU detect (wait 3) | 300s |
| Exit → Succeeded (wait 4) | 120s |
Add a single optional env: LATENCY_TEST_TIMEOUT_BUFFER (default 0).
Each phase budget is: default_base + LATENCY_TEST_TIMEOUT_BUFFER.
Wait budgets become:
wait 1: 120 + LATENCY_TEST_TIMEOUT_BUFFER
wait 2: 120 + LATENCY_TEST_TIMEOUT_BUFFER
wait 3: 300 + LATENCY_TEST_TIMEOUT_BUFFER + LATENCY_TEST_DELAY
wait 4: LATENCY_TEST_RUNTIME + 120 + LATENCY_TEST_TIMEOUT_BUFFER
Customers still use LATENCY_TEST_RUNTIME and LATENCY_TEST_DELAY as today,
and LATENCY_TEST_TIMEOUT_BUFFER will be used when the cluster needs extra slack.
What do you think?
| latencyTestMemory = defaultTestMemory | ||
| ) | ||
|
|
||
| // LATENCY_TEST_DELAY delay the run of the binary, can be useful to give time to the CPU manager reconcile loop |
There was a problem hiding this comment.
we may expand the LATENCY_TEST_DELAY to include the time it should take for the test to pull the image.
There was a problem hiding this comment.
also since this ENV behaves as a "stable" API we can't simply remove it.
this will require changing in our official docs/scripts and also users already count on this variable name, so unless there's a really really good reason to change the name, that's a no go.
There was a problem hiding this comment.
also since this ENV behaves as a "stable" API we can't simply remove it. this will require changing in our official docs/scripts and also users already count on this variable name, so unless there's a really really good reason to change the name, that's a no go.
ENV var changed back to original - LATENCY_TEST_DELAY
There was a problem hiding this comment.
we may expand the
LATENCY_TEST_DELAYto include the time it should take for the test to pull the image.
LATENCY_TEST_DELAY makes the runner sleep for the given time so the kubelet CPU manager can finish assigning/pinning exclusive CPUs to the latency pod before oslat / cyclictest / hwlatdetect start measuring.
I don’t think mixing LATENCY_TEST_DELAY with LATENCY_TEST_IMAGE_PULL_TIMEOUT is a good idea as they serve different roles.
What do you think?
| // LATENCY_TEST_DELAY delay the run of the binary, can be useful to give time to the CPU manager reconcile loop | ||
| // LATENCY_TEST_DELAY_TIMEOUT delay the run of the binary, can be useful to give time to the CPU manager reconcile loop | ||
| // to update the default CPU pool | ||
| // LATENCY_TEST_RUNTIME: the amount of time in seconds that the latency test should run |
|
/cc |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@tavital: This pull request references Jira Issue OCPBUGS-98060, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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-robot
removed
the
jira/invalid-bug
|
@tavital: This pull request references Jira Issue OCPBUGS-98060, which is valid. 3 validation(s) were run on this bug
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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/e2e/performanceprofile/functests/4_latency/latency.go (2)
527-536: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicates
pods.WaitForPhaselogic.The custom predicate here (
pod.Status.Phase == corev1.PodRunning) is exactly whatpods.WaitForPhasealready implements, and is used verbatim for wait 4/4 below. Using the existing helper here too keeps both stages consistent and removes the bespoke closure.♻️ Proposed simplification
- currentPod, err := pods.WaitForPredicate(context.TODO(), client.ObjectKeyFromObject(testPod), latencyTestPodStartTimeout, func(pod *corev1.Pod) (bool, error) { - if pod.Status.Phase == corev1.PodRunning { - return true, nil - } - return false, nil - }) + currentPod, err := pods.WaitForPhase(context.TODO(), client.ObjectKeyFromObject(testPod), corev1.PodRunning, latencyTestPodStartTimeout)🤖 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/4_latency/latency.go` around lines 527 - 536, Replace the custom predicate in the currentPod wait with the existing pods.WaitForPhase helper, passing the same context, testPod key, latencyTestPodStartTimeout, and corev1.PodRunning phase. Preserve the existing error logging and assertion behavior.
587-641: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFour near-identical timeout getters — extract a shared parser.
getLatencyTestImagePullTimeout,getLatencyTestPodStartTimeout,getLatencyTestStartupTimeout, andgetLatencyTestExitTimeoutrepeat the same lookup/parse/bounds-check/convert logic, differing only in env var name, error text, and default. A shared helper would reduce duplication and make adding future timeout knobs less error-prone.♻️ Proposed shared helper
func getDurationEnv(envName string, defaultVal time.Duration) (time.Duration, error) { v, ok := os.LookupEnv(envName) if !ok { return defaultVal, nil } sec, err := strconv.Atoi(v) if err != nil { return 0, fmt.Errorf("the environment variable %s has incorrect value %q, it must be a positive integer: %w", envName, v, err) } if sec < 1 || sec > math.MaxInt32 { return 0, fmt.Errorf("the environment variable %s has an invalid number %q, it must be a positive integer", envName, v) } return time.Duration(sec) * time.Second, nil } func getLatencyTestImagePullTimeout() (time.Duration, error) { return getDurationEnv("LATENCY_TEST_IMAGE_PULL_TIMEOUT", 2*time.Minute) }🤖 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/4_latency/latency.go` around lines 587 - 641, Extract the duplicated environment-variable parsing, validation, and duration conversion from getLatencyTestImagePullTimeout, getLatencyTestPodStartTimeout, getLatencyTestStartupTimeout, and getLatencyTestExitTimeout into a shared getDurationEnv helper accepting the variable name and default duration. Preserve the existing positive-integer and math.MaxInt32 validation, error context, and each getter’s current default by delegating to the helper.
🤖 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/4_latency/latency.go`:
- Around line 468-491: Update trackImagePullProgress to log the error returned
by events.GetEventsForObject before returning, using the existing test logging
mechanism and including enough context to identify the pod or image-pull
progress failure. Preserve the current return behavior after logging.
---
Nitpick comments:
In `@test/e2e/performanceprofile/functests/4_latency/latency.go`:
- Around line 527-536: Replace the custom predicate in the currentPod wait with
the existing pods.WaitForPhase helper, passing the same context, testPod key,
latencyTestPodStartTimeout, and corev1.PodRunning phase. Preserve the existing
error logging and assertion behavior.
- Around line 587-641: Extract the duplicated environment-variable parsing,
validation, and duration conversion from getLatencyTestImagePullTimeout,
getLatencyTestPodStartTimeout, getLatencyTestStartupTimeout, and
getLatencyTestExitTimeout into a shared getDurationEnv helper accepting the
variable name and default duration. Preserve the existing positive-integer and
math.MaxInt32 validation, error context, and each getter’s current default by
delegating to the helper.
🪄 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: 257ebd5d-2fa4-4738-93a5-6615d36dc4f6
📒 Files selected for processing (1)
test/e2e/performanceprofile/functests/4_latency/latency.go
|
Thanks for the PR. In addition to checking CI issues, coderabbit review, please make sure to write meaningful, albeit possibly short, commit messages describing the changes |
tavital
changed the title
openshift-ci-robot
removed
jira/severity-moderate
|
@tavital: No Jira issue is referenced in the title of this pull request. 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-robot
removed
the
jira/valid-bug
Tal-or
left a comment
Tal-or
left a comment
There was a problem hiding this comment.
Please squash the commits together when resubmit.
Also, maybe it worth consider to run the pod in a high level object such as deployment/cronjob? maybe those objects will provide richer status about the pod progress that we can expose to the user? that could be an option instead of filtering specific events for the pod.
| // LATENCY_TEST_IMAGE_PULL_TIMEOUT: seconds to wait for the image to be ready (Pulled); default 120 | ||
| // LATENCY_TEST_POD_START_TIMEOUT: seconds to wait for schedule + container start → Running (after image is ready); default 120 | ||
| // LATENCY_TEST_STARTUP_TIMEOUT: seconds after Running (excluding LATENCY_TEST_DELAY) to wait for the tool to start (default 600) | ||
| // LATENCY_TEST_EXIT_TIMEOUT: seconds beyond LATENCY_TEST_RUNTIME to wait for Succeeded (default 120) |
There was a problem hiding this comment.
I don't think dividing the timeout into sections is something that user should care about.
From user perspective the test should run from start to finish without hitting timeouts due to slow infra or different hardware layout (many CPUs).
Either we consolidate all of them under a single ENV like LATENCY_TEST_SETUP_DELAY or we consolidate that under the existing LATENCY_TEST_DELAY extending the comment about its usage.
The latency check has these stages:
I looked for a high-level object that would give richer status for that progress. There isn’t one. Pull, start, and runtime detail still come from the pod. I also checked a few higher-level options instead of a bare pod. They don’t add real value for our test logic: Deployment is meant to keep a workload available and always running. We create a pod, run a test, and exit, so it doesn’t help. CronJob is meant for scheduled or recurring runs. Customers typically run latency tests once to validate hardware, not on a schedule, so it doesn’t help either. I also checked Job which is a natural fit for one-shot work, but for a single completion, job complete is basically the same as the pod reaching succeeded. I agree that a useful improvement for users is a clear progress indicator. It can sit on top of filtering specific pod events, which remains the best way to track pull, start, and completion progress, as we do now. A higher-level object is not a better alternative for that. |
fair enough thank you for talking the time to look into it. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/4_latency/config/vars.go`:
- Around line 16-18: Replace config.PerTestTimeout with internal independent
lifecycle budgets for image readiness, pod startup, tool startup, and completion
while preserving the simplified public environment interface in config/vars.go.
In testpod/test_pod.go, update the lifecycle waits around the Running and
Succeeded phases to create and apply separate deadlines rather than sharing one
context; modify both listed sites accordingly.
In `@test/e2e/performanceprofile/functests/4_latency/latency.go`:
- Around line 23-29: In the BeforeEach setup around performanceProfile.Load and
the corresponding hwlatdetect setup near the referenced later context, invoke
the shared SkipIfNoIsolatedCPUs prerequisite immediately after loading the
profile and before SkipIfOddCpuNumber or any CPU selection. Ensure every test
path, including hwlatdetect and testpod.Define, skips profiles without isolated
CPUs instead of dereferencing Profile.Spec.CPU.Isolated.
In `@test/e2e/performanceprofile/functests/4_latency/testpod/test_pod.go`:
- Around line 97-103: Update the TestPod lifecycle methods, including Run and
Delete, to use the shared bounded test context or explicit deadlines for pod
creation and deletion instead of context.TODO(). Ensure API calls cannot outlive
the test timeout while preserving the existing cleanup behavior.
- Around line 128-142: Ensure the latency samples parsed in the maximumRegex
handling produce at least one value before calling slices.Max. Add an assertion
that latencies is non-empty after the parsing loop and before computing
maxLatencyMeasured, preserving the existing parsing and maximum-latency checks.
In `@test/e2e/performanceprofile/functests/5_latency_testing/latency_testing.go`:
- Line 225: Update clearEnv to handle the error returned by
os.Unsetenv(latencyTestSetupDelay), propagating or asserting it so cleanup
failures are reported and the environment variable cannot silently remain set
for subsequent tests.
🪄 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: bd11a58e-26fe-44c9-bdec-87a9c9128085
📒 Files selected for processing (11)
.gitignoredocs/performanceprofile/performance_controller.mdtest/e2e/performanceprofile/functests/4_latency/cluster/performance_profile.gotest/e2e/performanceprofile/functests/4_latency/cluster/worker_node.gotest/e2e/performanceprofile/functests/4_latency/config/consts.gotest/e2e/performanceprofile/functests/4_latency/config/env.gotest/e2e/performanceprofile/functests/4_latency/config/vars.gotest/e2e/performanceprofile/functests/4_latency/latency.gotest/e2e/performanceprofile/functests/4_latency/test_suite_latency_test.gotest/e2e/performanceprofile/functests/4_latency/testpod/test_pod.gotest/e2e/performanceprofile/functests/5_latency_testing/latency_testing.go
| // PerTestTimeout is RUNTIME+DELAY+SETUP_DELAY for a single tool (oslat/cyclictest/hwlatdetect). | ||
| func PerTestTimeout() time.Duration { | ||
| return time.Duration(LatencyTestRuntime+LatencyTestDelay+LatencyTestSetupDelay) * time.Second |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Use independent lifecycle-stage deadlines.
The aggregate timeout lets image pull/startup consume the completion budget, contrary to the PR objective of independently timing image readiness, pod startup, tool startup, and completion.
test/e2e/performanceprofile/functests/4_latency/config/vars.go#L16-L18: replace the single aggregate timeout API with internal phase budgets while retaining the simplified public environment interface.test/e2e/performanceprofile/functests/4_latency/testpod/test_pod.go#L197-L215: apply the separate budgets to each lifecycle wait instead of sharing one context across Running and Succeeded.
📍 Affects 2 files
test/e2e/performanceprofile/functests/4_latency/config/vars.go#L16-L18(this comment)test/e2e/performanceprofile/functests/4_latency/testpod/test_pod.go#L197-L215
🤖 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/4_latency/config/vars.go` around lines
16 - 18, Replace config.PerTestTimeout with internal independent lifecycle
budgets for image readiness, pod startup, tool startup, and completion while
preserving the simplified public environment interface in config/vars.go. In
testpod/test_pod.go, update the lifecycle waits around the Running and Succeeded
phases to create and apply separate deadlines rather than sharing one context;
modify both listed sites accordingly.
| func (p *TestPod) Delete() { | ||
| GinkgoHelper() | ||
| if p == nil || p.pod == nil { | ||
| return | ||
| } | ||
| Expect(pods.DeleteAndSync(context.TODO(), testclient.DataPlaneClient, p.pod)).To(Succeed()) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg 'test/e2e/performanceprofile/functests/4_latency/testpod/test_pod\.go|test/e2e/performanceprofile/functests/4_latency'
echo
echo "File outline:"
ast-grep outline test/e2e/performanceprofile/functests/4_latency/testpod/test_pod.go --view expanded || true
echo
echo "Relevant lines:"
nl -ba test/e2e/performanceprofile/functests/4_latency/testpod/test_pod.go | sed -n '1,240p'Repository: openshift/cluster-node-tuning-operator
Length of output: 1851
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="test/e2e/performanceprofile/functests/4_latency/testpod/test_pod.go"
echo "Relevant source with line numbers:"
awk '{printf "%6d\t%s\n", NR, $0}' "$file" | sed -n '1,240p'
echo
echo "Context / call sites:"
rg -n "context\.(TODO|Background|WithTimeout|WithDeadline)|NewTestPod|DeleteAndSync|Define\(" test/e2e/performanceprofile/functests/4_latency -SRepository: openshift/cluster-node-tuning-operator
Length of output: 11242
Bound pod lifecycle API calls with deadlines.
Run creates the pod with context.TODO() before the shared timeout context is constructed, and Delete() also uses an unbounded cleanup context. API-server stalls on pod creation or cleanup can outlive the test timeout and block cleanup; pass the bounded test context into the relevant operations or use explicit deadlines.
🤖 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/4_latency/testpod/test_pod.go` around
lines 97 - 103, Update the TestPod lifecycle methods, including Run and Delete,
to use the shared bounded test context or explicit deadlines for pod creation
and deletion instead of context.TODO(). Ensure API calls cannot outlive the test
timeout while preserving the existing cleanup behavior.
Source: Path instructions
| output := maximumRegex.FindStringSubmatch(out) | ||
| Expect(len(output)).To(Equal(2)) | ||
|
|
||
| var latencies []int | ||
| for _, lat := range strings.Split(output[1], " ") { | ||
| if lat == "" { | ||
| continue | ||
| } | ||
| curr, err := strconv.Atoi(lat) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| latencies = append(latencies, curr) | ||
| } | ||
|
|
||
| maxLatencyMeasured := slices.Max(latencies) | ||
| Expect(maxLatencyMeasured < config.MaximumLatency).To(BeTrue(), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg 'test/e2e/performanceprofile/functests/4_latency/testpod/test_pod.go|performanceprofile|latency' || true
echo
echo "Target section:"
if [ -f test/e2e/performanceprofile/functests/4_latency/testpod/test_pod.go ]; then
nl -ba test/e2e/performanceprofile/functests/4_latency/testpod/test_pod.go | sed -n '90,170p'
fi
echo
echo "Relevant symbols:"
rg -n "maximumRegex|latencies|slices\.Max|MaximumLatency|regex" test/e2e/performanceprofile/functests/4_latency/testpod/test_pod.go || true
echo
echo "Behavior probe for strings.Split and slices.Max edge cases:"
python3 - <<'PY'
import re
# The exact Go regexes are not guaranteed in the output, so model a plausible
# maximum line format if the regex is missing whitespace or line endings.
patterns = [
re.compile(r'Maximum latency:\s+([0-9 ]+).*'), # common pattern
re.compile(r'MAXIMUM:\s+([0-9 ]+).*'), # alternative
]
outputs = [
"Maximum latency:\n", # no captured numbers
"", # empty
]
for out in outputs:
print("output:", repr(out))
for p in patterns:
m = p.match(out)
if not m:
continue
cap = m.group(1)
print("capture:", repr(cap))
print("strings.Split(cap,' '):", cap.split(' '))
latencies = []
for lat in cap.split(' '):
if lat == '':
continue
latencies.append(lat)
print("latencies:", latencies)
try:
print("max:", max(latencies))
except Exception as e:
print("max error:", type(e).__name__, e)
PYRepository: openshift/cluster-node-tuning-operator
Length of output: 29509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="test/e2e/performanceprofile/functests/4_latency/testpod/test_pod.go"
if [ -f "$file" ]; then
echo "Line count:"
wc -l "$file"
echo
echo "Go syntax / parse check (if available):"
if command -v golint > /dev/null 2>&1; then golint "$file" || true; fi
if command -v go > /dev/null 2>&1; then go fmt "$file" > /tmp/gofmt.out 2>&1 || cat /tmp/gofmt.out; fi
echo
echo "Imports and target section:"
sed -n '1,180p' "$file" | nl -ba
echo
echo "All regex construction sites in target file:"
nl -ba "$file" | sed -n '1,180p' | rg -n "regexp|regex|Compile|Matches|FindStringSubmatch" || true
else
echo "Target file missing: $file"
fiRepository: openshift/cluster-node-tuning-operator
Length of output: 371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="test/e2e/performanceprofile/functests/4_latency/testpod/test_pod.go"
echo "Imports and target section:"
sed -n '1,180p' "$file" | awk '{printf "%5d %s\n", NR, $0}'
echo
echo "Behavior probe for Go-like edge cases using Python:"
python3 - <<'PY'
from pathlib import Path
import re
path = Path("test/e2e/performanceprofile/functests/4_latency/testpod/test_pod.go")
text = path.read_text()
for name, pat in re.finditer(r'[A-Za-z0-9_]+\s*:=\s*regexp\.Compile\(`([^`]+)`\)|regexp\.Compile\(`([^`]+)`\)', text):
src = pat.group(1) or pat.group(2)
print(f"REGEXP {name.group(0)[:50]}: {src}")
rx = re.compile(src)
for out in ("Maximum latency:\n", "Maximum latency: \n", ""):
m = rx.search(out)
if m:
print(f" match capture={m.group(1)!r}")
print(f" stringsFields equivalent={m.group(1).split()}")
print(f" stringsSplit space equivalent={m.group(1).split(' ')}")
latencies = []
try:
print("max([])", max(latencies))
except Exception as e:
print("max([]) raises", type(e).__name__.split(".")[-1], e)
PYRepository: openshift/cluster-node-tuning-operator
Length of output: 7396
🌐 Web query:
Go standard library slices Max empty slice panic docs
💡 Result:
In the Go standard library, the slices.Max and slices.MaxFunc functions will panic if they are called with an empty slice [1][2]. Official documentation explicitly states this behavior for both functions [1][3][4]. Because these functions are designed to return a single element from the slice, they cannot return a valid result when no elements exist; consequently, the standard library implementation checks the length of the slice and explicitly triggers a panic if it is less than one [2][5]. To avoid this panic, you must check the length of the slice before calling these functions [6]: if len(mySlice) > 0 { maxVal:= slices.Max(mySlice) //... handle maxVal } else { //... handle empty slice case } There have been community discussions regarding changing this behavior to return an error instead of panicking, but the consensus remains that the panic is the documented contract and callers are expected to ensure the slice is not empty [7].
Citations:
- 1: https://pkg.go.dev/slices
- 2: https://github.com/golang/go/blob/master/src/slices/sort.go
- 3: https://golang.org/pkg/slices/
- 4: https://go.dev/pkg/slices/?m=old
- 5: https://github.com/golang/go/blob/go1.21.0/src/slices/sort.go
- 6: https://zetcode.com/golang/slices-max/
- 7: proposal: slices: return the error instead of causing panic golang/go#63187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find latencyRegex usages:"
rg -n "ExpectLatencyUnderMaximum|latencyRegex|LatencyRegex|Maximum latency|latency" test/e2e/performanceprofile/functests/4_latency -g '*.go' || true
echo
echo "Show later test definitions around ExpectLatencyUnderMaximum calls:"
sed -n '180,260p' test/e2e/performanceprofile/functests/4_latency/testpod/test_pod.go | awk '{printf "%5d %s\n", NR+179, $0}'
echo
echo "Behavior probe:"
python3 - <<'PY'
from pathlib import Path
import re
text = Path("test/e2e/performanceprofile/functests/4_latency/testpod/test_pod.go").read_text()
print("MAXIMUM_REGEX occurs:", text.count("Maximum latency"))
for m in re.finditer(r'ExpectLatencyUnderMaximum\(\s*`([^`]+)`\s*\)', text, flags=re.S):
print("CALL:", m.group(1).replace('\n','\\n')[:300])
PYRepository: openshift/cluster-node-tuning-operator
Length of output: 13732
Assert that latency samples were parsed before calling slices.Max.
Both supplied regexes can match samples that are only whitespace or empty, leaving latencies empty and causing slices.Max(latencies) to panic. Add an empty-latency assertion before computing the maximum.
🤖 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/4_latency/testpod/test_pod.go` around
lines 128 - 142, Ensure the latency samples parsed in the maximumRegex handling
produce at least one value before calling slices.Max. Add an assertion that
latencies is non-empty after the parsing loop and before computing
maxLatencyMeasured, preserving the existing parsing and maximum-latency checks.
| func clearEnv() { | ||
| os.Unsetenv(latencyTestDelay) | ||
| os.Unsetenv(latencyTestRuntime) | ||
| os.Unsetenv(latencyTestSetupDelay) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
go doc os.UnsetenvRepository: openshift/cluster-node-tuning-operator
Length of output: 287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="test/e2e/performanceprofile/functests/5_latency_testing/latency_testing.go"
echo "=== file exists and line count ==="
wc -l "$file"
echo
echo "=== surrounding code around cleanup ==="
sed -n '200,240p' "$file" | nl -ba -v200
echo
echo "=== all Unsetenv/Osenv/unset references in file ==="
rg -n "Unsetenv|LATENCY_TEST_SETUP_DELAY|os\.(Getenv|Setenv)" "$file"
echo
echo "=== lint/check style for ignored error at target ==="
python3 - <<'PY'
from pathlib import Path
p = Path("test/e2e/performanceprofile/functests/5_latency_testing/latency_testing.go")
text = p.read_text().splitlines()
for i, line in enumerate(text, 1):
if "Unsetenv" in line:
print(f"{p}:{i}: {line!r}")
# Check typical Go compiler-style assignment of return value.
line_no_ws = "".join(line.split())
after_call = line_no_ws[:line_no_ws.find("Unsetenv(")+len("Unsetenv(")-3].split()
print(" has target:", "latencyTestSetupDelay" in line or "LATENCY_TEST_SETUP_DELAY" in line)
PYRepository: openshift/cluster-node-tuning-operator
Length of output: 372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="test/e2e/performanceprofile/functests/5_latency_testing/latency_testing.go"
echo "=== file exists and line count ==="
wc -l "$file"
echo
echo "=== surrounding code around cleanup ==="
sed -n '200,240p' "$file"
echo
echo "=== all Unsetenv/Osenv/unset references in file ==="
rg -n "Unsetenv|LATENCY_TEST_SETUP_DELAY|os\.(Getenv|Setenv)" "$file" || true
echo
echo "=== target assignment context ==="
python3 - <<'PY'
from pathlib import Path
p = Path("test/e2e/performanceprofile/functests/5_latency_testing/latency_testing.go")
text = p.read_text().splitlines()
for i, line in enumerate(text, 1):
if "Unsetenv" in line:
print(f"{p}:{i}: {line}")
PYRepository: openshift/cluster-node-tuning-operator
Length of output: 3214
Handle the os.Unsetenv error.
clearEnv() discards the error from os.Unsetenv(latencyTestSetupDelay); propagate or assert it so failed cleanup cannot leave LATENCY_TEST_SETUP_DELAY set for later tests.
🤖 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/5_latency_testing/latency_testing.go`
at line 225, Update clearEnv to handle the error returned by
os.Unsetenv(latencyTestSetupDelay), propagating or asserting it so cleanup
failures are reported and the environment variable cannot silently remain set
for subsequent tests.
Source: Path instructions
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/e2e/performanceprofile/functests/4_latency/latency.go (1)
23-30: 🩺 Stability & Availability | 🟠 MajorIsolated-CPU guard still runs after the CPU-selection path that depends on it.
SkipIfOddCpuNumber(Line 27) executes in the sharedBeforeEachfor every context, before the per-contextSkipIfNoIsolatedCPUsguard (Lines 46, 67) has a chance to run — and the hwlatdetect context (Lines 88-90) never callsSkipIfNoIsolatedCPUsat all. IfisOddCpuNumber(or latertestpod.Define, which dereferences*profile.Profile.Spec.CPU.IsolatedwhenLatencyTestCpus == DefaultTestCpus) is reached with a profile lacking isolated CPUs, this panics instead of skipping gracefully. This mirrors a previously raised, still-unresolved concern (no "Addressed in commit" marker) against this same line range and the hwlatdetect context.Move a shared
SkipIfNoIsolatedCPUscall immediately afterLoad()(Line 26), beforeSkipIfOddCpuNumber/CPU selection, so every context — including hwlatdetect — is protected.Also applies to: 88-90
🤖 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/4_latency/latency.go` around lines 23 - 30, Move the shared SkipIfNoIsolatedCPUs call in the BeforeEach hook to immediately follow performanceProfile.Load(), before SkipIfOddCpuNumber and worker CPU selection. Remove or avoid relying on the later per-context guards so every context, including hwlatdetect, is protected before accessing isolated-CPU profile data.
🤖 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.
Duplicate comments:
In `@test/e2e/performanceprofile/functests/4_latency/latency.go`:
- Around line 23-30: Move the shared SkipIfNoIsolatedCPUs call in the BeforeEach
hook to immediately follow performanceProfile.Load(), before SkipIfOddCpuNumber
and worker CPU selection. Remove or avoid relying on the later per-context
guards so every context, including hwlatdetect, is protected before accessing
isolated-CPU profile data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4cc20902-bf55-47d8-bf28-c9acc34d2e2b
📒 Files selected for processing (11)
.gitignoredocs/performanceprofile/performance_controller.mdtest/e2e/performanceprofile/functests/4_latency/cluster/performance_profile.gotest/e2e/performanceprofile/functests/4_latency/cluster/worker_node.gotest/e2e/performanceprofile/functests/4_latency/config/consts.gotest/e2e/performanceprofile/functests/4_latency/config/env.gotest/e2e/performanceprofile/functests/4_latency/config/vars.gotest/e2e/performanceprofile/functests/4_latency/latency.gotest/e2e/performanceprofile/functests/4_latency/test_suite_latency_test.gotest/e2e/performanceprofile/functests/4_latency/testpod/test_pod.gotest/e2e/performanceprofile/functests/5_latency_testing/latency_testing.go
🚧 Files skipped from review as they are similar to previous changes (10)
- test/e2e/performanceprofile/functests/4_latency/cluster/worker_node.go
- docs/performanceprofile/performance_controller.md
- test/e2e/performanceprofile/functests/4_latency/config/vars.go
- .gitignore
- test/e2e/performanceprofile/functests/4_latency/test_suite_latency_test.go
- test/e2e/performanceprofile/functests/4_latency/config/consts.go
- test/e2e/performanceprofile/functests/4_latency/cluster/performance_profile.go
- test/e2e/performanceprofile/functests/4_latency/config/env.go
- test/e2e/performanceprofile/functests/5_latency_testing/latency_testing.go
- test/e2e/performanceprofile/functests/4_latency/testpod/test_pod.go
Tal-or
left a comment
Tal-or
left a comment
There was a problem hiding this comment.
General idea is good, but we'll have to refine the implementation a little bit.
| Profile *performancev2.PerformanceProfile | ||
| } | ||
|
|
||
| func (p *PerformanceProfile) Load() { |
There was a problem hiding this comment.
func (p *PerformanceProfile) Load() change to func Load(p *PerformanceProfile)
the general rule is that you want to use methods for stateful execution which most of the time here we don't care about.
see: https://www.sohamkamani.com/golang/functions-vs-methods/
| } | ||
| } | ||
|
|
||
| type PerformanceProfile struct { |
There was a problem hiding this comment.
you can pass the performanceProfile directly to all the relevant functions and avoid the reciever function.
| @@ -0,0 +1,34 @@ | |||
| package config | |||
There was a problem hiding this comment.
This is usually a good idea to have separate packages when you have multiple consumers.
This is not the case here since the sole consumer is the latency.go so everything can and should be inline.
| "k8s.io/utils/cpuset" | ||
| ) | ||
|
|
||
| func SkipIfProfileNotFoundInDiscovery() { |
There was a problem hiding this comment.
Since this logic called just once. make this code inline is good enough and we don't need a function here.
| } | ||
| workerRTNode = &workerRTNodesWithSufficientCpu[0] | ||
|
|
||
| Expect(config.LoadLatencyTestEnvs()).To(Succeed()) |
There was a problem hiding this comment.
why do we call this in BeforeEach? this is not going to change nor setup again during the whole test suite run.
There was a problem hiding this comment.
Agreed,
loading the env vars in BeforeEach is unnecessary since they don’t change for the whole suite run.
That was already the pattern before this change.
I’ve moved the LATENCY_TEST_* loading to a one-time BeforeSuite in latency.go
BeforeEach now only keeps the per-spec setup (profile, node selection, skips).
| return p.pod.Status.Phase | ||
| } | ||
|
|
||
| func (p *TestPod) checkCPUs() { |
There was a problem hiding this comment.
seems like overkill but anyway this is something that is suppose to be sit in unit-tests and usually we don't have unit-tests for test code.
| if err != nil { | ||
| p.logEvents() | ||
| Fail(podTimeoutMessage(ctx, phase, podKey, p, err)) |
There was a problem hiding this comment.
can be replaced with Expect(err).ToNot(HaveOccurred(), p.logEvents(), podTimeoutMessage(ctx, phase, podKey, p, err))
| latencyTestDelay = "LATENCY_TEST_DELAY" | ||
| latencyTestRuntime = "LATENCY_TEST_RUNTIME" | ||
| maximumLatency = "MAXIMUM_LATENCY" | ||
| oslatMaxLatency = "OSLAT_MAXIMUM_LATENCY" | ||
| hwlatdetecMaxLatency = "HWLATDETECT_MAXIMUM_LATENCY" | ||
| cyclictestMaxLatency = "CYCLICTEST_MAXIMUM_LATENCY" | ||
| latencyTestCpus = "LATENCY_TEST_CPUS" | ||
| latencyTestDelay = "LATENCY_TEST_DELAY" | ||
| latencyTestRuntime = "LATENCY_TEST_RUNTIME" | ||
| latencyTestSetupDelay = "LATENCY_TEST_SETUP_DELAY" | ||
| maximumLatency = "MAXIMUM_LATENCY" | ||
| oslatMaxLatency = "OSLAT_MAXIMUM_LATENCY" | ||
| hwlatdetecMaxLatency = "HWLATDETECT_MAXIMUM_LATENCY" | ||
| cyclictestMaxLatency = "CYCLICTEST_MAXIMUM_LATENCY" | ||
| latencyTestCpus = "LATENCY_TEST_CPUS" |
There was a problem hiding this comment.
If we are doing a refactor we better also consider having the env vars public and use the same source in both 4_latency and 5_latency directories. I understand and completely agree the suites can use some refactoring, however, we should split that refactoring from the actual solution of the bug, and that would better fit in a different enhancement PR (no functional changes) because the refactoring is not a prerequisite for the bug fix.
There was a problem hiding this comment.
I’ve updated the branch with the actual bug fix commit.
Happy to adjust further if needed.
| testSet = append(testSet, latencyTest{testDelay: "0", testRuntime: successRuntime, testSetupDelay: successSetupDelay, testMaxLatency: untunedLatencyThreshold, testCpus: "4", outputMsgs: []string{success}, toolToTest: toolToTest, ginkgoTimeout: successGinkgoTimeout}) | ||
| // Covers the scenario of BZ 2055267 as well | ||
| testSet = append(testSet, latencyTest{testDelay: "0", testRuntime: successRuntime, testMaxLatency: untunedLatencyThreshold, testCpus: "6", outputMsgs: []string{success}, toolToTest: toolToTest, ginkgoTimeout: successGinkgoTimeout}) | ||
| testSet = append(testSet, latencyTest{testDelay: "0", testRuntime: successRuntime, testSetupDelay: successSetupDelay, testMaxLatency: untunedLatencyThreshold, testCpus: "6", outputMsgs: []string{success}, toolToTest: toolToTest, ginkgoTimeout: successGinkgoTimeout}) |
There was a problem hiding this comment.
for existing tests we should not override the default, because we must expect the tests to still work with the old scenarios (no regressions). To increase the coverage for the new functionality, we add more scenarios exercising the different values as it can fit.
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| ) | ||
|
|
||
| var latencyPodAnnotations = map[string]string{ |
There was a problem hiding this comment.
this is used only in newTestPod(), let's please move it as a local function var instead.
| func (p *TestPod) Delete() { | ||
| GinkgoHelper() | ||
| if p == nil || p.pod == nil { | ||
| return | ||
| } | ||
| Expect(pods.DeleteAndSync(context.TODO(), testclient.DataPlaneClient, p.pod)).To(Succeed()) | ||
| } |
There was a problem hiding this comment.
no need for this wrapper, DeleteAndSync already handles pod is nil. simply use direct
| func (p *TestPod) logs() (string, error) { | ||
| return pods.GetLogs(testclient.K8sClient, p.pod) | ||
| } |
There was a problem hiding this comment.
no need for this wrapper, GetLogs is anyhow a oneliner and is clear enough
| if pod != nil { | ||
| pod.DumpLogs() | ||
| pod.Delete() | ||
| pod = nil |
There was a problem hiding this comment.
pod = nil this should rather be in BeforeEach
| if latencyTestPod != nil { | ||
| Expect(pods.DeleteAndSync(context.TODO(), testclient.DataPlaneClient, latencyTestPod)).To(Succeed()) | ||
| if pod != nil { | ||
| pod.DumpLogs() |
There was a problem hiding this comment.
DumpLogs is only used here -> no need for a helper function
There was a problem hiding this comment.
Thanks for the PR. I left few comments from initial review. enhancements to the existing code are more than welcome, however in this case unless the refactoring is highly needed for the bug fix, we better split any refactoring/enhancments to a followup PR while also think of a better design for the new version. Let's only have the parts that are crucial for the bug fix in this PR and update the commit please.
| // LATENCY_TEST_DELAY delay the run of the binary, can be useful to give time to the CPU manager reconcile loop | ||
| // to update the default CPU pool | ||
| // LATENCY_TEST_RUNTIME: the amount of time in seconds that the latency test should run | ||
| // LATENCY_TEST_SETUP_DELAY is shared overhead (image pull, pod startup/CPU detect, exit). combined with RUNTIME+DELAY into one wait deadline. Default 150. |
There was a problem hiding this comment.
please remove Default 150. to avoid missing to update that fixed value and stay consistent with the rest of the env-var comments.
| // LoadLatencyTestEnv loads LATENCY_TEST_* once for the suite. | ||
| func LoadLatencyTestEnv() { |
There was a problem hiding this comment.
is called once in the BeforeSuite, let's inline it there then and remove the GinkgoHelper()
There was a problem hiding this comment.
The vars latencyTestDelay, latencyTestCpus, latencyTestRuntime, latencyTestSetupDelay are used only in latency.go (package __latency).
Inlining the load in test_suite_latency_test.go (package __latency_test) would require exporting them so the suite can set state that only __latency uses, which is redundant.
I prefer leaving them unexported in latency.go and calling LoadLatencyTestEnv() from BeforeSuite unless you see a reason to export them.
| return val, fmt.Errorf("the environment variable LATENCY_TEST_SETUP_DELAY has incorrect value %q, it must be a non-negative integer with maximum value of %d: %w", latencyTestSetupDelayEnv, math.MaxInt32, err) | ||
| } | ||
| if val < 0 || val > math.MaxInt32 { | ||
| return val, fmt.Errorf("the environment variable LATENCY_TEST_SETUP_DELAY has an invalid number %q, it must be a non-negative integer with maximum value of %d", latencyTestSetupDelayEnv, math.MaxInt32) |
There was a problem hiding this comment.
or predefine that var val int32 and only validate the err is not nil , and later that val >0
There was a problem hiding this comment.
Predefining math.MaxInt32 alone doesn’t let us drop the range check, we still need to validate that val is within bounds.
Could you clarify the change you had in mind?
| perTest := PerTestTimeout() | ||
| suiteTotal := 3 * perTest // oslat + cyclictest + hwlatdetect | ||
| testlog.Infof("MAX TIME FOR SUITE (3 tests): %s", suiteTotal) |
There was a problem hiding this comment.
we shouldn't assume that 3 tests will run, because the user can choose to focus on part of them only:
https://docs.redhat.com/en/documentation/openshift_container_platform/4.8/html/scalability_and_performance/cnf-performing-platform-verification-latency-tests
so when this is removed, you can inline PerTestTimeout within the function createLatencyTestPod for better readability, there you can also log the timeout for debugging details
| if val < 0 || val > math.MaxInt32 { | ||
| return val, fmt.Errorf("the environment variable LATENCY_TEST_SETUP_DELAY has an invalid number %q, it must be a non-negative integer with maximum value of %d", latencyTestSetupDelayEnv, math.MaxInt32) | ||
| } | ||
| return val, nil |
There was a problem hiding this comment.
I think we should guard the value to at least 120, or at least warn the users that to increase the chances of the pod to complete gracefully they need to set the setup delay to at least 120s.
There was a problem hiding this comment.
Setting the value to 120s leaves the issue unresolved,
I recommend adding a buffer and set it for at least 150s.
What do you think?
| testlog.Infof("timeout for %s test is %s (RUNTIME=%d DELAY=%d SETUP_DELAY=%d)", | ||
| testName, testTimeout, runtimeSec, latencyTestDelay, latencyTestSetupDelay) |
There was a problem hiding this comment.
this log is misplaced, the calculation in PerTestTimeout() can be changed and the log would then be inconsistent (another reason to inline PerTestTimeout()).
| Expect(err).ToNot(HaveOccurred(), podTimeoutMessage(testName)) | ||
| } | ||
|
|
||
| func podTimeoutMessage(testName string) string { |
| func waitForPhase(ctx context.Context, testPod *corev1.Pod, testName string, phase corev1.PodPhase) { | ||
| GinkgoHelper() | ||
|
|
||
| updatedPod, err := pods.WaitForPhase(ctx, client.ObjectKeyFromObject(testPod), phase, contextTimeLeft(ctx)) |
There was a problem hiding this comment.
contextTimeLeft(ctx) not sure if this is good or is more restrictive than before. the code is now reducing the waiting time for pod started to run, and waits for it to finish with the remaining timeout. before this change the code was separately waiting for the pod to start( go running) which is part of the product (test in this case) we are offering, so without it there would be no running pod.
the goal of the test is to run the pod and let it finish gracefully, if it took few extra seconds it's fine, I don't think we should be strict here.
| } | ||
|
|
||
| func podTimeoutMessage(testName string) string { | ||
| runtimeSec, _ := strconv.Atoi(latencyTestRuntime) |
There was a problem hiding this comment.
3 times processing the same value is fine but in this context we can do it only once
| } | ||
|
|
||
| var _ = BeforeSuite(func() { | ||
| latencytests.LoadLatencyTestEnv() |
There was a problem hiding this comment.
`Expect(testclient.ClientsEnabled).To(BeTrue())`
should be before any other check
e04d688 to
a70eaa2
Compare
Replace the hard-coded 120s slack in the Running→Succeeded wait with a configurable LATENCY_TEST_SETUP_DELAY (default 150s).
|
@tavital: 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. |
6 participants
OCPBUGS-98060 BUG-FIX Latency Test
Summary by CodeRabbit
LATENCY_TEST_SETUP_DELAY(default 150s).LATENCY_TEST_SETUP_DELAYand how it combines with other wait settings.