Add LayerFolders to Windows platform config by sunjayBhatia · Pull Request #828 · opencontainers/runtime-spec

sunjayBhatia · 2017-05-15T19:00:28Z

LayerFolders is a property that is missing from the runtime spec that is required for all useful containers on Windows.

This issue mentions extending the runtime spec with "required" platform specific fields.

wking · 2017-05-15T19:11:55Z

PullApprove doesn't like your Signed-off-by [1]. I'm guessing that's because the author is Sunjay Bhatia and the Signed-off-by is Matthew Horan. If you're forwarding Matthew's original Signed-off-by, you can use: Signed-off-by: Matthew Horan <mhoran@pivotal.io> Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io> Using clause (c) in the DCO [2,3]. [1]: https://pullapprove.com/opencontainers/runtime-spec/pull-request/828/ [2]: http://developercertificate.org [3]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc5/README.md#sign-your-work

wking · 2017-05-15T19:13:48Z


+## <a name="configWindowsLayerFolders" />LayerFolders
+
+**`layerfolders`** (array of strings, REQUIRED) specifies a list of layer folders the container image relies on. The list is ordered from topmost layer to base layer.


Just to be clear, this is something that is not covered by mounts or by image-spec unpacking? These layers need to be assembled by the runtime at container-creation time?

These layers need to be assembled by the runtime at container-creation time?
Yes, this is true. As mentioned in #817 (comment), the underlying subsystem requires that a list of layer paths be passed to the kernel in order to spin up the container. These are not mounts, and I don't believe any functionality like image-spec unpacking is applicable in this case.

For required arrays, you probably also want to require at least one entry (otherwise you might as well make the property OPTIONAL). See #769 for an example of the spec wording and minItems JSON Schema I've used for that sort of thing before.

Not my call, but it looks like the variables are all camelCase in here. Should this instead be "layerFolders"?

thanks @wking @TomSweeneyRedHat, we made these changes and modified the commit for this PR

sunjayBhatia · 2017-05-15T19:51:52Z

Thanks for the note about PullApprove. We use git-duet to automatically sign our commits when pairing, and it only adds a single sign-off which corresponds to the other pair. That doesn't work with the requirements of PullApprove.

wking · 2017-05-15T20:16:47Z

On Mon, May 15, 2017 at 12:51:53PM -0700, Sunjay Bhatia wrote: Thanks for the note about PullApprove. We use git-duet…

I've filed git-duet/git-duet#51 in case they have any ideas about making this easier for git-duet users.

lowenna · 2017-05-15T22:57:32Z

@sunjayBhatia Thanks for submitting this - I was just about to add it myself!

