NO-ISSUE: Fix CaaS boot failures caused by dirty database migrations - #260
Conversation
|
@omer-vishlitzky: This pull request explicitly references no jira issue. 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. |
WalkthroughConsolidates fulfillment scale-to-zero into a kustomize overlay edit before ChangesSnapshot Refresh Deployment Coordination
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Security & Operational Risk AssessmentSeverity: Medium — rehoming scale-to-zero into the overlay removes explicit imperative scaling calls; if the overlay edit or
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 1 warning)
✅ 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 |
tzvatot
left a comment
tzvatot
left a comment
There was a problem hiding this comment.
Clean fix, well-analyzed race condition. No findings.
/lgtm
eb9dda9 to
682f6de
Compare
![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`:
- Line 183: Remove the redundant scale-down command that re-scales the
fulfillment-controller to 0 replicas (the oc scale deploy/fulfillment-controller
-n "${INSTALLER_NAMESPACE}" --replicas=0 line) because the controller is already
scaled down earlier before the overlay apply; either delete this line or, if you
intentionally want a defensive re-check, keep it but add an inline comment
referencing the earlier pre-overlay scale-down and explaining the defensive
rationale so future readers understand why it's duplicated.
🪄 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: 7d702fb4-c828-455a-8d2e-4a7ceead6637
📒 Files selected for processing (1)
scripts/refresh-after-snapshot.sh
682f6de to
1e1f82a
Compare
1e1f82a to
f46ae16
Compare
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 script currently scales fulfillment-grpc-server to 0
but only restores fulfillment-controller later; update the restore step that
re-enables replicas (the block that currently resets fulfillment-controller
replicas) to also reset fulfillment-grpc-server to its desired replica count
(e.g., include fulfillment-grpc-server=<desired> in the same kustomize edit set
replicas call) and ensure any subsequent restart/wait loops that check or wait
for fulfillment-controller also include checks for fulfillment-grpc-server so
the grpc service is brought back and verified.
🪄 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: 85e79bb8-5161-42ab-8f73-bfec940ff6f1
📒 Files selected for processing (1)
scripts/refresh-after-snapshot.sh
f46ae16 to
5d931d0
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eliorerz, omer-vishlitzky, tzvatot 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 |
|
/retest |
…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>
5d931d0 to
cf01bd3
Compare
|
New changes are detected. LGTM label has been removed. |
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-182: The script currently mutates
overlays/${INSTALLER_KUSTOMIZE_OVERLAY}/kustomization.yaml with "kustomize edit
set replicas fulfillment-controller=0 fulfillment-grpc-server=0" and never
restores it; change the script to read and save the current replica values from
the overlay before the edit, perform the zero-replica kustomize edit and oc
apply -k as now, then after rollout verification restore the original replica
values back into the same overlay (use the saved values with kustomize edit set
replicas fulfillment-controller=<saved> fulfillment-grpc-server=<saved>),
placing the restore step after the deployment/rollout checks near the end of the
script so the working tree is not left modified (alternatively replace the
edit-within-file approach by using oc scale --replicas=0 before apply if you
prefer to avoid mutating kustomization.yaml).
🪄 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: 4243e022-8fbd-4fda-b8c8-c20b759b5d80
📒 Files selected for processing (1)
scripts/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 ) | ||
| oc apply -k "overlays/${INSTALLER_KUSTOMIZE_OVERLAY}" |
There was a problem hiding this comment.
Major risk: kustomize edit leaves overlay with replicas: 0 permanently.
The kustomize edit set replicas command on line 181 modifies overlays/${INSTALLER_KUSTOMIZE_OVERLAY}/kustomization.yaml on disk. While lines 322-323 restore replicas via oc scale in the cluster, the kustomization.yaml file retains replicas: 0.
Impact (Severity: Major):
- Any subsequent
oc apply -k overlays/${OVERLAY}(manual or automated) will scale fulfillment-controller and fulfillment-grpc-server to 0, causing immediate service outage - On shared clusters (development, hypershift2 per AGENTS.md), this creates a latent disruption risk for other developers
- Leaves git working tree dirty, potentially masking other overlay changes
Recommended fix: Restore the replicas configuration at the end of the script or after the deployments are successfully rolled out.
🛡️ Proposed fix: Restore replicas in kustomization.yaml
Add this near the end of the script (e.g., after line 418's rollout verification):
if (( failed )); then echo "ERROR: Fulfillment rollout failed after restart"; exit 1; fi
+# Restore overlay replicas so future applies don't scale to zero
+( cd "overlays/${INSTALLER_KUSTOMIZE_OVERLAY}" && kustomize edit set replicas fulfillment-controller=1 fulfillment-grpc-server=1 )
./scripts/prepare-tenant.shAlternatively, consider using oc scale --replicas=0 before the apply instead of modifying the overlay file, though this reintroduces the race window you're trying to avoid.
🤖 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 175 - 182, The script
currently mutates overlays/${INSTALLER_KUSTOMIZE_OVERLAY}/kustomization.yaml
with "kustomize edit set replicas fulfillment-controller=0
fulfillment-grpc-server=0" and never restores it; change the script to read and
save the current replica values from the overlay before the edit, perform the
zero-replica kustomize edit and oc apply -k as now, then after rollout
verification restore the original replica values back into the same overlay (use
the saved values with kustomize edit set replicas fulfillment-controller=<saved>
fulfillment-grpc-server=<saved>), placing the restore step after the
deployment/rollout checks near the end of the script so the working tree is not
left modified (alternatively replace the edit-within-file approach by using oc
scale --replicas=0 before apply if you prefer to avoid mutating
kustomization.yaml).
|
/retest |
|
@omer-vishlitzky: 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. |
4 participants
Summary
fulfillment-grpc-serverto 0 before applying the kustomize overlay in the refresh script, preventing database migrations from running while the database pod is being recycled2>/dev/null || truefrom scale-down commands — on snapshot boot the deployments always exist, and silent failures here mask real problemsRoot Cause
The kustomize overlay changes the
fulfillment-databaseStatefulSet image reference from a:latesttag to a@sha256:digest. This spec change triggers a StatefulSet pod recreation, killing the database server. Iffulfillment-grpc-serveris running database migrations at that moment,golang-migrateleaves theschema_migrationstable dirty (dirty=true), and all subsequent grpc-server starts crash withDirty database version N. Fix and force version.This was the root cause of intermittent CaaS boot failures in openshift/release#79512 — the failure rate depended on whether migrations finished before the database pod was killed (a race condition).
Evidence
Differential analysis across 5 CaaS rehearsal jobs from the same batch:
dirty=trueat v39Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit