-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
KEP-1040: Weighted borrowing #4733
base: master
Are you sure you want to change the base?
Conversation
MikeSpreitzer
commented
Jun 15, 2024
•
edited
Loading
edited
- One-line PR description: Add weighting to the borrowing between APF priority levels
- Issue link:
ab79ea4
to
5294bad
Compare
/cc @deads2k |
the next version, the common fields of | ||
`LimitedPriorityLevelConfiguration` and | ||
`ExemptPriorityLevelConfiguration` will move to their common ancestor | ||
`PriorityLevelConfigurationSpec`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cutting this out because this plan did not come to fruition.
CurrentCL(i) = MinCL(i) + ( MinCurrentCL(i) - MinCL(i) ) * Weight(i) * | ||
(RemainingServerCL-MinCLSum) / WeightedDiffSum | ||
WeightedDiffSum = sum[non-exempt priority level i] | ||
( MinCurrentCL(i) - MinCL(i) ) * Weight(i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might not have fully understood those formulae, but questions:
- why can't we just apply the previous logic -- scaling down the CurrentCL proportional to
RemainingServerCL / LowerBoundSum
? why do the weights have to impact this if no borrowing is happening here. - A more direct question: I tested APF a while ago and learned a rule of "the owner keeps the shares if inuse", now would the weights break this rule? i.e. PLs with lower weights will be borrowed even when their demands are higher than their NominalCL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, remember that the PR that you are looking at is actually combining two changes: one is #4623 , which aims to heal the long-standing divergence between code and KEP by moving both to a middle ground, and then there is the change to add weighting to the priority levels.
It is true that when MinCLSum < RemainingServerCL < MinCurrentCLSum
there is no borrowing between non-exempt priority levels, but the exempt level(s) have borrowed so much from the non-exempt ones that each must give up some concurrency. So it is actually a borrowing scenario.
Remember that you have not tested the behavior described in #4623 unless you tested its implementation in kubernetes/kubernetes#124736 .
Regarding "the owner keeps the shares if in use", I would be a little more precise: the rule is that each non-exempt level gets its nominal concurrency limit if at least that much has been in use AND the borrowing by exempt levels is not so great as to cut into that. The formulae that you have commented on concern the case where the exempt level(s) have borrowed so much that the non-exempt ones can not get their nominal concurrency limits.
/easycla |
/easycla |
Signed-off-by: Mike Spreitzer <[email protected]>
5294bad
to
869c313
Compare
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
|
||
| Name | Weight | | ||
| ---- | -----: | | ||
| leader-election | 100 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel LE should be higher than node-high & system as it can cause severe disruptions than the other two but not sure what practical difference it'd make with different weight values.
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closed this PR. In response to this:
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-sigs/prow repository. |
/remove-lifecycle rotten |
@MikeSpreitzer: Reopened this PR. In response to this:
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-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MikeSpreitzer 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 |