Skip to content
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.

Sign up for GitHub

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

Jump to bottom

APF borrowing by exempt does not match KEP #124125

Open
MikeSpreitzer opened this issue Apr 1, 2024 · 5 comments · May be fixed by #124737
Open

APF borrowing by exempt does not match KEP #124125

MikeSpreitzer opened this issue Apr 1, 2024 · 5 comments · May be fixed by #124737
Labels
kind/bug Categorizes issue or PR as related to a bug. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@MikeSpreitzer
Copy link
Member

MikeSpreitzer commented Apr 1, 2024 •
edited

What happened?

In reviewing the KEP and the implementation, I noticed a difference in the way that the exempt priority level borrows from the others. In the KEP, in section https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/1040-priority-and-fairness#dispatching (this material was added in kubernetes/enhancements#3906), we see (among many other things) the following definition.

MinCurrentCL(i) = max( MinCL(i), min( NominalCL(i), HighSeatDemand(i) ) ) -- if non-exempt
MinCurrentCL(i) = max( MinCL(i), HighSeatDemand(i) )                      -- if exempt

But in the implementation (

kubernetes/staging/src/k8s.io/apiserver/pkg/util/flowcontrol/apf_controller.go

Line 407 in 3dedb8e

minCurrentCL := math.Max(float64(plState.minCL), math.Min(float64(plState.nominalCL), plState.seatDemandStats.highWatermark))
, introduced in #118782) there is no special treatment for exempt. It gets its MinCurrentCL computed by the same formula as non-exempt levels. In the default configuration (in which the exempt priority level has a nominal concurrency limit of zero), this means that the exempt priority level gets MinCurrentCL=0.

The KEP continues with some considerations for what to do if the exempt priority levels want to use up the whole server concurrency limit, or enough to prevent the non-exempt priority levels from getting their minimum allocations. These are also absent from the implementation --- naturally enough, since those situations cannot arise in the current implementation.

What did you expect to happen?

Consistency

How can we reproduce it (as minimally and precisely as possible)?

This is from code and doc inspection, as described above.

Anything else we need to know?

No response

Kubernetes version

1.28.0 and later.

Cloud provider

N/A

OS version

N/A

Install tools

Container runtime (CRI) and version (if applicable)

Related plugins (CNI, CSI, ...) and versions (if applicable)
@MikeSpreitzer MikeSpreitzer added the kind/bug Categorizes issue or PR as related to a bug. label Apr 1, 2024
@k8s-ci-robot k8s-ci-robot added needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 1, 2024
@MikeSpreitzer
Copy link
Member Author

A short version of the difference is this: the KEP says that the exempt priority level gets everything it wants and the other levels compete over the rest of the server's concurrency limit, while the implementation has the exempt priority level competing with the others on a level playing field.

@MikeSpreitzer
Copy link
Member Author

/cc @tkashem
/cc @wojtek-t
/cc @deads2k

@MikeSpreitzer
Copy link
Member Author

@kubernetes/sig-api-machinery-bugs

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 1, 2024
@wojtek-t
Copy link
Member

wojtek-t commented Apr 3, 2024

I think we should fix that to what is described in the KEP - in the end all requests from exempt are executed anyway, so pretending they don't occupy resources is just cheating ourselves...

I think we should just change the code to reflect what's described in the KEP.

@alexzielenski
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 9, 2024
This was referenced May 8, 2024
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/bug Categorizes issue or PR as related to a bug. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make borrowing by exempt match the KEP MikeSpreitzer/kubernetes
4 participants
@alexzielenski @wojtek-t @MikeSpreitzer @k8s-ci-robot