fix: guard podgroup annotation patch by pod UID by YiD11 · Pull Request #5344 · volcano-sh/volcano

YiD11 · 2026-05-27T12:22:54Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

When a StatefulSet in Volcano is deleted and then recreated with the same namespace/name, the PodGroup controller may still have an old Pod request in the queue.

The podRequest only contains namespace and name. If the worker handles it after the Pod is recreated, the cache lookup can return the new Pod. Then the old PodGroup name may be patched into the new Pod's scheduling.k8s.io/group-name.

If that old PodGroup is garbage-collected, the new Pod keeps a annotation pointing to a missing PodGroup and can stay Pending.

This PR adds the Pod UID field in podRequest and skips queued work when the cached Pod UID mismatches the request. And it changes the annotation update to a JSON Patch with a /metadata/uid test, so the API server should reject the patch if the Pod was recreated after the controller read it from cache.

Which issue this PR fixes:

Fixes #5331

Special notes for your reviewer:

The original issue was reproduced with StatefulSet delete/recreate loops on Volcano v1.11.0. Details are shown in the original issue page.

Does this PR introduce a user-facing change?

NONE

volcano-sh-bot · 2026-05-27T12:23:06Z

Welcome @YiD11! It looks like this is your first PR to volcano-sh/volcano 🎉

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds a UID check to prevent the podgroup controller from acting on stale queue entries when a pod with the same namespace/name has been recreated, and converts the annotation patch to a JSON Patch with a UID test for atomicity.

Changes:

Include podUID in podRequest and skip processing when the cached pod's UID differs from the queued UID.
Rewrite updatePodAnnotations to use a JSON Patch including a test op on /metadata/uid, replacing the prior strategic-merge patch.
Add test coverage for the recreated-pod skip and the new JSON-patch behavior; add UIDs to existing test pods.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
pkg/controllers/podgroup/pg_controller.go	UID mismatch short-circuits `processNextReq`.
pkg/controllers/podgroup/pg_controller_handler.go	Track UID in `podRequest`; switch to JSONPatch with UID test op.
pkg/controllers/podgroup/pg_controller_test.go	New tests for UID skipping and patch format; UIDs added to fixtures.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

