in capacity plugin attr.deserved no need MinDimensionResource with attr.request #3946
Conversation
Signed-off-by: kingeasternsun <[email protected]>
volcano-sh-bot
added
kind/cleanup
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
volcano-sh-bot
added
the
size/XS
| @@ -654,7 +652,7 @@ func (cp *capacityPlugin) checkHierarchicalQueue(attr *queueAttr) error { | |||
| attr.guarantee = totalGuarantee | |||
| cp.totalGuarantee = totalGuarantee | |||
| attr.realCapability = cp.totalResource | |||
| attr.deserved = cp.totalResource | |||
| attr.deserved = totalDeserved | |||
There was a problem hiding this comment.
Is it necessary to change this to totalDeserved?
There was a problem hiding this comment.
Why is it resolved here? You didn't give your explaination in your PR's description
| @@ -654,7 +652,7 @@ func (cp *capacityPlugin) checkHierarchicalQueue(attr *queueAttr) error { | |||
| attr.guarantee = totalGuarantee | |||
| cp.totalGuarantee = totalGuarantee | |||
| attr.realCapability = cp.totalResource | |||
| attr.deserved = cp.totalResource | |||
| attr.deserved = totalDeserved | |||
There was a problem hiding this comment.
I guess that you will be interested in #3947. We can decouple of the queues' quota from physical resources by adding a new mode and I think we'd better avoid affecting existing features. FYI: #3929 (comment)
There was a problem hiding this comment.
Thanks, but in this PR this fix is needed, or the if attr.guarantee.LessPartly(totalGuarantee, api.Zero) may be fail.
There was a problem hiding this comment.
Thanks, but in this PR this fix is needed, or the
if attr.guarantee.LessPartly(totalGuarantee, api.Zero)may be fail.
That failure is expected according to https://cloud-native.slack.com/archives/C011GJDQS0N/p1735212321607659?thread_ts=1735042276.738339&cid=C011GJDQS0N and #3929 (comment). I think we should not fix it directly.
|
/ok-to-test |
volcano-sh-bot
added
the
ok-to-test
|
/lgtm |
|
We should add ut to cover these changes, such as make sure queue attr is rendered correctly, especially the deserved filed, both root and child queue, and queues reclaim behavior is also not affected. |
volcano-sh-bot
added
the
needs-rebase
|
@kingeasternsun: PR needs rebase. 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. |
What type of PR is this?
/kind cleanup
/area scheduling
What this PR does / why we need it:
In capacity plugin attr.deserved no need MinDimensionResource with attr.request.
In proportion plugin, attr.deserved is computed dynamically using min-max faire share algorithm, so attr.deserved needs
MinDimensionResourcewith attr.request to share the ununsed quota with other queues.But in capacity plugin, attr.deserved is configured by the user, so the
MinDimensionResourceis not need any more.Which issue(s) this PR fixes:
Fixes #3944
Special notes for your reviewer:
NONE
Does this PR introduce a user-facing change?
NONE