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

wking · 2017-05-18T18:29:17Z

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).

Following #820.

wking · 2017-05-18T18:35:41Z

specs-go/config.go

@@ -11,7 +11,7 @@ type Spec struct {
 	// Process configures the container process.
 	Process *Process `json:"process,omitempty"`
 	// Root configures the container's root filesystem.
-	Root Root `json:"root"`
+	Root *Root `json:"root,omitempty"`


This may fall under the no-pointers-if-required-on-any-platform policy that @mrunalp was going to document. If so, I can drop the pointer (and omitempty?), but it would be nice to get that policy documented.

Does it make sense to have a pointer-required-if-field-must-not-be-set-on-a-platform policy instead?

In this case: keep *Root and omitempty because it MUST NOT be set for Hyper-V containers

Does it make sense to have a pointer-required-if-field-must-not-be-set-on-a-platform policy instead?

We already have some policy wording for pointers, but they focus on input requirements. I think we also want to consider output requirements with Go's default JSON serializer, and would recommend:

Use omitempty on properties which MAY be unset for a platform that uses the parent type. Of omitempty properties, use pointers if the Go zero value is legal (so we can serialize "uid": 0 and such) or if the property is a Go type (to work around golang/go#11939).

as a new hole we'd want to poke in the current no-pointer preference. That would mean that we'd want omitempty here (because root MUST NOT be set for Hyper-V containers, which passes “MAY be unset for a platform that uses the parent type”) and we'd want a pointer here (because property is a Go type). However, it would also mean we'd want omitempty for User.UID and User.GID (because they MAY be unset on Windows, and Windows uses User too for User.Username) and we'd want pointers for them (because 0 is a valid ID), and those changes were rejected in #759. So instead of proposing a pointer policy, I'm just trying to flag changes that may be impacted by the maintainers' apparent policy and waiting for them to us how they want that case handled.

wking · 2017-05-23T18:54:58Z

Rebased around #839 with c652c06 → de31fae and updated to link to windows.hyperv now that #818 has landed with specifics about when a config is for a “Hyper-V Container”.

wking · 2017-05-25T21:28:38Z

Rebased around #849 with de31fae → b9b8a51. Now instead of just making the requirement clearner I'm also addressing inconsistencies that we'd missed when reviewing #849 (e.g. the rootfs suggestion conflicts with the volume GUID path requirement).

wking · 2017-05-25T21:38:07Z

Also, if we want the condition-shift to land before 2.0, it needs to land by 1.0 unless we preview Windows [1], because before this commit Hyper-V containers MUST set ‘"root": {}’ and after it they MUST NOT set ‘root’. My impression is that the preview is unlikely to happen, and I think this PR makes the Windows experience more straightforward, so I recommend putting this PR in the 1.0 freeze milestone [2]. Ping @jhowardmsft, @darrenstahlmsft, @sunjayBhatia, and other Windows folks in case they want to weigh in. [1]: #817 (comment) [2]: https://github.com/opencontainers/runtime-spec/milestone/15

wking · 2017-06-01T15:11:03Z

Rebased around #846 with b9b8a51 → c80f4a2.

crosbymichael · 2017-06-01T18:11:22Z

@jhowardmsft Please review this one

lowenna · 2017-06-01T21:07:01Z

LGTM (not a maintainer)

crosbymichael · 2017-06-01T21:08:11Z

config.md

+
+    * On Windows, `path` MUST be a [volume GUID path][naming-a-volume].
+
+    * On Linux and Solaris, `path` is either an absolute path or a relative path to the bundle.


Can you remove the linux && solaris here like we discussed in the call

Can you remove the linux && solaris here like we discussed in the call

Done with dfe9cb9 → de14115.

As requested by Michael [1], this reduces the number of changes required if BSD or some such is added to the config. This change leaves a bit of a gap between the platforms listed in spec.md, since readers have to figure out on their own that the POSIX properties apply to the linux and solaris platforms (and potentially other future platforms). But Michael felt like it's ok to leave that gap, at least for now [2]. [1]: opencontainers#838 (comment) [2]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2017/opencontainers.2017-06-01-17.41.log.html#l-60 Signed-off-by: W. Trevor King <wking@tremily.us>

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 <wking@tremily.us>

wking · 2017-06-16T20:22:14Z

I've rebased around the just-landed #850 with de14115 → 589d2eb.

As requested by Michael [1], this reduces the number of changes required if BSD or some such is added to the config. This change leaves a bit of a gap between the platforms listed in spec.md, since readers have to figure out on their own that the POSIX properties apply to the linux and solaris platforms (and potentially other future platforms). But Michael felt like it's ok to leave that gap, at least for now [2]. [1]: opencontainers#838 (comment) [2]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2017/opencontainers.2017-06-01-17.41.log.html#l-60 Signed-off-by: W. Trevor King <wking@tremily.us>

tianon · 2017-06-21T15:15:47Z

config.md

+
+    * On Windows, `path` MUST be a [volume GUID path][naming-a-volume].
+
+    * On POSIX, `path` is either an absolute path or a relative path to the bundle.


"On POSIX" doesn't make any sense -- POSIX isn't a platform, it's a specification, and it's one that Windows is compliant with in many cases.

I'd perfer to see this say either "On Unix" or "On Unix-like platforms".

See also #835 (comment) (from @crosbymichael).

"On POSIX" doesn't make any sense -- POSIX isn't a platform, it's a specification, and it's one that Windows is compliant with in many cases.

I've changed “On POSIX” to “On POSIX platforms” in 589d2eb → 767eeaf.

I'd perfer to see this say either "On Unix" or "On Unix-like platforms".

What is the gap between “Unix-like platforms” and “POSIX platforms”? “Unix-like platforms” seems too vague for a spec, and if we want a name for non-Windows that is distinct from POSIX, I think we need to define that name when we define our compliance tracks. I'd suggested that on the 1st, but @crosbymichael prefered to punt on that for now.

Looking at the properties I'm associating with POSIX here:

For root.path, POSIX platforms clearly do not include Windows, because POSIX paths are slash-separated.

mounts[].type is less clearly POSIX-specific (or even Unix-like-specific), since POSIX defines neither mount(8) nor mount(2). However, it's also unclear to me why Windows only supports volume GUID paths. Is that a limitation of the Windows platform, or just a limitation of the current Windows runtime?

process.user.uid and friends are POSIX-platform-specific, since numeric user and group IDs are defined by POSIX. I'm not aware of numeric user/group IDs on Windows, but if it does support them, I'd expect Windows runtimes to support process.user.uid, etc. as well.

hooks is less clearly POSIX-specific (or even Unix-like-specific), since these seem easy to implement regardless of your platform. I suspect this is a limitation of the current Windows runtime, and not a limitation of the Windows platform, so tying it to POSIX platforms seems no worse than tying it to Unix-like platforms. Maybe we want to leave this one as an explicit “Linux and Solaris Hooks” or “Hooks (not Windows)” or some such?

As requested by Michael [1], this reduces the number of changes required if BSD or some such is added to the config. This change leaves a bit of a gap between the platforms listed in spec.md, since readers have to figure out on their own that the POSIX properties apply to the linux and solaris platforms (and potentially other future platforms). But Michael felt like it's ok to leave that gap, at least for now [2]. I've used "On POSIX platforms" and similar (instead of "On POSIX") because, as Tianon points out [3], there is some space between POSIX-the-spec and platforms which implement it. [1]: opencontainers#838 (comment) [2]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2017/opencontainers.2017-06-01-17.41.log.html#l-60 [3]: opencontainers#838 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

crosbymichael

LGTM

dqminh · 2017-06-26T22:23:31Z

LGTM

mrunalp

LGTM

mrunalp · 2017-06-26T22:45:17Z

ping @tianon

Mashimiao · 2017-06-27T15:18:14Z

config.md

+
+    * On Windows, `path` MUST be a [volume GUID path][naming-a-volume].
+
+    * On POSIX platforms, `path` is either an absolute path or a relative path to the bundle.


I think POSIX here is also not suitable as in #835

vbatts · 2017-06-29T17:32:55Z

(@tianon is on vacation ✨ )

LGTM

tianon · 2017-07-03T18:52:09Z

Generally LGTM, although I still have a big eyebrow-raising at our use of POSIX to mean "non-Windows platforms".

Through f4d221c (Merge pull request opencontainers#880 from dqminh/wking-linux-only-capabilities-again, 2017-07-05). The rc6 release picked up an earlier version of these notes, and those entries are mostly unchanged except for: * The credentialSpec entry, which was opencontainers#814 for credentialspec and now also includes opencontainers#859 for credentialSpec. * The root(.path) Hyper-V entry, which was opencontainers#820 for root.path and now also includes opencontainers#838 for root. I also moved this into the "breaking changes" section, because rc5 Hyper-V configs required root to be set, and rc6 Hyper-V configs require it to not be set. Although whether rc5 allowed Hyper-V configs at all is not clear to me. * Fixed indenting for the typo-fixes entry, as well as a number of more recent typo-fix PRs. Signed-off-by: W. Trevor King <wking@tremily.us>

I'd broken this in 2022e3e (config: Bump Hyper-V condition from root.path to root itself, 2017-05-18, opencontainers#838), where I'd misread the initial bolding ** as a list bullet or something. The extra indents don't cause a problem for the first paragraph, but they cause the "On all other platforms..." paragraph to be interpreted as a <pre> block. Signed-off-by: W. Trevor King <wking@tremily.us>

wking commented May 18, 2017

View reviewed changes

wking mentioned this pull request May 23, 2017

format specs with 4 spaces indent #846

Merged

wking force-pushed the bump-path-requirement-to-root branch from c652c06 to de31fae Compare May 23, 2017 18:53

wking mentioned this pull request May 23, 2017

Windows: Remove Sandbox, additional tweaks #849

Merged

wking force-pushed the bump-path-requirement-to-root branch from de31fae to b9b8a51 Compare May 25, 2017 21:24

wking mentioned this pull request May 31, 2017

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

Merged

wking force-pushed the bump-path-requirement-to-root branch from b9b8a51 to 8a3b20b Compare June 1, 2017 15:10

wking force-pushed the bump-path-requirement-to-root branch from 8a3b20b to c80f4a2 Compare June 1, 2017 15:14

mrunalp added this to the 1.0.0 milestone Jun 1, 2017

wking force-pushed the bump-path-requirement-to-root branch from c80f4a2 to dfe9cb9 Compare June 1, 2017 18:12

crosbymichael requested changes Jun 1, 2017

View reviewed changes

wking force-pushed the bump-path-requirement-to-root branch from dfe9cb9 to de14115 Compare June 1, 2017 21:48

wking mentioned this pull request Jun 1, 2017

config: Make capabilities, noNewPrivileges, and rlimits Linux-only (again) #835

Closed

wking force-pushed the bump-path-requirement-to-root branch from de14115 to 589d2eb Compare June 16, 2017 20:39

tianon reviewed Jun 21, 2017

View reviewed changes

wking force-pushed the bump-path-requirement-to-root branch from 589d2eb to 767eeaf Compare June 21, 2017 17:14

crosbymichael approved these changes Jun 26, 2017

View reviewed changes

mrunalp approved these changes Jun 26, 2017

View reviewed changes

Mashimiao reviewed Jun 27, 2017

View reviewed changes

vbatts merged commit 31c6569 into opencontainers:master Jun 29, 2017

vbatts mentioned this pull request Jul 5, 2017

bump to v1.0.0-rc6 #882

Merged

wking deleted the bump-path-requirement-to-root branch July 10, 2017 21:06

wking mentioned this pull request Jul 10, 2017

ChangeLog: Document changes since c83b8c8 #888

Merged

wking mentioned this pull request Oct 10, 2017

config: runtime SHOULD create mountpoints and cwd #928

Closed

wking mentioned this pull request Nov 9, 2017

config: Dedent root paragraphs, since they aren't a list entry #936

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wking commented May 18, 2017

wking May 18, 2017

sunjayBhatia May 26, 2017

wking May 26, 2017 •

edited

Loading

wking commented May 23, 2017

wking commented May 25, 2017

wking commented May 25, 2017 via email •

edited

Loading

wking commented Jun 1, 2017 •

edited

Loading

crosbymichael commented Jun 1, 2017

lowenna commented Jun 1, 2017

crosbymichael Jun 1, 2017

wking Jun 1, 2017

wking commented Jun 16, 2017

tianon Jun 21, 2017

tianon Jun 21, 2017

wking Jun 21, 2017

crosbymichael left a comment

dqminh commented Jun 26, 2017 •

edited by caniszczyk

Loading

mrunalp left a comment

mrunalp commented Jun 26, 2017

Mashimiao Jun 27, 2017

vbatts commented Jun 29, 2017 •

edited by caniszczyk

Loading

tianon commented Jul 3, 2017


		* On Windows, `path` MUST be a [volume GUID path][naming-a-volume].

		* On Linux and Solaris, `path` is either an absolute path or a relative path to the bundle.


		* On Windows, `path` MUST be a [volume GUID path][naming-a-volume].

		* On POSIX, `path` is either an absolute path or a relative path to the bundle.


		* On Windows, `path` MUST be a [volume GUID path][naming-a-volume].

		* On POSIX platforms, `path` is either an absolute path or a relative path to the bundle.

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

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

Conversation

wking commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking May 26, 2017 • edited Loading

Choose a reason for hiding this comment

wking commented May 23, 2017

wking commented May 25, 2017

wking commented May 25, 2017 via email • edited Loading

wking commented Jun 1, 2017 • edited Loading

crosbymichael commented Jun 1, 2017

lowenna commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crosbymichael left a comment

Choose a reason for hiding this comment

dqminh commented Jun 26, 2017 • edited by caniszczyk Loading

mrunalp left a comment

Choose a reason for hiding this comment

mrunalp commented Jun 26, 2017

Choose a reason for hiding this comment

vbatts commented Jun 29, 2017 • edited by caniszczyk Loading

tianon commented Jul 3, 2017

wking May 26, 2017 •

edited

Loading

wking commented May 25, 2017 via email •

edited

Loading

wking commented Jun 1, 2017 •

edited

Loading

dqminh commented Jun 26, 2017 •

edited by caniszczyk

Loading

vbatts commented Jun 29, 2017 •

edited by caniszczyk

Loading