Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows: Clarify r/o root filesystem #819

Merged
merged 1 commit into from
May 15, 2017

Conversation

lowenna
Copy link
Contributor

@lowenna lowenna commented May 13, 2017

Signed-off-by: John Howard jhoward@microsoft.com

Clarification that Windows does not support a read-only root filesystem.

Signed-off-by: John Howard <jhoward@microsoft.com>
@@ -32,7 +32,7 @@ For example, if a configuration is compliant with version 1.1 of this specificat
The path is either an absolute path or a relative path to the bundle.
On Linux, for example, with a bundle at `/to/bundle` and a root filesystem at `/to/bundle/rootfs`, the `path` value can be either `/to/bundle/rootfs` or `rootfs`.
A directory MUST exist at the path declared by the field.
* **`readonly`** (bool, OPTIONAL) If true then the root filesystem MUST be read-only inside the container, defaults to false.
* **`readonly`** (bool, OPTIONAL) If true then the root filesystem MUST be read-only inside the container, defaults to false. On Windows, this field must be omitted or false.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous discussion in #23 and #476.

An alternative to this would be to label the property as Linux/Solaris-specific, so it wouldn't be specified for Windows and Windows runtimes could silently ignore it (using this). That gets us to the runtime behavior proposed in #476, but avoids confusing config authors (like the spec wording in #476) because Windows authors would know that the property did not apply to their platform, and non-Windows authors would know that the property did apply (and runtimes would have to implement it) on their platforms.

@tianon
Copy link
Member

tianon commented May 13, 2017

LGTM

Forcing this to be a known-supported value like this makes sense IMO -- that allows for it to be supported in a future update if the platform adds support.

This also ends up mirroring other places in the spec where features requested in the configuration that are unsupported by the runtime or the underlying kernel MUST result in an error (which is essentially what this is codifying for Windows).

Approved with PullApprove

@wking
Copy link
Contributor

wking commented May 13, 2017

This also ends up mirroring other places in the spec where features requested in the configuration that are unsupported by the runtime or the underlying kernel MUST result in an error ...

To clarify my alternative proposal, what do you think a Linux runtime should do if a Linux config sets the Windows-only process.user.username? I think it can ignore the unknown-to-its-platform property. Are you suggesting it MUST raise an error?

@tianon
Copy link
Member

tianon commented May 14, 2017 via email

@TomSweeneyRedHat
Copy link

LGTM

@vbatts
Copy link
Member

vbatts commented May 15, 2017

LGTM

Approved with PullApprove

@vbatts vbatts merged commit 3908f28 into opencontainers:master May 15, 2017
@lowenna lowenna deleted the clarifyreadonlyroot branch May 15, 2017 17:24
@vbatts vbatts mentioned this pull request Jul 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants