Windows: Remove Sandbox, additional tweaks #849

lowenna · 2017-05-23T22:33:48Z

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

This relates to conversations internally here finalising and verifying the changes necessary to support Windows in the v1.0 OCI spec. Specifically the first part of this comment: #828 (comment).

It turns out that between Technical Preview 5 (TP5) and RTM of Windows Server 2016, there was a platform change whereby the SandboxPath no longer needed to be passed, but was kept in for backwards compatibility as long as the layerfolderpath is populated between the runtime and HCS. I wasn't aware of this. Hence it's no longer needed in the spec. Apologies for the noise introducing it.

There are three other clarifications in this PR:

a) Clarify the root.Path for Windows Server containers and give an explicit example.
b) Fix the double-escaping in the utilityvmpath in config-windows.md.
c) Removed old unused hyperlinks

wking · 2017-05-23T22:44:12Z

config.md

@@ -31,7 +31,7 @@ For example, if a configuration is compliant with version 1.1 of this specificat
 * **`path`** (string, OPTIONAL) Specifies the path to the root filesystem for the container. The path is either an absolute path or a relative path to the bundle.
    Users SHOULD consider using a conventional name, such as `rootfs`.

-    On Windows, for Windows Server Containers, this field is REQUIRED. For Hyper-V Containers, this field MUST be omitted.
+    On Windows, for Windows Server Containers, this field is REQUIRED and MUST be specified as a Win32 file namespace volume path. For Hyper-V Containers, this field MUST be omitted.


What is “Win32 file namespace volume path”? Can we get a doc link (maybe this?)? Ideally this MUST would be backed up by a stable link to a spec defining that term with sufficient detail for runtime-tools validation to determine if a given root.path value met the requirement or not.

And I'm also touching this line in #838. That change is orthogonal to your addition here, but whichever PR lands second will likely need a rebase.

Yes, that is the link. Will add.

Actually, found a better link at https://msdn.microsoft.com/en-us/library/windows/desktop/aa365248(v=vs.85).aspx. Updated.

wking · 2017-05-23T22:44:17Z

config.md

+
+```json
+"root": {
+    "path": "\\\\?\\Volume{ec84d99e-3f02-11e7-ac6c-00155d7682cf}",


You should remove the trailing comma here to get a legal JSON fragment.

Good catch. Fixed.

wking · 2017-05-23T22:45:39Z

config.md

+```
+
+
+


Almost all of the rest of the spec has a single empty line before a new header, so I'd recommend reducing your number of empty lines here from three to one.

Yup, fixed too.

wking · 2017-05-23T23:12:08Z

config.md

@@ -31,7 +31,7 @@ For example, if a configuration is compliant with version 1.1 of this specificat
 * **`path`** (string, OPTIONAL) Specifies the path to the root filesystem for the container. The path is either an absolute path or a relative path to the bundle.
    Users SHOULD consider using a conventional name, such as `rootfs`.

-    On Windows, for Windows Server Containers, this field is REQUIRED. For Hyper-V Containers, this field MUST be omitted.
+    On Windows, for Windows Server Containers, this field is REQUIRED and MUST be specified as a volume GUID path. For more information, see [MSDN documentation][naming-a-volume]. For Hyper-V Containers, this field MUST be omitted.


Maybe “see MSDN documentation” → “see the MSDN documentation”? Or use:

… MUST be specified as a volume GUID path.

and drop the “For more information” sentence?

Sure. Went with the latter. Updated.

wking · 2017-05-23T23:20:43Z

config.md

