kubernetes / kubernetes

Production-Grade Container Scheduling and Management
https://kubernetes.io
Apache License 2.0
109.89k stars 39.34k forks source link

APF borrowing by exempt does not match KEP #124125

Open MikeSpreitzer opened 5 months ago

MikeSpreitzer commented 5 months ago

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 https://github.com/kubernetes/enhancements/pull/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 (https://github.com/kubernetes/kubernetes/blob/3dedb8eb8c122d0a3221a5842c1d6697d8958151/staging/src/k8s.io/apiserver/pkg/util/flowcontrol/apf_controller.go#L407, 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 commented 5 months ago

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 commented 5 months ago

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

MikeSpreitzer commented 5 months ago

@kubernetes/sig-api-machinery-bugs

wojtek-t commented 5 months ago

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 commented 5 months ago

/triage accepted