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
Add borrowing between priority levels in APF #113485
Conversation
fb0c039
to
8ea2fd2
Compare
8bf7873
to
4ba5cbd
Compare
f13d70b
to
5d0aea9
Compare
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
I'm gonna tag this to make sure it meets the code freeze criteria /lgtm @MikeSpreitzer if you could rebase that would be great, I can live with it if you fix the nits in a followup. |
a36f07e
to
722c858
Compare
/lgtm Can you update the release note text per #113485 (comment)? |
Also make some design changes exposed in testing and review. Do not remove the ambiguous old metric `apiserver_flowcontrol_request_concurrency_limit` because reviewers though it is too early. This creates a problem, that metric can not keep both of its old meanings. I chose the configured concurrency limit. Testing has revealed a design flaw, which concerns the initialization of the seat demand state tracking. The current design in the KEP is as follows. > Adjustment is also done on configuration change … For a newly > introduced priority level, we set HighSeatDemand, AvgSeatDemand, and > SmoothSeatDemand to NominalCL-LendableSD/2 and StDevSeatDemand to > zero. But this does not work out well at server startup. As part of its construction, the APF controller does a configuration change with zero objects read, to initialize its request-handling state. As always, the two mandatory priority levels are implicitly added whenever they are not read. So this initial reconfig has one non-exempt priority level, the mandatory one called catch-all --- and it gets its SmoothSeatDemand initialized to the whole server concurrency limit. From there it decays slowly, as per the regular design. So for a fairly long time, it appears to have a high demand and competes strongly with the other priority levels. Its Target is higher than all the others, once they start to show up. It properly gets a low NominalCL once other levels show up, which actually makes it compete harder for borrowing: it has an exceptionally high Target and a rather low NominalCL. I have considered the following fix. The idea is that the designed initialization is not appropriate before all the default objects are read. So the fix is to have a mode bit in the controller. In the initial state, those seat demand tracking variables are set to zero. Once the config-producing controller detects that all the default objects are pre-existing, it flips the mode bit. In the later mode, the seat demand tracking variables are initialized as originally designed. However, that still gives preferential treatment to the default PriorityLevelConfiguration objects, over any that may be added later. So I have made a universal and simpler fix: always initialize those seat demand tracking variables to zero. Even if a lot of load shows up quickly, remember that adjustments are frequent (every 10 sec) and the very next one will fully respond to that load. Also: revise logging logic, to log at numerically lower V level when there is a change. Also: bug fix in float64close. Also, separate imports in some file Co-authored-by: Han Kang <hankang@google.com>
d50719b
to
feb4227
Compare
The force-push to feb4227 renames the wait group to make its purpose clearer and switches from a formula to an array for the number of clients per flow. |
I updated the release note to list the new metrics and explain what happened to |
I'm retagging after just nits were addressed based on the fact that it was tagged before code-freeze in: FWIW - it LGTM too. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lavalamp, 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 implement the design change in kubernetes/enhancements#3391 and kubernetes/enhancements#3479 . That is, this PR adds borrowing of concurrency between priority levels in the API Priority and Fairness feature.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
The API change part of this was already reviewed in #112830 .
The first six commits of this PR are the commits of that earlier PR, rebased onto a later revision of the
master
branch.The "apiserver: update borrowing parameters for apf bootstrap objects" commit is a cherry-pick of #113016 .
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: