Skip to content
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

Allow preemptable tasks schedule without checking proportion #2967

Closed
wants to merge 1 commit into from

Conversation

Cdayz
Copy link

@Cdayz Cdayz commented Jul 13, 2023

Resolve #2966

@volcano-sh-bot
Copy link
Contributor

Welcome @Cdayz!

It looks like this is your first PR to volcano-sh/volcano 馃帀.

Thank you, and welcome to Volcano. 😃

@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign k82cn
You can assign the PR to them by writing /assign @k82cn in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@volcano-sh-bot volcano-sh-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 13, 2023
@Cdayz
Copy link
Author

Cdayz commented Jul 13, 2023

/assign @k82cn

@Cdayz
Copy link
Author

Cdayz commented Jul 31, 2023

/assign @Thor-wl

@Thor-wl Thor-wl requested review from wangyang0616 and Thor-wl and removed request for alcorj-mizar August 1, 2023 08:27
@@ -27,6 +27,12 @@ import (
// checkNodeResourceIsProportional checks if a gpu:cpu:memory is Proportional
func checkNodeResourceIsProportional(task *api.TaskInfo, node *api.NodeInfo, proportional map[v1.ResourceName]baseResource) (*api.Status, error) {
status := &api.Status{}

if task.Preemptable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task here is the role of the preemptor, and task.Preemptable indicates whether the current task is allowed to be preempted.
Sorry, I don't quite understand the purpose of the judgment here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main idea of this PR, that preemptable tasks can skip this proportional check, because they can be preempted, so there are no problem to schedule them. In system which I maintain in my organisation should be available ability to schedule preemptible tasks bypassing proportional plugin.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your response, I see what you mean.

However, the proportional logic is designed to ensure that resources are available in the allocate scene, and is not bound to preempt. In reality, many users will not configure preempt actions.

I am worried that the modification here will affect the existing proportional function. So I have reservations about the current pr, and I can refer to the ideas of other contributors in the community.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can add an optional parameter to proportional plugin configuration, which by default will save current behavior but allow to enable changes made by this pr?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds more reasonable to add a config. And what's more, job's queue should support Reclaimable so that the job in this queue can be reclaimed.

@lowang-bh
Copy link
Member

lowang-bh commented Aug 2, 2023

need to add more check about job/pod/queue's preemtable/reclaimable.

/hold

@stale
Copy link

stale bot commented Oct 15, 2023

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 15, 2023
@Cdayz
Copy link
Author

Cdayz commented Oct 29, 2023

I think we can close this PR in prior to #3158 where implemented more wide check.

@stale stale bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 29, 2023
@Cdayz Cdayz closed this Oct 29, 2023
@Cdayz Cdayz deleted the issue-2966 branch October 29, 2023 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow schedule preemptable tasks on nodes even if proportional predicate failed
6 participants