Add support for Windows based containers #573

lowenna · 2016-09-17T22:49:08Z

Signed-off-by: John Howard [email protected]

This adds Windows support to the OCI runtime-spec: code, JSON schema and markdown documentation. Note that the support has been added using 'aggressive namespacing', the matching changes of which for Linux and Solaris being currently out for review in #567.

This is a formal follow-on the the previous proof-of-concept PR to add support for Windows, #504

wking · 2016-09-18T02:18:41Z

config-windows.md

+
+* **`bps`** *(uint64, optional)* - specifies the maximum bytes per second for the system drive of the container.
+
+* **`sandboxSize`** *(uint64, optional)* - specifies the size to expand the system drive of the container to if it is currently smaller.


Maybe:

specifies the minimum size of the system drive in bytes.

Agreed. Far more succinct. Will be in next push.

wking

Also needs the README bits from #554.

wking · 2016-09-18T02:23:33Z

config-windows.md

+   "network": {
+        "egressBandwidth": 1048577
+   }
+```


Trailing newline.

Fixed in next push.

wking · 2016-09-18T02:25:03Z

specs-go/config.go

@@ -25,6 +25,8 @@ type Spec struct {
 	Linux *Linux `json:"linux,omitempty" platform:"linux"`
 	// Solaris is platform specific configuration for Solaris containers.
 	Solaris *Solaris `json:"solaris,omitempty" platform:"solaris"`
+	// Windows is platform specific configuration for Windows based containers, including Hyper-V containers.
+	Windows *Windows `json:"solaris,omitempty" platform:"windows"`


solaris -> windows.

Embarrassing copy/paste error! Yes, that one is definitely fixed in the upcoming push.

lowenna · 2016-09-18T03:33:37Z

@wking - comments addressed. And updated README from #554.

wking · 2016-09-18T04:17:21Z

config-windows.md

+
+#### Memory
+
+`memory` is used to set limits on the container's memory usage.


The existing spec is not very consistent about this, but I'd like to see an OPTIONAL dropped into this sentence. Maybe:

memory is an OPTIONAL configuration for the container's memory usage.

No objection at all. Modified memory, CPU, storage, and networking to all match this pattern.

wking · 2016-09-18T04:20:41Z

specs-go/config.go

+type Windows struct {
+	// Resources contains information for handling resource constraints for the container.
+	Resources *WindowsResources `json:"resources,omitempty"`
+}


You don't seem to define this in the Markdown. If you really want:

{ … "windows": { "resources": { "memory": {…} } } }

instead of:

{ … "windows": { "memory": {…} } }

You should drop a resources note in the Markdown.

wking · 2016-09-18T04:25:20Z

On Sat, Sep 17, 2016 at 09:21:03PM -0700, John Howard wrote:

+#### Memory
+
+memory is used to set limits on the container's memory usage.

No objection at all. Modified memory, CPU, storage, and networking
to all match this pattern.

I think we want all-caps OPTIONAL to trigger the RFC 2119 semantics
1. I'll file a separate PR to update the existing instances.

wking · 2016-09-18T04:29:29Z

config-windows.md

+
+* **`limit`** *(uint64, optional)* - sets limit of memory usage in bytes
+
+* **`reservation`** *(uint64, optional)* - sets soft limit of memory usage in bytes


Linux has resource limits with soft and hard settings:

The soft limit is the value that the kernel enforces for the corresponding resource. The hard limit acts as a ceiling for the soft limit: an unprivileged process may set only its soft limit to a value in the range from 0 up to the hard limit, and (irreversibly) lower its hard limit. A privileged process (under Linux: one with the CAP_SYS_RESOURCE capability) may make arbitrary changes to either limit value.

So when you say “soft limit”, that's what I'm thinking. However, “reservation” sounds more like “I need at least this much”. Is this setting a minimum amount required by the container? Or an initial limit on the amount that the container can consume?

Memory management is very different between Linux and Windows. Here, it means a guaranteed minimum amount of memory. I used the existing terminology though. Perhaps incorrectly?

I didn't realize we already had that language, and think it needs to be clarified for the Linux case too. Grepping through runC shows that reservation is backed by memory.soft_limit_in_bytes. Kernel docs for that have the following to say about soft limits:

.7. Soft limits

Soft limits allow for greater sharing of memory. The idea behind soft limits
is to allow control groups to use as much of the memory as needed, provided

a. There is no memory contention
b. They do not exceed their hard limit

When the system detects memory contention or low memory, control groups
are pushed back to their soft limits. If the soft limit of each control
group is very high, they are pushed back as much as possible to make
sure that one control group does not starve the others of memory.

Please note that soft limits is a best-effort feature; it comes with
no guarantees, but it does its best to make sure that when memory is
heavily contended for, memory is allocated based on the soft limit
hints/setup. Currently soft limit based reclaim is set up such that
it gets invoked from balance_pgdat (kswapd).

So in your case I think reservation makes a lot of sense for the property name, but you should reroll the description to say something like “guaranteed minimum amount of memory”.

lowenna · 2016-09-18T04:29:34Z

@wking And final update for now - change optional to OPTIONAL per your latest comment. Didn't realise you meant the capitalisation the previous time.

wking · 2016-09-18T04:29:57Z

config-windows.md

+
+```json
+    "windows": {
+        "resources" {


Missing a colon.

Fixed (all 4 occurances)

wking · 2016-09-18T04:30:45Z

config-windows.md

+                "limit": 2097152,
+                "reservation": 524288
+	        }
+	    }


