model-serving: guard pod deletion by UID

[avinxshKD]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Prevents a stale ModelServing grace timer from deleting a replacement pod with the same name.

The controller now checks the pod UID before deleting it and uses a UID precondition on the delete request.

Which issue(s) this PR fixes:
Fixes #1426

Bug evidence (required for bug-related PRs):

A ModelServing with the following configuration can hit this:

recoveryPolicy: RoleRecreate
template:
  restartGracePeriodSeconds: 60

A failed or restarted pod starts the grace timer. If that pod is deleted or replaced during the grace period, the replacement gets the same deterministic name but a different UID. The old handler previously looked up and deleted the pod by name only:

kthena/pkg/model-serving-controller/controller/model_serving_controller.go

Lines 1731 to 1768 in 3f3e715

	func (c *ModelServingController) handlePodAfterGraceTime(ms *workloadv1alpha1.ModelServing, errPod *corev1.Pod) {
	if ms.Spec.Template.RestartGracePeriodSeconds != nil && *ms.Spec.Template.RestartGracePeriodSeconds > 0 {
	// Wait for the grace period before making a decision
	time.Sleep(time.Duration(*ms.Spec.Template.RestartGracePeriodSeconds) * time.Second)
	klog.V(4).Infof("%s after grace time", errPod.Name)
	defer c.graceMap.Delete(utils.GetNamespaceName(errPod))
	newPod, err := c.podsLister.Pods(ms.Namespace).Get(errPod.Name)
	if err != nil {
	if apierrors.IsNotFound(err) {
	klog.V(4).Infof("pod %s has been deleted after grace time", errPod.Name)
	} else {
	klog.Errorf("cannot get pod %s after grace time, err: %v", errPod.Name, err)
	}
	return
	}
	if !utils.IsPodRunningAndReady(newPod) {
	// pod has not recovered after the grace period, needs to be rebuilt
	// After this pod has been deleted, we will rebuild the ServingGroup in deletePod function
	err = c.kubeClientSet.CoreV1().Pods(ms.Namespace).Delete(context.TODO(), newPod.Name, metav1.DeleteOptions{})
	if err != nil {
	klog.Errorf("cannot delete pod %s after grace time, err: %v", newPod.Name, err)
	return
	}
	klog.V(2).Infof("%s been deleted after grace time", errPod.Name)
	}
	} else {
	// grace period is not set or the grace period is 0, the deletion will be executed immediately.
	defer c.graceMap.Delete(utils.GetNamespaceName(errPod))
	err := c.kubeClientSet.CoreV1().Pods(ms.Namespace).Delete(context.TODO(), errPod.Name, metav1.DeleteOptions{})
	if err != nil {
	klog.Errorf("cannot delete pod %s when it error, err: %v", errPod.Name, err)
	return
	}
	klog.V(2).Infof("%s been deleted without grace time", errPod.Name)
	}

The added test reproduces this with separate failed and replacement pod UIDs. It also verifies UID preconditions for delayed and immediate deletion.

Tested with:

go test ./pkg/model-serving-controller/... -count=1

This was reproduced with the controller fake client, not on a live cluster.

Special notes for your reviewer:

AI assistance was used during code inspection and initial preparation. I reviewed the final diff and ran the relevant tests locally on my own.

Race tests were not run because the local Windows environment does not have gcc.

Does this PR introduce a user-facing change?:

NONE

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[volcano-sh-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign yaozengzeng for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• pkg/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[avinxshKD]

ptal @YaoZengzeng @LiZhenCheng9527 when get chance

[acsoto]

/lgtm

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[avinxshKD]

@LiZhenCheng9527 updated, we now track each pod’s grace timer using its name and UID, soo a replacement pod gets its own timer instead of being skipped because of the old pod. PTAL

@acsoto also take a look if u have time sir, as lgtm tag has been removed now.

[acsoto]

/lgtm

[avinxshKD]

thanks @acsoto for a lgtm 😄
@hzxuzhonghu @LiZhenCheng9527 ptal

[LiZhenCheng9527]

I don't think a Grace unit test is important enough to warrant creating a new file.

[LiZhenCheng9527]

nit: Other test functions also have the need to create controllers. Can a test helper function be created that can be used by most unit tests?

[LiZhenCheng9527]

Maybe you can to this in another PR.

[volcano-sh-bot]

New changes are detected. LGTM label has been removed.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[avinxshKD]

@LiZhenCheng9527 done, Grace tests moved into model_serving_controller_test.go. and
separate test file removed. PTAL

Maybe you can to this in another PR.

sure, separate PR for Shared helper refactor