-
Notifications
You must be signed in to change notification settings - Fork 971
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
change enqueue to optional action #2309
Conversation
Signed-off-by: wpeng102 <wpeng102@126.com>
@@ -13,6 +13,8 @@ import ( | |||
"volcano.sh/volcano/pkg/scheduler/api" | |||
) | |||
|
|||
var EnqueueExist bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kindly to ask about the reason why hard code enqueue action to make the judgement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduce ActionMap to store which action is configured by customer.
Backgroud: |
pkg/scheduler/conf/scheduler_conf.go
Outdated
@@ -16,6 +16,9 @@ limitations under the License. | |||
|
|||
package conf | |||
|
|||
// ActionMap check if a action exist in scheduler configmap | |||
var ActionMap map[string]bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe ExistActionMap
is better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EnabledActionMap is better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK about that.
@@ -98,6 +99,10 @@ func (ju *jobUpdater) updateJob(index int) { | |||
job := ju.jobQueue[index] | |||
ssn := ju.ssn | |||
|
|||
if !conf.EnabledActionMap["enqueue"] && job.PodGroup.Status.Phase == scheduling.PodGroupPending { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add comments here or extract a small func for this special treatment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comments for this block
Any e2e for scheduler configuration without |
Add e2e case |
…configmap and add E2E Signed-off-by: wpeng102 <wpeng102@126.com> Signed-off-by: wpeng102 <wpeng102@126.com> Signed-off-by: wpeng102 <wpeng102@126.com> Signed-off-by: wpeng102 <wpeng102@126.com> Signed-off-by: wpeng102 <wpeng102@126.com> Signed-off-by: wpeng102 <wpeng102@126.com> Signed-off-by: wpeng102 <wpeng102@126.com> Signed-off-by: wpeng102 <wpeng102@126.com> Signed-off-by: wpeng102 <wpeng102@126.com> Signed-off-by: wpeng102 <wpeng102@126.com> Signed-off-by: wpeng102 <wpeng102@126.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Thor-wl 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 |
change enqueue to optional action
Signed-off-by: wpeng102 wpeng102@126.com
The enqueue action is a mandatory action right now. It would be wonderful to make it optional and configurable.
Firstly the integration code with other community will be more simple and easy.
Secondly the other actions no need to depend on enqueue . All the actions are decoupled.
#2040
If not config
Enqueue
action, scheduler will change thePending
pg toInqueue
at closesession.