[release-1.14] Fix unbounded job.Status.Conditions growth - #5448
Conversation
This adds a maxJobConditions cap (32) and deduplicates consecutive identical condition phases, preventing etcd object-size exceeded errors when frequently retrying jobs. Signed-off-by: Avinash Kumar Deepak <avinash8655279@gmail.com>
Allocates a new slice and copies elements instead of slicing the existing array, ensuring old pointers are safely garbage collected. Signed-off-by: Avinash Kumar Deepak <avinash8655279@gmail.com>
Signed-off-by: Avinash Kumar Deepak <avinash8655279@gmail.com>
Signed-off-by: Avinash Kumar Deepak <avinash8655279@gmail.com>
volcano-sh-bot
added
the
size/M
![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 introduces a helper function appendJobCondition to deduplicate and append job status conditions, replacing direct slice appends across the controller, and adds corresponding unit tests. The review feedback suggests optimizing the slice pre-allocation capacity in the helper function to avoid reallocation, and strengthening the unit tests by asserting the exact slice structure using reflect.DeepEqual instead of only checking the length and the last element.
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.
| // appendJobCondition appends c to conditions. If a condition with the same | ||
| // phase already exists, it is removed and the new condition is appended at the end. | ||
| func appendJobCondition(conditions []batch.JobCondition, c batch.JobCondition) []batch.JobCondition { | ||
| res := make([]batch.JobCondition, 0, len(conditions)) |
There was a problem hiding this comment.
Pre-allocating the slice with a capacity of len(conditions) + 1 avoids a potential slice reallocation and memory copy when appending the new condition c (especially when c.Status is a new phase not present in the existing conditions).
| res := make([]batch.JobCondition, 0, len(conditions)) | |
| res := make([]batch.JobCondition, 0, len(conditions)+1) |
| func TestAppendJobCondition(t *testing.T) { | ||
| testcases := []struct { | ||
| Name string | ||
| Conditions []v1alpha1.JobCondition | ||
| NewCondition v1alpha1.JobCondition | ||
| ExpectedLen int | ||
| ExpectedPhase v1alpha1.JobPhase | ||
| }{ | ||
| { | ||
| Name: "Append to empty conditions", | ||
| Conditions: []v1alpha1.JobCondition{}, | ||
| NewCondition: v1alpha1.JobCondition{ | ||
| Status: v1alpha1.Pending, | ||
| }, | ||
| ExpectedLen: 1, | ||
| ExpectedPhase: v1alpha1.Pending, | ||
| }, | ||
| { | ||
| Name: "Replace existing condition with same phase", | ||
| Conditions: []v1alpha1.JobCondition{ | ||
| {Status: v1alpha1.Pending}, | ||
| {Status: v1alpha1.Running}, | ||
| }, | ||
| NewCondition: v1alpha1.JobCondition{ | ||
| Status: v1alpha1.Pending, | ||
| }, | ||
| ExpectedLen: 2, | ||
| ExpectedPhase: v1alpha1.Pending, | ||
| }, | ||
| { | ||
| Name: "Append if phase is different", | ||
| Conditions: []v1alpha1.JobCondition{ | ||
| {Status: v1alpha1.Pending}, | ||
| {Status: v1alpha1.Running}, | ||
| }, | ||
| NewCondition: v1alpha1.JobCondition{ | ||
| Status: v1alpha1.Completed, | ||
| }, | ||
| ExpectedLen: 3, | ||
| ExpectedPhase: v1alpha1.Completed, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testcases { | ||
| t.Run(tc.Name, func(t *testing.T) { | ||
| result := appendJobCondition(tc.Conditions, tc.NewCondition) | ||
|
|
||
| if len(result) != tc.ExpectedLen { | ||
| t.Errorf("Expected length %d, got %d", tc.ExpectedLen, len(result)) | ||
| } | ||
|
|
||
| if result[len(result)-1].Status != tc.ExpectedPhase { | ||
| t.Errorf("Expected last phase %v, got %v", tc.ExpectedPhase, result[len(result)-1].Status) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The current assertions only verify the length of the resulting slice and the status of the last element. This is a weak assertion that could pass even if the deduplication logic is incorrect (e.g., if other elements are lost or the order is wrong). Comparing the exact expected slice using reflect.DeepEqual makes the test much more robust and precise.
func TestAppendJobCondition(t *testing.T) {
testcases := []struct {
Name string
Conditions []v1alpha1.JobCondition
NewCondition v1alpha1.JobCondition
ExpectedConditions []v1alpha1.JobCondition
}{
{
Name: "Append to empty conditions",
Conditions: []v1alpha1.JobCondition{},
NewCondition: v1alpha1.JobCondition{
Status: v1alpha1.Pending,
},
ExpectedConditions: []v1alpha1.JobCondition{
{Status: v1alpha1.Pending},
},
},
{
Name: "Replace existing condition with same phase",
Conditions: []v1alpha1.JobCondition{
{Status: v1alpha1.Pending},
{Status: v1alpha1.Running},
},
NewCondition: v1alpha1.JobCondition{
Status: v1alpha1.Pending,
},
ExpectedConditions: []v1alpha1.JobCondition{
{Status: v1alpha1.Running},
{Status: v1alpha1.Pending},
},
},
{
Name: "Append if phase is different",
Conditions: []v1alpha1.JobCondition{
{Status: v1alpha1.Pending},
{Status: v1alpha1.Running},
},
NewCondition: v1alpha1.JobCondition{
Status: v1alpha1.Completed,
},
ExpectedConditions: []v1alpha1.JobCondition{
{Status: v1alpha1.Pending},
{Status: v1alpha1.Running},
{Status: v1alpha1.Completed},
},
},
}
for _, tc := range testcases {
t.Run(tc.Name, func(t *testing.T) {
result := appendJobCondition(tc.Conditions, tc.NewCondition)
if !reflect.DeepEqual(result, tc.ExpectedConditions) {
t.Errorf("Expected conditions %v, got %v", tc.ExpectedConditions, result)
}
})
}
}
|
/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
039ce76
into
volcano-sh:release-1.14
3 participants
This is an automated cherry-pick of #5422