+
+```json
+"root": {
+    "path": "\\\\?\\Volume{ec84d99e-3f02-11e7-ac6c-00155d7682cf}"


The docs you link give the pattern as \\?\Volume{GUID}\ and say:

All volume and mounted folder functions that take a volume GUID path as an input parameter require the trailing backslash.

before talking about a CreateFile caveat. I'm not sure if the Windows runtime will always use CreateFile to back this property, or if CreateFile distinguishes between backslash-terminated or not volume GUID paths, but I expect you either want to add a trailing backslash here or add some words up above about why you don't need it.

The trailing backslash doesn't actually matter. The example I gave was pasted from a breakpoint on a live running container.

That said, I'm seeing whether HCS itself can cope if a trailing backslash is supplied.

The trailing backslash doesn't actually matter.

So say that in the docs? Perhaps:

… MUST be specified as a volume GUID path (a trailing backslash is OPTIONAL)…

It doesn't. Will update here.

Actually, it's better that we follow MSDN from the spec. I will update the runtime to ensure the trailing backslash is removed if present before passing onto HCS. Updated here accordingly.

It's a path to a directory/volume, right? Trailing slash on those is almost always optional, and IMO there's no additional value in being explicit about that here.

Trailing slash on those is almost always optional, and IMO there's no additional value in being explicit about that here.

I'd agree with that if the docs for volume GUID paths didn't require a trailing backslash in most cases.

wking · 2017-05-24T00:08:48Z

config.md

+
+```json
+"root": {
+    "path": "\\\\?\\Volume{ec84d99e-3f02-11e7-ac6c-00155d7682cf}\"


You need to double the trailing backslash to avoid escaping the closing quote.

I thought so too, but it looks ok in preview mode.... Also in preview mode I see four backslashes at the start and I think there are only supposed to be two. So I'm beginning to question the need for the backslash of the backslashes.

The four/two is correct :)

But there should be two at the end.

Okey Doke, I'll take @jhowardmsft word on this one. Thanks for the follow up.

I've pushed an update.

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

lowenna · 2017-05-24T17:08:21Z

All comments addressed. Ready for review.

TomSweeneyRedHat

LGTM

tianon · 2017-05-24T17:12:29Z

LGTM

wking · 2017-05-24T17:13:56Z

On Wed, May 24, 2017 at 10:08:22AM -0700, John Howard wrote: All comments addressed. Ready for review.

I'm not familiar enough with the underlying technology to comment on that, but 2283e63 looks internally consistent to me.

lowenna · 2017-05-25T17:56:34Z

@vbatts @mrunalp @crosbymichael OK to get this one merged now for the 1.0 RC cut-off?

mrunalp · 2017-05-25T20:51:03Z

LGTM

Don't require users targetting Hyper-V to set an empty object ("root": {}). This also avoids confusion about whether you can set root.readonly without setting root.path (you can't). Move the relative, absolute, and rootfs bits into a POSIX paragraph, because they don't apply to Windows where the value MUST be a volume GUID path (since 2283e63, Windows: Remove Sandbox, additional tweaks, 2017-05-23, opencontainers#849). We don't need the "for Windows Server containers" condition on volume GUID paths, because with this commit that condition is already applied at the 'root' level and the Hyper-V case has already been handled there. Signed-off-by: W. Trevor King <[email protected]>

SHOULD means that [1]: ... there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course. So consideration is already baked into the definition, and we can just say "SHOULD be" and warn folks whenever they use a value other than 'rootfs'. Also move this under the non-Windows conditions, because the advice they doesn't apply to Windows where the value MUST be a volume GUID path (since 2283e63, Windows: Remove Sandbox, additional tweaks, 2017-05-23, opencontainers#849). [1]: https://tools.ietf.org/html/rfc2119#section-3 Signed-off-by: W. Trevor King <[email protected]>

Don't require users targetting Hyper-V to set an empty object ("root": {}). This also avoids confusion about whether you can set root.readonly without setting root.path (you can't). Move the relative, absolute, and rootfs bits into a POSIX paragraph, because they don't apply to Windows where the value MUST be a volume GUID path (since 2283e63, Windows: Remove Sandbox, additional tweaks, 2017-05-23, opencontainers#849). We don't need the "for Windows Server containers" condition on volume GUID paths, because with this commit that condition is already applied at the 'root' level and the Hyper-V case has already been handled there. Signed-off-by: W. Trevor King <[email protected]>

