KEP-4885: Windows CPU and Memory Affinity #4888

jsturtevant · 2024-09-30T21:17:04Z

One-line PR description: Add CPU and memory Affinity support to Windows nodes with CPUManager, MemoryManager and Topology manager

Issue link: Windows CPU and Memory Affinity #4885

Other comments:

Signed-off-by: James Sturtevant <[email protected]>

keps/sig-windows/4885-windows-cpu-and-memory-affinity/kep.yaml

keps/sig-windows/4885-windows-cpu-and-memory-affinity/README.md

kiashok · 2024-10-01T00:03:13Z

lgtm

Thanks for opening this @jsturtevant

Signed-off-by: James Sturtevant <[email protected]>

marosset · 2024-10-01T16:58:48Z

/milestone v1.32

Signed-off-by: James Sturtevant <[email protected]>

aravindhp

LGTM. Thanks for working on this, @jsturtevant

keps/sig-windows/4885-windows-cpu-and-memory-affinity/README.md

k8s-ci-robot · 2024-10-01T23:44:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jsturtevant, kevpar
Once this PR has been reviewed and has the lgtm label, please assign jpbetz for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

keps/prod-readiness/OWNERS
~~keps/sig-windows/OWNERS~~ [jsturtevant]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ffromani

No objections for cpumanager and topologymanager, we can use some tunings on the KEP but looks straightforward and uncontroversial. I have some concerns about memory manager, let's iterate over there

keps/sig-windows/4885-windows-cpu-and-memory-affinity/README.md

ffromani · 2024-10-02T11:43:07Z

keps/sig-windows/4885-windows-cpu-and-memory-affinity/README.md

+For sig-windows, we also see a risk to enabling a feature that has already Stable or fully featured on Linux. To mitigate this risk we have opted to create a 
+separate KEP with a feature flag so we can communicate our status effectively. 
+
+Another risk is the testing implementation for these features is mostly in e2e_node which doesn't currently support Windows. As a mitigation there was [some exploration ](https://github.com/jsturtevant/kubernetes/tree/e2e_node-windows) to see if these tests could be enabled on Windows so we can progress this feature with confidence in the testing suite.


e2e tests are "just" suggested for alpha, while they are beta blockers. I have no objections about starting this work in alpha with unit tests (which all need to run and pass on windows, though, but we should be here already) and work in parallel to enable e2e testing on windows.

ffromani · 2024-10-02T11:45:46Z

keps/sig-windows/4885-windows-cpu-and-memory-affinity/README.md

+On Windows systems with more than 64 cores the CPU's will be split into groups, 
+each processor is identified by its group number and its group-relative processor number. 


is this grouping somehow related to HW topology or is completely independent?

independent, though NUMA nodes will not span across groups.

ffromani · 2024-10-02T11:56:07Z

keps/sig-windows/4885-windows-cpu-and-memory-affinity/README.md

+NOTE: Also set `disable-supported` to `true` or `false` in `kep.yaml`.
+-->
+
+Yes. Restarting of the pods will be required to remove the CPU/Memory affinity.


OTOH running workload are not expected to be affected by both kubelet restart and downgrade once the feature was enabled and then disabled (right?)

a kubelet restart shouldn't affect running workloads.

keps/sig-windows/4885-windows-cpu-and-memory-affinity/README.md

ffromani · 2024-10-02T11:59:53Z

keps/sig-windows/4885-windows-cpu-and-memory-affinity/README.md

+[supported limits]: https://git.k8s.io/community//sig-scalability/configs-and-limits/thresholds.md
+-->
+
+We will monitor for cpu consumption to query the CPU topology. If required we may wish to implement a caching strategy.


on linux the HW topology is read only once at kublet start, but there are plans to enable dynamic node resizing. Let's keep that into account when designing the windows solution

During testing I was seeing repeat calls to the cadvisor.getMachineInfo() method, which is why I mention this here.

fair enough, and worth a deeper dive

Signed-off-by: James Sturtevant <[email protected]>

kannon92 · 2024-10-03T16:25:17Z

keps/sig-windows/4885-windows-cpu-and-memory-affinity/README.md

+
+## Infrastructure Needed (Optional)
+
+<!--


What kind of nodes would you need to test this features for windows?

How are you envisioning a e2e test for these features given that windows does not have e2e_node?

I see sig-windows re-using the existing testing infra and adding some basic tests in https://github.com/kubernetes/kubernetes/tree/master/test/e2e/windows for alpha to ensure the features work.

kannon92 · 2024-10-03T16:27:07Z

keps/sig-windows/4885-windows-cpu-and-memory-affinity/README.md

+
+##### e2e tests
+
+- e2e_node will need to be enabled for windows to add coverage


This seems to be a very important point. Are you okay with blocking this feature based on this?

The goal would be to enable this for Beta. I don't see a sustainable way for sig-windows to enable enough tests to ensure quality for these features without leveraging existing tests. Duplicating all the test in e2e for windows would be a large undertaking as well

Maybe it should be clear that you intend to add a e2e_node like test for this one. I read this as we are going to enable all of the tests as part of this feature..

kannon92 · 2024-10-03T16:27:59Z

keps/sig-windows/4885-windows-cpu-and-memory-affinity/README.md

+###### How can someone using this feature know that it is working for their instance?
+
+- [x] Events
+ - Event Reason: 


What events?

kannon92 · 2024-10-03T16:29:07Z

keps/sig-windows/4885-windows-cpu-and-memory-affinity/README.md

+On Windows systems with more than 64 cores the CPU's will be split into groups, 
+each processor is identified by its group number and its group-relative processor number. 
+
+In Cri we will add the following structure to the `WindowsContainerResources` in CRI:


Suggested change

In Cri we will add the following structure to the `WindowsContainerResources` in CRI:

In CRI we will add the following structure to the `WindowsContainerResources` in CRI:

knabben · 2024-10-04T16:30:19Z

keps/sig-windows/4885-windows-cpu-and-memory-affinity/README.md

+
+This kep outlines how to add support for the CPU, Memory and Topology Managers in kubelet for Windows. 
+The Managers are already available and support in kubelet on Linux and there have been requests to sig-windows
+to add support on Windows to help with workloads that require co-located workloads. The goal of the kep is to 


knabben · 2024-10-04T16:30:52Z

keps/sig-windows/4885-windows-cpu-and-memory-affinity/README.md

+This kep outlines how to add support for the CPU, Memory and Topology Managers in kubelet for Windows. 
+The Managers are already available and support in kubelet on Linux and there have been requests to sig-windows
+to add support on Windows to help with workloads that require co-located workloads. The goal of the kep is to 
+add Windows support without significant changes to the Managers logic while providing the same feature sets available


would be helpful to link the actual functionality KEP and documentation for reference

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory labels Sep 30, 2024

k8s-ci-robot requested review from claudiubelu and marosset September 30, 2024 21:17

k8s-ci-robot added sig/windows Categorizes an issue or PR as relevant to SIG Windows. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 30, 2024

jsturtevant mentioned this pull request Sep 30, 2024

Windows CPU and Memory Affinity #4885

Open

7 tasks

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 30, 2024

jsturtevant mentioned this pull request Sep 30, 2024

KEP-1769/KEP-3570/KEP693: Adding Windows Kubelet Manager implementation details #4738

Closed

jsturtevant force-pushed the windows-topology-managers branch 2 times, most recently from 593f44b to 0ecf8b5 Compare September 30, 2024 21:33

Add Windows cpu and memory affinity

1204ef3

Signed-off-by: James Sturtevant <[email protected]>

jsturtevant force-pushed the windows-topology-managers branch from 0ecf8b5 to 1204ef3 Compare September 30, 2024 21:38

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 30, 2024

marosset reviewed Sep 30, 2024

View reviewed changes

jsturtevant force-pushed the windows-topology-managers branch from c5b33b5 to 3456c1c Compare September 30, 2024 23:51

Respond to feedback

eef698f

Signed-off-by: James Sturtevant <[email protected]>

jsturtevant force-pushed the windows-topology-managers branch from 3456c1c to eef698f Compare October 1, 2024 00:15

k8s-ci-robot added this to the v1.32 milestone Oct 1, 2024

Update reviewers/approvers

2626c67

Signed-off-by: James Sturtevant <[email protected]>

aravindhp reviewed Oct 1, 2024

View reviewed changes

keps/sig-windows/4885-windows-cpu-and-memory-affinity/README.md Show resolved Hide resolved

kevpar approved these changes Oct 1, 2024

View reviewed changes

ffromani reviewed Oct 2, 2024

View reviewed changes

Address sig-node feedback

5e96687

Signed-off-by: James Sturtevant <[email protected]>

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 2, 2024

kannon92 reviewed Oct 3, 2024

View reviewed changes

knabben reviewed Oct 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-4885: Windows CPU and Memory Affinity #4888

KEP-4885: Windows CPU and Memory Affinity #4888

jsturtevant commented Sep 30, 2024

kiashok commented Oct 1, 2024

marosset commented Oct 1, 2024

aravindhp left a comment

k8s-ci-robot commented Oct 1, 2024

ffromani left a comment

ffromani Oct 2, 2024

ffromani Oct 2, 2024

jsturtevant Oct 2, 2024

ffromani Oct 2, 2024

jsturtevant Oct 2, 2024

ffromani Oct 2, 2024

jsturtevant Oct 2, 2024

ffromani Oct 2, 2024

kannon92 Oct 3, 2024

jsturtevant Oct 3, 2024

kannon92 Oct 3, 2024

jsturtevant Oct 3, 2024

kannon92 Oct 3, 2024

kannon92 Oct 3, 2024

kannon92 Oct 3, 2024

knabben Oct 4, 2024

knabben Oct 4, 2024

		On Windows systems with more than 64 cores the CPU's will be split into groups,
		each processor is identified by its group number and its group-relative processor number.


		##### e2e tests

		- e2e_node will need to be enabled for windows to add coverage

	In Cri we will add the following structure to the `WindowsContainerResources` in CRI:
	In CRI we will add the following structure to the `WindowsContainerResources` in CRI:

KEP-4885: Windows CPU and Memory Affinity #4888

Are you sure you want to change the base?

KEP-4885: Windows CPU and Memory Affinity #4888

Conversation

jsturtevant commented Sep 30, 2024

kiashok commented Oct 1, 2024

marosset commented Oct 1, 2024

aravindhp left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 1, 2024

ffromani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment