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]>
| ## 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.
Signed-off-by: James Sturtevant <[email protected]>
k8s-ci-robot
added
size/L
Signed-off-by: James Sturtevant <[email protected]>
jsturtevant
force-pushed
the
kube-managers-windows
branch
from
0a60293
to
62d1f24
Compare
|
lgtm |
k8s-ci-robot
added
the
lgtm
|
@kevpar @haircommander We've finalized this from a sig-windows perspective. I've also been able to get the proto-type working in kubernetes/kubernetes#125296 with the proposed changes to containerd @kiashok provided. Could you take another look? |
| - cpu manager selects fewer CPU's than Numa nodes and CPU's fall with in Numa node: Kubelet will only set only the CPU's selected by the cpu-manager as the memory from the memory manager will be used by default. | ||
| - cpu manager selects more CPU's than Numa nodes and CPU's fall within/or outside Numa node: kubelet will set selected only CPU's from cpu-manager | ||
| - cpu manager selects few CPU's than Numa nodes and CPU's fall outside the Numa Node: Kubelet would set the CPU's by cpu-manager plus all the CPU's associated with the Numa node. |
There was a problem hiding this comment.
I assume by "cpu manager selects fewer CPU's than Numa nodes" what we really mean is "cpu manager selects fewer CPU's than the CPU's associated with the Numa nodes selected by the memory manager"?
There was a problem hiding this comment.
Yes, I can update the wording to be clearer
|
/cc |
|
I would expect this to:
|
This was discussed with sig-node awhile back and was agreed that since this can be turned off with kubernetes/kubernetes#124285 (comment)
I can update the test section with some details as appropriate |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jsturtevant, kevpar, 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 |
|
After discussions with sig-node last week, we will break this out into a separate KEP with its own feature flag, this way we can communicate to users that the features are not Stable for windows as well as make it possible for users to opt into the feature. |
|
superseded by #4888 |
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