wking reviewed May 23, 2017

View reviewed changes

lowenna force-pushed the sandbox branch 4 times, most recently from 34aa5e2 to 9bf68d9 Compare May 23, 2017 23:07

wking reviewed May 23, 2017

View reviewed changes

lowenna force-pushed the sandbox branch from 9bf68d9 to af971ac Compare May 23, 2017 23:16

wking reviewed May 23, 2017

View reviewed changes

lowenna force-pushed the sandbox branch 2 times, most recently from 7e0a43f to 13a3169 Compare May 23, 2017 23:37

wking reviewed May 24, 2017

View reviewed changes

tianon added the platforms/windows label May 24, 2017

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

wking mentioned this pull request May 24, 2017

Windows targets for 1.0? #817

Closed

Windows: Remove Sandbox, additional tweaks

2283e63

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

lowenna force-pushed the sandbox branch from 13a3169 to 2283e63 Compare May 24, 2017 17:05

TomSweeneyRedHat reviewed May 24, 2017

View reviewed changes

This was referenced May 24, 2017

Windows: Remove unused SandboxPath moby/moby#33384

Merged

Remove SandboxPath microsoft/hcsshim#117

Merged

mrunalp merged commit e6c9689 into opencontainers:master May 25, 2017

lowenna deleted the sandbox branch May 25, 2017 21:14

This was referenced May 25, 2017

config: Bump Hyper-V condition from root.path to root itself #838

Merged

config: Replace "SHOULD consider" for 'rootfs' path value #847

Merged

vbatts mentioned this pull request Jul 5, 2017

bump to v1.0.0-rc6 #882

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows: Remove Sandbox, additional tweaks #849

Windows: Remove Sandbox, additional tweaks #849

lowenna commented May 23, 2017 •

edited

Loading

wking May 23, 2017

lowenna May 23, 2017

lowenna May 23, 2017

wking May 23, 2017

lowenna May 23, 2017

wking May 23, 2017

lowenna May 23, 2017

wking May 23, 2017

lowenna May 23, 2017

wking May 23, 2017 •

edited

Loading

lowenna May 23, 2017

lowenna May 23, 2017

wking May 23, 2017

lowenna May 23, 2017

lowenna May 23, 2017

tianon May 23, 2017

wking May 24, 2017

wking May 24, 2017

TomSweeneyRedHat May 24, 2017 •

edited

Loading

lowenna May 24, 2017

lowenna May 24, 2017

TomSweeneyRedHat May 24, 2017

lowenna May 24, 2017

lowenna commented May 24, 2017

TomSweeneyRedHat left a comment

tianon commented May 24, 2017 •

edited by caniszczyk

Loading

wking commented May 24, 2017 via email

lowenna commented May 25, 2017

mrunalp commented May 25, 2017 •

edited by caniszczyk

Loading

		```

Windows: Remove Sandbox, additional tweaks #849

Windows: Remove Sandbox, additional tweaks #849

Conversation

lowenna commented May 23, 2017 • edited Loading

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

Choose a reason for hiding this comment

wking May 23, 2017 • edited Loading

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

Choose a reason for hiding this comment

TomSweeneyRedHat May 24, 2017 • edited Loading

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

lowenna commented May 24, 2017

TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

tianon commented May 24, 2017 • edited by caniszczyk Loading

wking commented May 24, 2017 via email

lowenna commented May 25, 2017

mrunalp commented May 25, 2017 • edited by caniszczyk Loading

lowenna commented May 23, 2017 •

edited

Loading

wking May 23, 2017 •

edited

Loading

TomSweeneyRedHat May 24, 2017 •

edited

Loading

tianon commented May 24, 2017 •

edited by caniszczyk

Loading

mrunalp commented May 25, 2017 •

edited by caniszczyk

Loading