INTEROP-9337: Fix OCP 5.0 upgrade config and harden upgrade script - #82373
INTEROP-9337: Fix OCP 5.0 upgrade config and harden upgrade script#82373amp-rh wants to merge 3 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@amp-rh: This pull request references INTEROP-9337 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
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe OCP 5.0 upgrade policy adds backup and preflight steps, changes operator configuration, sources optional proxy settings, and replaces several ChangesOPP upgrade workflow
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: amp-rh 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 |
![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/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp5.0-upgrade.yaml`:
- Line 60: Update the `interop-opp-backup`, `interop-opp-preflight`, and
`interop-opp-upgrade` steps to pass an explicit `OPP_OPERATORS` value matching
the policy’s current operator list, excluding `rhacs-operator` and
`odf-operator`; alternatively, restore those operators to the list so all
workflow checks use the same scope.
In
`@ci-operator/step-registry/interop/opp/upgrade/interop-opp-upgrade-commands.sh`:
- Line 373: Update the awk predicate in the oppNamespaces derivation to require
each CSV name field ($2) to start with the operator name, matching the prefix
check used at line 350, rather than matching the operator name anywhere in the
field. Keep the existing namespace collection and output behavior unchanged.
🪄 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: Pro Plus
Run ID: fb503146-12f3-4ac8-9cd1-618be28be59e
📒 Files selected for processing (2)
ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp5.0-upgrade.yamlci-operator/step-registry/interop/opp/upgrade/interop-opp-upgrade-commands.sh
|
/pj-rehearse periodic-ci-stolostron-policy-collection-main-ocp4.22-upgrade-interop-opp-upgrade-aws |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
1a5ea35 to
c64002a
Compare
PR Review: INTEROP-9337 — OCP 5.0 Upgrade Config & Script HardeningOverall: Approach is sound, but has one critical gap that will cause runtime failures. The goals are well-motivated — removing unavailable operators from the 5.0 config, fixing the Quay channel, aligning steps with 4.22, and eliminating the 🚨 Critical:
|
| Priority | Issue | Fix |
|---|---|---|
| Critical | OPP_OPERATORS mismatch |
Add env override in 5.0 YAML |
| Major | Prefix match inconsistency | Change > 0 to == 1 on line 373 |
| Minor | Proxy credential leak | Wrap source with set +x / set -x |
| Minor | Fragile awk JSON parsing | Acceptable for now, note for future |
I'd recommend addressing at least the critical and major items before merge. The rest of the approach looks solid. 👍
Review by Chai Bot ([SHIP Public])
c64002a to
a70b502
Compare
Follow-up Review: All Flagged Items Addressed ✅Re-checked the current diff against the four items from my earlier review. All have been addressed. 🚨 Critical:
|
a70b502 to
3667882
Compare
|
/pj-rehearse periodic-ci-stolostron-policy-collection-main-ocp5.0-upgrade-interop-opp-upgrade-aws |
1 similar comment
|
/pj-rehearse periodic-ci-stolostron-policy-collection-main-ocp5.0-upgrade-interop-opp-upgrade-aws |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
1 similar comment
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-stolostron-policy-collection-main-ocp4.22-upgrade-interop-opp-upgrade-aws |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-stolostron-policy-collection-main-ocp4.22-upgrade-interop-opp-upgrade-aws |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@amp-rh: 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. |
|
/retest |
|
/auto-cc |
Review: INTEROP-9337 — OCP 5.0 upgrade config & script hardeningThanks for this PR. I've reviewed both files — the YAML config and the upgrade shell script. Overall the changes are well-motivated and clearly documented. A few observations below. ✅ What looks good
🔍 Observations and suggestions1. Eliminating the
The 2. Trap handler subshell wrapping The trap was changed from: trap 'exitCode=$?; DebugOnExit' EXIT TERMto: trap '{( exitCode=$?; DebugOnExit )}' EXIT TERMI assume the subshell is intentional to isolate the debug handler so a stray 3. Trailing Several functions ( 4. 5. CI rehearsal failures Both rehearsal jobs are currently failing:
Worth investigating whether these failures are related to the script changes (e.g. a SummaryThe config changes are clean and well-justified. The script hardening is solid overall — the Comment posted via Chai Bot at the request of a reviewer. |
Remove rhacs-operator and odf-operator from the 5.0 upgrade config (absent from v5.0 catalog). Fix quay-operator channel from stable-3.18 to stable-3.17. Add interop-opp-backup and interop-opp-preflight steps. Pass explicit OPP_OPERATORS so backup, preflight, and upgrade steps only validate installed operators. Harden the upgrade script: source proxy-conf.sh, replace jq with oc templates and awk, use function PascalCase declarations with typeset, wrap oc wait in if-! guard with diagnostics, use trap subshell form, replace pipeline || true with variable capture, use here-strings instead of echo pipes, and add terminal true to all functions.
3667882 to
a620c70
Compare
Chai Bot Review
CI Status: 15/16 checks pass ✅ (only Approval Status: Still needs approval from 1. YAML Config Changes (safe, well-motivated)
2. Shell Script Changes (sound approach, low-medium risk)The bulk of the change replaces Good:
Worth noting (low risk, acceptable):
3. Production Risk Assessment
Overall: The PR is in good shape. All previously flagged issues are resolved. Approach is sound — the config fixes are necessary and the jq→go-template migration is a reasonable hardening step. Ready for OWNERS approval from Review requested by PR author via Chai Bot ([SHIP Public]) |
Replace awk-based JSON field extraction with oc's native -o jsonpath for version and digest lookups, eliminating fragile positional parsing that depends on key ordering. Fix trap handler to run in current shell (brace group) instead of a subshell, so exitCode propagates correctly to DebugOnExit.
… drop unnecessary exports - Replace `|| true` on eval'd pipeline with explicit error handling to avoid masking oc failures under pipefail - Add `true` as final statement in functions with early returns - Replace `export` with `typeset -g` for script-local variables
|
Applied additional best-practices fixes from a shell scripting audit: Major:
Minor (convention):
|
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
Closing: primary changes (operator removal, quay channel fix, test steps) already landed in #82372. Remaining changes (proxy-conf.sh sourcing, CSV prefix-match fix) will be submitted in a separate focused PR. rhacs-operator has since reappeared in the OCP 5.0 catalog and will be re-added in a separate PR. |
…handling Replace all jq-based queries with go-template, jsonpath, and awk equivalents so the upgrade script no longer requires jq (unavailable in the cli base image). Additional improvements: - Add set +x/set -x guards around proxy-conf.sh sourcing - Fix trap handler to use brace group for exitCode propagation - Replace while-read pipes with for-in loops (avoid subshell scoping) - Use typeset -g instead of export for version variables - Add failure diagnostics to StabilizeCluster - Add defensive true at function ends - Use function keyword with PascalCase declarations - Set OPP_OPERATORS explicitly in the 5.0 config (excludes odf-operator) Addresses review feedback from sg-rh on openshift#82373 (jsonpath over awk, trap handler fix) and CodeRabbit findings (OPP_OPERATORS scope, CSV prefix matching).
4 participants
Summary
Fix multiple issues in the OCP 5.0 upgrade interop test configuration and harden the upgrade script. Replaces #82182 with expanded scope.
Changes
1. Remove deprecated operators from 5.0 upgrade config
rhacs-operator(absent from v5.0 catalog)odf-operator(absent from v5.0 catalog)quay-operator, fix channel fromstable-3.18tostable-3.17(present in v5.0 catalog;stable-3.18does not exist)2. Add missing backup and preflight steps
The 5.0 upgrade config had only 4 test steps while the 4.22 config has 6. Added
interop-opp-backupandinterop-opp-preflightbefore the upgrade step.3. Add proxy-conf.sh sourcing
Source
${SHARED_DIR}/proxy-conf.shwhen present, matching the pattern used by other interop step scripts.4. Fix CSV matching from substring to prefix
Change
index($2, op) > 0toindex($2, op) == 1inValidateOppOperators()to avoid false CSV matches.Verification
rhacs-operator: absent from v5.0 catalogodf-operator: absent from v5.0 catalogquay-operator: present with channels up tostable-3.17References
Summary by CodeRabbit
rhacs-operatorandodf-operator, switchingquay-operatortostable-3.17, and adding backup and preflight steps.jqusage withoctemplates andawk, and preserving health, release, admin-gate, and CSV validation checks.