Add support for windows CPU affinity #1258

kiashok · 2024-06-20T23:18:07Z

This PR adds support for certain CRI fields in OCI runtime spec for supporting CPU affinity for windows containers.

CRI PR : kubernetes/kubernetes#124285
Github issue: kubernetes/kubernetes#125262
KEP: kubernetes/kubernetes#125262

kiashok · 2024-06-20T23:19:07Z

cc @jsturtevant @kevpar @aravindhp @marosset

kiashok · 2024-06-20T23:21:17Z

@AkihiroSuda @thaJeztah could you please take a look? :) thanks!

specs-go/config.go

tianon · 2024-06-28T02:50:15Z

Is there a compelling reason not to roughly match the naming convention adopted for Linux in #1253?

Also, please don't mark conversations with useful discussion/links as "resolved" especially if the PR itself didn't change as a result of the discussion but the discussion is still useful and potentially something someone else might reasonably ask about or want to discuss -- it makes them much more difficult to discover when reading the PR's comments.

jsturtevant · 2024-07-09T15:31:38Z

Is there a compelling reason not to roughly match the naming convention adopted for Linux in #1253?

For cpu's, Linux is a simple list where as on windows it is a bitmap with a group number (https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/miniport/ns-miniport-_group_affinity) and the naming/structure was used to map to those Windows specific concepts

For memory, I think it was the same idea, making sure it was known this is "perferred" but not guarenteed do to the Windows API's. Depending on outcome of #1258 (comment) we might not have this field

config-windows.md

kiashok · 2024-09-17T19:19:43Z

@AkihiroSuda @kevpar addressed review comments. Would you be able to take a look when you have some time please? Thanks!

kolyshkin · 2024-09-18T01:30:13Z

config-windows.md

@@ -82,6 +82,12 @@ The following parameters can be specified (mutually exclusive):
 * **`count`** *(uint64, OPTIONAL)* - specifies the number of CPUs available to the container. It represents the fraction of the configured processor `count` in a container in relation to the processors available in the host. The fraction ultimately determines the portion of processor cycles that the threads in a container can use during each scheduling interval, as the number of cycles per 10,000 cycles.
 * **`shares`** *(uint16, OPTIONAL)* - limits the share of processor time given to the container relative to other workloads on the processor. The processor `shares` (`weight` at the platform level) is a value between 0 and 10,000.
 * **`maximum`** *(uint16, OPTIONAL)* - determines the portion of processor cycles that the threads in a container can use during each scheduling interval, as the number of cycles per 10,000 cycles. Set processor `maximum` to a percentage times 100.
+* **`affinityCPUs`** *(array of objects, OPTIONAL)* - specifies the set of CPU to affinitize for this container.


nit: why not just affinity (since this is a part of cpu struct/group, and other (ajacent) fields do not have CPU in them)

Answering to myself.

I see that earlier version had affinityCPUs and AffinityPreferredNumaNodes, but the latter was dropped.

Still, affinity would work, and, if we're to add NUMA affinity later, a name like NUMAaffinity (or affinityNUMA) would still work.

The sub fields could also be mask and group accordingly 👀

Indeed. The less tautology the better.

@tianon @kolyshkin keeping it similar to how we've defined this in k/k proto file. There was a similar comment in that PR : kubernetes/kubernetes#124285 (comment)

group alone is a keyword in .proto so we couldn't just have mask or group. Thought uniformity would be good across the fields- prefix "cpu" in both fields or neither.

similar rationale to why affinityCPUs was used :

It is part of WindowsContainerResources struct which can mean cpus/memory or any other resource. So we named it affinityCPUs there. I understand that this struct in runtimespec is specifically for CPU alone. So if you would prefer me to drop "cpu" and just use "affinity" let me know, I can have that updated. Thanks.

I imagine there's got to be a way to convince Protobuf to use a keyword as an identifier, right? Some way to escape it?

Regardless, this spec is for a canonically-JSON-format, which doesn't have any constraints like that (or reserved keys at all), so I definitely still agree with @kolyshkin that we should ditch the duplication in the names and go for the simpler and more consistent all-lowercase versions (affinity or affinities, group, and mask).

@tianon have addressed this. Please take a look and let me know if it looks good. Thanks.

config-windows.md

kiashok · 2024-10-20T14:39:56Z

@tianon @kolyshkin @kevpar @jsturtevant could you please take a look at this PR when you have some time please? Thanks!

jsturtevant · 2024-10-21T15:57:00Z

/lgtm
for the functionality, I will defer to the reviewers here on the naming discussion

rchincha · 2024-10-28T21:17:07Z

specs-go/config.go

@@ -635,6 +635,17 @@ type WindowsCPUResources struct {
 	// cycles per 10,000 cycles. Set processor `maximum` to a percentage times
 	// 100.
 	Maximum *uint16 `json:"maximum,omitempty"`
+	// Set of CPUs to affinitize for this container.
+	AffinityCPUs []WindowsCPUGroupAffinity `json:"affinityCPUs,omitempty"`


Why is this specific to "Windows"? CPU affinity is more general?

The specific structure/fields supported are Windows-specific -- see https://github.com/opencontainers/runtime-spec/blob/8f3fbc881602d85699e5c448634ec1288860d966/config.md#:~:text=to%207%20(lowest).-,execCPUAffinity,-(object%2C%20OPTIONAL)%20specifies (execCPUAffinity) for how this is specified on Linux (part of the process object).

config-windows.md

specs-go/config.go

config-windows.md

Signed-off-by: Kirtana Ashok <[email protected]>

kiashok · 2024-11-13T16:44:37Z

@tianon @kolyshkin @AkihiroSuda @thaJeztah could you please take a look when you have a chance please? thanks!

kevpar · 2024-11-20T17:29:53Z

config-windows.md

+Each entry has the following structure:
+
+Ref: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/miniport/ns-miniport-_group_affinity
+
+* **`mask`** *(string, REQUIRED)* - specifies the CPU mask relative to this CPU group.
+* **`group`** *(string, REQUIRED)* - specifies the processor group this mask refers to, as returned by GetLogicalProcessorInformationEx.


These lines need to be indented with two spaces, so they show up nested under affinity

kevpar · 2024-11-20T17:30:01Z

specs-go/config.go

+	// Set of CPUs to affinitize for this container.
+	Affinity []WindowsCPUGroupAffinity `json:"affinity,omitempty"`
+	// Specifies preferred set of numa node numbers to affinitize for this container.
+	AffinityPreferredNumaNodes []uint32 `json:"affinityPreferredNumaNodes,omitempty"`


Is this supposed to be here still?

Not sure why the PR is not updating :( was able to confirm from my local branch that these changes were infact removed. Checking what's happening

kiashok requested review from AkihiroSuda, crosbymichael, cyphar, dqminh, giuseppe, hqhq, kolyshkin, mrunalp, thaJeztah, tianon and utam0k as code owners June 20, 2024 23:18

AkihiroSuda reviewed Jun 21, 2024

View reviewed changes

specs-go/config.go Show resolved Hide resolved

kevpar reviewed Jun 21, 2024

View reviewed changes

specs-go/config.go Outdated Show resolved Hide resolved

kevpar reviewed Jun 21, 2024

View reviewed changes

specs-go/config.go Outdated Show resolved Hide resolved

kevpar reviewed Jun 21, 2024

View reviewed changes

specs-go/config.go Outdated Show resolved Hide resolved

kiashok force-pushed the cpuAffinity-oci branch from e9d4443 to e85a1c8 Compare June 26, 2024 23:42

kiashok force-pushed the cpuAffinity-oci branch from e85a1c8 to fd1ac23 Compare September 5, 2024 21:00

kevpar reviewed Sep 16, 2024

View reviewed changes

config-windows.md Outdated Show resolved Hide resolved

kiashok force-pushed the cpuAffinity-oci branch from fd1ac23 to 6be4095 Compare September 17, 2024 19:18

kolyshkin reviewed Sep 18, 2024

View reviewed changes

config-windows.md Outdated Show resolved Hide resolved

kevpar reviewed Sep 24, 2024

View reviewed changes

config-windows.md Outdated Show resolved Hide resolved

jsturtevant mentioned this pull request Sep 30, 2024

Windows CPU and Memory Affinity kubernetes/enhancements#4885

Open

9 tasks

kiashok force-pushed the cpuAffinity-oci branch from 6be4095 to 0679c9d Compare October 2, 2024 16:52

jsturtevant mentioned this pull request Oct 21, 2024

Docs: Windows CPU and Memory Affinity kubernetes/website#48469

Merged

rchincha reviewed Oct 28, 2024

View reviewed changes

kiashok force-pushed the cpuAffinity-oci branch from 0679c9d to 1ef7ea5 Compare October 29, 2024 13:04

kevpar reviewed Nov 13, 2024

View reviewed changes

config-windows.md Show resolved Hide resolved

kevpar reviewed Nov 13, 2024

View reviewed changes

specs-go/config.go Outdated Show resolved Hide resolved

kevpar reviewed Nov 13, 2024

View reviewed changes

config-windows.md Outdated Show resolved Hide resolved

Add support for windows CPU affinity

d71e8e3

Signed-off-by: Kirtana Ashok <[email protected]>

kiashok force-pushed the cpuAffinity-oci branch from 1ef7ea5 to d71e8e3 Compare November 13, 2024 16:41

kevpar reviewed Nov 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for windows CPU affinity #1258

Add support for windows CPU affinity #1258

kiashok commented Jun 20, 2024

kiashok commented Jun 20, 2024

kiashok commented Jun 20, 2024

tianon commented Jun 28, 2024

jsturtevant commented Jul 9, 2024

kiashok commented Sep 17, 2024

kolyshkin Sep 18, 2024

kolyshkin Sep 18, 2024

tianon Sep 18, 2024

kolyshkin Sep 18, 2024

kiashok Sep 18, 2024

kiashok Sep 18, 2024

tianon Oct 28, 2024

kiashok Oct 29, 2024

kiashok commented Oct 20, 2024

jsturtevant commented Oct 21, 2024

rchincha Oct 28, 2024 •

edited

Loading

tianon Oct 28, 2024

kiashok commented Nov 13, 2024

kevpar Nov 20, 2024

kevpar Nov 20, 2024

kiashok Nov 20, 2024

Add support for windows CPU affinity #1258

Are you sure you want to change the base?

Add support for windows CPU affinity #1258

Conversation

kiashok commented Jun 20, 2024

kiashok commented Jun 20, 2024

kiashok commented Jun 20, 2024

tianon commented Jun 28, 2024

jsturtevant commented Jul 9, 2024

kiashok commented Sep 17, 2024

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

kiashok commented Oct 20, 2024

jsturtevant commented Oct 21, 2024

rchincha Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiashok commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchincha Oct 28, 2024 •

edited

Loading