fix(cd): make prod data restore idempotent - #91
Open
rhanka wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughModifies the deploy-prod job in release-prod.yml to select checkout refs conditionally based on release_kind, passes FORCE_RESTORE/RELEASE_KIND/SNAPSHOT_NAME into the restore environment, adds a live Elasticsearch state check to skip redundant restores, and conditionally waits/logs the restore job. ChangesProd Deploy Restore Idempotency
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant BackendPod
participant Elasticsearch
participant RestoreJob as elasticsearch-restore job
Workflow->>BackendPod: query current restore state
BackendPod->>Elasticsearch: get snapshot_name and doc count
Elasticsearch-->>BackendPod: snapshot_name, doc_count
BackendPod-->>Workflow: return state
alt snapshot matches, doc_count > 0, not FORCE_RESTORE
Workflow->>Workflow: mark restore skipped
else
Workflow->>RestoreJob: apply restore overlay
end
alt restore not skipped
Workflow->>RestoreJob: wait for completion, fetch logs
else
Workflow->>Workflow: print skip message
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 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 @.github/workflows/release-prod.yml:
- Around line 431-435: The prod deploy job’s actions/checkout@v4 step is leaving
persisted Git credentials behind, which later shell/make/kubectl steps don’t
need. Update the checkout configuration in the release-prod workflow to disable
persisted credentials for this job, while keeping the existing ref logic intact.
Use the checkout step in the release context/job block as the location to apply
the credential-setting change.
- Around line 536-558: The pre-restore probe in the release workflow is too
brittle because the `backend_pod` lookup and `kubectl exec` checks can fail
under `set -e` and stop the job before the restore is applied. Update the
restore gate around `restore_already_current` so failures in the probe path (pod
missing, restarting, or exec error) are treated as “not already current” instead
of exiting, while still preserving the existing success path that compares
`live_snapshot` and `live_count` before deciding whether to skip
`elasticsearch-restore.job.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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e983fab-6004-4f73-a04e-0d6b7433b743
📒 Files selected for processing (1)
.github/workflows/release-prod.yml
Comment on lines
431
to
+435
| - uses: actions/checkout@v4 | ||
| with: | ||
| ref: ${{ needs.release-context.outputs.app_ref }} | ||
| # data_only/rollback must use the current workflow/runtime manifests; | ||
| # old prod tags can predate Kubernetes scheduling fixes. | ||
| ref: ${{ needs.release-context.outputs.release_kind == 'app_and_data' && needs.release-context.outputs.app_ref || github.sha }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Disable persisted checkout credentials in the prod deploy job.
This job does not appear to need Git credentials after checkout; avoid leaving the Actions token in the local git config for later shell/make/kubectl steps.
Proposed fix
- uses: actions/checkout@v4
with:
+ persist-credentials: false
# data_only/rollback must use the current workflow/runtime manifests;
# old prod tags can predate Kubernetes scheduling fixes.
ref: ${{ needs.release-context.outputs.release_kind == 'app_and_data' && needs.release-context.outputs.app_ref || github.sha }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - uses: actions/checkout@v4 | |
| with: | |
| ref: ${{ needs.release-context.outputs.app_ref }} | |
| # data_only/rollback must use the current workflow/runtime manifests; | |
| # old prod tags can predate Kubernetes scheduling fixes. | |
| ref: ${{ needs.release-context.outputs.release_kind == 'app_and_data' && needs.release-context.outputs.app_ref || github.sha }} | |
| - uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false | |
| # data_only/rollback must use the current workflow/runtime manifests; | |
| # old prod tags can predate Kubernetes scheduling fixes. | |
| ref: ${{ needs.release-context.outputs.release_kind == 'app_and_data' && needs.release-context.outputs.app_ref || github.sha }} |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 431-435: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/release-prod.yml around lines 431 - 435, The prod deploy
job’s actions/checkout@v4 step is leaving persisted Git credentials behind,
which later shell/make/kubectl steps don’t need. Update the checkout
configuration in the release-prod workflow to disable persisted credentials for
this job, while keeping the existing ref logic intact. Use the checkout step in
the release context/job block as the location to apply the credential-setting
change.
Source: Linters/SAST tools
Comment on lines
+536
to
+558
| restore_already_current=false | ||
| if [ "${FORCE_RESTORE:-false}" != "true" ]; then | ||
| backend_pod="$( | ||
| kubectl -n "$NAMESPACE" get pod -l app.kubernetes.io/name=deces-backend,app.kubernetes.io/part-of=matchid -o jsonpath='{.items[0].metadata.name}' | ||
| )" | ||
| backend_pod="${backend_pod# }" | ||
| live_count="$( | ||
| kubectl -n "$NAMESPACE" exec "$backend_pod" -c deces-backend -- node -e 'require("http").get("http://elasticsearch:9200/deces/_count", r => { let d = ""; r.on("data", c => d += c); r.on("end", () => { try { console.log(JSON.parse(d).count || 0); } catch (_) { console.log(0); } }); }).on("error", () => console.log(0));' | ||
| )" | ||
| live_count="${live_count# }" | ||
| live_snapshot="$( | ||
| kubectl -n "$NAMESPACE" exec "$backend_pod" -c deces-backend -- node -e 'require("http").get("http://elasticsearch:9200/.matchid-restore-state/_doc/deces", r => { let d = ""; r.on("data", c => d += c); r.on("end", () => { try { console.log(JSON.parse(d)._source?.snapshot_name || ""); } catch (_) { console.log(""); } }); }).on("error", () => console.log(""));' | ||
| )" | ||
| live_snapshot="${live_snapshot# }" | ||
| if [ "$live_snapshot" = "$SNAPSHOT_NAME" ] && [ "${live_count:-0}" -gt 0 ]; then | ||
| restore_already_current=true | ||
| echo "Elasticsearch snapshot $SNAPSHOT_NAME is already live ($live_count docs); skipping restore job." | ||
| fi | ||
| fi | ||
| echo "RESTORE_ALREADY_CURRENT=$restore_already_current" >> "$GITHUB_ENV" | ||
| if [ "$restore_already_current" != "true" ]; then | ||
| kubectl -n "$NAMESPACE" apply -f deploy/k8s/overlays/prod/elasticsearch-restore.job.yaml | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail open when the pre-restore probe cannot run.
Lines 538-548 make backend pod lookup/exec a hard gate under set -e; if the pod is missing, restarting, or exec fails, the workflow exits before applying the restore job. Treat probe failures as “not already current” and let elasticsearch-restore.job.yaml perform its own idempotent checks.
Proposed fix
restore_already_current=false
if [ "${FORCE_RESTORE:-false}" != "true" ]; then
backend_pod="$(
kubectl -n "$NAMESPACE" get pod -l app.kubernetes.io/name=deces-backend,app.kubernetes.io/part-of=matchid -o jsonpath='{.items[0].metadata.name}'
- )"
- backend_pod="${backend_pod# }"
- live_count="$(
- kubectl -n "$NAMESPACE" exec "$backend_pod" -c deces-backend -- node -e 'require("http").get("http://elasticsearch:9200/deces/_count", r => { let d = ""; r.on("data", c => d += c); r.on("end", () => { try { console.log(JSON.parse(d).count || 0); } catch (_) { console.log(0); } }); }).on("error", () => console.log(0));'
- )"
- live_count="${live_count# }"
- live_snapshot="$(
- kubectl -n "$NAMESPACE" exec "$backend_pod" -c deces-backend -- node -e 'require("http").get("http://elasticsearch:9200/.matchid-restore-state/_doc/deces", r => { let d = ""; r.on("data", c => d += c); r.on("end", () => { try { console.log(JSON.parse(d)._source?.snapshot_name || ""); } catch (_) { console.log(""); } }); }).on("error", () => console.log(""));'
- )"
- live_snapshot="${live_snapshot# }"
+ )" || true
+ backend_pod="$(printf '%s' "$backend_pod" | tr -d '[:space:]')"
+ live_count=0
+ live_snapshot=""
+ if [ -n "$backend_pod" ]; then
+ live_count="$(
+ kubectl -n "$NAMESPACE" exec "$backend_pod" -c deces-backend -- node -e 'require("http").get("http://elasticsearch:9200/deces/_count", r => { let d = ""; r.on("data", c => d += c); r.on("end", () => { try { console.log(JSON.parse(d).count || 0); } catch (_) { console.log(0); } }); }).on("error", () => console.log(0));'
+ )" || true
+ live_count="$(printf '%s' "$live_count" | tr -d '[:space:]')"
+ case "$live_count" in ''|*[!0-9]*) live_count=0 ;; esac
+ live_snapshot="$(
+ kubectl -n "$NAMESPACE" exec "$backend_pod" -c deces-backend -- node -e 'require("http").get("http://elasticsearch:9200/.matchid-restore-state/_doc/deces", r => { let d = ""; r.on("data", c => d += c); r.on("end", () => { try { console.log(JSON.parse(d)._source?.snapshot_name || ""); } catch (_) { console.log(""); } }); }).on("error", () => console.log(""));'
+ )" || true
+ live_snapshot="$(printf '%s' "$live_snapshot" | tr -d '\r\n')"
+ else
+ echo "No backend pod available for pre-restore probe; applying restore job."
+ fi
if [ "$live_snapshot" = "$SNAPSHOT_NAME" ] && [ "${live_count:-0}" -gt 0 ]; then
restore_already_current=true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| restore_already_current=false | |
| if [ "${FORCE_RESTORE:-false}" != "true" ]; then | |
| backend_pod="$( | |
| kubectl -n "$NAMESPACE" get pod -l app.kubernetes.io/name=deces-backend,app.kubernetes.io/part-of=matchid -o jsonpath='{.items[0].metadata.name}' | |
| )" | |
| backend_pod="${backend_pod# }" | |
| live_count="$( | |
| kubectl -n "$NAMESPACE" exec "$backend_pod" -c deces-backend -- node -e 'require("http").get("http://elasticsearch:9200/deces/_count", r => { let d = ""; r.on("data", c => d += c); r.on("end", () => { try { console.log(JSON.parse(d).count || 0); } catch (_) { console.log(0); } }); }).on("error", () => console.log(0));' | |
| )" | |
| live_count="${live_count# }" | |
| live_snapshot="$( | |
| kubectl -n "$NAMESPACE" exec "$backend_pod" -c deces-backend -- node -e 'require("http").get("http://elasticsearch:9200/.matchid-restore-state/_doc/deces", r => { let d = ""; r.on("data", c => d += c); r.on("end", () => { try { console.log(JSON.parse(d)._source?.snapshot_name || ""); } catch (_) { console.log(""); } }); }).on("error", () => console.log(""));' | |
| )" | |
| live_snapshot="${live_snapshot# }" | |
| if [ "$live_snapshot" = "$SNAPSHOT_NAME" ] && [ "${live_count:-0}" -gt 0 ]; then | |
| restore_already_current=true | |
| echo "Elasticsearch snapshot $SNAPSHOT_NAME is already live ($live_count docs); skipping restore job." | |
| fi | |
| fi | |
| echo "RESTORE_ALREADY_CURRENT=$restore_already_current" >> "$GITHUB_ENV" | |
| if [ "$restore_already_current" != "true" ]; then | |
| kubectl -n "$NAMESPACE" apply -f deploy/k8s/overlays/prod/elasticsearch-restore.job.yaml | |
| fi | |
| restore_already_current=false | |
| if [ "${FORCE_RESTORE:-false}" != "true" ]; then | |
| backend_pod="$( | |
| kubectl -n "$NAMESPACE" get pod -l app.kubernetes.io/name=deces-backend,app.kubernetes.io/part-of=matchid -o jsonpath='{.items[0].metadata.name}' | |
| )" || true | |
| backend_pod="$(printf '%s' "$backend_pod" | tr -d '[:space:]')" | |
| live_count=0 | |
| live_snapshot="" | |
| if [ -n "$backend_pod" ]; then | |
| live_count="$( | |
| kubectl -n "$NAMESPACE" exec "$backend_pod" -c deces-backend -- node -e 'require("http").get("http://elasticsearch:9200/deces/_count", r => { let d = ""; r.on("data", c => d += c); r.on("end", () => { try { console.log(JSON.parse(d).count || 0); } catch (_) { console.log(0); } }); }).on("error", () => console.log(0));' | |
| )" || true | |
| live_count="$(printf '%s' "$live_count" | tr -d '[:space:]')" | |
| case "$live_count" in ''|*[!0-9]*) live_count=0 ;; esac | |
| live_snapshot="$( | |
| kubectl -n "$NAMESPACE" exec "$backend_pod" -c deces-backend -- node -e 'require("http").get("http://elasticsearch:9200/.matchid-restore-state/_doc/deces", r => { let d = ""; r.on("data", c => d += c); r.on("end", () => { try { console.log(JSON.parse(d)._source?.snapshot_name || ""); } catch (_) { console.log(""); } }); }).on("error", () => console.log(""));' | |
| )" || true | |
| live_snapshot="$(printf '%s' "$live_snapshot" | tr -d '\r\n')" | |
| else | |
| echo "No backend pod available for pre-restore probe; applying restore job." | |
| fi | |
| if [ "$live_snapshot" = "$SNAPSHOT_NAME" ] && [ "${live_count:-0}" -gt 0 ]; then | |
| restore_already_current=true | |
| echo "Elasticsearch snapshot $SNAPSHOT_NAME is already live ($live_count docs); skipping restore job." | |
| fi | |
| fi | |
| echo "RESTORE_ALREADY_CURRENT=$restore_already_current" >> "$GITHUB_ENV" | |
| if [ "$restore_already_current" != "true" ]; then | |
| kubectl -n "$NAMESPACE" apply -f deploy/k8s/overlays/prod/elasticsearch-restore.job.yaml | |
| 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 @.github/workflows/release-prod.yml around lines 536 - 558, The pre-restore
probe in the release workflow is too brittle because the `backend_pod` lookup
and `kubectl exec` checks can fail under `set -e` and stop the job before the
restore is applied. Update the restore gate around `restore_already_current` so
failures in the probe path (pod missing, restarting, or exec error) are treated
as “not already current” instead of exiting, while still preserving the existing
success path that compares `live_snapshot` and `live_count` before deciding
whether to skip `elasticsearch-restore.job.yaml`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Production incident notes
Tests
Summary by CodeRabbit