OCPBUGS-98060: BUG-FIX Latency Test - #1566
Conversation
|
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. |
|
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: 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 |
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 |
|
@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. |
|
@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 |
|
/jira refresh |
|
@shajmakh: 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. |
|
@tavital: This pull request references Jira Issue OCPBUGS-98060, which is valid. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
jira/valid-reference |
|
@tavital: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/approve |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/verified by @mrniranjan |
|
@mrniranjan: This PR has been marked as verified by 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. |
|
@tavital: Jira Issue Verification Checks: Jira Issue OCPBUGS-98060 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. 🕓 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. |
|
/cherry-pick release-4.22 |
|
@yanirq: new pull request created: #1583 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 kubernetes-sigs/prow repository. |
|
Fix included in release 5.0.0-0.nightly-2026-08-13-145619 |
|
/cherry-pick release-4.21 |
|
@tavital: new pull request created: #1592 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 kubernetes-sigs/prow repository. |
|
/cherry-pick release-4.20 |
|
@tavital: new pull request created: #1594 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 kubernetes-sigs/prow repository. |
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.