Fix wrong caculation for deserved in proportion plugin #666
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
k8s-ci-robot
added
the
cncf-cla: no
k8s-ci-robot
added
the
size/XS
zionwu
force-pushed
the
fix_deserved_cal
branch
from
fa74250
to
40c6f18
Compare
k8s-ci-robot
added
cncf-cla: yes
| @@ -134,7 +135,7 @@ func (pp *proportionPlugin) OnSessionOpen(ssn *framework.Session) { | |||
| glog.V(4).Infof("The attributes of queue <%s> in proportion: deserved <%v>, allocate <%v>, request <%v>, share <%0.2f>", | |||
| attr.name, attr.deserved, attr.allocated, attr.request, attr.share) | |||
|
|
|||
| deserved.Add(attr.deserved) | |||
| deserved.Add(queueDeserved) | |||
There was a problem hiding this comment.
If !attr.deserved.LessEqual(attr.request), queueDeserved need be changed to attr.deserved increased value.
There was a problem hiding this comment.
Sorry, I did not get it. Could you explain a bit more?
I think the queueDeserved is already the increased value. In the for loop we add it to attr.deserved and deserved. After the for loop, we calculate the remaining by subtracting deserved
There was a problem hiding this comment.
Suppose that queueDeserved is {cpu: 2, mem: 2G}, attr.deserved is {cpu: 1, mem: 1G} and attr.request is {cpu: 2, mem: 2G}, attr.deserved will be {cpu: 3, mem: 3G} after attr.deserved.Add(queueDeserved)(line 128). Then attr.deserved will be {cpu: 2, mem: 2G} after attr.deserved = helpers.Min(attr.deserved, attr.request)(line 130). So attr.deserved's increased value is actually {cpu: 1, mem: 1G}, and we just need add {cpu: 1, mem: 1G} instead of {cpu: 2, mem: 2G} to deserved.
There was a problem hiding this comment.
got it, thanks. Let me update it.
There was a problem hiding this comment.
Thanks! You code works. Just give some suggestion.
|
@zionwu Thanks for the patch! Could you please help add a test for it? |
zionwu
force-pushed
the
fix_deserved_cal
branch
from
40c6f18
to
ab7e52e
Compare
| @@ -124,17 +124,20 @@ func (pp *proportionPlugin) OnSessionOpen(ssn *framework.Session) { | |||
| continue | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Add: oldDeserved = attr.deserved.Clone()
| } | ||
| pp.updateShare(attr) | ||
|
|
||
| glog.V(4).Infof("The attributes of queue <%s> in proportion: deserved <%v>, allocate <%v>, request <%v>, share <%0.2f>", | ||
| attr.name, attr.deserved, attr.allocated, attr.request, attr.share) | ||
|
|
||
| deserved.Add(attr.deserved) |
There was a problem hiding this comment.
Changed to: deserved.Add(attr.deserved.Clone().sub(oldDeserved))
zionwu
force-pushed
the
fix_deserved_cal
branch
from
ab7e52e
to
06c4428
Compare
|
lgtm except missing tests... /assign @k82cn |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: k82cn, zionwu 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 |
k8s-ci-robot
added
the
approved
…#642-#638-#645-#647-#651-#652-#655-#658-#649-#660-#666-#671-#673-upstream-release-0.4 Automated cherry pick of #643: Return err in Allocate if any error occurs #642: Add event when task is scheduled #638: Take init containers into account when getting pod resource #645: Order task by CreationTimestamp first, then by UID #647: In allocate, skip adding Job if its queue is not found #651: Return err in functions of session.go if any error occurs #652: Change run option SchedulePeriod's type to make it clear #655: Do graceful eviction using default policy #658: Address helm install error in tutorial.md #649: Preempt lowest priority task first #660: Fix sub exception in reclaim and preempt #666: Fix wrong caculation for deserved in proportion plugin #671: Change base image to alphine to reduce image size #673: Do not create PodGroup and Job for task whose scheduler is
Fix wrong caculation for deserved in proportion plugin
Fix wrong caculation for deserved in proportion plugin
Fix wrong caculation for deserved in proportion plugin
Fix issue #665