Make QueueSet support exempt behavior; use it #118955
Conversation
k8s-ci-robot
added
release-note-none
MikeSpreitzer
force-pushed
the
add-exempt-queuesets
branch
from
92a0807
to
cde9467
Compare
Signed-off-by: Mike Spreitzer <mspreitz@us.ibm.com>
MikeSpreitzer
force-pushed
the
add-exempt-queuesets
branch
from
cde9467
to
f269acd
Compare
|
/test pull-kubernetes-e2e-gce-100-performance |
|
/assign @wojtek-t |
k8s-ci-robot
added
triage/accepted
|
/test pull-kubernetes-e2e-gce-100-performance |
| if currentCL > 0 { | ||
| concurrencyDenominator = currentCL | ||
| } else { | ||
| concurrencyDenominator = int(math.Max(1, math.Round(float64(cfgCtlr.serverConcurrencyLimit)/10))) |
There was a problem hiding this comment.
How did you choose the "concurrencyLimit/10"?
Why e.g. not "concurrencyLimit/5" or "concurrencyLimit/20"?
There was a problem hiding this comment.
It's just a number I pulled out of the air. We have to do something, and I thought this is as good a choice as any. If you want to argue for a better specific choice, go ahead. I do not think it worth the trouble to make this configurable.
| if state.pl.Spec.Limited != nil { | ||
| meal.shareSum += float64(state.pl.Spec.Limited.NominalConcurrencyShares) | ||
| } | ||
| nominalConcurrencyShares, _, _ := plSpecCommons(state.pl) |
There was a problem hiding this comment.
nit: what I was originally asking for is to split the refactorings (e.g. things like introducing plSpecCommons function or changed apf_controller_debug.go) which are easy to review and makes the main PR smaller [and thus easier to review]. But I guess it might be too late now...
There was a problem hiding this comment.
I am not sure I understand. The plSpecCommons function is a small part of this PR. This PR is much smaller than #118782 .
There was a problem hiding this comment.
plSpecCommons + all plumbing of it + apf_controler_debug, etc. are maybe not majority, but non-negligible part
and these are no-brainer and trivial to reason about
given that review is exponential to the size of PR, even if that's 1/3rd of PR, the impact on review time is actually quite significant :)
There was a problem hiding this comment.
That said - as I mentioned, it's probably too late.
I managed to go over it and it looks fine now.
|
I looked at the metrics from the run of pull-kubernetes-e2e-gce-100-performance and they look sensible. Following are some graphs. 
|
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: c8686a31af10ab98000a982db6bd3c99594952b9
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MikeSpreitzer, wojtek-t 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR makes the API Priority and Fairness implementation more regular, by making the QueueSet abstraction support the behavior of being exempt from limitation and making the config-consuming APF controller utilize that new behavior (rather than working around the lack of it).
This contributes to the implementation of a new feature, borrowing by the exempt priority level. In the PR at hand that borrowing is limited to zero seats (i.e., effectively disabled). #118782 adds the configuration of this borrowing.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Currently #118782 contains a duplicate of this PR; once this one merges, I will rebase #118782 and eliminate the duplication.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/cc @wojtek-t
@tkashem
@deads2k