Looks like an extra level of indents for these two closing braces.

Fixed. Tab rather than 4 spaces. Blame editor 😇

wking · 2016-09-18T04:52:41Z

config-windows.md

+The Windows container specification uses APIs provided by the Windows Host Compute Service (HCS) to fulfill the spec.
+
+You can configure a container's resource limits via the `resources` field of the Windows configuration.
+Do not specify `resources` unless required.


This last line seems overly strict. How about dropping it and using “…the OPTIONAL resources…” in the preceding line?

And probably worth a ## Resources before this paragraph so you can link to it specifically after the Windows-specific file grows other types of configurations (which is presumably why you have a resources level at all ;).

Re wording - I pulled that wording from the Linux spec, or so I thought, so I must have been a little over-keen modifying for the Windows version. So yes, have toned down the verbiage accordingly. Will be in next push.

Updated to include ## Resources too. Update pushed.

wking · 2016-09-18T04:54:41Z

config-windows.md

+Do not specify `resources` unless required.
+
+
+#### Memory


With ## Resources above, this one should be an h3.

Indeed. Have fixed the h1..h4s to be consistent throughout.

wking · 2016-09-18T04:57:57Z

This is looking pretty good to me. Still needs a JSON Schema, but it may be better to wait on that until the Markdown/Go settle down.

lowenna · 2016-09-19T21:10:38Z

Now with JSON schema 😄

wking · 2016-09-19T21:17:38Z

On Mon, Sep 19, 2016 at 01:36:21PM -0700, John Howard wrote:

+You can configure a container's resource limits via the resources field of the Windows configuration.
+Do not specify resources unless required.

Re wording - I pulled that wording from the Linux spec…

Looks like you did ;). I've filed #576 to adjust this language on
Linux.

wking · 2016-09-19T21:25:14Z

schema/config-windows.json

+                            },
+                            "percent": {
+                                "id": "https://opencontainers.org/schema/bundle/windows/resources/cpu/percent",
+                                "$ref": "defs.json#/definitions/uint8Pointer"


Maybe we want to use defs.json#/definitions/percentPointer backed by something like:

"percent": { "type": "integer", "minimum": 0, "maximum": 100 }, "percentPointer": { "oneOf": [ { "$ref": "#/definitions/percent" }, { "type": "null" } ] },

Good catch - didn't think of that. Will update.

Updated in 40bf0b7

wking · 2016-09-19T21:46:29Z

config-windows.md

+
+The following parameters can be specified:
+
+* **`count`** *(uint64, optional)* - specifies the number of CPUs available to the container.


I'm fine with uint64 here, but am enjoying a mental image of a container that needs 4.3 billion CPUs ;).

Future proofing :)

wking · 2016-09-19T21:47:30Z

config-windows.md

+
+* **`count`** *(uint64, optional)* - specifies the number of CPUs available to the container.
+
+* **`shares`** *(uint64, optional)* - specifies the relative weight to other containers with CPU shares. The range is from 1 to 10000.


If we know the cap is 10000, we can use a uint16.

Fixed to cpuShares type.

wking · 2016-09-19T21:49:03Z

schema/config-windows.json

+                            "shares": {
+                                "id": "https://opencontainers.org/schema/bundle/windows/resources/cpu/shares",
+                                "$ref": "defs.json#/definitions/uint64Pointer"
+                            },


This probably needs defs-windows.json for CPUShares so we can check the 1–10k range.

Yup, will update.

@wking - Updated in d73cf3e

vishh · 2016-09-19T21:52:05Z

config-windows.md

+
+The following parameters can be specified:
+
+* **`limit`** *(uint64, optional)* - sets limit of memory usage in bytes.


Is limit being over-committed?

Yes. More information on overall memory management of containers on Windows (which is different between Windows Server and Hyper-V containers) will eventually make it into the documentation on microsoft.com, but for now there's nothing published as such.

vishh · 2016-09-19T21:54:07Z

config-windows.md

+
+* **`shares`** *(uint64, 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.


I assume the CPU object is essentially a union across three parameters?

@vishh Well yes, but bearing in mind any or all of them are optional.

wking · 2016-09-19T22:18:29Z

schema/defs-windows.json

+        "cpuSharesPointer": {
+            "oneOf": [
+                {
+                    "$ref": "#/definitions/CPUShares"


I don't have much of a preference between cpuShares and CPUShares, but we should pick one and stick to it ;).

I'll stick with camelCase. Mea culpa

wking · 2016-09-19T22:18:57Z

schema/defs-windows.json

+        "cpuShares": {
+            "description": "Relative weight to other containers with CPU Shares defined",
+            "type": "integer",
+            "minimum": 0,


The Markdown docs say the minimum is 1.

And they're right, this is incorrect. Thanks for catching. Updated in 6c66d16

wking · 2016-09-19T22:53:10Z

d57e63f looks good to me :).

mrunalp · 2016-09-22T18:26:52Z

config-windows.md

+
+* **`limit`** *(uint64, optional)* - sets limit of memory usage in bytes.
+
+* **`reservation`** *(uint64, optional)* - sets the guaranteed minimum amount of memory for a container in bytes.


optional -> OPTIONAL

Can you make similar changes throughout this PR?

@mrunalp Yes, of course. Done and pushed.

mrunalp · 2016-09-22T18:29:03Z

config-windows.md

+
+The following parameters can be specified:
+
+* **`iops`** *(uint64, optional)* - specifies the maximum Iops for the system drive of the container.


Iops --> IO operations or iops?

@mrunalp Updated to IO operations per seconds.

Signed-off-by: John Howard <[email protected]>

mrunalp · 2016-09-22T23:52:33Z

LGTM

RobDolinMS · 2016-09-23T23:40:18Z

@vbatts Would you please give this a look?

vbatts · 2016-09-26T20:19:27Z

Good stuff. Thanks @jhowardmsft

LGTM

The shift happened in c35cf57 (config: Replace "optional" with "OPTIONAL", 2016-09-17, opencontainers#574) and the 'windows' entry landed in parallel with dc8f2c2 (Add support for Windows-based containers, 2016-09-16, opencontainers#573). Signed-off-by: W. Trevor King <[email protected]>

lowenna changed the title ~~In progress - adding Windows~~ Add support for Windows based containers Sep 17, 2016

lowenna force-pushed the jjh/addwindows branch from 1aff146 to c18d87e Compare September 17, 2016 22:54

lowenna mentioned this pull request Sep 18, 2016

Windows OCI: Remove endpoint list moby/moby#26677

Merged