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.

Sign up for GitHub

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

Jump to bottom

Fix Queue capability overuse when specify minAvailable less than task replicas #1042

Merged
merged 1 commit into from
Sep 16, 2020
Merged

Fix Queue capability overuse when specify minAvailable less than task replicas #1042

merged 1 commit into from
Sep 16, 2020

Conversation

zen-xu
Copy link
Contributor

@zen-xu zen-xu commented Sep 11, 2020

Ref issue #834.
I have checked my pull request can fix this bug.

@volcano-sh-bot
Copy link
Contributor

Welcome @zen-xu!

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

Thank you, and welcome to Volcano. 😃

@volcano-sh-bot volcano-sh-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 11, 2020
@zen-xu
Copy link
Contributor Author

zen-xu commented Sep 11, 2020

/assign @k82cn

@hzxuzhonghu
Copy link
Collaborator

@zen-xu Isn't this fixed by #959

@zen-xu
Copy link
Contributor Author

zen-xu commented Sep 12, 2020

@zen-xu Isn't this fixed by #959
Yes

@hzxuzhonghu
Copy link
Collaborator

So i am confused what's this for

@zen-xu
Copy link
Contributor Author

zen-xu commented Sep 14, 2020

So i am confused what's this for

asciicast

The test queue spec is 2CPU, 1Gi Memory.
Job spec is 1CPU, 500Mi Memory with 5replicas.
When I submit the Job, all tasks will be scheduled!!! So Job is using 5CPU, 2500Mi Memory in test queue!

@zen-xu
Copy link
Contributor Author

zen-xu commented Sep 14, 2020

So i am confused what's this for

asciicast

The test queue spec is 2CPU, 1Gi Memory.
Job spec is 1CPU, 500Mi Memory with 5replicas.
When I submit the Job, all tasks will be scheduled!!! So Job is using 5CPU, 2500Mi Memory in test queue!

I hope 2 tasks will be running and others will be pending

@hzxuzhonghu
Copy link
Collaborator

That's right, i remember we have fixed this issue. So that's why i am confused. Let me have a look

@hzxuzhonghu
Copy link
Collaborator

@zen-xu I suspect you do not enable enqueue action right?

This pr looks good, bu expect file a fix into master branch first, and cherry-pick back to release-1.0 and release-0.4 after that.

@zen-xu
Copy link
Contributor Author

zen-xu commented Sep 14, 2020

@zen-xu I suspect you do not enable enqueue action right?

This pr looks good, bu expect file a fix into master branch first, and cherry-pick back to release-1.0 and release-0.4 after that.

enqueue is enabled

@hzxuzhonghu
Copy link
Collaborator

IC, the minavailable is 1

@zen-xu
Copy link
Contributor Author

zen-xu commented Sep 15, 2020

IC, the minavailable is 1

Yes，I forget to mention it🤦🏻‍♂️

@zen-xu zen-xu changed the title Fix Queue capability overuse when specify Job replicas Fix Queue capability overuse when specify Job replicas when minAvailable is 1 Sep 15, 2020
@zen-xu zen-xu changed the title Fix Queue capability overuse when specify Job replicas when minAvailable is 1 Fix Queue capability overuse when specify Job replicas, minAvailable is 1 Sep 15, 2020
@zen-xu zen-xu changed the title Fix Queue capability overuse when specify Job replicas, minAvailable is 1 Fix Queue capability overuse when specify minAvailable less than task replicas Sep 15, 2020
@zen-xu zen-xu changed the base branch from release-0.4 to master September 15, 2020 01:51
@volcano-sh-bot volcano-sh-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 15, 2020
@zen-xu
Fix Queue capability overuse when specify Job minAvailable less than …
f8bb04c 
…replicas

Signed-off-by: Xu ZhengYu <zen-xu@outlook.com>
@volcano-sh-bot volcano-sh-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 15, 2020
hzxuzhonghu
hzxuzhonghu reviewed Sep 15, 2020
View reviewed changes
pkg/scheduler/plugins/proportion/proportion.go
attr.deserved = helpers.Min(attr.deserved, attr.request)
meet[attr.queueID] = struct{}{}
klog.V(4).Infof("queue <%s> is meet cause of the capability", attr.name)
} else if attr.request.Less(attr.deserved) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

lgtm

maybe we can clean it a little bit more, remove the less check

if attr.capability != nil{
    attr.deserved = helpers.Min(attr.deserved, attr.capability)
}

attr.deserved = helpers.Min(attr.deserved, attr.request)

@hzxuzhonghu
Copy link
Collaborator

/lgtm

/approve

@hzxuzhonghu
Copy link
Collaborator

/lgtm
/approve

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2020
@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@volcano-sh-bot volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 16, 2020
@volcano-sh-bot volcano-sh-bot merged commit 200431f into volcano-sh:master Sep 16, 2020
@zen-xu zen-xu deleted the queue-capability branch April 6, 2021 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hzxuzhonghu hzxuzhonghu hzxuzhonghu left review comments

@k82cn k82cn Awaiting requested review from k82cn

@kevin-wangzefeng kevin-wangzefeng Awaiting requested review from kevin-wangzefeng

Assignees

@hzxuzhonghu hzxuzhonghu

@k82cn k82cn

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zen-xu @volcano-sh-bot @hzxuzhonghu @k82cn