TESTING HELM INSTALLATION IGNORE - #346
omer-vishlitzky wants to merge 2 commits into
Conversation
…pply The kustomize overlay changes the fulfillment-database StatefulSet image reference from a :latest tag to a @sha256: digest. This triggers a StatefulSet pod recreation, killing the database mid-connection. If the grpc-server is running database migrations at that moment, golang-migrate leaves the schema_migrations table in a dirty state and all subsequent grpc-server starts refuse to run — causing a boot failure. Fix: use `kustomize edit set replicas` to set fulfillment-controller and fulfillment-grpc-server to 0 before applying the overlay. The apply itself deploys with zero replicas, eliminating the race entirely. At step [5/9], the script now: 1. Waits for TLS certificates 2. Waits for the database StatefulSet rollout to complete 3. Scales grpc-server and controller back to 1 4. Waits for all fulfillment deployment rollouts This ensures the database is healthy before any migration-running pod starts, and that grpc-server is available before rest-gateway's readiness probe checks the gRPC upstream. Depends on: openshift/release#80431 (adds kustomize binary to the installer container image). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThe script now sets fulfillment controller and gRPC server replicas to zero through the overlay apply step, then reorders the post-certificate phase to wait on background rollout processes, the database rollout, service restarts, deployment rollouts, and CDI certificate refresh completion. ChangesSnapshot refresh flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 inconclusive)
✅ Passed checks (8 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 |
|
/test vmaas-e2e-full-setup-helm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
![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 `@scripts/refresh-after-snapshot.sh`:
- Around line 175-181: The temporary scale-down in refresh-after-snapshot.sh
leaves overlays/${INSTALLER_KUSTOMIZE_OVERLAY}/kustomization.yaml mutated at 0
replicas after the live cluster is restored. Update the script so the change
made by kustomize edit set replicas in the deployment/setup flow is reverted
once the pods are brought back, either by setting fulfillment-controller and
fulfillment-grpc-server back to 1, restoring the file from git, or trapping and
reverting the modification on exit. Use the existing deployment restore section
and the kustomize edit set replicas command as the anchor points for the fix.
🪄 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: 96af474f-4c72-4257-81c4-6d46bdbe9102
📒 Files selected for processing (2)
empty-test-file-1.txtscripts/refresh-after-snapshot.sh
| # Deploy with fulfillment pods scaled to zero. The apply changes the database | ||
| # StatefulSet image ref (tag → digest), triggering a pod recreation. If the | ||
| # grpc-server were running, it could be mid-migration when the database is | ||
| # killed, leaving golang-migrate's schema dirty. Deploying at zero replicas | ||
| # eliminates the race entirely — pods are brought back at step [5/9] after | ||
| # the database rollout completes. | ||
| ( cd "overlays/${INSTALLER_KUSTOMIZE_OVERLAY}" && kustomize edit set replicas fulfillment-controller=0 fulfillment-grpc-server=0 ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cat -n scripts/refresh-after-snapshot.shRepository: osac-project/osac-installer
Length of output: 23637
Restore the overlay kustomization.yaml after the temporary scale-down.
The kustomize edit set replicas command mutates overlays/${INSTALLER_KUSTOMIZE_OVERLAY}/kustomization.yaml in place. While oc scale corrects the live cluster state later in the script, the local file remains set to 0 replicas. This state leakage causes subsequent kustomize build or oc apply runs to incorrectly scale the pods down to zero.
Add a kustomize edit set replicas ...=1 or a git checkout for the file after the deployment is back up, or trap the modification and restore it on exit.
Current state at end of script
```bash # Line 181: File set to 0 ( cd "overlays/${INSTALLER_KUSTOMIZE_OVERLAY}" && kustomize edit set replicas fulfillment-controller=0 fulfillment-grpc-server=0 )Line 322-323: Live deployment scaled to 1
oc scale deploy/fulfillment-controller -n "${INSTALLER_NAMESPACE}" --replicas=1
oc scale deploy/fulfillment-grpc-server -n "${INSTALLER_NAMESPACE}" --replicas=1
File remains at 0
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
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 175 - 181, The temporary
scale-down in refresh-after-snapshot.sh leaves
overlays/${INSTALLER_KUSTOMIZE_OVERLAY}/kustomization.yaml mutated at 0 replicas
after the live cluster is restored. Update the script so the change made by
kustomize edit set replicas in the deployment/setup flow is reverted once the
pods are brought back, either by setting fulfillment-controller and
fulfillment-grpc-server back to 1, restoring the file from git, or trapping and
reverting the modification on exit. Use the existing deployment restore section
and the kustomize edit set replicas command as the anchor points for the fix.
</details>
<!-- cr-indicator-types:potential_issue -->
<!-- cr-comment:v1:89fd9bcaa6f0abec4ef31ba9 -->
<!-- This is an auto-generated comment by CodeRabbit -->
|
/test e2e-vmaas-full-setup-helm |
|
@omer-vishlitzky: The following test 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. |
|
💀 CI Triage: Root cause: Helm deployment missing storage class configuration for AAP provisioning jobs Explanation: The e2e-vmaas-full-setup-helm CI job is completely broken (no successful runs in 14+ days). The test suite runs for exactly 60 minutes before hitting the hard timeout, with only 9 out of 47 tests completing. The timeout occurs because ComputeInstance VMs never reach Running state—they remain stuck in "Starting" phase indefinitely. Root cause: AAP provision jobs (job IDs 78, 79, 84, 99, 100, 101, 103) all fail with the same error: "ComputeInstance has no tenant_storage_classes available. Either the osac-operator CI controller should inject the resolved storageClasses list before triggering provisioning, or JIT storage provisioning via STORAGE_TIERS must succeed." The Helm deployment is missing the storage configuration that the Kustomize deployment includes. The osac-operator is not injecting the required The causal chain: missing storage config → AAP jobs fail → VMs never created → ComputeInstances stuck in Starting → tests wait 15 minutes per VM → 60-minute timeout kills test suite at ~19% completion. This affects ONLY the Evidence: [ [ [ Suggestion: Fix the Helm chart deployment to configure storage classes for AAP provisioning. Two options: Option 1 (operator injection): Configure the osac-operator Helm chart to inject the Option 2 (AAP JIT provisioning): Set the Immediate verification: Compare the osac-operator deployment between a working Kustomize-based job and this failing Helm-based job. Look for differences in environment variables, annotations on Tenant CRs, or ConfigMaps that control storage class resolution. File to check: /tmp/osac-installer/charts/osac/values.yaml and charts/osac/templates/operator/* — ensure storage configuration is exposed as a Helm value and injected into the operator deployment. Prow job | Build For deeper investigation, use the |
1 participant
This is a test PR, please ignore.
Summary by CodeRabbit