fix(controller): refill deleted pod for RecoveryPolicy=None - #1669
fix(controller): refill deleted pod for RecoveryPolicy=None#1669chenhuiluo wants to merge 4 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@0xjasoncao Please review this pr. |
handleDeletedPod's switch had cases only for ServingGroupRecreate and RoleRecreate; None fell through with no-op, so a deleted pod was never refilled and the replica went permanently missing. Add a None case that re-enqueues the ModelServing so the reconcile loop tops up the single missing replica via manageRoleReplicasPerGroup (deployment-style), without deleting the whole role/serving group. Signed-off-by: luochenhui1 <luochenhui1@kingsoft.com>
c7f39fa to
441ff88
Compare
…er.go Co-authored-by: lizhencheng <lizhencheng6@huawei.com> Signed-off-by: chenhuiluo <41547730+chenhuiluo@users.noreply.github.com>
handleErrorPod unconditionally deleted a pod whose container the kubelet had restarted, even under RecoveryPolicy=None, causing the pod to be deleted and refilled in a loop (CrashLoopBackOff -> delete -> refill -> restart). None should follow the pod's own restartPolicy and leave a restarted, still-alive pod in place. Skip deletion when RecoveryPolicy=None and the pod is ContainerRestarted but not PodFailed; a terminal PodFailed pod still falls through to deletion plus the None refill path in handleDeletedPod. Signed-off-by: luochenhui1 <luochenhui1@kingsoft.com>
kube-gopher
left a comment
kube-gopher
left a comment
There was a problem hiding this comment.
both new None-policy paths can leave unavailable ServingGroups recorded as Running
| if ms.Spec.RecoveryPolicy == workloadv1alpha1.NoneRestartPolicy && | ||
| utils.ContainerRestarted(errPod) && !utils.IsPodFailed(errPod) { | ||
| klog.V(4).Infof("RecoveryPolicy=None: leave restarted pod %s to kubelet", errPod.Name) | ||
| return nil |
There was a problem hiding this comment.
Suggest: Preserve failure bookkeeping when skipping pod deletion; leave the pod to kubelet, but still perform the failure-state bookkeeping and reconcile.
| case workloadv1alpha1.NoneRestartPolicy: | ||
| // None (deployment-style): re-enqueue so the reconcile loop refills the | ||
| // single missing pod, without deleting the whole role/serving group. | ||
| klog.V(4).Infof("RecoveryPolicy=None: re-enqueue to refill deleted pod %s", pod.Name) | ||
| c.enqueueModelServing(ms) |
There was a problem hiding this comment.
Suggest: Mark the group unavailable before refilling a deleted pod, transition the affected role/group out of Running before enqueueing
… accurate Both None paths returned/enqueued without transitioning the affected role and serving group out of Running, so an unavailable replica (a crashed pod left to the kubelet, or a deleted pod pending refill) stayed counted as available. Extract markPodUnavailable for the shared failure bookkeeping (remove the pod from the running set, role/group Running -> Creating) and call it from both None branches before leaving the pod to the kubelet / enqueuing the refill. A bookkeeping error is logged, not fatal, so the refill still proceeds. Addresses review feedback. Signed-off-by: luochenhui1 <luochenhui1@kingsoft.com>
f82815e to
183e880
Compare
Agreed — both None paths now transition the affected role and serving group out of Running before leaving the pod to the kubelet / enqueuing the refill, via the shared markPodUnavailable helper. status.AvailableReplicas now reflects the unavailable replica. Done in the latest push. |
4 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
RecoveryPolicy=None left deleted pods permanently missing. handleDeletedPod only handled ServingGroupRecreate and RoleRecreate, so None fell through with no-op and nothing refilled the replica. This adds a None case that re-enqueues the ModelServing so the existing reconcile loop refills the single missing replica(deployment-style), without deleting the whole role or serving group.
Which issue(s) this PR fixes:
Fixes #1661
Bug evidence (required for bug-related PRs):
Reproduced in issue #1661: a ModelServing with recoveryPolicy: None, a role of 3 replicas, restartPolicy: Always, and a busybox container running
sleep 10; exit 1for index 0. worker-0's container exits, the controller deletes the pod, and nothing recreates it — the replica is gone permanently while worker-1/worker-2 keep running. The controller log shows the pod "been deleted without grace time" with no subsequent refill.Root cause: the deleted-pod handler switches on RecoveryPolicy but has no None case, so None returns without rebuilding. The NoneRestartPolicy constant is otherwise unreferenced in the codebase, confirming the None path was never wired up. The fix wires None to the reconcile refill path already used for scale-up.
Special notes for your reviewer:
This fixes the clearly-broken "deleted pod is never refilled under None" path. It does NOT change the upstream delete path: under None a failed pod is still deleted then refilled (same as the other policies). The issue reporter additionally asked for "controller should not delete the pod; kubelet restarts the container in place." That is a larger behavioral change and is intentionally out of scope here. Please confirm whether deployment-style refill satisfies the None contract, or whether the stronger "leave the pod to kubelet" semantics should be a follow-up.
This PR was written in part with the assistance of generative AI.
Does this PR introduce a user-facing change?: