gather-extra: don't let PLATFORM lookup abort entire gather step - #83299
gather-extra: don't let PLATFORM lookup abort entire gather step#83299kaovilai wants to merge 2 commits into
Conversation
|
/pj-rehearse gather-extra Note Responses generated with Claude |
|
@kaovilai: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
WalkthroughThe gather script adds timeout and failure handling to the platform query. It stores and invokes the downloaded ChangesGather command updates
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 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/gather/extra/gather-extra-commands.sh`:
- Line 45: Update the platform lookup and CAPI artifact queuing flow around
PLATFORM and the logic at lines 101–106 so a failed or empty oc infrastructure
lookup cannot produce empty CAPI_PLATFORM resource names. Detect and handle
lookup failure before queuing, either by skipping the CAPI queues or by
assigning a validated fallback platform value, while preserving normal queuing
for valid platform results.
🪄 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: 3b5ea1eb-a24c-4b43-b579-3d3e5ba7bc55
📒 Files selected for processing (2)
.gitpod.ymlci-operator/step-registry/gather/extra/gather-extra-commands.sh
| oc --insecure-skip-tls-verify --request-timeout=5s adm inspect clusteroperators --dest-dir ${ARTIFACT_DIR}/inspect || true | ||
|
|
||
| PLATFORM=$(oc get infrastructure cluster -o jsonpath="{.status.platform}") | ||
| PLATFORM=$(oc --insecure-skip-tls-verify --request-timeout=5s get infrastructure cluster -o jsonpath="{.status.platform}") || true |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 '(^|[[:space:]])PLATFORM[[:space:]]*=' \
ci-operator/step-registry/gather/extra/gather-extra-commands.sh || true
result=$(bash -c 'PLATFORM=previous; PLATFORM=$(false) || true; printf "%s" "$PLATFORM"')
test -z "$result"
rg -n -C 8 'CAPI_PLATFORM|PLATFORM' \
ci-operator/step-registry/opendatahub-io/hypershift/gather-extra/opendatahub-io-hypershift-gather-extra-commands.sh || trueRepository: openshift/release
Length of output: 5760
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- gather-extra-commands.sh ---'
sed -n '1,70p' ci-operator/step-registry/gather/extra/gather-extra-commands.sh
printf '%s\n' '--- all PLATFORM references in the target script ---'
rg -n -C 4 '\bPLATFORM\b|CAPI_PLATFORM' \
ci-operator/step-registry/gather/extra/gather-extra-commands.sh
printf '%s\n' '--- referenced HyperShift consumer ---'
sed -n '55,90p' ci-operator/step-registry/opendatahub-io/hypershift/gather-extra/opendatahub-io-hypershift-gather-extra-commands.sh
printf '%s\n' '--- shell assignment behavior ---'
bash -c 'unset PLATFORM; PLATFORM=$(false) || true; declare -p PLATFORM'
bash -c 'PLATFORM=previous; PLATFORM=$(false) || true; printf "existing=%q\n" "$PLATFORM"'Repository: openshift/release
Length of output: 11793
Handle a failed platform lookup before queuing CAPI artifacts.
This script has no existing PLATFORM fallback. When oc fails without output, PLATFORM and CAPI_PLATFORM become empty. Lines 101–106 then construct invalid CAPI resource names. Skip these queues when the lookup fails or use a validated fallback.
🤖 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/gather/extra/gather-extra-commands.sh` at line 45,
Update the platform lookup and CAPI artifact queuing flow around PLATFORM and
the logic at lines 101–106 so a failed or empty oc infrastructure lookup cannot
produce empty CAPI_PLATFORM resource names. Detect and handle lookup failure
before queuing, either by skipping the CAPI queues or by assigning a validated
fallback platform value, while preserving normal queuing for valid platform
results.
|
@kaovilai: job(s): gather-extra either don't exist or were not found to be affected, and cannot be rehearsed |
|
/pj-rehearse pull-ci-openshift-oadp-operator-oadp-dev-4.22-e2e-test-cli-aws Note Responses generated with Claude |
|
@kaovilai: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-openshift-oadp-operator-oadp-dev-4.22-e2e-test-cli-aws Note Responses generated with Claude |
|
@kaovilai: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-openshift-oadp-operator-oadp-dev-4.22-e2e-test-cli-aws Note Responses generated with Claude |
|
@kaovilai: 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.
🧹 Nitpick comments (1)
ci-operator/step-registry/gather/extra/gather-extra-commands.sh (1)
771-776: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winQuote every
ARTIFACT_DIRexpansion.Line 771 passes an unquoted path to
chmod. Line 776 passes unquoted paths tojqand the output redirection. Whitespace or glob characters inARTIFACT_DIRcan split the path or expand it unexpectedly.- curl -sL https://github.com/stedolan/jq/releases/download/jq-1.6/jq-linux64 >${ARTIFACT_DIR}/jq && chmod ug+x ${ARTIFACT_DIR}/jq + curl -sL https://github.com/stedolan/jq/releases/download/jq-1.6/jq-linux64 >"${ARTIFACT_DIR}/jq" && chmod ug+x "${ARTIFACT_DIR}/jq"Apply the same quoting to all
ARTIFACT_DIRpaths on Line 776.This addresses the reported ShellCheck SC2086 findings.
🤖 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/gather/extra/gather-extra-commands.sh` around lines 771 - 776, Quote every ARTIFACT_DIR expansion in the jq download/chmod command and the jq invocation around the clusteroperators processing, including input and output redirection paths. Update the existing command without changing its jq logic, ensuring paths remain safe when ARTIFACT_DIR contains whitespace or glob characters.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@ci-operator/step-registry/gather/extra/gather-extra-commands.sh`:
- Around line 771-776: Quote every ARTIFACT_DIR expansion in the jq
download/chmod command and the jq invocation around the clusteroperators
processing, including input and output redirection paths. Update the existing
command without changing its jq logic, ensuring paths remain safe when
ARTIFACT_DIR contains whitespace or glob characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 75147e12-bbaa-444d-adc2-b362751668dc
📒 Files selected for processing (1)
ci-operator/step-registry/gather/extra/gather-extra-commands.sh
|
Rehearsal passed on the current head commit (
/pj-rehearse ack Note Responses generated with Claude |
|
@kaovilai: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-merge-bot
Bot
added
the
rehearsals-ack
The bare 'oc get infrastructure cluster' call for PLATFORM detection is missing --insecure-skip-tls-verify/--request-timeout used by every sibling oc call in this script, and unlike them isn't tolerant of failure. Because ci-operator wraps multi-stage step commands in an ambient 'set -e', a single transient API-server hiccup on this one line aborts the entire 886-line gather-extra step immediately, losing all other diagnostic artifact gathering. Confirmed via a rehearse run on openshift#83282 where the raw gather-extra pod log showed only 'Gathering artifacts ...' followed by 'Unable to connect to the server: context deadline exceeded' with a ~12s total runtime -- consistent with this exact line failing before any of the backgrounded queue() calls could run. A sibling periodic job's gather-extra run succeeded cleanly in the same timeframe, confirming this is an intermittent API-server blip rather than a chronic failure. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
The cluster-operator-status-to-JSON fallback path downloads a jq binary to /tmp and chmods it exec, then invokes it directly. Some CI nodes mount /tmp noexec, so the container fails with exit 126 "/tmp/jq: Permission denied" even though the script otherwise ran fine. Confirmed node-dependent rather than chronic: a rehearse run on openshift#83286 (pull-ci-openshift-oadp-operator-oadp-1.5-4.20-e2e-test-aws) hit this, while sibling jobs on the same commit (4.20-e2e-test-cli-aws, 4.20-e2e-test-hcp-aws) ran the identical script without issue. ARTIFACT_DIR is already used elsewhere in this script and is not subject to the same noexec mount, so download+chmod+exec jq there instead. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
b29dac9 to
8830df5
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai 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
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/gather/extra/gather-extra-commands.sh`:
- Line 771: Update the jq download and related commands around the jq
installation flow to quote every ARTIFACT_DIR-based path, including the jq
output path, input and output paths, and chmod operand, preventing whitespace
splitting and glob expansion.
🪄 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: d3166635-2c2c-422e-8112-0dc6be1f67d8
📒 Files selected for processing (1)
ci-operator/step-registry/gather/extra/gather-extra-commands.sh
| fi | ||
| # This is a temporary conversion of cluster operator status to JSON matching the upgrade - may be moved to code in the future | ||
| curl -sL https://github.com/stedolan/jq/releases/download/jq-1.6/jq-linux64 >/tmp/jq && chmod ug+x /tmp/jq | ||
| curl -sL https://github.com/stedolan/jq/releases/download/jq-1.6/jq-linux64 >${ARTIFACT_DIR}/jq && chmod ug+x ${ARTIFACT_DIR}/jq |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
shellcheck -x ci-operator/step-registry/gather/extra/gather-extra-commands.shRepository: openshift/release
Length of output: 50373
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file='ci-operator/step-registry/gather/extra/gather-extra-commands.sh'
printf '%s\n' '--- relevant source ---'
sed -n '755,785p' "$file"
printf '%s\n' '--- ARTIFACT_DIR assignments and relevant uses ---'
rg -n 'ARTIFACT_DIR|clusteroperators\.json|junit_install_status' "$file" | tail -30
printf '%s\n' '--- shellcheck diagnostics for the relevant lines only ---'
shellcheck -x -f gcc "$file" 2>/dev/null | awk -F: '$2 == 771 || $2 == 776'Repository: openshift/release
Length of output: 8838
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
mkdir -p "$tmp/artifacts with space/junit"
touch "$tmp/artifacts with space/clusteroperators.json"
touch "$tmp/artifacts with space/jq"
cat >"$tmp/probe.sh" <<'EOF'
#!/usr/bin/env bash
set -euo pipefail
ARTIFACT_DIR="$1"
set -- ${ARTIFACT_DIR}/clusteroperators.json ${ARTIFACT_DIR}/jq
printf 'unquoted argc=%s\n' "$#"
printf 'unquoted arg=%q\n' "$@"
set -- "${ARTIFACT_DIR}/clusteroperators.json" "${ARTIFACT_DIR}/jq"
printf 'quoted argc=%s\n' "$#"
printf 'quoted arg=%q\n' "$@"
EOF
chmod +x "$tmp/probe.sh"
bash "$tmp/probe.sh" "$tmp/artifacts with space"
printf '%s\n' '--- ARTIFACT_DIR definitions in repository metadata and scripts ---'
rg -n --hidden --glob '!*.md' --glob '!vendor/**' \
'ARTIFACT_DIR[[:space:]]*(=|:)|export[[:space:]]+ARTIFACT_DIR|artifacts?[-_]dir' . \
| head -120Repository: openshift/release
Length of output: 19517
Quote all ARTIFACT_DIR expansions.
At lines 771 and 776, unquoted paths split when ARTIFACT_DIR contains whitespace and expand glob characters. Quote the jq path, input path, output path, and chmod operand.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 771-771: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 771-771: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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/gather/extra/gather-extra-commands.sh` at line 771,
Update the jq download and related commands around the jq installation flow to
quote every ARTIFACT_DIR-based path, including the jq output path, input and
output paths, and chmod operand, preventing whitespace splitting and glob
expansion.
Source: Linters/SAST tools
openshift-merge-bot
Bot
removed
the
rehearsals-ack
|
[REHEARSALNOTIFIER]
A total of 44407 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: |
|
@kaovilai: 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. |
1 participant
What
ci-operator/step-registry/gather/extra/gather-extra-commands.shdetects the cluster platform via:PLATFORM=$(oc get infrastructure cluster -o jsonpath="{.status.platform}")This is the only early, sequential
occall in the script missing both--insecure-skip-tls-verifyand--request-timeout, unlike every siblingoccall around it (lines 39-43, 56+). It's also not tolerant of failure, unlike theadm inspect clusteroperatorscall immediately above it (|| true).Because ci-operator wraps multi-stage step commands in an ambient
set -e, a single transient API-server hiccup on this one line aborts the entire 886-linegather-extrastep immediately — losing every other diagnostic artifact this step would otherwise have gathered (node status, pod lists, clusteroperators, config resources, etc).Fix: add the same
--insecure-skip-tls-verify --request-timeout=5sflags used everywhere else in this file, and|| trueso a timeout here degrades to an emptyPLATFORM/CAPI_PLATFORM(which only gates a few platform-specificqueue()'d — i.e. backgrounded, non-fatal — resource lookups further down) instead of killing the whole step.Evidence
Found via a
/pj-rehearserun on #83282 (unrelated oadp-operator bundle-promotion work). Thegather-extrastep's raw pod log showed only:with a ~12s total step runtime — consistent with this exact unguarded line failing before any of the script's backgrounded
queue()calls could even start.Cross-checked against a same-timeframe periodic job's
gather-extrarun (unrelated to #83282), which completed cleanly in ~6m41s — confirming this is an intermittent API-server blip hitting an unguarded call, not a chronic/systemic failure of the step.Test plan
/pj-rehearsethis PR and confirmgather-extrastill gathers its full artifact set on a normal run (no regression from the added flags)Summary by CodeRabbit
gather-extrastep now limits the infrastructure platform lookup to 5 seconds and tolerates API failures. Remaining diagnostic artifact collection can continue after a transient API-server timeout.jqbinary from${ARTIFACT_DIR}instead of/tmp.