-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
KEP-1769/KEP-3570/KEP693: Adding Windows Kubelet Manager implementation details #4738
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: James Sturtevant <jsturtevant@gmail.com>
Signed-off-by: James Sturtevant <jsturtevant@gmail.com>
/cc @marosset @aravindhp @knabben @kiashok /cc @haircommander @mrunalp |
Signed-off-by: James Sturtevant <jsturtevant@gmail.com>
``` | ||
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does processor numbering start at 1 in Windows? Or is this just an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is PRR in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Signed-off-by: James Sturtevant <jsturtevant@gmail.com>
b5669c3
to
6a12567
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <jsturtevant@gmail.com>
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