Skip to content

OCPBUGS-98060: BUG-FIX Latency Test - #1566

Merged
openshift-merge-bot[bot] merged 1 commit into
openshift:mainfrom
tavital:dev
Aug 12, 2026
Merged

OCPBUGS-98060: BUG-FIX Latency Test#1566
openshift-merge-bot[bot] merged 1 commit into
openshift:mainfrom
tavital:dev

Conversation

@tavital

@tavital tavital commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

OCPBUGS-98060 BUG-FIX Latency Test

Summary by CodeRabbit

  • New Features
    • Added support for configuring latency test setup overhead via LATENCY_TEST_SETUP_DELAY (default 150s).
    • Latency e2e suite now automatically derives per-tool timeouts and applies maximum-latency thresholds per test via environment overrides.
  • Bug Fixes
    • Improved validation and handling of invalid latency test settings, including clearer failure/timeout behavior.
    • More robust selection of suitable worker resources and conditional skipping when requirements aren’t met.
  • Documentation
    • Updated latency test documentation to describe LATENCY_TEST_SETUP_DELAY and how it combines with other wait settings.

@openshift-ci
openshift-ci Bot requested review from ffromani and yanirq July 19, 2026 14:56
@openshift-ci

openshift-ci Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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 kubernetes-sigs/prow repository.

@openshift-ci openshift-ci Bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 19, 2026
@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The 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.

Changes

Latency test execution and configuration

Layer / File(s) Summary
Centralized latency configuration
test/e2e/performanceprofile/functests/4_latency/config/*
Adds validated environment loading, resource calculation, latency overrides, ordered test names, and combined timeout calculation.
Profile and worker selection
test/e2e/performanceprofile/functests/4_latency/cluster/*
Loads profiles, applies CPU-based skips, and selects workers with sufficient allocatable CPU.
Shared pod execution flow
test/e2e/performanceprofile/functests/4_latency/testpod/test_pod.go, test/e2e/performanceprofile/functests/4_latency/latency.go
Defines and runs latency pods, waits for lifecycle phases, validates resources, captures diagnostics, and checks latency output.
Setup-delay integration and validation
test/e2e/performanceprofile/functests/4_latency/test_suite_latency_test.go, test/e2e/performanceprofile/functests/5_latency_testing/latency_testing.go, docs/performanceprofile/performance_controller.md
Adds setup-delay loading, timeout accounting, documentation, cleanup, and positive/negative test coverage.
Repository support
.gitignore
Ignores the .vscode directory.

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
Loading

Suggested reviewers: yanirq, ffromani, shajmakh


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (2 errors, 4 warnings)

Check name Status Explanation Resolution
Container-Privileges ❌ Error New latency test pod sets SecurityContext.Privileged=true in test_pod.go, which violates the check. Remove privileged mode from the test container or justify it and switch to the least-privilege settings needed for the test.
No-Sensitive-Data-In-Logs ❌ Error FAIL: the suite logs workerRTNode.Name (node hostname) and dumps raw pod logs/events, which may expose internal hostnames and other sensitive content. Remove or redact hostname/pod-name logging and avoid raw log/event dumps; filter outputs to exclude secrets and internal identifiers.
Docstring Coverage ⚠️ Warning Docstring coverage is 9.52% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Structure And Quality ⚠️ Warning FAIL: The new suite has many bare Gomega assertions without context (node selection, env loading, pod create/wait), so failures will be hard to diagnose. Add descriptive messages to the new Expect/To(Succeed) calls in suite setup and helpers, especially for env loading, node selection, pod lifecycle, and latency parsing.
Microshift Test Compatibility ⚠️ Warning New latency e2e tests load performance.openshift.io/v2 PerformanceProfile resources and lack any MicroShift skip/tag/guard. Add a MicroShift guard ([Skipped:MicroShift], [apigroup:...], or exutil.IsMicroShiftCluster()), or remove the OpenShift API dependency.
Ipv6 And Disconnected Network Test Compatibility ⚠️ Warning New latency e2e pods use images.Test(), whose default registry is quay.io/openshift-kni/ with cnf-tests:4.19, requiring public image pulls. Use an internal/mirrored registry or cluster-local image pull source for images.Test(), then verify in the IPv6 disconnected CI job.
✅ Passed checks (9 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed PASS: The modified Ginkgo titles are static string literals; no pod/node/namespace/timestamp/UUID-derived names or other runtime values appear.
Single Node Openshift (Sno) Test Compatibility ✅ Passed The new latency tests run a single pod on one selected worker node and don’t require multiple nodes or HA behavior.
Topology-Aware Scheduling Compatibility ✅ Passed No topology-sensitive scheduling was introduced: the e2e pod is pinned to one selected node by hostname, with no anti-affinity, spread, replica, or control-plane targeting.
Ote Binary Stdout Contract ✅ Passed No process-level stdout writes were added; suite setup logs use GinkgoWriter/stderr, and touched init/BeforeSuite/RunSpecs paths don’t print to stdout.
No-Weak-Crypto ✅ Passed Touched files contain no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB usage, no custom crypto, and no secret/token comparisons.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the latency test bug fix and relates to the pull request's main timeout-handling changes.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@tavital tavital changed the title Latency Test OCPBUGS-98060 FIX OCPBUGS-98060 Latency Test FIX Jul 19, 2026
@tavital tavital changed the title OCPBUGS-98060 Latency Test FIX OCPBUGS-98060: Latency Test FIX Jul 19, 2026
@openshift-ci-robot openshift-ci-robot added jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Jul 19, 2026
@openshift-ci-robot

Copy link
Copy Markdown
Contributor

@tavital: This pull request references Jira Issue OCPBUGS-98060, which is invalid:

  • expected the bug to target the "5.0.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Latency Test OCPBUGS-98060 FIX

latency test pod timeout too short: hard-coded 120s buffer in createLatencyTestPod causes false failures on cold clusters

What changed?

The suite no longer uses one shared clock from Running to Succeeded.

It now waits in four steps, each with its own timeout env vars, so slow image pull or slow startup cannot steal time from the tool run / exit.

The bug (old version)

Create pod
   → wait up to 2m (hardcoded) until Running     ← pull + schedule + start mixed together
   → wait until Succeeded
        budget = LATENCY_TEST_RUNTIME
               + LATENCY_TEST_DELAY
               + 120s hardcoded slack

That 120s slack had to cover both:

  1. Everything after Running until the tool actually starts (init, CPU topology, delay)
  2. Everything after the tool finishes until the pod is Succeeded

On a large-CPU node, (1) could use ~117s. Then (2) had only ~3s left → context deadline exceeded even when oslat had already finished.

Old name Meaning
LATENCY_TEST_RUNTIME How long the tool runs
LATENCY_TEST_DELAY Sleep before the tool
120 (hardcoded) Shared slack for startup and exit

The new version (end to end)

1. Create pod on the selected worker (API Create — no timeout env)
2. Wait 1 — image ready
3. Wait 2 — pod is Running
4. CPU check (instant; not a wait)
5. Wait 3 — latency tool has started
6. Wait 4 — pod is Succeeded

Wait 1 — image ready

Success when: kubelet reports the image is ready (Pulled — downloaded or already on the node).

Timeout: LATENCY_TEST_IMAGE_PULL_TIMEOUT (default 120s)

Only covers the image. Does not include schedule or container start.

Wait 2 — pod Running

Success when: pod phase is Running.

Timeout: LATENCY_TEST_POD_START_TIMEOUT (default 120s)

Covers schedule onto the node + container start. Starts after wait 1. Separate from image pull so a slow pull cannot shrink this budget.

CPU check (not a wait)

After Running, the suite checks that CPU request and limit both match LATENCY_TEST_CPUS. Instant assert — no timeout.

Wait 3 — tool started

Success when: pod logs contain running (oslat|cyclictest|hwlatdetect) command.

Timeout: LATENCY_TEST_STARTUP_TIMEOUT + LATENCY_TEST_DELAY_TIMEOUT

Env Default Role
LATENCY_TEST_STARTUP_TIMEOUT 600s Max time for init / CPU detect after the pod is Running (before the delay)
LATENCY_TEST_DELAY_TIMEOUT 0 Seconds the runner sleeps before starting the tool (CPU manager settle). Also added to this wait’s budget

One wait, because the suite only sees one signal: the “tool is running” log line (after startup and delay).

Wait 4 — Succeeded

Success when: pod phase is Succeeded.

Timeout: LATENCY_TEST_RUN_TIMEOUT + LATENCY_TEST_EXIT_TIMEOUT

This wait starts only after wait 3 succeeds (tool has started).

Env Default Role
LATENCY_TEST_RUN_TIMEOUT 300s How long the tool is supposed to run (also passed into the tool as -runtime / -duration)
LATENCY_TEST_EXIT_TIMEOUT 120s Extra time after the tool should have finished for process exit + kubelet → Succeeded

Env var cheat sheet

Renamed (old names no longer work)

Old New Role
LATENCY_TEST_RUNTIME LATENCY_TEST_RUN_TIMEOUT Tool run duration
LATENCY_TEST_DELAY LATENCY_TEST_DELAY_TIMEOUT Sleep before the tool

New (did not exist before)

Env Default When it applies Plain-language role
LATENCY_TEST_IMAGE_PULL_TIMEOUT 120s Wait 1 “How long may image pull take?”
LATENCY_TEST_POD_START_TIMEOUT 120s Wait 2 “How long may schedule + container start take after the image is ready?”
LATENCY_TEST_STARTUP_TIMEOUT 600s Wait 3 (part) “How long may init / CPU detect take before the tool?”
LATENCY_TEST_EXIT_TIMEOUT 120s Wait 4 (part) “How long after the tool ends may we wait for Succeeded?”

Unchanged in role (new names only for run/delay)

Env Default Role
LATENCY_TEST_RUN_TIMEOUT 300s Tool run length + part of wait 4 budget
LATENCY_TEST_DELAY_TIMEOUT 0 Pre-tool sleep + part of wait 3 budget

Old vs new at a glance

OLD:
 Create → [2m] Running → [RUNTIME + DELAY + 120] Succeeded
                             └─ one clock; startup and exit share 120s

NEW:
 Create
   → [IMAGE_PULL_TIMEOUT]     image ready
   → [POD_START_TIMEOUT]      Running
   → [STARTUP + DELAY]        tool started
   → [RUN + EXIT]             Succeeded   ← new clock, only after tool started

How to run (new)

make dist-latency-tests

# Timeout exports below are suite defaults — omit any of them and you get the same value.
export DISCOVERY_MODE=true
export LATENCY_TEST_RUN_TIMEOUT=300
export LATENCY_TEST_DELAY_TIMEOUT=0
export LATENCY_TEST_IMAGE_PULL_TIMEOUT=120
export LATENCY_TEST_POD_START_TIMEOUT=120
export LATENCY_TEST_STARTUP_TIMEOUT=600
export LATENCY_TEST_EXIT_TIMEOUT=120
./build/_output/bin/latency-e2e.test -ginkgo.focus=oslat -ginkgo.v

Do not set LATENCY_TEST_RUNTIME or LATENCY_TEST_DELAY — they are ignored.

Example logs

cnf-tests image already on the node:

[INFO]: selected worker node "…" for latency test
[INFO]: created pod "…/oslat-…" targeting node "…" (image quay.io/openshift-kni/cnf-tests:4.19)
STEP: [wait 1/4] image pull (timeout budget LATENCY_TEST_IMAGE_PULL_TIMEOUT 2m0s)
[INFO]: [wait 1/4] OK: image already on node (elapsed 1s, LATENCY_TEST_IMAGE_PULL_TIMEOUT 2m0s) — Container image "quay.io/openshift-kni/cnf-tests:4.19" already present on machine
STEP: [wait 2/4] pod → Running (timeout budget LATENCY_TEST_POD_START_TIMEOUT 2m0s)
[INFO]: [wait 2/4] OK: pod Running on node "…" (elapsed 1s, LATENCY_TEST_POD_START_TIMEOUT 2m0s)
[INFO]: CPU check: expected=4 limits=4 requests=4
STEP: [wait 3/4] tool start (timeout budget 10m0s = LATENCY_TEST_STARTUP_TIMEOUT 10m0s + LATENCY_TEST_DELAY_TIMEOUT 0s)
[INFO]: [wait 3/4] OK: latency tool started (elapsed 2s, budget was 10m0s = LATENCY_TEST_STARTUP_TIMEOUT + LATENCY_TEST_DELAY_TIMEOUT)
STEP: [wait 4/4] pod → Succeeded (timeout budget 7m0s = LATENCY_TEST_RUN_TIMEOUT 5m0s + LATENCY_TEST_EXIT_TIMEOUT 2m0s)
[INFO]: [wait 4/4] OK: pod Succeeded (elapsed 5m2s, budget was 7m0s = LATENCY_TEST_RUN_TIMEOUT + LATENCY_TEST_EXIT_TIMEOUT)

with cnf-tests image pull:

[INFO]: selected worker node "…" for latency test
[INFO]: created pod "…/oslat-…" targeting node "…" (image quay.io/openshift-kni/cnf-tests:4.19)
STEP: [wait 1/4] image pull (timeout budget LATENCY_TEST_IMAGE_PULL_TIMEOUT 2m0s)
[INFO]: [wait 1/4] image pull: downloading — Pulling image "quay.io/openshift-kni/cnf-tests:4.19"
[INFO]: [wait 1/4] OK: image pulled (elapsed 45s, LATENCY_TEST_IMAGE_PULL_TIMEOUT 2m0s) — Successfully pulled image "quay.io/openshift-kni/cnf-tests:4.19" in 44.8s
STEP: [wait 2/4] pod → Running (timeout budget LATENCY_TEST_POD_START_TIMEOUT 2m0s)
[INFO]: [wait 2/4] OK: pod Running on node "…" (elapsed 2s, LATENCY_TEST_POD_START_TIMEOUT 2m0s)
[INFO]: CPU check: expected=4 limits=4 requests=4
STEP: [wait 3/4] tool start (timeout budget 10m0s = LATENCY_TEST_STARTUP_TIMEOUT 10m0s + LATENCY_TEST_DELAY_TIMEOUT 0s)
[INFO]: [wait 3/4] OK: latency tool started (elapsed 2s, budget was 10m0s = LATENCY_TEST_STARTUP_TIMEOUT + LATENCY_TEST_DELAY_TIMEOUT)
STEP: [wait 4/4] pod → Succeeded (timeout budget 7m0s = LATENCY_TEST_RUN_TIMEOUT 5m0s + LATENCY_TEST_EXIT_TIMEOUT 2m0s)
[INFO]: [wait 4/4] OK: pod Succeeded (elapsed 5m2s, budget was 7m0s = LATENCY_TEST_RUN_TIMEOUT + LATENCY_TEST_EXIT_TIMEOUT)

Summary by CodeRabbit

  • Documentation

  • Updated latency test documentation with the new environment variable names and descriptions.

  • Bug Fixes

  • Improved latency test phase tracking for image pulls, pod startup, tool execution, and completion.

  • Added clearer timeout handling, validation, and progress logging.

  • Preserved CPU request and limit verification during test execution.

  • Tests

  • Updated end-to-end tests to use separate configurable start-delay and run-duration timeouts.

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.

@oblau

oblau commented Jul 19, 2026

Copy link
Copy Markdown
Member

/ok-to-test

@openshift-ci openshift-ci Bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 19, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Wait 2/4 predicate ignores terminal phases; can spin the full pod-start budget on fast or crashing pods.

The predicate only returns true for PodRunning. If the pod transitions straight through to Succeeded/Failed between 1s polls (short LATENCY_TEST_RUN_TIMEOUT, or an immediate container crash), this wait never succeeds and burns the entire latencyTestPodStartTimeout budget 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 win

Extract a shared helper for these four near-identical timeout parsers.

getLatencyTestImagePullTimeout, getLatencyTestPodStartTimeout, getLatencyTestStartupTimeout, and getLatencyTestExitTimeout differ 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

📥 Commits

Reviewing files that changed from the base of the PR and between 19b0d74 and 12d37f9.

📒 Files selected for processing (3)
  • docs/performanceprofile/performance_controller.md
  • test/e2e/performanceprofile/functests/4_latency/latency.go
  • test/e2e/performanceprofile/functests/5_latency_testing/latency_testing.go

Comment thread test/e2e/performanceprofile/functests/4_latency/latency.go Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 12d37f9 and db89a55.

📒 Files selected for processing (3)
  • docs/performanceprofile/performance_controller.md
  • test/e2e/performanceprofile/functests/4_latency/latency.go
  • test/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

Comment on lines +115 to +119
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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

@Tal-or Tal-or left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +72 to +75
// 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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may expand the LATENCY_TEST_DELAY to include the time it should take for the test to pull the image.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may expand the LATENCY_TEST_DELAY to 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@shajmakh

Copy link
Copy Markdown
Contributor

/cc

@openshift-ci
openshift-ci Bot requested a review from shajmakh July 20, 2026 13:06
@tavital

tavital commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

/jira refresh

@openshift-ci-robot openshift-ci-robot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Jul 20, 2026
@openshift-ci-robot

Copy link
Copy Markdown
Contributor

@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
  • bug is open, matching expected state (open)
  • bug target version (5.0.0) matches configured target version for branch (5.0.0)
  • bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)
Details

In response to this:

/jira refresh

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 openshift-ci-robot removed the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Jul 20, 2026
@openshift-ci-robot

Copy link
Copy Markdown
Contributor

@tavital: This pull request references Jira Issue OCPBUGS-98060, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (5.0.0) matches configured target version for branch (5.0.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)
Details

In response to this:

Latency Test OCPBUGS-98060 FIX

latency test pod timeout too short: hard-coded 120s buffer in createLatencyTestPod causes false failures on cold clusters

What changed?

The suite no longer uses one shared clock from Running to Succeeded.

It now waits in four steps, each with its own timeout env vars, so slow image pull or slow startup cannot steal time from the tool run / exit.

The bug (old version)

Create pod
   → wait up to 2m (hardcoded) until Running     ← pull + schedule + start mixed together
   → wait until Succeeded
        budget = LATENCY_TEST_RUNTIME
               + LATENCY_TEST_DELAY
               + 120s hardcoded slack

That 120s slack had to cover both:

  1. Everything after Running until the tool actually starts (init, CPU topology, delay)
  2. Everything after the tool finishes until the pod is Succeeded

On a large-CPU node, (1) could use ~117s. Then (2) had only ~3s left → context deadline exceeded even when oslat had already finished.

Old name Meaning
LATENCY_TEST_RUNTIME How long the tool runs
LATENCY_TEST_DELAY Sleep before the tool
120 (hardcoded) Shared slack for startup and exit

The new version (end to end)

1. Create pod on the selected worker (API Create — no timeout env)
2. Wait 1 — image ready
3. Wait 2 — pod is Running
4. CPU check (instant; not a wait)
5. Wait 3 — latency tool has started
6. Wait 4 — pod is Succeeded

Wait 1 — image ready

Success when: kubelet reports the image is ready (Pulled — downloaded or already on the node).

Timeout: LATENCY_TEST_IMAGE_PULL_TIMEOUT (default 120s)

Only covers the image. Does not include schedule or container start.

Wait 2 — pod Running

Success when: pod phase is Running.

Timeout: LATENCY_TEST_POD_START_TIMEOUT (default 120s)

Covers schedule onto the node + container start. Starts after wait 1. Separate from image pull so a slow pull cannot shrink this budget.

CPU check (not a wait)

After Running, the suite checks that CPU request and limit both match LATENCY_TEST_CPUS. Instant assert — no timeout.

Wait 3 — tool started

Success when: pod logs contain running (oslat|cyclictest|hwlatdetect) command.

Timeout: LATENCY_TEST_STARTUP_TIMEOUT + LATENCY_TEST_DELAY_TIMEOUT

Env Default Role
LATENCY_TEST_STARTUP_TIMEOUT 600s Max time for init / CPU detect after the pod is Running (before the delay)
LATENCY_TEST_DELAY_TIMEOUT 0 Seconds the runner sleeps before starting the tool (CPU manager settle). Also added to this wait’s budget

One wait, because the suite only sees one signal: the “tool is running” log line (after startup and delay).

Wait 4 — Succeeded

Success when: pod phase is Succeeded.

Timeout: LATENCY_TEST_RUN_TIMEOUT + LATENCY_TEST_EXIT_TIMEOUT

This wait starts only after wait 3 succeeds (tool has started).

Env Default Role
LATENCY_TEST_RUN_TIMEOUT 300s How long the tool is supposed to run (also passed into the tool as -runtime / -duration)
LATENCY_TEST_EXIT_TIMEOUT 120s Extra time after the tool should have finished for process exit + kubelet → Succeeded

Env var cheat sheet

Renamed (old names no longer work)

Old New Role
LATENCY_TEST_RUNTIME LATENCY_TEST_RUN_TIMEOUT Tool run duration
LATENCY_TEST_DELAY LATENCY_TEST_DELAY_TIMEOUT Sleep before the tool

New (did not exist before)

Env Default When it applies Plain-language role
LATENCY_TEST_IMAGE_PULL_TIMEOUT 120s Wait 1 “How long may image pull take?”
LATENCY_TEST_POD_START_TIMEOUT 120s Wait 2 “How long may schedule + container start take after the image is ready?”
LATENCY_TEST_STARTUP_TIMEOUT 600s Wait 3 (part) “How long may init / CPU detect take before the tool?”
LATENCY_TEST_EXIT_TIMEOUT 120s Wait 4 (part) “How long after the tool ends may we wait for Succeeded?”

Unchanged in role (new names only for run/delay)

Env Default Role
LATENCY_TEST_RUN_TIMEOUT 300s Tool run length + part of wait 4 budget
LATENCY_TEST_DELAY_TIMEOUT 0 Pre-tool sleep + part of wait 3 budget

Old vs new at a glance

OLD:
 Create → [2m] Running → [RUNTIME + DELAY + 120] Succeeded
                             └─ one clock; startup and exit share 120s

NEW:
 Create
   → [IMAGE_PULL_TIMEOUT]     image ready
   → [POD_START_TIMEOUT]      Running
   → [STARTUP + DELAY]        tool started
   → [RUN + EXIT]             Succeeded   ← new clock, only after tool started

How to run (new)

make dist-latency-tests

# Timeout exports below are suite defaults — omit any of them and you get the same value.
export DISCOVERY_MODE=true
export LATENCY_TEST_RUN_TIMEOUT=300
export LATENCY_TEST_DELAY_TIMEOUT=0
export LATENCY_TEST_IMAGE_PULL_TIMEOUT=120
export LATENCY_TEST_POD_START_TIMEOUT=120
export LATENCY_TEST_STARTUP_TIMEOUT=600
export LATENCY_TEST_EXIT_TIMEOUT=120
./build/_output/bin/latency-e2e.test -ginkgo.focus=oslat -ginkgo.v

Do not set LATENCY_TEST_RUNTIME or LATENCY_TEST_DELAY — they are ignored.

Example logs

cnf-tests image already on the node:

[INFO]: selected worker node "…" for latency test
[INFO]: created pod "…/oslat-…" targeting node "…" (image quay.io/openshift-kni/cnf-tests:4.19)
STEP: [wait 1/4] image pull (timeout budget LATENCY_TEST_IMAGE_PULL_TIMEOUT 2m0s)
[INFO]: [wait 1/4] OK: image already on node (elapsed 1s, LATENCY_TEST_IMAGE_PULL_TIMEOUT 2m0s) — Container image "quay.io/openshift-kni/cnf-tests:4.19" already present on machine
STEP: [wait 2/4] pod → Running (timeout budget LATENCY_TEST_POD_START_TIMEOUT 2m0s)
[INFO]: [wait 2/4] OK: pod Running on node "…" (elapsed 1s, LATENCY_TEST_POD_START_TIMEOUT 2m0s)
[INFO]: CPU check: expected=4 limits=4 requests=4
STEP: [wait 3/4] tool start (timeout budget 10m0s = LATENCY_TEST_STARTUP_TIMEOUT 10m0s + LATENCY_TEST_DELAY_TIMEOUT 0s)
[INFO]: [wait 3/4] OK: latency tool started (elapsed 2s, budget was 10m0s = LATENCY_TEST_STARTUP_TIMEOUT + LATENCY_TEST_DELAY_TIMEOUT)
STEP: [wait 4/4] pod → Succeeded (timeout budget 7m0s = LATENCY_TEST_RUN_TIMEOUT 5m0s + LATENCY_TEST_EXIT_TIMEOUT 2m0s)
[INFO]: [wait 4/4] OK: pod Succeeded (elapsed 5m2s, budget was 7m0s = LATENCY_TEST_RUN_TIMEOUT + LATENCY_TEST_EXIT_TIMEOUT)

with cnf-tests image pull:

[INFO]: selected worker node "…" for latency test
[INFO]: created pod "…/oslat-…" targeting node "…" (image quay.io/openshift-kni/cnf-tests:4.19)
STEP: [wait 1/4] image pull (timeout budget LATENCY_TEST_IMAGE_PULL_TIMEOUT 2m0s)
[INFO]: [wait 1/4] image pull: downloading — Pulling image "quay.io/openshift-kni/cnf-tests:4.19"
[INFO]: [wait 1/4] OK: image pulled (elapsed 45s, LATENCY_TEST_IMAGE_PULL_TIMEOUT 2m0s) — Successfully pulled image "quay.io/openshift-kni/cnf-tests:4.19" in 44.8s
STEP: [wait 2/4] pod → Running (timeout budget LATENCY_TEST_POD_START_TIMEOUT 2m0s)
[INFO]: [wait 2/4] OK: pod Running on node "…" (elapsed 2s, LATENCY_TEST_POD_START_TIMEOUT 2m0s)
[INFO]: CPU check: expected=4 limits=4 requests=4
STEP: [wait 3/4] tool start (timeout budget 10m0s = LATENCY_TEST_STARTUP_TIMEOUT 10m0s + LATENCY_TEST_DELAY_TIMEOUT 0s)
[INFO]: [wait 3/4] OK: latency tool started (elapsed 2s, budget was 10m0s = LATENCY_TEST_STARTUP_TIMEOUT + LATENCY_TEST_DELAY_TIMEOUT)
STEP: [wait 4/4] pod → Succeeded (timeout budget 7m0s = LATENCY_TEST_RUN_TIMEOUT 5m0s + LATENCY_TEST_EXIT_TIMEOUT 2m0s)
[INFO]: [wait 4/4] OK: pod Succeeded (elapsed 5m2s, budget was 7m0s = LATENCY_TEST_RUN_TIMEOUT + LATENCY_TEST_EXIT_TIMEOUT)

Summary by CodeRabbit

  • Bug Fixes
  • Improved latency test readiness tracking with separate stages for image pulling, pod startup, tool startup, and completion.
  • Added configurable timeout settings for each stage, improving diagnostics and reducing false failures.
  • Added CPU request and limit verification during latency test setup.
  • Improved logging by reporting the worker node selected for each latency test.

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 coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
test/e2e/performanceprofile/functests/4_latency/latency.go (2)

527-536: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicates pods.WaitForPhase logic.

The custom predicate here (pod.Status.Phase == corev1.PodRunning) is exactly what pods.WaitForPhase already 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 win

Four near-identical timeout getters — extract a shared parser.

getLatencyTestImagePullTimeout, getLatencyTestPodStartTimeout, getLatencyTestStartupTimeout, and getLatencyTestExitTimeout repeat 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

📥 Commits

Reviewing files that changed from the base of the PR and between db89a55 and 4644a46.

📒 Files selected for processing (1)
  • test/e2e/performanceprofile/functests/4_latency/latency.go

Comment thread test/e2e/performanceprofile/functests/4_latency/latency.go Outdated
@ffromani

Copy link
Copy Markdown
Contributor

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 tavital changed the title OCPBUGS-98060: Latency Test FIX OCPBUGS-98060 BUG-FIX Latency Test Jul 21, 2026
@openshift-ci-robot openshift-ci-robot removed the jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. label Jul 21, 2026
@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Aug 11, 2026
@shajmakh

Copy link
Copy Markdown
Contributor

/jira refresh

@openshift-ci-robot

Copy link
Copy Markdown
Contributor

@shajmakh: No Jira issue is referenced in the title of this pull request.
To reference a jira issue, add 'XYZ-NNN:' to the title of this pull request and request another refresh with /jira refresh.

Details

In response to this:

/jira refresh

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.

@tavital tavital changed the title OCPBUGS-98060 BUG-FIX Latency Test OCPBUGS-98060: BUG-FIX Latency Test Aug 11, 2026
@openshift-ci-robot openshift-ci-robot added jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels Aug 11, 2026
@openshift-ci-robot

Copy link
Copy Markdown
Contributor

@tavital: This pull request references Jira Issue OCPBUGS-98060, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (5.0.0) matches configured target version for branch (5.0.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

OCPBUGS-98060 BUG-FIX Latency Test

Summary by CodeRabbit

  • New Features
  • Added support for configuring latency test setup overhead via LATENCY_TEST_SETUP_DELAY (default 150s).
  • Latency e2e suite now automatically derives per-tool timeouts and applies maximum-latency thresholds per test via environment overrides.
  • Bug Fixes
  • Improved validation and handling of invalid latency test settings, including clearer failure/timeout behavior.
  • More robust selection of suitable worker resources and conditional skipping when requirements aren’t met.
  • Documentation
  • Updated latency test documentation to describe LATENCY_TEST_SETUP_DELAY and how it combines with other wait settings.

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.

@tavital

tavital commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

jira/valid-reference

@openshift-ci

openshift-ci Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

@tavital: all tests passed!

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

@yanirq

yanirq commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

/approve

@openshift-ci

openshift-ci Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shajmakh, tavital, yanirq

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2026
@mrniranjan

Copy link
Copy Markdown
Contributor

/verified by @mrniranjan

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Aug 12, 2026
@openshift-ci-robot

Copy link
Copy Markdown
Contributor

@mrniranjan: This PR has been marked as verified by @mrniranjan.

Details

In response to this:

/verified by @mrniranjan

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-merge-bot
openshift-merge-bot Bot merged commit c331ad1 into openshift:main Aug 12, 2026
20 checks passed
@openshift-ci-robot

Copy link
Copy Markdown
Contributor

@tavital: Jira Issue Verification Checks: Jira Issue OCPBUGS-98060
✔️ This pull request was pre-merge verified.
✔️ All associated pull requests have merged.
✔️ All associated, merged pull requests were pre-merge verified.

Jira Issue OCPBUGS-98060 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓

Details

In response to this:

OCPBUGS-98060 BUG-FIX Latency Test

Summary by CodeRabbit

  • New Features
  • Added support for configuring latency test setup overhead via LATENCY_TEST_SETUP_DELAY (default 150s).
  • Latency e2e suite now automatically derives per-tool timeouts and applies maximum-latency thresholds per test via environment overrides.
  • Bug Fixes
  • Improved validation and handling of invalid latency test settings, including clearer failure/timeout behavior.
  • More robust selection of suitable worker resources and conditional skipping when requirements aren’t met.
  • Documentation
  • Updated latency test documentation to describe LATENCY_TEST_SETUP_DELAY and how it combines with other wait settings.

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.

@yanirq

yanirq commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

/cherry-pick release-4.22

@openshift-cherrypick-robot

Copy link
Copy Markdown

@yanirq: new pull request created: #1583

Details

In response to this:

/cherry-pick release-4.22

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 kubernetes-sigs/prow repository.

@openshift-merge-robot

Copy link
Copy Markdown
Contributor

Fix included in release 5.0.0-0.nightly-2026-08-13-145619

@tavital

tavital commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

/cherry-pick release-4.21

@openshift-cherrypick-robot

Copy link
Copy Markdown

@tavital: new pull request created: #1592

Details

In response to this:

/cherry-pick release-4.21

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 kubernetes-sigs/prow repository.

@tavital

tavital commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

/cherry-pick release-4.20

@openshift-cherrypick-robot

Copy link
Copy Markdown

@tavital: new pull request created: #1594

Details

In response to this:

/cherry-pick release-4.20

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 kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants