Change Windows CPU Percent to Maximum #777
Conversation
schema/defs.json
Outdated
| @@ -41,10 +41,10 @@ | |||
| "minimum": 0, | |||
| "maximum": 18446744073709552000 | |||
| }, | |||
| "percent": { | |||
| "maximum": { | |||
| "type": "integer", | |||
| "minimum": 0, | |||
There was a problem hiding this comment.
You probably want to bump this from 0 to 1 to match your Markdown docs.
There was a problem hiding this comment.
Looking at this again. I think this is actually a generic percent definition, rather than specific to CPU Percent (Oops), so I should not touch it. Am I reading that right?
There was a problem hiding this comment.
Looking at this again. I think this is actually a generic percent definition, rather than specific to CPU Percent…
Ah, right. But it looks like the Windows field is the sole consumer. I'd recommend dropping percent from defs.json and inlining the new limits in the maximum entry in config-windows.json. But I'm not a maintainer, so you may want to wait for one of them to chime in.
There was a problem hiding this comment.
I think I agree with that, and I've made the change locally, but I'll wait for more input before I push those changes.
There was a problem hiding this comment.
Another possibility, now that this shares the same 1–10k range as cpuShares, would be to rename the cpuShares entry in defs-windows.json to basisPoint or some such (permyriad? perTenThousand?). That's generic enough that it could live in defs.json
config-windows.md
Outdated
| @@ -40,15 +40,15 @@ The following parameters can be specified: | |||
|
|
|||
| * **`shares`** *(uint16, OPTIONAL)* - specifies the relative weight to other containers with CPU shares. The range is from 1 to 10000. | |||
|
|
|||
| * **`percent`** *(uint, OPTIONAL)* - specifies the percentage of available CPUs usable by the container. | |||
| * **`maximum`** *(uint, OPTIONAL)* - specifies the portion of processor cycles that this container can use as a percentage times 100 . Range is from 1 to 10000. | |||
There was a problem hiding this comment.
I think we want the change from percent to maximum, but we dont want to encode the limit into the specs. Also #780
There was a problem hiding this comment.
I think we want the change from percent to maximum, but we dont want to encode the limit into the specs.
The important exception to this, discussed in today's meeting, is when the value is unique to this spec and there are no matching kernel docs. I expect this property, and likely shares (adjusted in #780) both fall into that category, and need to retain their runtime-spec-defined limits.
There was a problem hiding this comment.
FWIW, I agree that for the Windows shares and maximum values, the ranges should be left in the spec. While the values could be followed through to the implementation in Microsoft/hcsshim, the ranges are not to change between kernel versions, and not easily mapped back to matching docs without looking at implementation.
There was a problem hiding this comment.
(Not a maintainer) but I concur with @darrenstahlmsft - on Windows, the ranges probably should remain in the OCI spec.
|
Should I remove the limits from this PR and add a new PR to add them back in? I think this change is important, and that the limits should remain in the spec for Windows, but if that's the only thing holding this PR then I can split them up to get this one merged. |
|
@darrenstahlmsft would probably be better. |
In order to increase the granularity of CPU resource control, change the CPU Percent (0-100) resource setting to CPU Maximum (0-10000) Signed-off-by: Darren Stahl <darst@microsoft.com>
darstahl
force-pushed
the
WindowsCpuMaximum
branch
from
464bdfe to
7d007ca
Compare
|
Updated to remove limits. Once this is merged I'll open another PR to continue that discussion. |
|
LGTM |
1 similar comment
|
LGTM |
In order to increase the granularity of CPU resource control, change the CPU Percent (0-100) resource setting to CPU Maximum (0-10000)
The HCS API accepts a ProcessorMaximum, which is a percentage multiplied by 100. This PR allows finer control over the CPU resources of Windows containers by extending the ProcessorMaximum into the OCI spec, rather than truncating to a 0-100 percent.
The discussion context is moby/moby#32014 and moby/moby#32608. The implementation of NanoCPUs in Moby should be able to scale to precision smaller than a single percent of total CPU resources.
/cc @jhowardmsft
Signed-off-by: Darren Stahl darst@microsoft.com