This repository was archived by the owner on Sep 9, 2026. It is now read-only.
OSAC-1305: fix CI stability issues in installer scripts - #233
Merged
openshift-merge-bot[bot] merged 1 commit intoJun 8, 2026
Merged
openshift-merge-bot[bot] merged 1 commit into
openshift-merge-bot[bot] merged 1 commit into
Conversation
|
@omer-vishlitzky: This pull request references OSAC-1305 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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. |
|
Warning Review limit reached
More reviews will be available in 58 minutes and 41 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (7)
WalkthroughAdds HTTP retry helpers; makes hub-access kubeconfig server-specific; splits prepare-fulfillment-service into concurrent create_hub, sync_aap_project, and publish_templates; hardens refresh rollouts and fulfillment-controller lifecycle; improves CaaS agent network provisioning, ISO polling, and diagnostics. ChangesInstaller Infrastructure and Fulfillment Service Improvements
Security: Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (7 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@scripts/create-hub-access-kubeconfig.sh`:
- Line 24: Summary: Quote the server_name variable expansion in the here-doc
output redirection to avoid potential word-splitting. Fix: change the
redirection target in the cat <<EOF line that writes
/tmp/kubeconfig.hub-access.${server_name} to use a quoted expansion (e.g.
/tmp/kubeconfig.hub-access."${server_name}" or
"/tmp/kubeconfig.hub-access.${server_name}") and apply the same quoting pattern
to any other occurrences of server_name in this script; locate the cat <<EOF
>/tmp/kubeconfig.hub-access.${server_name} line to make the change.
In `@scripts/prepare-fulfillment-service.sh`:
- Line 42: Check that the AAP_TOKEN variable (set by the oc get secret command)
is non-empty and that the oc command succeeded before using it: after the line
that sets AAP_TOKEN, add a check that AAP_TOKEN is not empty (e.g., test -n
"$AAP_TOKEN") and if it is empty print a clear error to stderr including context
("osac-aap-api-token secret missing or token empty") and exit non-zero; also
guard/fail fast if the oc command returned a non-zero status so the script does
not continue with an invalid token.
- Line 120: The AAP_TOKEN assignment (AAP_TOKEN=$(oc get secret
osac-aap-api-token -n "${INSTALLER_NAMESPACE}" -o jsonpath='{.data.token}' |
base64 -d)) must be validated immediately after retrieval: add a check for empty
or unset AAP_TOKEN (e.g., test -z "$AAP_TOKEN") and if empty, emit a clear error
to stderr and exit non‑zero before any call that uses AAP_TOKEN (for example
before sync_aap_project or any API invocation). Ensure the error message names
the secret and namespace and suggests checking secret data so failures are
explicit and early.
In `@scripts/refresh-after-snapshot.sh`:
- Line 167: The oc scale call that shuts down the fulfillment-controller is
currently fail-open because it appends "|| true" and discards errors; change the
handling of oc scale deploy/fulfillment-controller -n "${INSTALLER_NAMESPACE}"
--replicas=0 to detect failures (remove "|| true"), check the command's exit
status, and on non-zero print a clear error and exit non-zero so the script
aborts rather than continuing with the controller running; apply the same fix to
the second identical call (the other oc scale deploy/fulfillment-controller
occurrence) so both scale-down attempts are treated as fatal on failure.
- Around line 170-176: The script currently mutates the checked-in kustomize
base by running sed to remove job.yaml from
base/osac-aap/config/base/kustomization.yaml and moving it back in place,
leaving the repo dirty and breaking subsequent kustomize builds; instead, change
the logic to operate on a temporary copy of the kustomization (or pass a
filtered stream) so the original base/osac-aap/config/base/kustomization.yaml is
never overwritten—use the temporary file only for oc apply -k (or feed
kustomize/oc from stdout) and ensure any temp is cleaned up; update references
to the sed pipeline that targets kustomization.yaml and the oc apply -k
"overlays/${INSTALLER_KUSTOMIZE_OVERLAY}" invocation to use the non-mutating
temp copy.
In `@scripts/setup-caas-agents.sh`:
- Around line 131-143: The readiness loop and ISO download use unbounded curl
calls and the shared helper lacks timeouts; update the curl invocation in
scripts/setup-caas-agents.sh (the loop that calls curl -L '${ISO_URL}' and the
subsequent download to ${AGENT_VM_STORAGE_DIR}/discovery.iso) to include
sensible --connect-timeout and --max-time values (e.g. --connect-timeout 5
--max-time 30) and modify the scripts/lib.sh helper function that wraps curl
(the function using curl -ksS --fail-with-body "$@") to accept/default to the
same timeout flags so callers that don't pass timeouts inherit bounded behavior;
ensure existing callers still work by preserving argument ordering and allowing
overrides.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 0ff72e7c-1261-4611-91d5-30a87c548d8f
📒 Files selected for processing (6)
scripts/create-hub-access-kubeconfig.shscripts/lib.shscripts/prepare-fulfillment-service.shscripts/refresh-after-snapshot.shscripts/setup-caas-agents.shscripts/teardown.sh
| # Trigger a one-time publish-templates AAP job | ||
| AAP_ROUTE_HOST=$(oc get routes -n "${INSTALLER_NAMESPACE}" --no-headers osac-aap -o jsonpath='{.spec.host}') | ||
| AAP_URL="https://${AAP_ROUTE_HOST}" | ||
| AAP_TOKEN=$(oc get secret osac-aap-api-token -n "${INSTALLER_NAMESPACE}" -o jsonpath='{.data.token}' | base64 -d) |
There was a problem hiding this comment.
Validate AAP token is non-empty before API calls.
Risk: If the osac-aap-api-token secret is missing or has an empty token field, subsequent API calls will fail with authentication errors that may be harder to diagnose.
Impact: Delayed failure detection, unclear error messages in CI logs.
🛡️ Proposed fix
AAP_TOKEN=$(oc get secret osac-aap-api-token -n "${INSTALLER_NAMESPACE}" -o jsonpath='{.data.token}' | base64 -d)
+[[ -z "${AAP_TOKEN}" ]] && { echo "ERROR: osac-aap-api-token secret has empty token"; exit 1; }🤖 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 `@scripts/prepare-fulfillment-service.sh` at line 42, Check that the AAP_TOKEN
variable (set by the oc get secret command) is non-empty and that the oc command
succeeded before using it: after the line that sets AAP_TOKEN, add a check that
AAP_TOKEN is not empty (e.g., test -n "$AAP_TOKEN") and if it is empty print a
clear error to stderr including context ("osac-aap-api-token secret missing or
token empty") and exit non-zero; also guard/fail fast if the oc command returned
a non-zero status so the script does not continue with an invalid token.
| wait ${pid_creds} || failed=1 | ||
| if (( failed )); then echo "ERROR: Failed to create fulfillment credentials"; exit 1; fi | ||
|
|
||
| oc scale deploy/fulfillment-controller -n "${INSTALLER_NAMESPACE}" --replicas=0 2>/dev/null || true |
There was a problem hiding this comment.
Major: the fulfillment-controller freeze step is fail-open.
Both oc scale ... --replicas=0 calls discard every failure. If either scale-down is rejected or the deployment lookup breaks, the script keeps going with fulfillment-controller still running, which reopens the rollout/cache race this guard is supposed to block.
Suggested fix
-oc scale deploy/fulfillment-controller -n "${INSTALLER_NAMESPACE}" --replicas=0 2>/dev/null || true
+oc scale deploy/fulfillment-controller -n "${INSTALLER_NAMESPACE}" --replicas=0
...
-oc scale deploy/fulfillment-controller -n "${INSTALLER_NAMESPACE}" --replicas=0 2>/dev/null || true
+oc scale deploy/fulfillment-controller -n "${INSTALLER_NAMESPACE}" --replicas=0Also applies to: 177-177
🤖 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 `@scripts/refresh-after-snapshot.sh` at line 167, The oc scale call that shuts
down the fulfillment-controller is currently fail-open because it appends "||
true" and discards errors; change the handling of oc scale
deploy/fulfillment-controller -n "${INSTALLER_NAMESPACE}" --replicas=0 to detect
failures (remove "|| true"), check the command's exit status, and on non-zero
print a clear error and exit non-zero so the script aborts rather than
continuing with the controller running; apply the same fix to the second
identical call (the other oc scale deploy/fulfillment-controller occurrence) so
both scale-down attempts are treated as fatal on failure.
Comment on lines
+170
to
176
| # Exclude only the bootstrap job — it's redundant on snapshot boot and races | ||
| # the operator. The AnsibleAutomationPlatform CR (aap.yaml) IS included because | ||
| # it carries probe settings (task_readiness_period). Including it consolidates | ||
| # operator triggers into one reconciliation instead of a separate oc patch. | ||
| sed '/job\.yaml/d' base/osac-aap/config/base/kustomization.yaml > base/osac-aap/config/base/kustomization.yaml.tmp \ | ||
| && mv base/osac-aap/config/base/kustomization.yaml.tmp base/osac-aap/config/base/kustomization.yaml | ||
| oc apply -k "overlays/${INSTALLER_KUSTOMIZE_OVERLAY}" |
There was a problem hiding this comment.
Major: this mutates the checked-out kustomize base and never restores it.
sed ... && mv ... rewrites base/osac-aap/config/base/kustomization.yaml in place. After one refresh, later kustomize build or deploy commands from the same checkout silently keep job.yaml removed, and any failure after Line 176 leaves the workspace dirty.
Suggested fix
+backup_kustomization="$(mktemp)"
+cp base/osac-aap/config/base/kustomization.yaml "${backup_kustomization}"
+trap 'cp "${backup_kustomization}" base/osac-aap/config/base/kustomization.yaml; rm -f "${backup_kustomization}"' EXIT
+
sed '/job\.yaml/d' base/osac-aap/config/base/kustomization.yaml > base/osac-aap/config/base/kustomization.yaml.tmp \
&& mv base/osac-aap/config/base/kustomization.yaml.tmp base/osac-aap/config/base/kustomization.yaml
oc apply -k "overlays/${INSTALLER_KUSTOMIZE_OVERLAY}"As per coding guidelines, validation runs kustomize build against the checked-in tree, so in-place edits under base/ change subsequent builds from the same checkout.
🤖 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 `@scripts/refresh-after-snapshot.sh` around lines 170 - 176, The script
currently mutates the checked-in kustomize base by running sed to remove
job.yaml from base/osac-aap/config/base/kustomization.yaml and moving it back in
place, leaving the repo dirty and breaking subsequent kustomize builds; instead,
change the logic to operate on a temporary copy of the kustomization (or pass a
filtered stream) so the original base/osac-aap/config/base/kustomization.yaml is
never overwritten—use the temporary file only for oc apply -k (or feed
kustomize/oc from stdout) and ensure any temp is cleaned up; update references
to the sed pipeline that targets kustomization.yaml and the oc apply -k
"overlays/${INSTALLER_KUSTOMIZE_OVERLAY}" invocation to use the non-mutating
temp copy.
Source: Coding guidelines
Comment on lines
+131
to
+143
| dnf install -y virt-install | ||
|
|
||
| mkdir -p ${AGENT_VM_STORAGE_DIR} | ||
|
|
||
| echo "Waiting for assisted-image-service to be ready..." | ||
| for attempt in \$(seq 1 30); do | ||
| HTTP_CODE=\$(curl -k -s -o /dev/null -w "%{http_code}" -L '${ISO_URL}') || true | ||
| [[ "\${HTTP_CODE}" == "200" ]] && break | ||
| echo " attempt \${attempt}/30 - HTTP \${HTTP_CODE}, retrying in 10s..." | ||
| sleep 10 | ||
| done | ||
| [[ "\${HTTP_CODE}" != "200" ]] && { echo "ERROR: assisted-image-service not ready after 30 attempts (last HTTP \${HTTP_CODE})"; exit 1; } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether curl calls in this path already enforce bounded timeouts.
rg -n -C2 --type=sh 'curl .*ISO_URL|--connect-timeout|--max-time|--fail-with-body' scripts/setup-caas-agents.sh scripts/lib.shRepository: osac-project/osac-installer
Length of output: 1503
Major: Add bounded timeouts to curl to prevent readiness loop / ISO download from hanging CI
Unbounded curl calls can stall indefinitely on network issues, turning a transient outage into a CI/automation denial-of-service (runner/resource exhaustion) and blocking agent provisioning past the intended retry window.
scripts/setup-caas-agents.shreadiness loop (curl ... '${ISO_URL}'at ~line 137) lacks--connect-timeout/--max-time, so the “30 attempts” window may never complete.scripts/setup-caas-agents.shISO download (curl ... -o ${AGENT_VM_STORAGE_DIR}/discovery.iso '${ISO_URL}'at ~line 145) also lacks time bounds.scripts/lib.sh’scurl -ksS --fail-with-body "$@"helper likewise doesn’t enforce timeouts, so callers that don’t pass them inherit unbounded behavior.
Suggested fix
- HTTP_CODE=$(curl -k -s -o /dev/null -w "%{http_code}" -L '${ISO_URL}') || true
+ HTTP_CODE=$(curl -k -s --connect-timeout 5 --max-time 20 -o /dev/null -w "%{http_code}" -L '${ISO_URL}') || true
@@
-curl -k -L --fail-with-body -o ${AGENT_VM_STORAGE_DIR}/discovery.iso '${ISO_URL}'
+curl -k -L --fail-with-body --connect-timeout 10 --max-time 300 -o "${AGENT_VM_STORAGE_DIR}/discovery.iso" '${ISO_URL}'🤖 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 `@scripts/setup-caas-agents.sh` around lines 131 - 143, The readiness loop and
ISO download use unbounded curl calls and the shared helper lacks timeouts;
update the curl invocation in scripts/setup-caas-agents.sh (the loop that calls
curl -L '${ISO_URL}' and the subsequent download to
${AGENT_VM_STORAGE_DIR}/discovery.iso) to include sensible --connect-timeout and
--max-time values (e.g. --connect-timeout 5 --max-time 30) and modify the
scripts/lib.sh helper function that wraps curl (the function using curl -ksS
--fail-with-body "$@") to accept/default to the same timeout flags so callers
that don't pass timeouts inherit bounded behavior; ensure existing callers still
work by preserving argument ordering and allowing overrides.
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 (1)
scripts/lib.sh (1)
92-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSecurity risk: TLS certificate verification disabled.
Line 99 uses
curl -k, which disables SSL certificate verification and allows man-in-the-middle attacks. While this may be intentional (similar to theOSAC_AAP_INSECURE_SKIP_VERIFYsetting across overlays due to missing CA-bundle support), it represents a significant security posture degradation.Impact: An attacker with network access can intercept and modify HTTPS traffic to AAP and fulfillment APIs, potentially stealing credentials or corrupting configuration data.
Additionally, consider adding curl timeout.
Individual curl calls lack
--max-timeor--connect-timeout, so a hanging connection could delay the retry loop indefinitely. Adding a timeout would improve resilience:- curl -ksS --fail-with-body "$@" && return 0 + curl -ksS --fail-with-body --max-time 30 "$@" && return 0🤖 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 `@scripts/lib.sh` around lines 92 - 107, The http_retry function currently forces curl to skip TLS verification with -k and lacks timeouts; change it so TLS verification is enabled by default (remove -k/-kS) and make insecure-skip-verify configurable via an environment variable (e.g., AAP_INSECURE_SKIP_VERIFY) that, when truthy, adds --insecure; also add sensible timeout flags (e.g., --connect-timeout and --max-time) driven by variables or defaults and use those variables in the curl invocation inside http_retry (refer to the http_retry function and its local params err_msg, retries, interval) so callers can control or rely on safe defaults.Source: Learnings
🤖 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 `@scripts/lib.sh`:
- Around line 92-107: The http_retry function currently forces curl to skip TLS
verification with -k and lacks timeouts; change it so TLS verification is
enabled by default (remove -k/-kS) and make insecure-skip-verify configurable
via an environment variable (e.g., AAP_INSECURE_SKIP_VERIFY) that, when truthy,
adds --insecure; also add sensible timeout flags (e.g., --connect-timeout and
--max-time) driven by variables or defaults and use those variables in the curl
invocation inside http_retry (refer to the http_retry function and its local
params err_msg, retries, interval) so callers can control or rely on safe
defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 9fc09d56-e420-4e26-a7be-b7619556b345
📒 Files selected for processing (1)
scripts/lib.sh
danmanor
approved these changes
Jun 7, 2026
danmanor
approved these changes
Jun 7, 2026
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 (2)
scripts/prepare-fulfillment-service.sh (2)
170-178:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMajor: the retry path can fail while the AAP project update is still progressing.
Lines 170-178 only give the forced project sync 60s, but Lines 87-101 already allow 300s for the same controller operation. On a slow cluster this exits with
Project sync failed before retryeven though the update is still converging, so the new retry logic loses the recovery it was added to provide.⏱️ Minimal fix
- while (( SECONDS - _proj_start < 60 )); do + while (( SECONDS - _proj_start < 300 )); do _proj_status=$(http_json "Failed to poll project ${PROJECT_ID} status" 1 0 '.status // empty' \ -H "Authorization: Bearer ${AAP_TOKEN}" \ "${AAP_URL}/api/controller/v2/projects/${PROJECT_ID}/") || true [[ "${_proj_status}" == "successful" ]] && break sleep 5 done - [[ "${_proj_status}" != "successful" ]] && { echo "ERROR: Project sync failed before retry"; exit 1; } + [[ "${_proj_status}" != "successful" ]] && { echo "ERROR: Project sync failed before retry after 300s (last status: ${_proj_status})"; exit 1; }🤖 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 `@scripts/prepare-fulfillment-service.sh` around lines 170 - 178, The retry loop that polls project status (_proj_start, _proj_status) uses a 60s timeout which is shorter than the earlier 300s wait and can fail while the AAP project update is still progressing; update the loop to use the same timeout and polling behavior as the earlier controller wait (i.e., extend the SECONDS comparison to 300 or reuse the same polling logic used in lines 87-101), continue calling http_json with the same Authorization headers and break when _proj_status == "successful", and only error/exit if the extended timeout elapses without success.
116-121:⚠️ Potential issue | 🔴 CriticalCritical: Remove
set -xfrompublish_templates()to prevent AAP token/headers leaking into logs
publish_templates()enablesset -xbeforeAAP_TOKENis decoded and before calls that pass-H "Authorization: Bearer ${AAP_TOKEN}"; bash xtrace will print the decoded token and bearer header values into CI logs (direct credential exposure).- Also, the retry “force project update” wait is capped at 60s (
while ... < 60) vs 300s insync_aap_project()(< 300), making recoverable sync delays fail the publish retry path.🔐 Minimal fix
publish_templates() { - set -x local AAP_ROUTE_HOST AAP_URL AAP_TOKEN JT_ID PROJECT_ID🤖 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 `@scripts/prepare-fulfillment-service.sh` around lines 116 - 121, publish_templates() currently enables bash xtrace (set -x) before decoding AAP_TOKEN and before curl calls that send -H "Authorization: Bearer ${AAP_TOKEN}", which will leak credentials into CI logs; remove the global set -x from publish_templates() (or move it so that it is only enabled after AAP_TOKEN is decoded and all Authorization headers are no longer printed, with corresponding set +x around sensitive sections) and ensure AAP_TOKEN decoding and any curl -H "Authorization: Bearer ${AAP_TOKEN}" invocations occur with xtrace disabled; also change the retry loop in publish_templates() that currently uses a timeout check of "< 60" to use "< 300" to match sync_aap_project() behavior so the force-project-update retry window is consistent.
🤖 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 `@scripts/prepare-fulfillment-service.sh`:
- Around line 170-178: The retry loop that polls project status (_proj_start,
_proj_status) uses a 60s timeout which is shorter than the earlier 300s wait and
can fail while the AAP project update is still progressing; update the loop to
use the same timeout and polling behavior as the earlier controller wait (i.e.,
extend the SECONDS comparison to 300 or reuse the same polling logic used in
lines 87-101), continue calling http_json with the same Authorization headers
and break when _proj_status == "successful", and only error/exit if the extended
timeout elapses without success.
- Around line 116-121: publish_templates() currently enables bash xtrace (set
-x) before decoding AAP_TOKEN and before curl calls that send -H "Authorization:
Bearer ${AAP_TOKEN}", which will leak credentials into CI logs; remove the
global set -x from publish_templates() (or move it so that it is only enabled
after AAP_TOKEN is decoded and all Authorization headers are no longer printed,
with corresponding set +x around sensitive sections) and ensure AAP_TOKEN
decoding and any curl -H "Authorization: Bearer ${AAP_TOKEN}" invocations occur
with xtrace disabled; also change the retry loop in publish_templates() that
currently uses a timeout check of "< 60" to use "< 300" to match
sync_aap_project() behavior so the force-project-update retry window is
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: e4373d62-633b-43b9-9cb0-3ef9279efb62
📒 Files selected for processing (1)
scripts/prepare-fulfillment-service.sh
danmanor
approved these changes
Jun 7, 2026
Contributor
Author
|
/hold |
omer-vishlitzky
force-pushed
the
OSAC-1305/ci-stability-fixes
branch
2 times, most recently
from
June 7, 2026 21:58
468531f to
727ec41
Compare
Shared wrappers for HTTP requests with structured retry logic.
http_retry retries a curl request N times with --fail-with-body,
http_json adds jq parsing on top. Both print a clear ERROR on
persistent failure. Used by prepare-fulfillment-service.sh to
replace raw curl calls that silently swallowed errors.
OSAC-1305
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
prepare-fulfillment-service: fail-fast error handling and AAP project sync
Replace silent error swallowing with fail-fast http_json/http_retry
wrappers. Fixes multiple CI stability issues found through stress testing:
- Silent curl failures masked by || "" fallbacks now fail immediately
- Detect stale AAP project after snapshot boot (EDA misses Secret
change because activation pods aren't running yet) — conditionally
PATCH scm_url/scm_branch, force sync, verify scm_revision
- Detect failed/error project status during sync and re-trigger the
update instead of passively waiting 300s for the scheduled retry
- Add 3-attempt retry loop for publish-templates job with forced
project update between attempts (handles receptor worker stream
failures during controller-task rolling updates)
OSAC-1305
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
refresh-after-snapshot: fix controller crash-loops and AAP rollout race
- Scale fulfillment-controller to 0 before kustomize apply and back to
1 after TLS certs are ready. Prevents OIDC sync.Once caching an empty
token endpoint when Keycloak isn't reachable yet.
- Remove manual controller-task pod recycle from step [7/9]. The recycle
raced with the AAP operator's own reconciliation, causing a 3rd
rollout that killed in-flight jobs. Let the operator handle rollouts.
- Include aap.yaml in kustomize apply (exclude only job.yaml) to
consolidate AAP operator triggers into one reconciliation instead of
a separate oc patch causing a second rollout.
- Use portable sed (redirect+mv) instead of platform-specific sed -i.
- Increase Keycloak sync timeout from 60s to 300s for resource
contention under concurrent VMs.
- Change || true to || failed=1 on AAP rollout waits and remove
2>/dev/null from rollout status commands.
OSAC-1305
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
hub-access: per-cluster kubeconfig path to prevent concurrent collisions
Write kubeconfig to /tmp/kubeconfig.hub-access.${server_name} instead
of a hardcoded path. Concurrent scripts on the same machine were
overwriting each other, causing hub registration with the wrong
cluster's kubeconfig. teardown.sh updated to clean up the glob.
OSAC-1305
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
setup-caas-agents: improve robustness and error reporting
- Auto-configure MetalLB IPAddressPool based on node subnet.
- Wait for assisted-image-service to be ready before downloading
discovery ISO (was failing on slow clusters).
- Print response body on host type registration failure.
- Dump InfraEnv/Agent state on timeout for debugging.
- Use --fail-with-body for ISO download.
OSAC-1305
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
review fixes: show curl errors on all attempts, add poll error messages
- Remove 2>/dev/null from http_retry and http_json so curl diagnostics
(connection errors, TLS failures) are visible on every attempt
- Add "retrying..." log on non-final attempts
- Add descriptive error messages to single-attempt status polls in
publish_templates instead of empty strings
OSAC-1305
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
lib: use if-guard in http_json to prevent set -e pipeline exit
The previous pattern `result=$(curl|jq) && { ... }` can trigger
set -e exit under pipefail when the command substitution wrapping
a pipeline fails — bash may exit before the && handler runs.
Using `if result=$(curl|jq); then` is explicitly exempt from set -e
in all bash versions, while keeping both curl and jq stderr visible.
OSAC-1305
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
DEBUG: add set -x to publish_templates to trace CI exit
Temporary — the script dies silently between "Job launched" and the
first polling iteration. set -x will show exactly which command
triggers the exit.
OSAC-1305
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fix publish_templates silent exit: [[ ]] && as last function line
[[ cond ]] && { action } as the last line of a function returns
exit code 1 when the condition is false (the [[ ]] returns 1, &&
short-circuits). Under set -o errexit, the caller sees exit 1 and
dies silently — no error message is printed because the action block
was skipped.
Replace with if/fi which returns 0 on the false branch. Also removes
the temporary set -x debug trace.
OSAC-1305
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
prepare-aap: retry AAP token creation during controller-task rollout
The AAP gateway returns 502 while the controller-task pod is rolling
out. This happens when the AAP operator triggers a second
reconciliation after step [3/9] kustomize apply (receptor secret
regeneration). Step [7/9] confirms the gateway is up after the first
rollout, but the second rollout can start before step [8/9].
Use http_json with 30 retries (5 min) instead of a single bare curl.
OSAC-1305
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
omer-vishlitzky
force-pushed
the
OSAC-1305/ci-stability-fixes
branch
from
June 7, 2026 21:59
727ec41 to
e88eea1
Compare
danmanor
approved these changes
Jun 8, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danmanor, omer-vishlitzky 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 |
Contributor
Author
|
/unhold |
1 task
This was referenced Jun 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
http_json/http_retrywrappers. Detect and fix stale AAP projects after snapshot boot. Retry failed project syncs and publish-templates jobs.http_retryandhttp_jsonhelpers with--fail-with-body, structured retry, and visible curl diagnostics.Test plan
./scripts/kustomize-build-all.shpasses (verified locally)Related
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements