fix(checkpoint): restore startup ordering - #2944
Conversation
Signed-off-by: Sebastian Łaskawiec <sebastian.laskawiec@defenseunicorns.com>
|
@greptileai review |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
The new scripts have a few concrete reliability/idempotency issues (shell strictness, kubectl wait flakiness, patch robustness, and cluster-name consistency) that should be addressed to avoid intermittent checkpoint workflow failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Restores explicit quiesce/restore ordering for checkpoint capture & restore to avoid suspending core infrastructure (e.g., Istiod) and timing out during restore (CORE-689).
Changes:
- Add an explicit “quiesce dependencies” step before checkpoint capture (Keycloak, Pepr watcher/admission, ztunnel).
- Add an explicit “restore dependencies” step after restore (ztunnel, Pepr admission, Pepr watcher, Keycloak), waiting for rollouts.
File summaries
| File | Description |
|---|---|
| packages/checkpoint-dev/zarf.yaml | Adds ordered pre-capture scale-down/suspend steps and post-restore scale-up/resume steps to stabilize checkpoint restore behavior. |
Review details
Suppressed comments (4)
packages/checkpoint-dev/zarf.yaml:57
kubectl wait --for=deleteexits non-zero when there are no matching pods (e.g., if they terminate between the scale and wait steps), which can make this action flaky. Add--ignore-not-found=trueto make the wait idempotent.
./zarf tools kubectl --context k3d-uds -n pepr-system wait --for=delete pod -l app=pepr-uds-core-watcher --timeout=60s
packages/checkpoint-dev/zarf.yaml:60
kubectl wait --for=deleteexits non-zero when there are no matching pods (e.g., if they terminate between the scale and wait steps), which can make this action flaky. Add--ignore-not-found=trueto make the wait idempotent.
./zarf tools kubectl --context k3d-uds -n pepr-system wait --for=delete pod -l app=pepr-uds-core --timeout=60s
packages/checkpoint-dev/zarf.yaml:63
kubectl wait --for=deleteexits non-zero when there are no matching pods (e.g., if they terminate between the scale and wait steps), which can make this action flaky. Add--ignore-not-found=trueto make the wait idempotent.
./zarf tools kubectl --context k3d-uds -n istio-system wait --for=delete pod -l app=ztunnel --timeout=60s
packages/checkpoint-dev/zarf.yaml:129
- Use
set -euo pipefailconsistently here as well to avoid masking failures (e.g., from unset variables or failed pipeline commands).
set -e
- Files reviewed: 1/1 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Greptile SummaryRestores explicit checkpoint dependency ordering while leaving Istiod and other cluster infrastructure running:
The ordering is coherent for the default slim-development topology, but Keycloak autoscaling can defeat the new quiescence step when HA is enabled. Confidence Score: 4/5The PR should not merge until checkpoint capture handles or explicitly rejects Keycloak HA, because its HPA can restart Keycloak during quiescence. The default startup ordering is consistent, but an exposed source-cluster configuration enables an HPA with a two-replica minimum, conflicting with the new attempt to scale Keycloak to zero before capture. Files Needing Attention: packages/checkpoint-dev/zarf.yaml Important Files Changed
Sequence DiagramsequenceDiagram
participant Z as Zarf
participant K as Keycloak
participant P as Pepr
participant T as ztunnel
participant C as Checkpoint
Z->>K: Scale StatefulSet to 0
Z->>P: Scale watcher to 0
Z->>P: Scale admission to 0
Z->>T: Add unsatisfiable node selector
Z->>C: Capture cluster
C-->>Z: Restore cluster
Z->>T: Remove node selector and await rollout
Z->>P: Restore admission and await rollout
Z->>P: Restore watcher and await rollout
Z->>K: Restore StatefulSet and await rollout
Reviews (1): Last reviewed commit: "fix(CORE-689): restore checkpoint startu..." | Re-trigger Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f1e2d1f2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Sebastian Łaskawiec <sebastian.laskawiec@defenseunicorns.com>
Signed-off-by: Sebastian Łaskawiec <sebastian.laskawiec@defenseunicorns.com>
Signed-off-by: Sebastian Łaskawiec <sebastian.laskawiec@defenseunicorns.com>
mjnagel
left a comment
There was a problem hiding this comment.
I think generally looks fine and mirrors what we previously did (f91ff5a). Only concern would be that it adds ~1 minute to the deploy process based on the latest runs in this branch vs release CI. I'd say that's probably worth it for removing flakiness, just wanted to call it out. Left one comment that might help get a few seconds back but probably won't change much.
Would still be curious if we could identify the root cause and a different way to resolve this issue - in theory Istio should be able to "heal" things on startup (which is part of why we were able to remove the scale down/up before). I wonder if something changed in Istio and/or k3d/k3s that affected this?
| ./zarf tools kubectl --context k3d-${ZARF_VAR_CLUSTER_NAME} -n pepr-system scale deploy pepr-uds-core --replicas=2 | ||
| ./zarf tools kubectl --context k3d-${ZARF_VAR_CLUSTER_NAME} -n pepr-system rollout status deploy pepr-uds-core --timeout=60s | ||
|
|
||
| ./zarf tools kubectl --context k3d-${ZARF_VAR_CLUSTER_NAME} -n pepr-system scale deploy pepr-uds-core-watcher --replicas=1 | ||
| ./zarf tools kubectl --context k3d-${ZARF_VAR_CLUSTER_NAME} -n pepr-system rollout status deploy pepr-uds-core-watcher --timeout=60s |
There was a problem hiding this comment.
Could we potentially scale these up concurrently to save some time? Since Pepr ignores its own namespace there wouldn't be any issues with the webhook I don't think?
Description
Restore explicit checkpoint startup ordering for CORE-689.
Before capture:
After restore, start those dependencies in reverse order and wait for each rollout.
This keeps cluster infrastructure, including Istiod, running. The previous automatic workload-discovery approach suspended infrastructure and timed out while restoring Istiod.
Related Issue
Relates to CORE-689
Type of change
Steps to Validate
Local checks:
yamllint packages/checkpoint-dev/zarf.yaml -c .yamllint --no-warningsuds zarf dev lint packages/checkpoint-devgit diff --checkThe existing checkpoint workflow validates capture and restoration on amd64 and arm64. The reference implementation passed both jobs.
Checklist before merging