Support minSuccess #1384
Support minSuccess #1384
Conversation
volcano-sh-bot
added
the
size/L
|
/assign @william-wang @wpeng102 @shinytang6 |
|
@Thor-wl: GitHub didn't allow me to assign the following users: wpeng102. Note that only volcano-sh members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
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. |
| @@ -50,6 +50,10 @@ spec: | |||
| to the summary of tasks' replicas | |||
| format: int32 | |||
| type: integer | |||
| minSuccess: | |||
There was a problem hiding this comment.
It's better to set the replicas as the default value.
|
|
||
| // The minimal success pods to run for this Job | ||
| // +optional | ||
| MinSuccess *int32 `json:"minSuccess,omitempty" protobuf:"varint,11,opt,name=minSuccess"` |
There was a problem hiding this comment.
Why set this field as *int , not int?
| @@ -56,6 +56,13 @@ func (ps *runningState) Execute(action v1alpha1.Action) error { | |||
| // when scale down to zero, keep the current job phase | |||
| return false | |||
| } | |||
|
|
|||
| minSuccess := ps.job.Job.Spec.MinSuccess | |||
There was a problem hiding this comment.
Got the reason why you set it as *int instead of int. I perfer to int and set default value to relicas. That may be better. How do you think about it?
There was a problem hiding this comment.
I still think this field should be *int.
MinSuccess is an optional feature, If the user has not set this field, the logic should same as the original.
There was a problem hiding this comment.
I still think this field should be *int.
MinSuccessis an optional feature, If the user has not set this field, the logic should same as the original.
Yes, it's reasonable.
| status.State.Phase = vcbatch.Completed | ||
| return true | ||
| } | ||
|
|
||
| if status.Succeeded+status.Failed == jobReplicas { | ||
| if status.Succeeded >= ps.job.Job.Spec.MinAvailable { | ||
| status.State.Phase = vcbatch.Completed |
There was a problem hiding this comment.
If we use minSuccess, do we still need MinAvailable to determine job status?
There was a problem hiding this comment.
We can use MinAvailable to determine job status when users don't want to use MinSuccess
There was a problem hiding this comment.
yes, l agree. So maybe we can add another layer of logic inside if status.Succeeded+status.Failed == jobReplicas ? If minSuccess is specified, we use minSuccess instead of minAvailable.
There was a problem hiding this comment.
If minSuccess has matched，Job will be mark Completed. So it's not necessary to handle the next compare logic.
There was a problem hiding this comment.
l mean if minSuccess is specified while status.Succeeded < *minSuccess all the time, then after all the pods completed, it will enter if status.Succeeded+status.Failed == jobReplicas logic, we should replace if status.Succeeded >= ps.job.Job.Spec.MinAvailable with if status.Succeeded >= minSuccess in that case. WDYT?
Signed-off-by: ZhengYu, Xu <zen-xu@outlook.com>
Signed-off-by: ZhengYu, Xu <zen-xu@outlook.com>
|
related issue: #1379 |
Signed-off-by: ZhengYu, Xu <zen-xu@outlook.com>
|
/lgtm |
|
@Thor-wl needs approved label |
|
/approve |
volcano-sh-bot
added
the
approved
Signed-off-by: ZhengYu, Xu <zen-xu@outlook.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shinytang6, Thor-wl, zen-xu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
|
There seems to be lack of doc design about this feature? @zen-xu |
|
A little busy these weeks, I will complete it later. |
/assign @zen-xu |
ref: #1379