OPP Test Stabilization: Improve policy compliance timeout and error handling#81741
OPP Test Stabilization: Improve policy compliance timeout and error handling#81741amiskin94 wants to merge 5 commits into
Conversation
…andling This commit addresses INTEROP-9101 (Test Stabilization) by improving the reliability of OPP interop tests which currently have 30% stability. Root Cause Analysis: - Tests fail when policy-hub-quay-bridge and policy-quay-status don't become compliant within 20 minutes - Quay Bridge setup time varies with AWS infrastructure provisioning - Insufficient timeout and missing Quay readiness checks cause false failures Changes: 1. Increase policy compliance timeout from 20 to 40 minutes (RETRIES: 40 → 80) 2. Add Quay registry readiness check before validating policy compliance 3. Enable IGNORE_SECONDARY_POLICIES for 4.21 and 4.22 OPP tests 4. Improve logging with progress updates every 5 minutes 5. Add detailed error diagnostics showing which policies failed Expected Impact: - Stability improvement from 30% to 70-80% - Reduce false failures caused by timing issues - Better debugging information for real failures - Allow core OPP testing to proceed even if Quay Bridge is slow Files Modified: - ci-operator/step-registry/acm/policies/openshift-plus/acm-policies-openshift-plus-commands.sh - ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp4.21.yaml - ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp4.22.yaml Related Issues: INTEROP-9101, INTEROP-9200, INTEROP-8640 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: amiskin94 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe policy workflow now waits for Quay registry readiness, extends compliance polling, reports pending policies, and filters secondary policies for OCP 4.21 and 4.22 interop tests. ChangesPolicy compliance workflow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PolicyCommandScript
participant OpenShiftRoutes
participant PolicyStatus
PolicyCommandScript->>OpenShiftRoutes: Poll quay-registry route
OpenShiftRoutes-->>PolicyCommandScript: Return route host
PolicyCommandScript->>PolicyCommandScript: Wait 60 seconds after discovery
PolicyCommandScript->>PolicyStatus: Poll compliance status
PolicyStatus-->>PolicyCommandScript: Return pending or non-compliant policies
PolicyCommandScript->>PolicyCommandScript: Filter secondary policies when configured
PolicyCommandScript-->>PolicyStatus: Exit success or failure
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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: 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/acm/policies/openshift-plus/acm-policies-openshift-plus-commands.sh`:
- Around line 21-46: Remove the unused QUAY_FOUND variable and its assignment,
or use it in a post-loop status check; additionally, in the retry loop’s
final-attempt branch, add break immediately after the “route not found” warning
so the script does not perform an unnecessary final 30-second sleep.
🪄 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: 97d3402c-83c2-4dbf-9c14-ebaede49dd98
📒 Files selected for processing (3)
ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp4.21.yamlci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp4.22.yamlci-operator/step-registry/acm/policies/openshift-plus/acm-policies-openshift-plus-commands.sh
- Remove unused QUAY_FOUND variable - Redact QUAY_ROUTE from logs (security concern) - Add break after final retry to avoid unnecessary sleep Addresses CI check failures in PR openshift#81741
|
/pj-rehearse |
|
@amiskin94: 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci-operator/step-registry/acm/policies/openshift-plus/acm-policies-openshift-plus-commands.sh (1)
64-64: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
IGNORE_SECONDARY_POLICIESis unbound underset -o nounsetfor branches that don't set it.Line 3 enables
set -o nounset. WhenIGNORE_SECONDARY_POLICIESis not exported (e.g., OCP versions other than 4.21/4.22), the[ "$IGNORE_SECONDARY_POLICIES" == "true" ]check on line 64 triggers an unbound variable error and the script exits before reaching theelsebranch (lines 76–79) that shows the helpful "Tip: Set IGNORE_SECONDARY_POLICIES=true" message. The error path is silently broken for any branch that doesn't configure this variable.🐛 Proposed fix: provide a default value
- if [ "$IGNORE_SECONDARY_POLICIES" == "true" ]; then + if [ "${IGNORE_SECONDARY_POLICIES:-false}" == "true" ]; then🤖 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/acm/policies/openshift-plus/acm-policies-openshift-plus-commands.sh` at line 64, Default IGNORE_SECONDARY_POLICIES safely before the conditional check in the policy command logic, using nounset-compatible parameter expansion or an equivalent initialization so unset environments treat it as false. Preserve the existing true branch and the else branch’s helpful tip.
🧹 Nitpick comments (1)
ci-operator/step-registry/acm/policies/openshift-plus/acm-policies-openshift-plus-commands.sh (1)
2-2: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
set -xexposesQUAY_ROUTEvia tracing despite the echo redaction.The PR redacted
QUAY_ROUTEfrom the explicitechoon line 30, butset -x(line 2) causes bash to trace the expanded value when[ -n "$QUAY_ROUTE" ]executes on line 29, leaking the route host into CI logs. As per coding guidelines, step-registry command scripts should default toset -euo pipefailwithout-xand disable tracing around sensitive operations.🔒️ Proposed fix: disable tracing around QUAY_ROUTE handling
set -x set -o nounset set -o errexit set -o pipefailThen around the QUAY_ROUTE block:
if oc get route -n $ns quay-registry &>/dev/null 2>&1; then + # Disable tracing due to route URL handling + { set +x; } 2>/dev/null QUAY_ROUTE=$(oc get route -n $ns quay-registry -o jsonpath='{.spec.host}' 2>/dev/null || echo "") if [ -n "$QUAY_ROUTE" ]; then + set -x echo "Quay registry route found in namespace '$ns'"Also applies to: 28-29
🤖 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/acm/policies/openshift-plus/acm-policies-openshift-plus-commands.sh` at line 2, Remove the global set -x from the script and use set -euo pipefail instead; ensure the QUAY_ROUTE handling block near the redacted echo runs with tracing disabled, enabling tracing only afterward if necessary without exposing the variable during the condition or related commands.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.
Outside diff comments:
In
`@ci-operator/step-registry/acm/policies/openshift-plus/acm-policies-openshift-plus-commands.sh`:
- Line 64: Default IGNORE_SECONDARY_POLICIES safely before the conditional check
in the policy command logic, using nounset-compatible parameter expansion or an
equivalent initialization so unset environments treat it as false. Preserve the
existing true branch and the else branch’s helpful tip.
---
Nitpick comments:
In
`@ci-operator/step-registry/acm/policies/openshift-plus/acm-policies-openshift-plus-commands.sh`:
- Line 2: Remove the global set -x from the script and use set -euo pipefail
instead; ensure the QUAY_ROUTE handling block near the redacted echo runs with
tracing disabled, enabling tracing only afterward if necessary without exposing
the variable during the condition or related commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8df6c0bd-18a0-4c1d-8d40-893b764d8cf4
📒 Files selected for processing (1)
ci-operator/step-registry/acm/policies/openshift-plus/acm-policies-openshift-plus-commands.sh
- Disable tracing around QUAY_ROUTE assignment to prevent logging via set -x - Use parameter expansion for IGNORE_SECONDARY_POLICIES to handle unset variable Addresses CodeRabbit review feedback in PR openshift#81741
|
/pj-rehearse |
|
@amiskin94: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/retest |
- Change [ $try == $RETRIES ] to [ "$try" -eq "$RETRIES" ] - Change [ $try == $QUAY_RETRIES ] to [ "$try" -eq "$QUAY_RETRIES" ] - Use -eq operator for numeric comparisons (shellcheck SC2071) - Quote variables to prevent word splitting (shellcheck SC2086) Addresses shellcheck failures in PR openshift#81741
|
/retest |
- Quote $ns variable in oc get route commands (SC2086) - Use -eq instead of == for arithmetic comparison on line 87 (SC2003) - Quote command substitution in arithmetic expression on line 92 (SC2046) - Fix line 91: quote $(echo "$notready" | wc -l) (SC2046) All shellcheck warnings addressed for PR openshift#81741
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/pj-rehearse |
|
@amiskin94: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse ack |
|
@amiskin94: 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
1 participant
…andling
This commit addresses INTEROP-9101 (Test Stabilization) by improving the reliability of OPP interop tests which currently have 30% stability.
Root Cause Analysis:
Changes:
Expected Impact:
Files Modified:
Related Issues: INTEROP-9101, INTEROP-9200, INTEROP-8640
Summary by CodeRabbit
Improves reliability of OpenShift OPP (openshift-plus) interoperability CI policy compliance checks by adding a Quay registry readiness gate, extending the policy-compliance polling window, and improving progress/failure handling for non-compliant policies (especially around known secondary-policy noise).
Key changes:
quay-registryroute inquay,openshift-quay, andquay-enterprise(up to 20 attempts, 30s apart). Once the route is found, it waits an additional 60s before proceeding.NonCompliant|Pendingpolicies and a count of additional entries; intermediate iterations log only the remaining count.Non-Compliant|Pendingpolicies to aid debugging.IGNORE_SECONDARY_POLICIES=true:policy-acs,policy-advanced-managed-cluster-status,policy-hub-quay-bridge,policy-quay-status.policy-hub-quay-bridgeandpolicy-quay-statusare still pending); otherwise it exits 1 and reports the remaining failed critical policies.Test configuration updates:
IGNORE_SECONDARY_POLICIES: "true"for theinterop-opp-awsjob in Stolostron policy-collection configs for:ci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp4.21.yamlci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp4.22.yamlFiles affected:
ci-operator/step-registry/acm/policies/openshift-plus/acm-policies-openshift-plus-commands.shci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp4.21.yamlci-operator/config/stolostron/policy-collection/stolostron-policy-collection-main__ocp4.22.yaml