Add readiness checks before running ocp-virt-validation-checkup - #83157
Add readiness checks before running ocp-virt-validation-checkup#83157orenc1 wants to merge 1 commit into
Conversation
Wait for ODF StorageClass and HCO CLI download route to be ready before starting the validation Job. The StorageCluster can report Ready before all storage classes are created, and the HCO CLI download route may not be serving when the checkup tries to download virtctl, both causing immediate Job failures. Signed-off-by: Oren Cohen <ocohen@redhat.com>
WalkthroughThe validation script now waits for the configured StorageClass and checks HCO CLI route readiness before continuing. ChangesValidation readiness checks
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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: orenc1 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
|
[REHEARSALNOTIFIER]
Prior to this PR being merged, you will need to either run and acknowledge or opt to skip these rehearsals. Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/agent-qe/baremetal/install/ove/disconnected/run-validation-checkup/agent-qe-baremetal-install-ove-disconnected-run-validation-checkup-commands.sh`:
- Around line 26-38: Update the HCO CLI route readiness loop around COUNTER to
track an explicit ROUTE_READY flag, set it only when the curl probe returns an
accepted status, and fail with a clear error after the 120-second deadline if it
remains unset. Ensure execution cannot continue into validation when the route
is unavailable.
- Line 28: Update the curl invocation in the route probe to include bounded
connection and total request timeouts, using values such as --connect-timeout 3
and --max-time 5, so stalled DNS, connection, TLS, or response operations cannot
delay retry counter progress.
🪄 Autofix
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: 9e43ee10-169d-4445-9bcf-568dfdcabc96
📒 Files selected for processing (1)
ci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/run-validation-checkup/agent-qe-baremetal-install-ove-disconnected-run-validation-checkup-commands.sh
| COUNTER=0 | ||
| while [ $COUNTER -lt 120 ]; do | ||
| if curl -sk -o /dev/null -w '%{http_code}' "https://${CLI_ROUTE}/" 2>/dev/null | grep -qE '^(200|301|302)'; then | ||
| echo "HCO CLI download route is ready" | ||
| break | ||
| fi | ||
| sleep 10 | ||
| COUNTER=$((COUNTER + 10)) | ||
| echo "Waiting ${COUNTER}s for HCO CLI route to respond..." | ||
| done | ||
| else | ||
| echo "WARNING: HCO CLI download route not found, skipping readiness check" | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail closed when the HCO CLI download route is not ready.
After COUNTER reaches 120, the loop exits and execution continues. No branch verifies that a probe succeeded. The validation flow can therefore proceed while the route still returns errors.
Track an explicit ROUTE_READY flag and exit with a clear error when the deadline expires. If continuing is intentional, make that fallback explicit and verify that the downstream validation flow supports it.
Proposed fix
COUNTER=0
+ ROUTE_READY=false
while [ $COUNTER -lt 120 ]; do
if curl -sk -o /dev/null -w '%{http_code}' "https://${CLI_ROUTE}/" 2>/dev/null | grep -qE '^(200|301|302)'; then
+ ROUTE_READY=true
echo "HCO CLI download route is ready"
break
@@
echo "Waiting ${COUNTER}s for HCO CLI route to respond..."
done
+ if [[ "${ROUTE_READY}" != true ]]; then
+ echo "ERROR: HCO CLI download route did not become ready within 120s"
+ exit 1
+ fi📝 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.
| COUNTER=0 | |
| while [ $COUNTER -lt 120 ]; do | |
| if curl -sk -o /dev/null -w '%{http_code}' "https://${CLI_ROUTE}/" 2>/dev/null | grep -qE '^(200|301|302)'; then | |
| echo "HCO CLI download route is ready" | |
| break | |
| fi | |
| sleep 10 | |
| COUNTER=$((COUNTER + 10)) | |
| echo "Waiting ${COUNTER}s for HCO CLI route to respond..." | |
| done | |
| else | |
| echo "WARNING: HCO CLI download route not found, skipping readiness check" | |
| fi | |
| COUNTER=0 | |
| ROUTE_READY=false | |
| while [ $COUNTER -lt 120 ]; do | |
| if curl -sk -o /dev/null -w '%{http_code}' "https://${CLI_ROUTE}/" 2>/dev/null | grep -qE '^(200|301|302)'; then | |
| ROUTE_READY=true | |
| echo "HCO CLI download route is ready" | |
| break | |
| fi | |
| sleep 10 | |
| COUNTER=$((COUNTER + 10)) | |
| echo "Waiting ${COUNTER}s for HCO CLI route to respond..." | |
| done | |
| if [[ "${ROUTE_READY}" != true ]]; then | |
| echo "ERROR: HCO CLI download route did not become ready within 120s" | |
| exit 1 | |
| fi | |
| else | |
| echo "WARNING: HCO CLI download route not found, skipping readiness check" | |
| fi |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 27-27: curl is invoked with -k/--insecure, which disables TLS certificate verification and exposes the connection to man-in-the-middle attacks. Remove the insecure flag and let curl validate the server certificate; if you need to trust a private CA, pin it with --cacert instead.
Context: curl -sk -o /dev/null -w '%{http_code}' "https://${CLI_ROUTE}/"
Note: [CWE-295] Improper Certificate Validation.
(curl-insecure-tls-bash)
🤖 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/agent-qe/baremetal/install/ove/disconnected/run-validation-checkup/agent-qe-baremetal-install-ove-disconnected-run-validation-checkup-commands.sh`
around lines 26 - 38, Update the HCO CLI route readiness loop around COUNTER to
track an explicit ROUTE_READY flag, set it only when the curl probe returns an
accepted status, and fail with a clear error after the 120-second deadline if it
remains unset. Ensure execution cannot continue into validation when the route
is unavailable.
| if [[ -n "${CLI_ROUTE}" ]]; then | ||
| COUNTER=0 | ||
| while [ $COUNTER -lt 120 ]; do | ||
| if curl -sk -o /dev/null -w '%{http_code}' "https://${CLI_ROUTE}/" 2>/dev/null | grep -qE '^(200|301|302)'; then |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
curl_help="$(curl --help all)"
grep -q -- '--connect-timeout' <<<"${curl_help}"
grep -q -- '--max-time' <<<"${curl_help}"Repository: openshift/release
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='ci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/run-validation-checkup/agent-qe-baremetal-install-ove-disconnected-run-validation-checkup-commands.sh'
printf '%s\n' '--- target script ---'
cat -n "$file" | sed -n '1,90p'
printf '%s\n' '--- diff summary ---'
git diff --stat -- "$file"
printf '%s\n' '--- timeout and curl references ---'
rg -n -C 3 'curl|120|CLI_ROUTE|set -' "$file" ci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/run-validation-checkupRepository: openshift/release
Length of output: 11763
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- curl timeout documentation ---'
curl --manual 2>/dev/null |
grep -i -E -A 4 -B 4 'maximum time.*complete|default.*timeout|--max-time|--connect-timeout' |
head -80
printf '%s\n' '--- no curl deadline: external timeout is required ---'
set +e
timeout 1s curl -sS -o /dev/null -w '%{http_code}' 'file:///dev/zero' >/tmp/curl-no-max-time.out 2>/dev/null
status_no_max_time=$?
set -e
printf 'status=%s output_bytes=%s\n' \
"$status_no_max_time" "$(wc -c </tmp/curl-no-max-time.out)"
printf '%s\n' '--- curl max-time enforces a request deadline ---'
set +e
curl -sS --max-time 0.2 -o /dev/null -w '%{http_code}' 'file:///dev/zero' >/tmp/curl-max-time.out 2>/dev/null
status_max_time=$?
set -e
printf 'status=%s output_bytes=%s\n' \
"$status_max_time" "$(wc -c </tmp/curl-max-time.out)"
test "$status_no_max_time" -eq 124
test "$status_max_time" -eq 28Repository: openshift/release
Length of output: 190
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- curl without --max-time ---'
timeout 1s curl -sS -o /dev/null -w '%{http_code}' \
'file:///dev/zero' >/tmp/curl-no-max-time.out 2>/dev/null
status_no_max_time=$?
printf 'status=%s output_bytes=%s\n' \
"$status_no_max_time" "$(wc -c </tmp/curl-no-max-time.out)"
printf '%s\n' '--- curl with --max-time ---'
curl -sS --max-time 0.2 -o /dev/null -w '%{http_code}' \
'file:///dev/zero' >/tmp/curl-max-time.out 2>/dev/null
status_max_time=$?
printf 'status=%s output_bytes=%s\n' \
"$status_max_time" "$(wc -c </tmp/curl-max-time.out)"
test "$status_no_max_time" -eq 124
test "$status_max_time" -eq 28Repository: openshift/release
Length of output: 267
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- curl without --max-time ---'
timeout 1s curl -sS -o /dev/null -w '%{http_code}' \
'file:///dev/zero' >/tmp/curl-no-max-time.out 2>/dev/null
status_no_max_time=$?
printf 'status=%s output_bytes=%s\n' \
"$status_no_max_time" "$(wc -c </tmp/curl-no-max-time.out)"
printf '%s\n' '--- curl with --max-time ---'
curl -sS --max-time 0.2 -o /dev/null -w '%{http_code}' \
'file:///dev/zero' >/tmp/curl-max-time.out 2>/dev/null
status_max_time=$?
printf 'status=%s output_bytes=%s\n' \
"$status_no_max_time" "$(wc -c </tmp/curl-no-max-time.out)"
test "$status_no_max_time" -eq 124
test "$status_max_time" -eq 28Repository: openshift/release
Length of output: 268
🌐 Web query:
Official curl documentation --max-time default zero --connect-timeout timeout
💡 Result:
In curl, the --max-time option has no default timeout; it effectively defaults to zero, meaning there is no time limit on the operation [1]. When --max-time is set (or left at its default of zero), curl will allow the operation to continue indefinitely until completion or until another network-related error occurs [1]. Regarding --connect-timeout, this option limits only the connection phase, which includes DNS lookup and TCP, TLS, or QUIC handshakes [2][3]. If curl does not successfully connect within the time specified by --connect-timeout, it will exit with a timeout error (exit code 28) [3][4]. There is no default "timeout" value for --connect-timeout that forces a connection to fail; if the option is not used, curl relies on the operating system's default connection timeout [1][5]. Both options accept decimal values for sub-second precision (e.g., --max-time 0.5 or --connect-timeout 2.781) [1][5][4].
Citations:
- 1: https://everything.curl.dev/usingcurl/timeouts.html
- 2: https://man7.org/linux/man-pages/man1/curl.1.html
- 3: https://github.com/curl/everything-curl/blob/master/usingcurl/timeouts.md
- 4: https://curl.se/docs/manpage.html
- 5: https://everything.curl.dev/usingcurl/connections/timeout.html
Bound each route probe.
The retry counter advances only after curl returns. Without a default operation timeout, a stalled DNS lookup, connection, TLS handshake, or response can exceed the intended 120-second limit. Add per-request --connect-timeout and --max-time values, such as --connect-timeout 3 --max-time 5.
🤖 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/agent-qe/baremetal/install/ove/disconnected/run-validation-checkup/agent-qe-baremetal-install-ove-disconnected-run-validation-checkup-commands.sh`
at line 28, Update the curl invocation in the route probe to include bounded
connection and total request timeouts, using values such as --connect-timeout 3
and --max-time 5, so stalled DNS, connection, TLS, or response operations cannot
delay retry counter progress.
|
/pj-rehearse periodic-ci-openshift-eng-agent-qe-infra-release-4.21-amd64-nightly-baremetal-ove-compact |
|
@orenc1: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@orenc1: The following test 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. |
|
/pj-rehearse periodic-ci-openshift-eng-agent-qe-infra-release-4.21-amd64-nightly-baremetal-ove-compact |
|
@orenc1: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
1 participant
Wait for ODF StorageClass and HCO CLI download route to be ready before starting the validation Job.
The StorageCluster can report Ready before all storage classes are created, and the HCO CLI download route may not be serving when the checkup tries to download virtctl, both causing immediate Job failures.
Summary by CodeRabbit
The disconnected bare-metal OVE validation job now waits for required dependencies before running
ocp-virt-validation-checkup:virtctl.These checks reduce validation Job failures caused by delayed storage and HCO CLI availability.