[release-1.14] Fix isDependsOnPodsReady to treat Succeeded pods as ready - #5547
Conversation
Signed-off-by: Avinash Kumar Deepak <avinash8655279@gmail.com>
volcano-sh-bot
added
the
size/L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the isDependsOnPodsReady method in the job controller to correctly count succeeded pods as running pods, and adds comprehensive unit tests to verify this behavior. The review feedback suggests improving the test setup by using the returned objects from the fake client's Create calls when adding them to the cache and informer indexers, as well as replacing context.TODO() with context.Background().
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| _, err := fakeController.vcClient.BatchV1alpha1().Jobs(namespace).Create(context.TODO(), tc.Job, metav1.CreateOptions{}) | ||
| if err != nil { | ||
| t.Fatalf("Failed to create job: %v", err) | ||
| } | ||
| err = fakeController.cache.Add(tc.Job) | ||
| if err != nil { | ||
| t.Fatalf("Failed to add job to cache: %v", err) | ||
| } | ||
| fakeController.jobInformer.Informer().GetIndexer().Add(tc.Job) |
There was a problem hiding this comment.
It is recommended to use the returned createdJob object from the fake client's Create method when adding it to the cache and informer indexer. This ensures that any system-populated fields (such as UID, ResourceVersion, or default values) are preserved in the local caches, preventing potential discrepancies in controller tests. Additionally, context.Background() is preferred over context.TODO() in test contexts.
| _, err := fakeController.vcClient.BatchV1alpha1().Jobs(namespace).Create(context.TODO(), tc.Job, metav1.CreateOptions{}) | |
| if err != nil { | |
| t.Fatalf("Failed to create job: %v", err) | |
| } | |
| err = fakeController.cache.Add(tc.Job) | |
| if err != nil { | |
| t.Fatalf("Failed to add job to cache: %v", err) | |
| } | |
| fakeController.jobInformer.Informer().GetIndexer().Add(tc.Job) | |
| createdJob, err := fakeController.vcClient.BatchV1alpha1().Jobs(namespace).Create(context.Background(), tc.Job, metav1.CreateOptions{}) | |
| if err != nil { | |
| t.Fatalf("Failed to create job: %v", err) | |
| } | |
| err = fakeController.cache.Add(createdJob) | |
| if err != nil { | |
| t.Fatalf("Failed to add job to cache: %v", err) | |
| } | |
| fakeController.jobInformer.Informer().GetIndexer().Add(createdJob) |
| for _, pod := range tc.Pods { | ||
| _, err := fakeController.kubeClient.CoreV1().Pods(namespace).Create(context.TODO(), pod, metav1.CreateOptions{}) | ||
| if err != nil { | ||
| t.Fatalf("Failed to create pod: %v", err) | ||
| } | ||
| fakeController.podInformer.Informer().GetIndexer().Add(pod) | ||
| } |
There was a problem hiding this comment.
Similarly to the Job creation, it is recommended to use the returned createdPod object from the fake client's Create method when adding it to the informer indexer to ensure system-populated fields are preserved. Also, context.Background() should be used instead of context.TODO().
| for _, pod := range tc.Pods { | |
| _, err := fakeController.kubeClient.CoreV1().Pods(namespace).Create(context.TODO(), pod, metav1.CreateOptions{}) | |
| if err != nil { | |
| t.Fatalf("Failed to create pod: %v", err) | |
| } | |
| fakeController.podInformer.Informer().GetIndexer().Add(pod) | |
| } | |
| for _, pod := range tc.Pods { | |
| createdPod, err := fakeController.kubeClient.CoreV1().Pods(namespace).Create(context.Background(), pod, metav1.CreateOptions{}) | |
| if err != nil { | |
| t.Fatalf("Failed to create pod: %v", err) | |
| } | |
| fakeController.podInformer.Informer().GetIndexer().Add(createdPod) | |
| } |
|
/approve |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
volcano-sh-bot
added
the
approved
3 participants
This is an automated cherry-pick of #5439