fix(kubernetes): recover BatchSandbox after Pod restart - #1523
Conversation
Persist failed Pod UIDs so a recovered original Pod can clear a stale Failed phase without treating a replacement Pod as recovery. Fixes opensandbox-group#1497
github-actions
Bot
added
component/k8s
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e29c5ea28
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| } | ||
|
|
||
| status.FailedPodUIDs = nil |
There was a problem hiding this comment.
When a failed pod recovers, setting FailedPodUIDs to nil is not enough to remove the persisted status field because updateStatus marshals the struct into a JSON merge patch and the field is tagged omitempty, so failedPodUIDs is omitted rather than sent as null. After recovery, the API server keeps the old UID list, the desired status keeps comparing unequal to the stored status, and the reconciler will keep trying to patch the same object instead of becoming idempotent.
AGENTS.md reference: kubernetes/AGENTS.md:L166-L166
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed against the current head (1e29c5e). This is more than a stale-status cosmetic issue: applyResumingRuntimePhase intentionally clears this provenance so resume failures remain terminal, but the merge patch omits the key. On the next steady reconcile, the stale UID can satisfy failedPodsRecovered and incorrectly move that lifecycle failure back to Succeed. Please serialize an explicit deletion/null (or use a status update) and add a persistence-level test that round-trips the API patch, rather than checking only the in-memory runtime view.
| // current Failed phase. The controller uses these UIDs to distinguish an | ||
| // in-place recovery from a replacement Pod that reuses the same name. | ||
| // +optional | ||
| FailedPodUIDs []types.UID `json:"failedPodUIDs,omitempty"` |
There was a problem hiding this comment.
This status field is added to the Go type and the Kustomize CRD, but the bundled Helm CRD copy at kubernetes/charts/opensandbox-controller/templates/crds/batchsandboxes.yaml still lacks status.failedPodUIDs. In Helm-installed clusters, that unknown status field is pruned by the CRD schema, so the controller cannot persist pod UID provenance and the transient Pod recovery path remains ineffective for Helm users.
AGENTS.md reference: kubernetes/AGENTS.md:L164-L164
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed by rendering the chart: the Helm CRD contains pauseObservedGeneration but not failedPodUIDs. With the status schema missing this property, Kubernetes prunes the UID provenance, so the next reconciliation sees no recorded identity and the recovery path remains terminal for Helm-installed clusters. Please sync the chart CRD and, if practical, add a generated-CRD parity check to prevent the Helm copy from drifting again.
|
@Pangjiping 您好，#1497 的修复已在 PR #1523 中更新，之前 review 提出的两项问题均已处理：
目前 PR 正在等待维护者复审和合并。方便时烦请帮忙审核；如果实现符合预期，请协助 approve 并 merge。若仍有冲突、CI 门禁或其他问题需要作者处理，也请指出，我会及时跟进。 @jianpingpei 也烦请帮忙确认最新修改是否覆盖了 #1497 中报告的场景。谢谢！ EnglishHi @Pangjiping, the fix for #1497 has been updated in PR #1523. Both issues raised during the previous review have been addressed:
The PR is currently awaiting maintainer review and merge. When convenient, could you please review it? If the implementation looks good, please approve and merge it. If any conflicts, CI requirements, or other changes still need to be handled by the author, please let me know and I will follow up promptly. @jianpingpei, could you also please confirm whether the latest changes cover the scenario reported in #1497? Thank you! |
2 participants
Summary
BatchSandboxwhen the original failed Pod returns to Running and Ready.Testing
cd docs && pnpm docs:build)make test)Breaking Changes
Checklist