ci: improve image-based e2e step reliability with retries and validation - #82628
ci: improve image-based e2e step reliability with retries and validation#82628sebrandon1 wants to merge 1 commit into
Conversation
|
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:
WalkthroughImage-based infrastructure, installation, network configuration, and upgrade scripts now include retry handling, failure traps, bounded waits, SSH readiness checks, and configurable IP stack and upgrade timeout parameters. ChangesImage-based workflow reliability
Estimated code review effort: 3 (Moderate) | ~30 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sebrandon1 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/openshift/image-based/infra/aws/ec2/openshift-image-based-infra-aws-ec2-commands.sh`:
- Around line 539-541: Update the SSH readiness probe near the connection
command to remove StrictHostKeyChecking=no and UserKnownHostsFile=/dev/null.
Obtain the expected host key from the trusted provisioning source, store or
reuse it in a persistent known_hosts file, and configure SSH to verify the host
strictly before accepting the probe.
In
`@ci-operator/step-registry/openshift/image-based/install/openshift-image-based-install-commands.sh`:
- Around line 10-19: Update the retry loops in ssh_retry and the corresponding
helper so the failure message, sleep, and delay increase occur only when the
current attempt is less than the retry limit. After the final failed attempt,
report the overall failure immediately without logging “retrying” or sleeping.
- Around line 127-132: Quote the source and destination path arguments in the
scp invocations within the install-script and pull-secrets transfer blocks,
including the ${SHARED_DIR} paths and remote_workdir destination, while
preserving the existing ssh_retry and SSHOPTS usage.
- Around line 67-76: Add bounded backoff to the retry loop in dnf_install_retry:
after a failed dnf install, sleep before the next attempt but skip sleeping
after the third and final attempt. Keep the existing three-attempt limit,
success return, and failure messaging unchanged.
- Around line 67-76: Update dnf_install_retry so each retry iteration guards dnf
clean and dnf install as one combined condition, continuing only when both
commands succeed and returning success only after both complete successfully.
Preserve the three-attempt retry loop and failure return behavior, while
ensuring a failed cleanup does not terminate the caller before later retries.
In
`@ci-operator/step-registry/openshift/image-based/network-configuration/configure/openshift-image-based-network-configuration-configure-commands.sh`:
- Around line 68-69: Remove the trap - EXIT statement before the final ssh
invocation so the existing failure trap remains active and reports remote
command failures. Preserve the current SSH call and its non-zero exit behavior.
- Around line 64-65: Update the scp invocation in the network configuration
transfer step to quote the local script path and remote destination, using
"${SHARED_DIR}/network-configuration.sh" and "${ssh_host_ip}:${remote_workdir}"
while preserving the existing SSH options.
- Around line 32-39: Validate IP_STACK against the documented values before
generating the remote script: accept v4, v4v6, and v6v4, while exiting with an
error for empty or any other value. Update the generated script assignment for
IP_STACK to quote the value, using the existing validation/configuration flow
around the dual-stack checks and make ipc generation.
🪄 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: 4d8f09f3-49dd-458c-9b35-19d805f19fff
📒 Files selected for processing (8)
ci-operator/step-registry/openshift/image-based/infra/aws/ec2/openshift-image-based-infra-aws-ec2-commands.shci-operator/step-registry/openshift/image-based/install/openshift-image-based-install-commands.shci-operator/step-registry/openshift/image-based/network-configuration/configure/openshift-image-based-network-configuration-configure-commands.shci-operator/step-registry/openshift/image-based/network-configuration/configure/openshift-image-based-network-configuration-configure-ref.yamlci-operator/step-registry/openshift/image-based/upgrade/metal/config/openshift-image-based-upgrade-metal-config-commands.shci-operator/step-registry/openshift/image-based/upgrade/seed/create/openshift-image-based-upgrade-seed-create-commands.shci-operator/step-registry/openshift/image-based/upgrade/target/openshift-image-based-upgrade-target-commands.shci-operator/step-registry/openshift/image-based/upgrade/target/openshift-image-based-upgrade-target-ref.yaml
020f372 to
abee46a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ci-operator/step-registry/openshift/image-based/install/openshift-image-based-install-commands.sh (1)
71-77: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep DNF cleanup inside the retry condition.
A failed
dnf cleancan terminate the generated script underset -ebefore later attempts. Guard cleanup and installation together.Proposed fix
- sudo dnf clean -y all - sudo dnf install -y \${packages[*]} && return 0 + if sudo dnf clean -y all && sudo dnf install -y "\${packages[@]}"; then + return 0 + fi🤖 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/openshift/image-based/install/openshift-image-based-install-commands.sh` around lines 71 - 77, Update dnf_install_retry so each retry attempt guards sudo dnf clean -y all and the subsequent sudo dnf install command within the same conditional, allowing a failed cleanup to continue to the next attempt under set -e while preserving success on a successful installation.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.
Inline comments:
In
`@ci-operator/step-registry/openshift/image-based/install/openshift-image-based-install-commands.sh`:
- Line 8: Make the EXIT traps success-aware by logging the failure message only
when the captured exit status is non-zero, while preserving the exit status and
retaining each trap through remote execution. Apply this in
ci-operator/step-registry/openshift/image-based/install/openshift-image-based-install-commands.sh:8,
ci-operator/step-registry/openshift/image-based/network-configuration/configure/openshift-image-based-network-configuration-configure-commands.sh:6,
ci-operator/step-registry/openshift/image-based/upgrade/metal/config/openshift-image-based-upgrade-metal-config-commands.sh:8,
ci-operator/step-registry/openshift/image-based/upgrade/seed/create/openshift-image-based-upgrade-seed-create-commands.sh:8,
and
ci-operator/step-registry/openshift/image-based/upgrade/target/openshift-image-based-upgrade-target-commands.sh:8;
remove the corresponding trap - EXIT commands at lines 149, 78, 289, 243, and
176 respectively.
---
Duplicate comments:
In
`@ci-operator/step-registry/openshift/image-based/install/openshift-image-based-install-commands.sh`:
- Around line 71-77: Update dnf_install_retry so each retry attempt guards sudo
dnf clean -y all and the subsequent sudo dnf install command within the same
conditional, allowing a failed cleanup to continue to the next attempt under set
-e while preserving success on a successful installation.
🪄 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: ad324729-6736-4d5d-b5f8-d9414211dd7b
📒 Files selected for processing (8)
ci-operator/step-registry/openshift/image-based/infra/aws/ec2/openshift-image-based-infra-aws-ec2-commands.shci-operator/step-registry/openshift/image-based/install/openshift-image-based-install-commands.shci-operator/step-registry/openshift/image-based/network-configuration/configure/openshift-image-based-network-configuration-configure-commands.shci-operator/step-registry/openshift/image-based/network-configuration/configure/openshift-image-based-network-configuration-configure-ref.yamlci-operator/step-registry/openshift/image-based/upgrade/metal/config/openshift-image-based-upgrade-metal-config-commands.shci-operator/step-registry/openshift/image-based/upgrade/seed/create/openshift-image-based-upgrade-seed-create-commands.shci-operator/step-registry/openshift/image-based/upgrade/target/openshift-image-based-upgrade-target-commands.shci-operator/step-registry/openshift/image-based/upgrade/target/openshift-image-based-upgrade-target-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- ci-operator/step-registry/openshift/image-based/network-configuration/configure/openshift-image-based-network-configuration-configure-ref.yaml
- ci-operator/step-registry/openshift/image-based/upgrade/target/openshift-image-based-upgrade-target-ref.yaml
abee46a to
20cf3bd
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ci-operator/step-registry/openshift/image-based/install/openshift-image-based-install-commands.sh (1)
76-79: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep
dnf cleaninside the retry condition.On retry attempts, a failed cleanup exits the strict generated script before another
dnf installattempt. Guard cleanup and installation as one retriable operation. As per coding guidelines, step-registry command scripts should default toset -euo pipefail.Proposed fix
- if [ "\$attempt" -gt 1 ]; then - sudo dnf clean -y all - fi - sudo dnf install -y \${packages[*]} && return 0 + if { [ "\$attempt" -eq 1 ] || sudo dnf clean -y all; } && \ + sudo dnf install -y "\${packages[@]}"; then + return 0 + fi🤖 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/openshift/image-based/install/openshift-image-based-install-commands.sh` around lines 76 - 79, Update the retry logic around the package installation command so the `dnf clean -y all` cleanup and subsequent `dnf install` are guarded as one retriable operation, preventing cleanup failure from aborting retries. Keep cleanup conditional on attempts greater than one, and ensure the step-registry command script defaults to `set -euo pipefail`.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.
Duplicate comments:
In
`@ci-operator/step-registry/openshift/image-based/install/openshift-image-based-install-commands.sh`:
- Around line 76-79: Update the retry logic around the package installation
command so the `dnf clean -y all` cleanup and subsequent `dnf install` are
guarded as one retriable operation, preventing cleanup failure from aborting
retries. Keep cleanup conditional on attempts greater than one, and ensure the
step-registry command script defaults to `set -euo pipefail`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 778ff8bd-7c77-4b9f-b7b0-3a54ad586429
📒 Files selected for processing (8)
ci-operator/step-registry/openshift/image-based/infra/aws/ec2/openshift-image-based-infra-aws-ec2-commands.shci-operator/step-registry/openshift/image-based/install/openshift-image-based-install-commands.shci-operator/step-registry/openshift/image-based/network-configuration/configure/openshift-image-based-network-configuration-configure-commands.shci-operator/step-registry/openshift/image-based/network-configuration/configure/openshift-image-based-network-configuration-configure-ref.yamlci-operator/step-registry/openshift/image-based/upgrade/metal/config/openshift-image-based-upgrade-metal-config-commands.shci-operator/step-registry/openshift/image-based/upgrade/seed/create/openshift-image-based-upgrade-seed-create-commands.shci-operator/step-registry/openshift/image-based/upgrade/target/openshift-image-based-upgrade-target-commands.shci-operator/step-registry/openshift/image-based/upgrade/target/openshift-image-based-upgrade-target-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- ci-operator/step-registry/openshift/image-based/network-configuration/configure/openshift-image-based-network-configuration-configure-ref.yaml
- ci-operator/step-registry/openshift/image-based/upgrade/target/openshift-image-based-upgrade-target-ref.yaml
20cf3bd to
abff636
Compare
|
/pj-rehearse pull-ci-openshift-kni-lifecycle-agent-main-ibu-e2e-flow pull-ci-openshift-kni-lifecycle-agent-main-ibi-e2e-flow pull-ci-openshift-kni-lifecycle-agent-main-ipc-e2e-flow-v4v6 |
|
@sebrandon1: your |
|
/pj-rehearse pull-ci-openshift-kni-lifecycle-agent-main-ibu-e2e-flow pull-ci-openshift-kni-lifecycle-agent-main-ibi-e2e-flow pull-ci-openshift-kni-lifecycle-agent-main-ipc-e2e-flow-v4v6 |
|
@sebrandon1: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Add SSH retry, trap handlers, pre-condition validation, and configurable timeouts to the lifecycle-agent image-based CI step scripts to reduce flaky e2e failures.
abff636 to
88bb9f8
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ci-operator/step-registry/openshift/image-based/install/openshift-image-based-install-commands.sh (1)
76-80: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep
dnf cleaninside the guarded retry condition.At Line 78, a transient cleanup failure terminates the generated installer under
errexit, so attempts 2 and 3 never run. This is the previously reported issue, still present in this range.Proposed fix
- if [ "\$attempt" -gt 1 ]; then - sudo dnf clean -y all - fi - sudo dnf install -y \${packages[*]} && return 0 + if { [ "\$attempt" -eq 1 ] || sudo dnf clean -y all; } \ + && sudo dnf install -y "\${packages[@]}"; then + return 0 + fiAs per coding guidelines, step registry command scripts must default to
set -euo pipefail.🤖 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/openshift/image-based/install/openshift-image-based-install-commands.sh` around lines 76 - 80, Update the retry loop around the package installation command so the `sudo dnf clean -y all` invocation in the `attempt > 1` branch cannot terminate the script under `errexit`; guard or otherwise handle cleanup failure while still allowing attempts 2 and 3 to proceed. Preserve the existing `set -euo pipefail` default and the successful `dnf install` return path.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.
Duplicate comments:
In
`@ci-operator/step-registry/openshift/image-based/install/openshift-image-based-install-commands.sh`:
- Around line 76-80: Update the retry loop around the package installation
command so the `sudo dnf clean -y all` invocation in the `attempt > 1` branch
cannot terminate the script under `errexit`; guard or otherwise handle cleanup
failure while still allowing attempts 2 and 3 to proceed. Preserve the existing
`set -euo pipefail` default and the successful `dnf install` return path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8146489e-7ae6-4af9-a336-ff10842f9bc5
📒 Files selected for processing (8)
ci-operator/step-registry/openshift/image-based/infra/aws/ec2/openshift-image-based-infra-aws-ec2-commands.shci-operator/step-registry/openshift/image-based/install/openshift-image-based-install-commands.shci-operator/step-registry/openshift/image-based/network-configuration/configure/openshift-image-based-network-configuration-configure-commands.shci-operator/step-registry/openshift/image-based/network-configuration/configure/openshift-image-based-network-configuration-configure-ref.yamlci-operator/step-registry/openshift/image-based/upgrade/metal/config/openshift-image-based-upgrade-metal-config-commands.shci-operator/step-registry/openshift/image-based/upgrade/seed/create/openshift-image-based-upgrade-seed-create-commands.shci-operator/step-registry/openshift/image-based/upgrade/target/openshift-image-based-upgrade-target-commands.shci-operator/step-registry/openshift/image-based/upgrade/target/openshift-image-based-upgrade-target-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- ci-operator/step-registry/openshift/image-based/network-configuration/configure/openshift-image-based-network-configuration-configure-ref.yaml
- ci-operator/step-registry/openshift/image-based/upgrade/target/openshift-image-based-upgrade-target-ref.yaml
|
[REHEARSALNOTIFIER]
A total of 132 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: |
|
/pj-rehearse pull-ci-openshift-kni-lifecycle-agent-main-ibu-e2e-flow pull-ci-openshift-kni-lifecycle-agent-main-ibi-e2e-flow pull-ci-openshift-kni-lifecycle-agent-main-ipc-e2e-flow-v4v6 |
|
@sebrandon1: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@sebrandon1: 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. |
Jira: CNF-26211
Summary
Adds retry logic, pre-condition validation, trap handlers, and configurable timeouts to the lifecycle-agent image-based CI step scripts (
ci-operator/step-registry/openshift/image-based/). These changes target the root causes of flaky e2e failures observed across 9 open lifecycle-agent PRs, where dual-stack IPC tests had 100% failure rates and other e2e jobs failed 40-75% of the time.Motivation
Analysis of 15 failed Prow job logs across lifecycle-agent PRs revealed that non-infrastructure failures fell into preventable categories:
no route to hosttimeouts — theIP_STACKenv var was silently dropped because it wasn't declared in the IPC configure ref YAML, somake ipcnever received the stack modevalidNextStagesChanges
SSH retry with exponential backoff (all 5 step scripts)
Every
scpcall is now wrapped inssh_retry()with 3 attempts and exponential backoff (10s/20s/40s). The final long-runningsshcommand that executes the actual test is intentionally not wrapped — these remote scripts are not idempotent, so retrying a partial run could leave the system in a broken state. Only the file transfer operations (which are safe to retry) get the wrapper.Trap handlers for failure diagnostics (all 5 step scripts)
Added
trap 'echo "ERROR: Step failed at line $LINENO with exit code $?"' EXITto all scripts. The trap is cleared withtrap - EXITbefore the final SSH command so test exit codes propagate cleanly. This matches the pattern already established in the EC2 provisioning script.IPC dual-stack fix (network-configuration configure)
IP_STACKto the ref YAML — previously, the ci-operator config setIP_STACK: v4v6for dual-stack jobs but the ref didn't declare it, so it was silently droppedIP_STACKtomake ipc— the orchestration Makefile needs this to configure dual-stack networkingIP_STACKisv4v6orv6v4, validates thatIPC_IPV6_ADDRESS,IPC_IPV6_MACHINE_NETWORK, andIPC_IPV6_GATEWAYare non-empty before proceedingDNF and container image pull retries (install + metal-config)
dnf_install_retryto the IBI install script (reusing the pattern from metal-config)podman_retrywith 3 attempts and exponential backoff (15s/30s/60s) for container image pulls in both install and metal-config scriptsAPI call retries and validation (seed-create)
--retry 3 --retry-delay 5tocurlcalls infindImage()that query OCP release APIsbase_infois non-empty after API calls — previously, a failed API response would silently produce emptySEED_VERSIONandRELEASE_IMAGEvaluessleep 5mtosleep 5s— the retry loop already handles pollingmax_attempts=20counter to theoc waitretry loop to prevent unbounded looping (previously infinite)Configurable upgrade timeout (IBU target)
UPGRADE_TIMEOUT="60m"toUPGRADE_TIMEOUT="${UPGRADE_TIMEOUT:-60m}"UPGRADE_TIMEOUTto the ref YAML so it can be overridden per-jobSSH connectivity probe (EC2 provisioning)
Added a post-provisioning SSH connectivity verification (10 attempts, 5s apart) after
aws ec2 wait instance-status-ok. The AWS waiter confirms the instance is healthy, but doesn't guaranteesshdis listening or that cloud-init has written the SSH key. This prevents the metal-config step from failing on a not-yet-ready sshd.Bug fixes (pre-existing)
cat <<EOF > ...string) were being written into the nsswitch.conf filessh_retry— removedssh_retryfrom metal-config'sinstall.shexecution (not safe to retry) andsudo mvof nsswitch.conf (replaced withcp+rmso retries don't fail on missing sourceFiles Modified
All under
ci-operator/step-registry/openshift/image-based/:infra/aws/ec2/*-commands.shinstall/*-commands.shnetwork-configuration/configure/*-commands.shnetwork-configuration/configure/*-ref.yamlIP_STACKenv varupgrade/metal/config/*-commands.shupgrade/seed/create/*-commands.shupgrade/target/*-commands.shUPGRADE_TIMEOUTupgrade/target/*-ref.yamlUPGRADE_TIMEOUTenv var, fixed trailing whitespaceSummary by CodeRabbit
Improves reliability of OpenShift image-based CI e2e steps (lifecycle-agent) on AWS EC2 by hardening remote execution, reducing transient failure impact, and improving diagnostics across install, network configuration, and upgrade workflows.
Key reliability & diagnostics updates
EXITtraps across the relevant scripts to print the failing$LINENOand non-zero exit code on failure.ssh_retry, pluspodman_retrywhere needed) with exponential backoff and attempt/failure logging, and applies them to remotessh/scptransfers, remote workdir creation, package install, and container image pull/copy steps.instance-status-ok, validates SSH readiness by retrying non-interactive SSH connections to the instance public IP (HOST_PUBLIC_IP) before proceeding.Install / provisioning hardening
scpofimage_based_install.shand pull-secret files with retry logic, and improves transfer/status messaging.sudo dnf install -y(includingdnf clean -y allon subsequent attempts).podman run ... cp /bin/openshift-install /tmp/openshift-installcopy step used during install.Network configuration dual-stack correctness
IP_STACKto the network-configuration configure step (documenting allowed valuesv4,v4v6,v6v4).IP_STACKand, for dual-stack modes, requires the required IPC IPv6 inputs (IPC_IPV6_ADDRESS,IPC_IPV6_MACHINE_NETWORK,IPC_IPV6_GATEWAY).IP_STACKinto the generated remote IPC generation (make ipc) and uses retry-safescp/remote execution for transferring and runningnetwork-configuration.sh.Upgrade robustness and timeouts
curlwith retry/delay/max-time semantics (for bothciandrelease) and fails fast if resolvedbase_infois empty.oc wait(attempt-counted, with explicit timeout/error behavior).create_seed.shusing retry-safescp.UPGRADE_TIMEOUT(defaulting to60m).scpwhen transferringupgrade_from_seed.sh.mkdir -pworkdir creation and staged config/secret transfers).podman_retry.nsswitch.confinstallation by copying the staged file into place and then removing the staged file.Cleanup