Add support for windows CPU affinity #1258
Conversation
kiashok
requested review from
AkihiroSuda,
crosbymichael,
cyphar,
dqminh,
giuseppe,
hqhq,
kolyshkin,
mrunalp,
thaJeztah,
tianon and
utam0k
as code owners
|
@AkihiroSuda @thaJeztah could you please take a look? :) thanks! |
|
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. |
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 |
kiashok
force-pushed
the
cpuAffinity-oci
branch
from
e85a1c8 to
fd1ac23
Compare
kiashok
force-pushed
the
cpuAffinity-oci
branch
from
fd1ac23 to
6be4095
Compare
|
@AkihiroSuda @kevpar addressed review comments. Would you be able to take a look when you have some time please? Thanks! |
config-windows.md
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
nit: why not just affinity (since this is a part of cpu struct/group, and other (ajacent) fields do not have CPU in them)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The sub fields could also be mask and group accordingly 👀
There was a problem hiding this comment.
Indeed. The less tautology the better.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@tianon have addressed this. Please take a look and let me know if it looks good. Thanks.
kiashok
force-pushed
the
cpuAffinity-oci
branch
from
6be4095 to
0679c9d
Compare
|
@tianon @kolyshkin @kevpar @jsturtevant could you please take a look at this PR when you have some time please? Thanks! |
|
/lgtm |
specs-go/config.go
Outdated
| @@ -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"` | |||
There was a problem hiding this comment.
Why is this specific to "Windows"? CPU affinity is more general?
There was a problem hiding this comment.
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).
kiashok
force-pushed
the
cpuAffinity-oci
branch
from
0679c9d to
1ef7ea5
Compare
kiashok
force-pushed
the
cpuAffinity-oci
branch
from
1ef7ea5 to
d71e8e3
Compare
|
@tianon @kolyshkin @AkihiroSuda @thaJeztah could you please take a look when you have a chance please? thanks! |
kiashok
force-pushed
the
cpuAffinity-oci
branch
from
d71e8e3 to
e0be0a1
Compare
|
LGTM @tianon @kevpar @kolyshkin could you give a final look? |
Signed-off-by: Kirtana Ashok <[email protected]>
kiashok
force-pushed
the
cpuAffinity-oci
branch
from
e0be0a1 to
fd2f1d3
Compare
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