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 pod cannot be allocated with sufficient resource #1851

Merged
merged 1 commit into from
Nov 27, 2021
Merged

fix pod cannot be allocated with sufficient resource #1851

merged 1 commit into from
Nov 27, 2021

Conversation

aidaizyy
Copy link
Contributor

background: #1850

@volcano-sh-bot
Copy link
Contributor

Welcome @aidaizyy!

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/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 24, 2021
@aidaizyy aidaizyy mentioned this pull request Nov 24, 2021
Closed
@aidaizyy
Copy link
Contributor Author

/assign @Thor-wl

@aidaizyy aidaizyy force-pushed the aidaizyy-dev branch 2 times, most recently from d2be3e7 to eba4681 Compare November 24, 2021 09:01
Thor-wl
Thor-wl reviewed Nov 24, 2021
View reviewed changes
pkg/scheduler/plugins/proportion/proportion.go Outdated
attr.deserved = helpers.Min(attr.deserved, attr.capability)
}

if attr.capability != nil && attr.capability.LessEqual(attr.deserved, api.Zero) {
Copy link
Member

Choose a reason for hiding this comment

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

What if some dimensions are lack in some queue's capability? For example, capability of q1 = {cpu:10c}, but there must be cpu and memory in deserved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the previous code, helpers.Min also make the deserved memory is 0.

if attr.capability != nil && !attr.deserved.LessEqual(attr.capability, api.Infinity) {
        attr.deserved = helpers.Min(attr.deserved, attr.capability)
……

In the current semantics, this situation means that the memory deserved is 0 always.
I think this pr didn't make it worse.

But if necessary, I can fix this problem in this pr or another pr soon.

Copy link
Member

@Thor-wl Thor-wl Nov 25, 2021 •
edited

Choose a reason for hiding this comment

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

I get you. In fact, deserved cpu and memory will never be zero for they are necessary dimensions. What I want to express is that lack dimension in capability should be infinity.

@volcano-sh-bot volcano-sh-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 25, 2021
@aidaizyy
Copy link
Contributor Author

@Thor-wl pls review agin.
I reorganized the code here.
Not only solved the scheduling bugs I found. I also fixes the previously unresolved problems you mentioned.
Now, no matter whether it is cpu, memory or scalar resources, If they are not set, they are regarded as infinity.

eggiter
eggiter suggested changes Nov 25, 2021
View reviewed changes
pkg/scheduler/api/resource_info.go Outdated
func (r *Resource) MinDimensionResource(rr *Resource) *Resource {
if rr.MilliCPU < r.MilliCPU {
r.MilliCPU = rr.MilliCPU
// @param defaultValue "default value for resource dimension not defined in CPU/Memory/ScalarResources. Its value can only be one of 'Zero' and 'Infinity'"
Copy link
Member

Choose a reason for hiding this comment

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

defaultValue is ALWAYS represents the undefined ScalarResources in the previous codes. Please maintain the consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There has never been a default value for the main resource (cpu/memory) when there is no definition in the project, but this is meaningful for the capability. I am more embarrassed how to express, please review whether my latest revision is reasonable.

Copy link
Member

@eggiter eggiter Nov 26, 2021 •
edited

Choose a reason for hiding this comment

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

The better way to achieve that is to set the default value of capacity.cpu, capacity.memory in the admission web-hook or the AddQueue in the event_handler.
/cc @Thor-wl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion! I set math.maxFloat64 for cpu and memory in OnSessionOpen. This modification should be the best with few changes.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @eggiter. Perhaps we can do that at another PR. Can you help for that? @aidaizyy

pkg/scheduler/api/resource_info.go Outdated Show resolved Hide resolved
pkg/scheduler/plugins/proportion/proportion.go Show resolved Hide resolved
@Thor-wl
Copy link
Member

Thor-wl commented Nov 25, 2021

Pls fix the DCO @aidaizyy

@Thor-wl Thor-wl requested a review from eggiter November 26, 2021 01:38
@aidaizyy
fix pod cannot be allocated with sufficient resource
19a940b 
Signed-off-by: zhangyunyao <aidaizyy@gmail.com>
eggiter
eggiter reviewed Nov 26, 2021
View reviewed changes
pkg/scheduler/api/resource_info.go
if rr.MilliCPU < r.MilliCPU {
r.MilliCPU = rr.MilliCPU
}
if rr.Memory < r.Memory {
r.Memory = rr.Memory
}

if r.ScalarResources == nil {
Copy link
Member

Choose a reason for hiding this comment

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

no needs for the == nil comparation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a kind-hearted redundancy. :)

@eggiter
Copy link
Member

eggiter commented Nov 26, 2021

/lgtm

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2021
@Thor-wl
Copy link
Member

Thor-wl commented Nov 27, 2021

/approve

@volcano-sh-bot
Copy link
Contributor

[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 /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 Nov 27, 2021
@Thor-wl
Copy link
Member

Thor-wl commented Nov 27, 2021

/lgtm

@volcano-sh-bot volcano-sh-bot merged commit c238353 into volcano-sh:master Nov 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eggiter eggiter eggiter requested changes

@Thor-wl Thor-wl Thor-wl left review comments

@shinytang6 shinytang6 Awaiting requested review from shinytang6

@william-wang william-wang Awaiting requested review from william-wang

@hwdef hwdef Awaiting requested review from hwdef

@jasonliu747 jasonliu747 Awaiting requested review from jasonliu747

@xiaoanyunfei xiaoanyunfei Awaiting requested review from xiaoanyunfei

@yahaa yahaa Awaiting requested review from yahaa

@zen-xu zen-xu Awaiting requested review from zen-xu

Assignees

@eggiter eggiter

@Thor-wl Thor-wl

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@aidaizyy @volcano-sh-bot @Thor-wl @eggiter