Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1662,6 +1662,14 @@ func (c *ModelServingController) handleReadyPod(ms *workloadv1alpha1.ModelServin
}

func (c *ModelServingController) handleErrorPod(ms *workloadv1alpha1.ModelServing, servingGroupName string, errPod *corev1.Pod) error {
// RecoveryPolicy=None: leave a restarted, still-alive pod to the kubelet's
// restartPolicy instead of deleting it. A terminal PodFailed pod still falls
// through to deletion + refill (handleDeletedPod None case).
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest: Preserve failure bookkeeping when skipping pod deletion; leave the pod to kubelet, but still perform the failure-state bookkeeping and reconcile.

}
// pod is already in the grace period and does not need to be processed for the time being.
key := getPodGracePeriodKey(errPod)
now := time.Now()
Expand Down Expand Up @@ -1775,6 +1783,11 @@ func (c *ModelServingController) handleDeletedPod(ms *workloadv1alpha1.ModelServ
}
}
c.DeleteRole(context.Background(), ms, servingGroupName, utils.GetRoleName(pod), utils.GetRoleID(pod))
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)
Comment on lines +1793 to +1798

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest: Mark the group unavailable before refilling a deleted pod, transition the affected role/group out of Running before enqueueing

}
return nil
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2051,6 +2051,40 @@ func createStandardModelServing(name string, replicas int32, roleReplicas int32)
}
}

// TestHandleDeletedPodNoneEnqueues verifies the RecoveryPolicy=None branch of
// handleDeletedPod: a deleted pod must re-enqueue the ModelServing so the
// reconcile loop refills it (deployment-style), and must NOT delete the whole
// role/serving group.
func TestHandleDeletedPodNoneEnqueues(t *testing.T) {
ms := createStandardModelServing("ms-none-recovery", 1, 1)
ms.Spec.RecoveryPolicy = workloadv1alpha1.NoneRestartPolicy
h := newTestController(t, ms)
controller := h.controller

require.Eventually(t, func() bool {
_, err := controller.modelServingLister.ModelServings("default").Get(ms.Name)
return err == nil
}, 2*time.Second, 10*time.Millisecond)
drainWorkqueue(t, controller.workqueue)
assertQueueEmpty(t, controller.workqueue)

pod := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Namespace: ms.Namespace,
Name: ms.Name + "-0-prefill-0-0",
Labels: map[string]string{
workloadv1alpha1.ModelServingNameLabelKey: ms.Name,
workloadv1alpha1.GroupNameLabelKey: ms.Name + "-0",
},
},
}

// None must enqueue the ModelServing for reconcile; the other policies
// delete the role/serving group here, None must not.
require.NoError(t, controller.handleDeletedPod(ms, ms.Name+"-0", pod))
h.expectQueuedKey(namespacedKey(ms.Namespace, ms.Name))
}

// createGangModelServing creates a ModelServing with gang policy
func createGangModelServing(name string, replicas int32, roleReplicas int32) *workloadv1alpha1.ModelServing {
ms := createStandardModelServing(name, replicas, roleReplicas)
Expand Down Expand Up @@ -6094,6 +6128,45 @@ func TestHandleErrorPodTracksReplacementByUID(t *testing.T) {
}, 2*time.Second, 10*time.Millisecond)
}

// TestHandleErrorPodNoneLeavesRestartedPod verifies that under RecoveryPolicy=None,
// a pod whose container the kubelet has restarted (pod still alive, not PodFailed)
// is NOT deleted by handleErrorPod — it is left to the pod's own restartPolicy,
// avoiding the pod churn the Recreate policies cause. A terminal PodFailed pod
// still falls through to deletion + refill (handled by handleDeletedPod None case).
func TestHandleErrorPodNoneLeavesRestartedPod(t *testing.T) {
const (
namespace = "default"
podName = "test-none-0-prefill-0-0"
)
restartedPod := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Namespace: namespace,
Name: podName,
UID: types.UID("restarted-pod"),
},
Status: corev1.PodStatus{
Phase: corev1.PodRunning,
ContainerStatuses: []corev1.ContainerStatus{
{RestartCount: 1},
},
},
}
ms := &workloadv1alpha1.ModelServing{
ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: "test-none"},
Spec: workloadv1alpha1.ModelServingSpec{RecoveryPolicy: workloadv1alpha1.NoneRestartPolicy},
}
controller, kubeClient := newGracePeriodTestController(t, restartedPod)

// None must early-return before touching the store/graceMap or deleting the pod.
require.NoError(t, controller.handleErrorPod(ms, "test-none-0", restartedPod))

// No pod delete was issued — the pod is left to kubelet's restartPolicy.
for _, action := range kubeClient.Actions() {
require.Falsef(t, action.Matches("delete", "pods"),
"None must not delete a restarted pod; got unexpected action %v", action)
}
}

func newGracePeriodTestController(t *testing.T, pod *corev1.Pod) (*ModelServingController, *kubefake.Clientset) {
t.Helper()

Expand Down
Loading