Take kube/system reserved cores into account #5
Conversation
|
@ConnorDoyle - what do you think about that ? |
|
Nice work, thanks! Do you think it's better to have a different flag or piggyback on allocatable? This looks fine to me but we'd have to update the config section in the proposal. Could you update this to acquire CPUs from low-numbered physical cores topologically? E.g., 0,3,1,5,... We can get this in now though if you want to do that in a separate PR. |
|
Will update this PR - i like piggybacking on allocatable :) IMHO another cpu manager flag (only valid for one policy) may introduce some confusion |
|
OK, I can update the config section in the proposal (will make a PR this time). I was convinced on Friday that a separate flag is better but on second look I am less sure. A solid argument for piggybacking is that the best-practice would involve setting those flags properly anyway! |
|
@flyingcougar @ConnorDoyle +1 on this |
| @@ -88,6 +94,20 @@ func NewManager(policyName PolicyName, cr internalapi.RuntimeService, kletGetter | |||
| }, nil | |||
| } | |||
|
|
|||
| func getReserverdCores(nodeStatus v1.NodeStatus) int64 { | |||
There was a problem hiding this comment.
Mind adding a test? Know we're already in test-debt but this seems like an easy add.
| } | ||
| s.SetDefaultCPUSet(shared) | ||
| // Figure out which cores shall not be used in shared pool | ||
| reserved, _ := takeByTopology(p.topology, fullCpuset, p.topology.NumReservedCores) |
There was a problem hiding this comment.
Could we leave a note here saying that this depends on the order in which cores are taken in that function (low-numbered cores first?) We have to make this explicit in the contract so operators can reliably configure their systems around the CPU manager.
Glad this could be reused by the way, 👍
| reserved.Sub(nodeStatus.Allocatable[v1.ResourceCPU]) | ||
| glog.Infof("[cpumanager] %v cores reserved for kube/system and shared pool", reserved.Value()) | ||
| if reserved.IsZero() { | ||
| glog.Infof("[cpumanager] defaulting to 1 core") |
There was a problem hiding this comment.
We are going to need to change this at some point as just unilaterally reserving one core messes with the scheduler i.e. on a 4 core node, the schedule sees nothing wrong with scheduling 4 cores of pinned pods to the node; however, the cpumanager can only accommodate 3.
I'm working on new content for the proposal to cover the setting of a CPUPressure=true condition on the node to prevent placement of CPUless pods (Bu pods without CPU requests or BE pods) and eviction of existing CPUless pods. This lets us deplete the shared pool and the scheduler accounting Just Works.
This is just an "keep in mind", not an objection to this change. Thanks!
There was a problem hiding this comment.
That sounds great Seth, there's a little blurb in the proposal about adding taints in the static manager scenarios (shared pool becomes empty, shared pool becomes nonempty) that could use some expansion.
There was a problem hiding this comment.
I'm wondering whether we'd be better off dropping this conditional for now... if the operator really wants zero CPUs to be reserved, then there's nothing really "wrong" with it IMO. I think reserving CPU 0 is more of an artifact of the stopgap we had in place before this PR.
There was a problem hiding this comment.
I'd be ok with dropping it. Avoids us needing to remember to remove it later.
|
@ConnorDoyle - done |
flyingcougar
force-pushed
the
cpu-res
branch
4 times, most recently
from
cbdd56a
to
cb76746
Compare
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Remove redundant call to StartLogging in service_controller. Fixes #5… …4339 **What this PR does / why we need it**: Removes redundant call to StartLogging introduced in 96b48d4#diff-1f7f903e25ab8bcbc514bb1e532e997e **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # kubernetes#54339 **Special notes for your reviewer**: **Release note**: ```release-note ```
Take kube/system reserved cores into account.