KEP-1769/KEP-3570/KEP693: Adding Windows Kubelet Manager implementation details #4738
Conversation
Signed-off-by: James Sturtevant <[email protected]>
k8s-ci-robot
added
sig/node
k8s-ci-robot
added
kind/kep
Signed-off-by: James Sturtevant <[email protected]>
|
/cc @marosset @aravindhp @knabben @kiashok /cc @haircommander @mrunalp |
k8s-ci-robot
requested review from
aravindhp,
haircommander,
kiashok,
knabben,
marosset and
mrunalp
Signed-off-by: James Sturtevant <[email protected]>
| ``` | ||
|
|
||
| Since the Kubelet API's are looking for a distinct ProcessorId, the id will be calculated by: | ||
| `(group *64) + procesorid` resulting in unique process id's from `group 0` as `1-64` and |
There was a problem hiding this comment.
Does processor numbering start at 1 in Windows? Or is this just an example?
There was a problem hiding this comment.
it is a bitmap so doesn't really have a number persay. The plan is to translate each processor in the bitmap to a Unique ID that we can then translate back to a bitmap. I started to implement this and my logic might be off a bit. I will update the logic here once I get it working.
There was a problem hiding this comment.
I've updated the logic, I've also started the id's from zero to avoid having to add +1 and -1 everywhere.
There was a problem hiding this comment.
This looks much better now. One suggestion I have is to clarify variable names. You have GROUP_AFFINITY.Mask in one spot and groupaffinity.Mask in another.
| ## Windows considerations | ||
|
|
||
| Topology manager is already enabled on Windows in order to support the device manager. The same configuration options | ||
| and PRR applies to Windows. The CPU manager and Memory Manager can independently be enabled to support advance configuration |
There was a problem hiding this comment.
what is PRR in this case?
There was a problem hiding this comment.
I meant the same Production Readiness Review answers, in the sense that these features can be set to None to disable if required to roll back do to errors. I updated to clarify
k8s-ci-robot
added
size/L
Signed-off-by: James Sturtevant <[email protected]>
jsturtevant
force-pushed
the
kube-managers-windows
branch
from
b5669c3
to
6a12567
Compare
k8s-ci-robot
added
size/M
| In order to support multiple numa nodes and be able to apply the Numa affinity to job objects the container runtime will be expected to mimic | ||
| the behavior of [PROC_THREAD_ATTRIBUTE_PREFERRED_NODE](https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute) | ||
| by finding the associated CPU's for the Numa Nodes that are passed via the Cri API and setting the preferred affinity for the job object. |
There was a problem hiding this comment.
Do we know that setting PROC_THREAD_ATTRIBUTE_PREFERRED_NODE is exactly equivalent to setting the job object's affinity to the CPUs on that node? Or could there be underlying differences?
There was a problem hiding this comment.
I've updated the wording here. We don't know for certain if there are underlying differences, but will use the same general idea to work around the single Numa node limitation of this API.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jsturtevant, marosset The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: James Sturtevant <[email protected]>
k8s-ci-robot
added
size/L
Adding Windows Kubelet Manager implementation details
This is a follow up to CRI: Add field to support CPU affinity on Windows kubernetes#124285 (comment) to add notes on Windows implementation so we can review and discuss changes/differences.
Related PR:
CRI changes: kubernetes/kubernetes#124285
Kubelet changes: kubernetes/kubernetes#125296
/sig node
/sig windows