OSSM: Run Istio/Sail builder-pod suites detached to survive oc rsh drops#82368
OSSM: Run Istio/Sail builder-pod suites detached to survive oc rsh drops#82368fjglira wants to merge 3 commits into
Conversation
…t drops. Long-lived oc rsh sessions were failing with websocket close 1006 on cluster instability; start suites with nohup, poll done/RC markers, and dump the full test log to Prow. Co-authored-by: Cursor <cursoragent@cursor.com>
fjglira
changed the title
WalkthroughThe scripts replace long-lived ChangesDetached test execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CIShell
participant TestPod
participant DetachedRunner
participant MarkerFiles
CIShell->>TestPod: Copy and start detached runner
TestPod->>DetachedRunner: Run test command
DetachedRunner->>MarkerFiles: Write RC and DONE markers
CIShell->>TestPod: Poll markers with oc exec
TestPod-->>CIShell: Return completion state and RC
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fjglira 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 |
openshift-ci
Bot
added
the
approved
|
/pj-rehearse |
|
@fjglira: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
![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
🤖 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
`@ci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.sh`:
- Around line 65-90: Add a wall-clock deadline or maximum elapsed-poll limit to
the wait_for_tests function in
ci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.sh
(lines 65-90),
ci-operator/step-registry/servicemesh/sail-operator/e2e-ocp/servicemesh-sail-operator-e2e-ocp-commands.sh
(lines 81-106), and
ci-operator/step-registry/servicemesh/sail-operator/scorecard/servicemesh-sail-operator-scorecard-commands.sh
(lines 83-108). Make the loop return failure when the deadline is exceeded,
while preserving marker detection and consecutive poll-failure handling so retry
logic can run promptly.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0156eb78-9716-4578-805a-98548eb5a332
📒 Files selected for processing (3)
ci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.shci-operator/step-registry/servicemesh/sail-operator/e2e-ocp/servicemesh-sail-operator-e2e-ocp-commands.shci-operator/step-registry/servicemesh/sail-operator/scorecard/servicemesh-sail-operator-scorecard-commands.sh
| wait_for_tests() { | ||
| echo "Polling for detached test completion (${DONE_MARKER} + ${RC_MARKER})..." | ||
| local poll_failures=0 | ||
| local max_consecutive_poll_failures=20 | ||
|
|
||
| while true; do | ||
| if are_tests_done; then | ||
| echo "Detached tests finished (markers present)." | ||
| return 0 | ||
| fi | ||
|
|
||
| if pod_exec sh -c "true" >/dev/null 2>&1; then | ||
| poll_failures=0 | ||
| tail_test_log | ||
| else | ||
| poll_failures=$((poll_failures + 1)) | ||
| echo "WARNING: short oc exec failed while polling (${poll_failures}/${max_consecutive_poll_failures}). Will retry." | ||
| if [ "${poll_failures}" -ge "${max_consecutive_poll_failures}" ]; then | ||
| echo "ERROR: too many consecutive poll failures talking to ${MAISTRA_SC_POD}" >&2 | ||
| return 1 | ||
| fi | ||
| fi | ||
|
|
||
| sleep "${POLL_INTERVAL}" | ||
| done | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
ci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.sh \
ci-operator/step-registry/servicemesh/sail-operator/e2e-ocp/servicemesh-sail-operator-e2e-ocp-commands.sh \
ci-operator/step-registry/servicemesh/sail-operator/scorecard/servicemesh-sail-operator-scorecard-commands.sh
do
echo "===== $f ====="
wc -l "$f"
echo "--- outline ---"
ast-grep outline "$f" --view expanded || true
echo "--- wait_for_tests and surrounding context ---"
rg -n -A40 -B20 'wait_for_tests|run_tests|execute_and_collect_artifacts|tail_test_log|are_tests_done|POLL_INTERVAL|DONE_MARKER|RC_MARKER' "$f"
echo
done
echo "===== references to step/job timeouts or watchdogs in these directories ====="
rg -n 'timeout|deadline|POLL_INTERVAL|max_consecutive_poll_failures|wait_for_tests' ci-operator/step-registry/servicemesh/istio ci-operator/step-registry/servicemesh/sail-operatorRepository: openshift/release
Length of output: 39512
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
ci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-ref.yaml \
ci-operator/step-registry/servicemesh/sail-operator/e2e-ocp/servicemesh-sail-operator-e2e-ocp-ref.yaml \
ci-operator/step-registry/servicemesh/sail-operator/scorecard/servicemesh-sail-operator-scorecard-ref.yaml
do
echo "===== $f ====="
cat -n "$f"
echo
doneRepository: openshift/release
Length of output: 5095
Add a deadline to wait_for_tests
ci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.sh#L65-L90ci-operator/step-registry/servicemesh/sail-operator/e2e-ocp/servicemesh-sail-operator-e2e-ocp-commands.sh#L81-L106ci-operator/step-registry/servicemesh/sail-operator/scorecard/servicemesh-sail-operator-scorecard-commands.sh#L83-L108
If the detached test process dies while the pod stays reachable, this loop can run until the enclosing step timeout before the retry path is reached. Add a wall-clock deadline (or max elapsed polls) so a dead runner fails fast and the retry logic can kick in.
📍 Affects 3 files
ci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.sh#L65-L90(this comment)ci-operator/step-registry/servicemesh/sail-operator/e2e-ocp/servicemesh-sail-operator-e2e-ocp-commands.sh#L81-L106ci-operator/step-registry/servicemesh/sail-operator/scorecard/servicemesh-sail-operator-scorecard-commands.sh#L83-L108
🤖 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
`@ci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.sh`
around lines 65 - 90, Add a wall-clock deadline or maximum elapsed-poll limit to
the wait_for_tests function in
ci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.sh
(lines 65-90),
ci-operator/step-registry/servicemesh/sail-operator/e2e-ocp/servicemesh-sail-operator-e2e-ocp-commands.sh
(lines 81-106), and
ci-operator/step-registry/servicemesh/sail-operator/scorecard/servicemesh-sail-operator-scorecard-commands.sh
(lines 83-108). Make the loop return failure when the deadline is exceeded,
while preserving marker detection and consecutive poll-failure handling so retry
logic can run promptly.
Keeps Prow build logs quieter during polling while still printing the complete suite output (and artifact copy) when the run finishes or fails. Co-authored-by: Cursor <cursoragent@cursor.com>
|
From the 3 failures from rehearse: CI FAILURE ** Cert Manager failure on ocp 4.23** |
|
/pj-rehearse more |
|
@fjglira: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
ci-operator/step-registry/servicemesh/sail-operator/e2e-ocp/servicemesh-sail-operator-e2e-ocp-commands.sh (4)
45-47: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not ignore marker-cleanup failures before starting a run.
Because
DONE_MARKERandRC_MARKERare fixed paths, a failedrmcan leave stale markers thatare_tests_doneaccepts for the next attempt. The workflow may then return an old RC before the new runner finishes. Make cleanup a verified prerequisite, or use a per-run marker namespace/token.🤖 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 `@ci-operator/step-registry/servicemesh/sail-operator/e2e-ocp/servicemesh-sail-operator-e2e-ocp-commands.sh` around lines 45 - 47, Update clear_test_markers to make removal of DONE_MARKER, RC_MARKER, TEST_LOG, and RUNNER_SCRIPT a verified prerequisite by removing the unconditional “|| true” and propagating or explicitly handling rm failures before starting a run. Ensure stale fixed-path markers cannot be accepted by are_tests_done for a new attempt.
77-101: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAdd a deadline for marker polling.
The loop only aborts after repeated
oc execfailures. If the detached runner exits before writing markers while the pod remains reachable, the health probe succeeds forever and the retry logic is never reached. Add a wall-clock timeout and/or runner-liveness check.🤖 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 `@ci-operator/step-registry/servicemesh/sail-operator/e2e-ocp/servicemesh-sail-operator-e2e-ocp-commands.sh` around lines 77 - 101, Add a wall-clock deadline or detached-runner liveness check to wait_for_tests, so polling terminates when the runner exits or markers never appear even if pod_exec continues succeeding. Preserve the existing consecutive-failure handling and return a nonzero status when the new deadline or liveness condition is reached.
199-202: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep Quay credentials out of the generated script and argv. The heredoc expands
QUAY_PASSWORDinto${local_runner}, anddocker login -pexposes it in process arguments. Use--password-stdinand pass the secret at execution time instead.🤖 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 `@ci-operator/step-registry/servicemesh/sail-operator/e2e-ocp/servicemesh-sail-operator-e2e-ocp-commands.sh` around lines 199 - 202, Update the generated script created by the heredoc in the e2e command flow so it does not expand QUAY_PASSWORD into local_runner or pass it through docker login arguments. Use docker login’s --password-stdin option and provide the password securely at script execution time, while preserving the existing QUAY_USERNAME and registry login behavior.
201-219: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep error handling enabled in the detached runner
set +emakesdocker login,oc version,cd, and the marker writes non-fatal; if any of those fail, the poller can wait until timeout becauseDONE_MARKER/RC_MARKERnever appear. Keepset -euo pipefailand relax it only aroundentrypointwith an explicit RC capture.🤖 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 `@ci-operator/step-registry/servicemesh/sail-operator/e2e-ocp/servicemesh-sail-operator-e2e-ocp-commands.sh` around lines 201 - 219, The detached runner currently disables error handling for setup and marker operations. Replace set +e with set -euo pipefail, and scope any non-fatal handling only around entrypoint so its exit status is explicitly captured in rc; ensure RC_MARKER and DONE_MARKER are still written after entrypoint while failures in docker login, oc version, cd, or marker writes remain fatal.Source: Coding guidelines
🤖 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.
Outside diff comments:
In
`@ci-operator/step-registry/servicemesh/sail-operator/e2e-ocp/servicemesh-sail-operator-e2e-ocp-commands.sh`:
- Around line 45-47: Update clear_test_markers to make removal of DONE_MARKER,
RC_MARKER, TEST_LOG, and RUNNER_SCRIPT a verified prerequisite by removing the
unconditional “|| true” and propagating or explicitly handling rm failures
before starting a run. Ensure stale fixed-path markers cannot be accepted by
are_tests_done for a new attempt.
- Around line 77-101: Add a wall-clock deadline or detached-runner liveness
check to wait_for_tests, so polling terminates when the runner exits or markers
never appear even if pod_exec continues succeeding. Preserve the existing
consecutive-failure handling and return a nonzero status when the new deadline
or liveness condition is reached.
- Around line 199-202: Update the generated script created by the heredoc in the
e2e command flow so it does not expand QUAY_PASSWORD into local_runner or pass
it through docker login arguments. Use docker login’s --password-stdin option
and provide the password securely at script execution time, while preserving the
existing QUAY_USERNAME and registry login behavior.
- Around line 201-219: The detached runner currently disables error handling for
setup and marker operations. Replace set +e with set -euo pipefail, and scope
any non-fatal handling only around entrypoint so its exit status is explicitly
captured in rc; ensure RC_MARKER and DONE_MARKER are still written after
entrypoint while failures in docker login, oc version, cd, or marker writes
remain fatal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d7fde5e6-e061-484f-85aa-b3b0746c7f62
📒 Files selected for processing (3)
ci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.shci-operator/step-registry/servicemesh/sail-operator/e2e-ocp/servicemesh-sail-operator-e2e-ocp-commands.shci-operator/step-registry/servicemesh/sail-operator/scorecard/servicemesh-sail-operator-scorecard-commands.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- ci-operator/step-registry/servicemesh/sail-operator/scorecard/servicemesh-sail-operator-scorecard-commands.sh
- ci-operator/step-registry/servicemesh/istio/int-tests/servicemesh-istio-int-tests-commands.sh
|
/pj-rehearse pull-ci-openshift-service-mesh-sail-operator-main-ocp-4.22-e2e-ocp-arm-retest |
|
@fjglira: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Temporary rehearsal aid against Insufficient memory on default 1x m5.xlarge guests; drop before merge if kept as a separate change. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/pj-rehearse pull-ci-openshift-service-mesh-sail-operator-main-ocp-4.22-e2e-ocp-arm-retest |
|
@fjglira: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse |
|
@fjglira: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
[REHEARSALNOTIFIER]
A total of 57 jobs have been affected by this change. The above listing is non-exhaustive and limited to 25 jobs. A full list of affected jobs can be found here Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
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
`@ci-operator/step-registry/servicemesh/istio/e2e/hypershift/servicemesh-istio-e2e-hypershift-workflow.yaml`:
- Around line 17-19: Remove the workflow-scoped COMPUTE_NODE_TYPE override from
the hypershift workflow, or relocate it to an explicitly rehearsal-only
job/configuration. Keep the default worker sizing for normal runs and ensure
only the intended rehearsal path provisions m5.2xlarge workers.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0082d55b-7745-410f-bb15-a7652be0ad1c
📒 Files selected for processing (1)
ci-operator/step-registry/servicemesh/istio/e2e/hypershift/servicemesh-istio-e2e-hypershift-workflow.yaml
| # Default hypershift-hostedcluster-create uses 1x m5.xlarge (~16Gi), which often cannot | ||
| # schedule the privileged 8Gi builder pod. Upsize guest workers for headroom. | ||
| COMPUTE_NODE_TYPE: "m5.2xlarge" |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Do not ship the rehearsal-only worker-size override unconditionally.
COMPUTE_NODE_TYPE is workflow-scoped, so every run now provisions m5.2xlarge workers. The PR objective identifies this as temporary rehearsal capacity; remove it before merge or make it an explicit job/rehearsal-specific override to avoid unnecessary resource consumption and region-dependent provisioning failures.
Proposed fix
- # Default hypershift-hostedcluster-create uses 1x m5.xlarge (~16Gi), which often cannot
- # schedule the privileged 8Gi builder pod. Upsize guest workers for headroom.
- COMPUTE_NODE_TYPE: "m5.2xlarge"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Default hypershift-hostedcluster-create uses 1x m5.xlarge (~16Gi), which often cannot | |
| # schedule the privileged 8Gi builder pod. Upsize guest workers for headroom. | |
| COMPUTE_NODE_TYPE: "m5.2xlarge" |
🤖 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
`@ci-operator/step-registry/servicemesh/istio/e2e/hypershift/servicemesh-istio-e2e-hypershift-workflow.yaml`
around lines 17 - 19, Remove the workflow-scoped COMPUTE_NODE_TYPE override from
the hypershift workflow, or relocate it to an explicitly rehearsal-only
job/configuration. Keep the default worker sizing for normal runs and ensure
only the intended rehearsal path provisions m5.2xlarge workers.
|
@fjglira: 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. |
|
Closing this PR because this will not be the final option. We will go for a workaround while we apply Option B (#82374) across all the jobs as is a stronger way to execute the test |
1 participant
Summary
oc rshin Istio integ, Sail e2e, and scorecard steps with detachednohupexecution + done/RC marker polling.Reason
Cluster instability was closing the multi-hour
oc rshWebSocket (close 1006/ unexpected EOF), killing suites mid-run and leaving retries on dirty state.Made with Cursor
Summary by CodeRabbit
nohupbackground processes (started from a copied in-pod runner script) rather than keeping long-lived interactiveoc rshsessions open.DONEmarker and anRCexit-code marker exist in the builder/test pod./tmp/test-run.log) into the Prow build log and saving it toARTIFACT_DIRasdetached-test-run.log.oc execfailures during polling, retrying when completion markers (and previously also JUnit reports) are missing/incomplete, and continuing gracefully when artifact/JUnit copy steps fail.COMPUTE_NODE_TYPEtom5.2xlargeto provide scheduling headroom for the builder pod (instead of the defaultm5.xlarge).