(@wking FYI, this is the last of the additions required for Windows - the complete set is #801, #814, #815, #816, #817 and #818)

@sunjayBhatia There's a few issues here which need addressing. The LayerPaths is correct as an array of ordered folders, but it needs to be part of an object at the same level to Resources similar to how it is defined in moby/moby libcontainerd\types_windows.go.

typeLayerOptionstruct {
// LayerFolderPath is the path to the current layer folder. Empty for Hyper-V containers.LayerFolderPathstring`json:",omitempty"`// Layer paths of the parent layersLayerPaths []string
}

It also needs an update to schema\config-windows.json. Let me know if you need any help with this. And thanks again 😸

sunjayBhatia · 2017-05-16T02:37:49Z

@jhowardmsft I'm not sure I agree with including the LayerFolderPath as something that gets included in the runtime spec, mostly because it feels like duplicate info and tying the spec to Docker implementation/hcsshim types/fields feels a little odd.

The "current layer" (sandbox layer in hcsshim parlance) seems to be equivalent to the bundle directory that an OCI compliant CLI (like runc) is given when creating a container. The bundle config.json should specify a Root.Path to the image that containers created from the bundle are based on and the CLI should activate a sandbox layer in the bundle directory since that is where the container's file system lives. When actually creating a Windows Server container from the current layer with hcsshim, we can pass the bundle directory as LayerFolderPath in the hcsshim.ContainerConfig.

With other config options, we have little choice to do anything other than bubble up the hcsshim.ContainerConfig fields up to this spec but I feel like we don't have to do that here.

sunjayBhatia · 2017-05-16T02:54:57Z

Updated schema/config-windows.json to reflect the new field

tianon · 2017-05-17T23:46:34Z

(Taking back my LGTM for now -- @jhowardmsft tells me he's going to review and comment on this one. ❤️)

sunjayBhatia · 2017-05-22T04:59:45Z

@wking any more thoughts on this?

sunjayBhatia · 2017-05-22T05:00:53Z

@jhowardmsft ^

lowenna · 2017-05-23T21:29:03Z

@sunjayBhatia TL;DR LGTM (not a maintainer). I'm good with this change.

However, addressing your comment above, it's slightly more complicated. We actually always need to populate a LayerFolderPath for HCS which needs to be a top-level item. I'll submit a PR for that shortly. And also at the same time, remove the sandbox path which I recently introduced. Between TP5 and RS1, there was a change in HCS such that if SandboxPath is set, it overwrites LayerFolderPath internally, even though LayerFolderPath is set. IOW, SandboxPath is completely redundant in all RTM version of Windows, and LayerFolderPath is required.

As for your comment about Root, there is no Root as such in Hyper-V containers, as the root is really only meaningful inside the Utility VM, not the host itself. In Windows Server Containers, there is a root which is a volume-style path (\\?\Volume{GUID} style) of the containers root file-system which is mounted on the host, and required.

Edit - I'm actually going to add it as SandboxPath as that's a better description than LayerFolderPath

Edit2 - There's no reason to add it. From a spec perspective, it can be in the LayerFolders as the top-most layer. And split by the runtime into the HCS API.

wking · 2017-05-23T21:40:41Z


+## <a name="configWindowsLayerFolders" />LayerFolders
+
+**`layerFolders`** (array of strings, REQUIRED) specifies a list of layer folders the container image relies on. The list is ordered from topmost layer to base layer.


This is the first instance (as far as I know) of a REQUIRED property that is a direct child of a platform property (windows, in this case). Because REQUIRED is only “if the parent is set”, I think you'll want to adjust this section to say that windows MUST be set when platform.os is windows.
Discussion in #830 may end up with platform being removed entirely. If that lands first, it will change the phrasing for the windows requirement, but requiring the windows object for configs aimed at the windows platform will still be something you want.

Updated config.md to specify that the windows configuration section is required if the windows platform is set. We can update this again if #830 lands before this PR.

Signed-off-by: Matthew Horan <mhoran@pivotal.io> Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>

wking · 2017-05-24T04:08:22Z

 This MAY be set if **`platform.os`** is `linux` and MUST NOT be set otherwise.
 * **`windows`** (object, OPTIONAL) [Windows-specific configuration](config-windows.md).
- This MAY be set if **`platform.os`** is `windows` and MUST NOT be set otherwise.
+ This MUST be set if **`platform.os`** is `windows` and MUST NOT be set otherwise.


I'm not entirely clear on the maintainers' preference for this (@mrunalp was going to document them), but it's possible that they'll want *Windows changed to Windowsin the Go structure now that this property is required on one platform. At least, something like that was the reason given for not wanting pointers for User.UID and User.GID.

Thats an interesting point. Will it be a little odd that the other platform specific fields are not pointers as well? I would expect the maintainers would like to keep these fields uniform.

tianon · 2017-05-24T15:09:10Z

LGTM

lowenna · 2017-05-24T21:52:37Z

 // Windows defines the runtime configuration for Windows based containers, including Hyper-V containers.
 type Windows struct {
+	// LayerFolders contains a list of absolute paths to directories containing image layers.
+	LayerFolders []string `json:"layerFolders"`


Just spotted this. Should be all lower-case for consistency. And the config-windows.json schema file needs the matching update.

Should be all lower-case for consistency.
There are currently some camelCase entries in config-windows.md. For example, the network properties.

@darrenstahlmsft Could you open a PR to fix ^^

@wking @jhowardmsft i modified the field name as per this suggestion, should I revert it back?
It looks like there are linux fields that use camel case as well.

It looks like there are linux fields that use camel case as well.
The Linux config tries to consistently use camelCase. I don't know if the maintainers care what convention the Windows-only fields use; probably wait for them to weigh in.

OK, lets leave this as is. I'll take an action item to clean up the casing once this is merged. Opened #857 to track.

lowenna · 2017-05-25T17:58:33Z

@wking - Are you OK with this now?

wking · 2017-05-25T18:21:05Z

On Thu, May 25, 2017 at 10:58:34AM -0700, John Howard wrote: @wking - Are you OK with this now?

I'm not familiar with the underlying tech, so I can't comment on that. I've just been pushing for internal consistency, and that looks pretty good to me with 284ab58. My only outstanding concern is the discrepancy between pointer policy for windows and process.user.uid [1], but Go types aren't part of the spec so we don't have to resolve that discrepancy before landing this PR. [1]: #828 (comment)

lowenna · 2017-05-25T23:18:30Z

ping @mrunalp @vbatts @crosbymichael Could one of you review please. Thanks. 👍

lowenna · 2017-05-25T23:20:45Z

I should note this is the last of the structural missing pieces in Windows for 1.0. I have proved now that the docker engine using the current spec plus this PR is fully functional.

mrunalp · 2017-05-26T00:26:52Z

LGTM

TomSweeneyRedHat · 2017-05-26T12:13:42Z

LGTM

lowenna · 2017-05-30T23:02:00Z

@mrunalp @tianon Can we merge? Has all the right number of "LGTM"s 😄

lowenna · 2017-05-30T23:17:51Z

Thanks @tianon ❤️

wking reviewed May 15, 2017
View reviewed changes

sunjayBhatia mentioned this pull request May 16, 2017
Windows targets for 1.0? #817
Closed

vbatts added the platforms/windows label May 17, 2017

wking reviewed May 23, 2017
View reviewed changes

lowenna mentioned this pull request May 23, 2017
Windows: Remove Sandbox, additional tweaks #849
Merged

Add LayerFolders to Windows platform config
284ab58
Signed-off-by: Matthew Horan <mhoran@pivotal.io> Signed-off-by: Sunjay Bhatia <sbhatia@pivotal.io>

wking reviewed May 24, 2017
View reviewed changes

wking mentioned this pull request May 24, 2017
Remove platform field #850
Merged

vbatts added this to the 1.0 freeze milestone May 24, 2017

lowenna reviewed May 24, 2017
View reviewed changes

wking mentioned this pull request May 24, 2017
Expand platform to be more sophisticated #830
Closed

lowenna mentioned this pull request May 25, 2017
Camel casing needs fixing in config.go and matching schema #857
Closed

lowenna mentioned this pull request May 25, 2017
Windows: Fix camelCase inconsistencies #859
Merged

tianon merged commit aaa2481 into opencontainers:masterMay 30, 2017

sunjayBhatia deleted the windows-layer-paths branch May 31, 2017 02:22

wking mentioned this pull request May 31, 2017
Add readonly option for mount #868
Merged

wking mentioned this pull request Jun 11, 2017
Signoff uses Git's --signoff which only includes the committer git-duet/git-duet#51
Open

vbatts mentioned this pull request Jul 5, 2017
bump to v1.0.0-rc6 #882
Merged


		## <a name="configWindowsLayerFolders" />LayerFolders

		`layerfolders` (array of strings, REQUIRED) specifies a list of layer folders the container image relies on. The list is ordered from topmost layer to base layer.

Uh oh!

Conversation

sunjayBhatia commented May 15, 2017

Uh oh!

wking commented May 15, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunjayBhatia commented May 15, 2017

Uh oh!

wking commented May 15, 2017 via email

Uh oh!

lowenna commented May 15, 2017

Uh oh!

sunjayBhatia commented May 16, 2017

Uh oh!

sunjayBhatia commented May 16, 2017

Uh oh!

tianon commented May 17, 2017

Uh oh!

sunjayBhatia commented May 22, 2017

Uh oh!

sunjayBhatia commented May 22, 2017

Uh oh!

lowenna commented May 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tianon commented May 24, 2017• edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lowenna commented May 25, 2017

Uh oh!

wking commented May 25, 2017 via email

Uh oh!

lowenna commented May 25, 2017

Uh oh!

lowenna commented May 25, 2017

Uh oh!

mrunalp commented May 26, 2017• edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomSweeneyRedHat commented May 26, 2017

Uh oh!

lowenna commented May 30, 2017

Uh oh!

lowenna commented May 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

lowenna commented May 23, 2017•
edited
Loading

tianon commented May 24, 2017•
edited by caniszczyk
Loading

mrunalp commented May 26, 2017•
edited by caniszczyk
Loading