[release-1.13] fix: replicaset KubeGroupNameAnnotation handling #4826
Conversation
Replicasets have created a podgroup that wasn't used by the pods when the "scheduling.k8s.io/group-name" annotation was on the pods. Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
Minor error logging enhancements in the podGroup test cases additionally adding two test cases that will cover these problems when the annotation is present but the podgroup is created. Signed-off-by: Hajnal Máté <hajnalmt@gmail.com>
Summary of ChangesHello @volcano-sh-bot, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a potential issue where the PodGroup controller might redundantly create new PodGroups for pods that are already correctly annotated with an existing PodGroup. It introduces a safeguard to prevent this behavior, ensuring that pods with the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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 cherry-picks a fix for handling KubeGroupNameAnnotation for ReplicaSets. The change prevents the creation of a new PodGroup if a pod managed by a ReplicaSet already belongs to one. This is the correct behavior and aligns it with how StatefulSets are handled. The other changes, such as using a for range loop and improving test error logging, are also good improvements. The new test case for the fix is comprehensive. I've left one comment regarding code duplication in the new test, which could be addressed to improve maintainability.
| pgList, err := c.vcClient.SchedulingV1beta1().PodGroups(namespace).List(context.TODO(), metav1.ListOptions{}) | ||
| assert.NoError(t, err) | ||
| names := make([]string, 0, len(pgList.Items)) | ||
| for _, pg := range pgList.Items { | ||
| names = append(names, pg.Name) | ||
| } | ||
| assert.Equal(t, 1, len(pgList.Items), "Expected 1 PodGroup, found %d: %v", len(pgList.Items), names) |
There was a problem hiding this comment.
This block of code for asserting the PodGroup count is duplicated in the StatefulSet test case below (lines 1409-1415). To improve maintainability and reduce duplication, consider extracting this logic into a helper function. This function could take the testing object t, the controller, namespace, and expected count as arguments, and provide a detailed failure message if the assertion fails.
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hajnalmt, hzxuzhonghu 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
volcano-sh-bot
merged commit 1368d23
into
volcano-sh:release-1.13
3 participants
This is an automated cherry-pick of #4792