YiD11 · 2026-05-28T08:19:56Z

 	if pod.Annotations == nil {
-		pod.Annotations = make(map[string]string)
-	}
-	if pod.Annotations[scheduling.KubeGroupNameAnnotationKey] == "" {
-		patch := metadataForMergePatch{
-			Metadata: annotationForMergePatch{
-				Annotations: map[string]string{
-					scheduling.KubeGroupNameAnnotationKey: pgName,
-				},
+		patch = append(patch, map[string]any{
+			"op":   "add",
+			"path": "/metadata/annotations",
+			"value": map[string]string{
+				scheduling.KubeGroupNameAnnotationKey: pgName,
 			},
-		}
-
-		patchBytes, err := json.Marshal(&patch)
-		if err != nil {
-			klog.Errorf("Failed to json.Marshal pod annotation: %v", err)
-			return err
-		}
-
-		if _, err := pg.kubeClient.CoreV1().Pods(pod.Namespace).Patch(context.TODO(), pod.Name, types.StrategicMergePatchType, patchBytes, metav1.PatchOptions{}); err != nil {
-			klog.Errorf("Failed to update pod <%s/%s>: %v", pod.Namespace, pod.Name, err)
-			return err
-		}
-		klog.V(4).Infof("Bound Pod <%s/%s> to PodGroup <%s/%s>", pod.Namespace, pod.Name, pod.Namespace, pgName)
+		})
+	} else if pod.Annotations[scheduling.KubeGroupNameAnnotationKey] == "" {
+		patch = append(patch, map[string]any{
+			"op":    "add",
+			"path":  "/metadata/annotations/" + strings.ReplaceAll(scheduling.KubeGroupNameAnnotationKey, "/", "~1"),
+			"value": pgName,
+		})


done on new commit

 		klog.Errorf("Failed to get pod by <%v> from cache: %v", req, err)
 		return true
 	}
+	if pod.UID != req.podUID {


+	for _, action := range c.kubeClient.(*kubeclient.Clientset).Actions() {
+		if action.Matches("patch", "pods") {
+			patchAction = action.(k8stesting.PatchAction)
+			break
+		}
+	}
+	if !assert.NotNil(t, patchAction) {


gemini-code-assist

Code Review

This pull request introduces pod UID validation during pod group processing to prevent operations on recreated pods with the same namespace and name. It updates the podRequest struct to include the pod's UID, adds a UID mismatch check in the controller queue processing, and refactors updatePodAnnotations to use JSONPatch with a UID test operation. A review comment points out a potential issue where pod.Annotations is empty but not nil, which could cause a JSONPatch failure if the annotations field is absent on the API server, and suggests checking len(pod.Annotations) == 0 instead.

gemini-code-assist · 2026-05-27T12:24:15Z

 	if pod.Annotations == nil {
-		pod.Annotations = make(map[string]string)
-	}
-	if pod.Annotations[scheduling.KubeGroupNameAnnotationKey] == "" {
-		patch := metadataForMergePatch{
-			Metadata: annotationForMergePatch{
-				Annotations: map[string]string{
-					scheduling.KubeGroupNameAnnotationKey: pgName,
-				},
+		patch = append(patch, map[string]any{
+			"op":   "add",
+			"path": "/metadata/annotations",
+			"value": map[string]string{
+				scheduling.KubeGroupNameAnnotationKey: pgName,
 			},
-		}
-
-		patchBytes, err := json.Marshal(&patch)
-		if err != nil {
-			klog.Errorf("Failed to json.Marshal pod annotation: %v", err)
-			return err
-		}
-
-		if _, err := pg.kubeClient.CoreV1().Pods(pod.Namespace).Patch(context.TODO(), pod.Name, types.StrategicMergePatchType, patchBytes, metav1.PatchOptions{}); err != nil {
-			klog.Errorf("Failed to update pod <%s/%s>: %v", pod.Namespace, pod.Name, err)
-			return err
-		}
-		klog.V(4).Infof("Bound Pod <%s/%s> to PodGroup <%s/%s>", pod.Namespace, pod.Name, pod.Namespace, pgName)
+		})
+	} else if pod.Annotations[scheduling.KubeGroupNameAnnotationKey] == "" {


If pod.Annotations is initialized but empty (i.e., len(pod.Annotations) == 0), pod.Annotations == nil evaluates to false. In this case, the controller attempts to patch the path /metadata/annotations/scheduling.k8s.io~1group-name. However, if the annotations field is absent/nil on the Kubernetes API server, this patch operation will fail with a 422 Unprocessable Entity error (specifically, add operation does not apply: doc is missing path).

Using len(pod.Annotations) == 0 instead of pod.Annotations == nil safely handles both nil and empty maps, ensuring that the entire /metadata/annotations object is created when no annotations exist.

Suggested change

if pod.Annotations == nil {

pod.Annotations = make(map[string]string)

}

if pod.Annotations[scheduling.KubeGroupNameAnnotationKey] == "" {

patch := metadataForMergePatch{

Metadata: annotationForMergePatch{

Annotations: map[string]string{

scheduling.KubeGroupNameAnnotationKey: pgName,

},

patch = append(patch, map[string]any{

"op": "add",

"path": "/metadata/annotations",

"value": map[string]string{

scheduling.KubeGroupNameAnnotationKey: pgName,

},

}

patchBytes, err := json.Marshal(&patch)

if err != nil {

klog.Errorf("Failed to json.Marshal pod annotation: %v", err)

return err

}

if _, err := pg.kubeClient.CoreV1().Pods(pod.Namespace).Patch(context.TODO(), pod.Name, types.StrategicMergePatchType, patchBytes, metav1.PatchOptions{}); err != nil {

klog.Errorf("Failed to update pod <%s/%s>: %v", pod.Namespace, pod.Name, err)

return err

}

klog.V(4).Infof("Bound Pod <%s/%s> to PodGroup <%s/%s>", pod.Namespace, pod.Name, pod.Namespace, pgName)

})

} else if pod.Annotations[scheduling.KubeGroupNameAnnotationKey] == "" {

if len(pod.Annotations) == 0 {

patch = append(patch, map[string]any{

"op": "add",

"path": "/metadata/annotations",

"value": map[string]string{

scheduling.KubeGroupNameAnnotationKey: pgName,

},

})

} else if pod.Annotations[scheduling.KubeGroupNameAnnotationKey] == "" {

JesseStutler · 2026-05-28T02:30:00Z

Will take a look, thanks for your contribution.
/cc
/ok-to-test

JesseStutler · 2026-06-15T12:17:11Z

+	// UID test keeps this patch from hitting a recreated pod with the same namespace/name.
+	patch := []map[string]any{
+		{
+			"op":    "test",


I haven't seen this usage very often. Can you explain why it's written this way?

Because the patch request comes from the worker queue, it may become stale if the Pod is deleted and recreated while the request is still queued.

The purpose of using JSON Patch with a /metadata/uid test is to prevent this stale patch. If the live Pod UID is different from the UID in the patch, the API server will reject the patch.

JesseStutler · 2026-06-15T12:23:22Z

I'm wondering that in #5331, "The old PodGroup is patched/reused for the new Pods", I don't know whether we will still meet it in latest codes since the podgroup name is got from pod's ownerreference, which contains new statefulset's UID, so the new pod won't be patched the old pg name:

volcano/staging/src/volcano.sh/apis/pkg/apis/helpers/helpers.go

Lines 192 to 208 in fdb6ae0

    
           // GeneratePodgroupName generate podgroup name of normal pod. 
        
           func GeneratePodgroupName(pod *v1.Pod) string { 
        
           	pgName := vcbatch.PodgroupNamePrefix 
        
           	if len(pod.OwnerReferences) != 0 { 
        
           		for _, ownerReference := range pod.OwnerReferences { 
        
           			if ownerReference.Controller != nil && *ownerReference.Controller { 
        
           				pgName += string(ownerReference.UID) 
        
           				return pgName 
        
           			} 
        
           		} 
        
           	} 
        
           	pgName += string(pod.UID) 
        
           	return pgName 
        
           }

YiD11 · 2026-06-15T17:48:15Z

I'm wondering that in #5331, "The old PodGroup is patched/reused for the new Pods", I don't know whether we will still meet it in latest codes since the podgroup name is got from pod's ownerreference, which contains new statefulset's UID, so the new pod won't be patched the old pg name:

volcano/staging/src/volcano.sh/apis/pkg/apis/helpers/helpers.go

Lines 192 to 208 in fdb6ae0

// GeneratePodgroupName generate podgroup name of normal pod.

func GeneratePodgroupName(pod *v1.Pod) string {

pgName := vcbatch.PodgroupNamePrefix

if len(pod.OwnerReferences) != 0 {

for _, ownerReference := range pod.OwnerReferences {

if ownerReference.Controller != nil && *ownerReference.Controller {

pgName += string(ownerReference.UID)

return pgName

}

}

}

pgName += string(pod.UID)

return pgName

}

The code in this commit can reduce the probability of race conditions, but I was still able to reproduce the issue on this commit in an extreme case.

I temporarily deployed an admission webhook to delay only requests for podgroup creation, then recreated the StatefulSet/Pod with the same names before releasing the old request.

The pseudocode of the reproduction is:

hold requests for podgroup creation 

create StatefulSet notebook
oldPod = get Pod notebook-0
oldOwnerUID = oldPod.ownerReferences[0].uid

wait until Volcano starts creating podgroup-${oldOwnerUID}

delete StatefulSet notebook
force delete Pod notebook-0
create StatefulSet notebook again

newPod = wait for Pod notebook-0
assert newPod.uid != oldPod.uid
assert newPod.ownerReferences[0].uid != oldOwnerUID

release requests for podgroup creation 

newPod = get Pod notebook-0
assert newPod.annotations["scheduling.k8s.io/group-name"] == "podgroup-${oldOwnerUID}"
assert podgroup "podgroup-${oldOwnerUID}" is NotFound

The final live Pod was owned by the new StatefulSet, but its scheduling.k8s.io/group-name still pointed to the old podgroup name. This stale patch may happen when podgroup creation is delayed by admission webhooks, apiserver/etcd latency, or controller queue backlog while a same-name StatefulSet Pod is recreated.

This is the stale patch I think the /metadata/uid test can prevent.

YiD11 · 2026-07-05T15:44:47Z

Hi @JesseStutler , I think I have replied your question on Jun 15, about the reproduction details and why the /metadata/uid JSON Patch test is needed.
According to my reproduction, I found this problem still existed in fdb6ae0.
Almost all checks are green now. Could you please take another look when you get a chance?

YiD11 · 2026-09-14T17:48:34Z

Hi @JesseStutler , I've updated the PR based on your comment. I changed the patch strategic, using strategic merge patch that carried metadata.uid. So the API server rejects it if the pod was recreated. Thanks for reviewing the new commit again.

JesseStutler · 2026-09-15T01:34:38Z

Hi @JesseStutler , I've updated the PR based on your comment. I changed the patch strategic, using strategic merge patch that carried metadata.uid. So the API server rejects it if the pod was recreated. Thanks for reviewing the new commit again.

OK thanks! Will take a look

JesseStutler · 2026-09-15T07:26:58Z

+	}
+}
+
+func TestUpdatePodAnnotationsCarriesUID(t *testing.T) {


The UID guard lgtm

But Could we also add coverage through the controller’s normal processing path, rather than only testing the annotation helper directly?

TestUpdatePodAnnotationsCarriesUID calls the helper directly with a reactor that always succeeds, so it does not cover PodGroup creation, patch failure, or requeue behavior.

I’d suggest table-driven tests that enqueue requests through addPod and process them through processNextReq, covering matching UIDs, stale requests, and patch failures. We should also have a regression scenario where the old Pod’s patch is rejected, then the recreated Pod’s request is processed and bound to the PodGroup derived from its new owner UID.

Manually setting up the informer cache and stepping the worker is fine—we don’t need a fully asynchronous controller test. The important part is to exercise the actual processing path and assert the resulting Pod/PodGroup state and retry behavior, rather than only inspecting the patch payload.

Add a UID check to the patch that sets the PodGroup annotation on a pod, so a stale request no longer binds a recreated pod to the old PodGroup and leaves it Pending. Signed-off-by: momo <YiD11@outlook.com>

YiD11 · 2026-09-15T18:14:05Z

Hi @JesseStutler, I've reworked the tests as you suggested. I created a new file pg_controller_recreated_pod_test.go to complete all tests of my change in podgroup controller, and the old test is removed.

All cases go through addPod/processNextReq. Since the fake clientset doesn't validate metadata.uid, I added a patch reactor that rejects a uid mismatch the same way the apiserver does. The table test covers a matching uid, a stale request (cache already updated, and cache still stale), and a patch failure with retry, checking the pod annotation, the podgroup owner and the requeue count. Two more tests replay the original bug: one drives the old and the new pod requests in order and checks which podgroup the new pod ends up in, the other does the same through addStatefulSet.

Hope this is what you had in mind. Thanks for the review.

JesseStutler · 2026-09-16T03:29:37Z

/approve
/lgtm
Thanks

volcano-sh-bot · 2026-09-16T03:29:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JesseStutler

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/controllers/OWNERS~~ [JesseStutler]

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

JesseStutler · 2026-09-16T03:29:52Z

/cherrypick release-1.15

volcano-sh-bot · 2026-09-16T03:29:56Z

@JesseStutler: once the present PR merges, I will cherry-pick it on top of release-1.15 in a new PR and assign it to you.

Details

In response to this:

/cherrypick release-1.15

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

JesseStutler · 2026-09-16T03:30:02Z

/cherrypick release-1.14

volcano-sh-bot · 2026-09-16T03:30:05Z

@JesseStutler: once the present PR merges, I will cherry-pick it on top of release-1.14 in a new PR and assign it to you.

Details

In response to this:

/cherrypick release-1.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

JesseStutler · 2026-09-16T03:30:15Z

/cherrypick release-1.13

volcano-sh-bot · 2026-09-16T03:30:18Z

@JesseStutler: once the present PR merges, I will cherry-pick it on top of release-1.13 in a new PR and assign it to you.

Details

In response to this:

/cherrypick release-1.13

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

volcano-sh-bot · 2026-09-16T03:31:06Z

@JesseStutler: new pull request created: #5980

Details

In response to this:

/cherrypick release-1.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

volcano-sh-bot · 2026-09-16T03:31:44Z

@JesseStutler: new pull request created: #5981

Details

In response to this:

/cherrypick release-1.13

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

volcano-sh-bot · 2026-09-16T03:32:23Z

@JesseStutler: new pull request created: #5982

Details

In response to this:

/cherrypick release-1.15

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copilot AI review requested due to automatic review settings May 27, 2026 12:22

volcano-sh-bot added the kind/bug Categorizes issue or PR as related to a bug. label May 27, 2026

volcano-sh-bot requested review from hwdef and wangyang0616 May 27, 2026 12:22

volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 27, 2026

Copilot AI reviewed May 27, 2026

View reviewed changes

gemini-code-assist Bot reviewed May 27, 2026

View reviewed changes

volcano-sh-bot requested a review from JesseStutler May 28, 2026 02:30

volcano-sh-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label May 28, 2026

YiD11 force-pushed the fix/stale-podgroup-patch-5331 branch 5 times, most recently from fa49fd2 to 8675150 Compare May 30, 2026 07:24

YiD11 force-pushed the fix/stale-podgroup-patch-5331 branch from 8675150 to 84dd0e5 Compare June 12, 2026 07:49

JesseStutler reviewed Jun 15, 2026

View reviewed changes

YiD11 force-pushed the fix/stale-podgroup-patch-5331 branch from 84dd0e5 to 788b3e8 Compare July 5, 2026 14:22

YiD11 force-pushed the fix/stale-podgroup-patch-5331 branch 3 times, most recently from 0daa8c6 to 05f5574 Compare September 14, 2026 17:41

JesseStutler reviewed Sep 15, 2026

View reviewed changes

YiD11 force-pushed the fix/stale-podgroup-patch-5331 branch from 05f5574 to c038059 Compare September 15, 2026 17:57

fix(podgroup): guard annotation patch by pod UID

3198dcf

Add a UID check to the patch that sets the PodGroup annotation on a pod, so a stale request no longer binds a recreated pod to the old PodGroup and leaves it Pending. Signed-off-by: momo <YiD11@outlook.com>

YiD11 force-pushed the fix/stale-podgroup-patch-5331 branch from c038059 to 3198dcf Compare September 15, 2026 18:01

volcano-sh-bot assigned JesseStutler Sep 16, 2026

volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2026

volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 16, 2026

volcano-sh-bot merged commit 243062e into volcano-sh:master Sep 16, 2026
56 of 59 checks passed

volcano-sh-bot mentioned this pull request Sep 16, 2026

[release-1.14] fix: guard podgroup annotation patch by pod UID #5980

Merged

volcano-sh-bot mentioned this pull request Sep 16, 2026

[release-1.13] fix: guard podgroup annotation patch by pod UID #5981

Merged

volcano-sh-bot mentioned this pull request Sep 16, 2026

[release-1.15] fix: guard podgroup annotation patch by pod UID #5982

Merged

JesseStutler mentioned this pull request Sep 21, 2026

fix(scheduler): latch PodGroup Completed phase to prevent quota re-reservation after Pods deleted #5996

Merged

3 tasks done

Conversation

YiD11 commented May 27, 2026

What type of PR is this?

What this PR does / why we need it:

Which issue this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

volcano-sh-bot commented May 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

YiD11 May 28, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

JesseStutler commented May 28, 2026

Uh oh!

JesseStutler Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

YiD11 Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

JesseStutler commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YiD11 commented Jun 15, 2026

Uh oh!

YiD11 commented Jul 5, 2026

Uh oh!

YiD11 commented Sep 14, 2026

Uh oh!

JesseStutler commented Sep 15, 2026

Uh oh!

JesseStutler Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

YiD11 commented Sep 15, 2026

Uh oh!

JesseStutler commented Sep 16, 2026

Uh oh!

volcano-sh-bot commented Sep 16, 2026

Uh oh!

JesseStutler commented Sep 16, 2026

Uh oh!

volcano-sh-bot commented Sep 16, 2026

Uh oh!

JesseStutler commented Sep 16, 2026

Uh oh!

volcano-sh-bot commented Sep 16, 2026

Uh oh!

JesseStutler commented Sep 16, 2026

Uh oh!

volcano-sh-bot commented Sep 16, 2026

Uh oh!

Uh oh!

volcano-sh-bot commented Sep 16, 2026

Uh oh!

volcano-sh-bot commented Sep 16, 2026

Uh oh!

volcano-sh-bot commented Sep 16, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

JesseStutler commented Jun 15, 2026 •

edited

Loading