Add readonly option for mount

[aminjam]

hcsshim supports readonly option.

Signed-off-by: Amin Jamali ajamali@pivotal.io

[wking]

Without any RFC 2119 wording, this wording places no compliance conditions on either runtimes or config JSON. I'd rather see wording like:

• Windows: runtimes MUST support ro, mounting the filesystem read-only when ro is given.

Then:

• Windows runtimes are free to add support for additional options if they want.
• Config authors know they can rely on ro support in compliant Windows runtimes.

The drawback to allowing extensions is that configs that typo rp or some such won't always fail compliance testing. You can cover that case with stricter-than-required validation (e.g the --base level in my opencontainers/image-tools#66 proposal) though, so I don't think it's worth MUSTing configs to only use ro.

The other platform entries for options aren't currently great examples of this, although I hope to revive #771 if/when opencontainers/runc#1460 lands, and that would help the Linux case.

[aminjam]

@wking Should we just change the wording to be the following?

Windows: runtimes MUST support ro, mounting the filesystem read-only when ro is given.

If ro is not a great option, is there a better replacement we should consider?

[wking]

Should we just change the wording to be the following?

Windows: runtimes MUST support ro, mounting the filesystem read-only when ro is given.

I'd suggested backticks around the literal ro, but other than that your line looks like my suggestion. So yes, I think that's what you should change the wording to.

If ro is not a great option, is there a better replacement we should consider?

I have no opinion on the value. ro is what Linux uses for this concept, but I'm fine with Windows using whatever it likes.

[wking]

PullApprove doesn't like the mismatch between author and signer (previous instance of this due to git-duet here, see also git-duet/git-duet#51).

$ git show origin/pr/868 | grep '<'
Author: Mark DeLillo <mdelillo@pivotal.io>
    Signed-off-by: Amin Jamali <ajamali@pivotal.io>

[aminjam]

@wking We have made the changes as suggested.

[crosbymichael]

@aminjam Why is this option so special that it needs to be called out specifically for windows? It seems kinda weird that we would make a big deal about it.

[sunjayBhatia]

@crosbymichael Windows containers are enabled by the Host Compute Service. It only makes a few options available for mounts, one of which is read only.

This PR enables the end user to set that flag which gets passed through to HCS. Without a set of defined values that are supposed to produce well defined behavior on the mount, an implementation of the spec won't be able to take the values provided and translate them into settings on the mount.

[crosbymichael]

@sunjayBhatia do you have a list of the supported options somewhere?

[sunjayBhatia]

@crosbymichael the only current possible/relevant options are here: https://github.com/Microsoft/hcsshim/blob/master/interface.go#L29-L33

We chose to supply ro because it is the only option that is comparable to the Linux/Solaris options

[wking]

On Thu, Jun 01, 2017 at 11:57:53AM -0700, Sunjay Bhatia wrote: @crosbymichael the only current possible/relevant options are here: https://github.com/Microsoft/hcsshim/blob/master/interface.go#L29-L33 We chose to supply `ro` because it is the only option that is comparable to the Linux/Solaris options

With only one Windows runtime in the works, I don't see much reason to distinguish between options defined in this spec and options which are defined in the runtime docs. I'm fine following #814 and just making the values implementation-defined on Windows, ideally with a stable link to docs listing whatever the Windows runtime supports. See similar previous discussion of making credentialSpec implementation-defined [1]. I'm also fine if the Windows folks want to push those docs up here and document most/all of their current options in the spec. Although then you couldn't drop an option later without bumping the spec to 2.0, so you'd only want to push options you are comfortable carrying for the life of runtime-spec 1.x. But “this one is like the Linux/Solaris options” doesn't seem like a good criterion for spec inclusion to me. Having this spec require runtimes to support a particular option is more useful on Linux, where there may be multiple runtimes and the spec requirement gives config authors confidence that the option is portable between those implementations. [1]: #814 (comment)

[sunjayBhatia]

@wking

With only one Windows runtime in the works

We are working on a runtime implementation separate from docker

I am similarly confused about why credentialSpec is "implementation defined" as that doesn't make this feel like a "standard"

[crosbymichael]

So windows only supports the ro options and we can consider the other bandwidth options as "mount data"?

[wking]

On Thu, Jun 01, 2017 at 02:13:13PM -0700, Sunjay Bhatia wrote: @wking > With only one Windows runtime in the works We are working on a runtime implementation separate from `docker`

And then presumably Docker would just use your implementation? Or are they both going to be maintained in parallel, with possibly divergent sets of mount options?

I am similarly confused about why `credentialSpec` is "implementation defined" as that doesn't make this feel like a "standard"

Implementation-defined values just mean you have to go to the implementation docs to figure out what values are supported and what they mean. If you aren't trying to write a runtime-agnostic config, that's fine. If you are trying to write a runtime-agnostic config, it's nice to have details in the spec about values that runtimes MUST support. So if we expect multiple Windows runtimes with divergent sets of mount options (not clear to me), then requiring runtime support for critical options is a good thing to do here. Runtimes can always extend beyond that set, and config authors can use the extensions where they aren't concerned with portability.

[sunjayBhatia]

@crosbymichael not sure how to treat those, makes sense that the spec should support them at some level and they could be things any windows runtime could support because they will be built on HCS, so they could be options that have the MUST be supported label as well

[sunjayBhatia]

@wking Docker could use our implementation just like it uses runc, not really up to us now of course

While these projects are not connected, it would be nice to have a runtime agnostic config as you say because any runtime built for Windows will for at least now be built with the same libraries/subsystem.

This field is just like other Windows fields where we have said "you have to support what HCS/hcsshim provides as configurable options"

[tianon]

This looks like it needs a rebase (although seems fine/sane to me).

[sunjayBhatia]

@tianon rebased the commit on top of master

[tianon]

LGTM

[crosbymichael]

LGTM