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

[ Config | Root Config ] Clarify readonly #476

Closed
wants to merge 1 commit into from

Conversation

RobDolinMS
Copy link
Collaborator

Clarifies how root config of readonly==true should be handled if not supported by the platform.

Signed-off-by: Rob Dolin RobDolin@microsoft.com

Clarifies how root config of readonly==true should be handled if not supported by the platform.  

Signed-off-by: Rob Dolin <RobDolin@microsoft.com>
@@ -27,6 +27,7 @@ Each container has exactly one *root filesystem*, specified in the *root* object

* **`path`** (string, required) Specifies the path to the root filesystem for the container. 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.
If readonly is present and set to true, but not supported by the platform, the platform MAY ignore.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that failing may be a better option.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree failing seems better.

Copy link
Member

@cyphar cyphar Jun 10, 2016

Choose a reason for hiding this comment

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

In general with configs, failing is much better than silently ignoring errors. A readonly path means that the user has made certain assumptions about the setup and might be quite confused if suddenly things break underneath then.

For the Microsoft case, just make it so that your runtime errors out if a user specifies readonly (though we should make it clear somewhere in the spec whether or not all runtimes need to implement all optional options).

Copy link
Member

Choose a reason for hiding this comment

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

+1; @RobDolinMS can you either update, close, or make a good argument for why you think ignoring is preferable? 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

On Thu, Jun 09, 2016 at 10:25:55PM -0700, Aleksa Sarai wrote:

For the Microsoft case, just make it so that your runtime errors out
if a user specifies readonly (though we should make it clear
somewhere in the spec whether or not all runtimes need to implement
all optional options).

I've taken a stab at part of the general behavior in #559. I'm not
quite sure how to flag options which a runtime can not support while
still being certified. Some thoughts on that in #559 too.

@tianon
Copy link
Member

tianon commented Nov 4, 2016

@RobDolinMS was this change covered by #559? (ie, can this one be closed or updated? 😄 🙏)

@cyphar
Copy link
Member

cyphar commented Nov 6, 2016

@tianon I think this needs to be updated to specify that not being able to implement readonly is fine (otherwise someone might think that some runtimes are not valid implementations of the OCI spec).

@wking
Copy link
Contributor

wking commented Nov 6, 2016

On Sat, Nov 05, 2016 at 08:35:58PM -0700, Aleksa Sarai wrote:

I think this needs to be updated to specify that not being able to
implement readonly is fine (otherwise someone might think that
some runtimes are not valid implementations of the OCI spec).

If we add optional runtime support for a feature, we need to declare
that feature as a separate protocol in our protocol list 1.

But ideally we can just require runtime support. It sounds like
Windows can support readonly mounts in general [2,3]; is there a
specific problem with the root filesystem?

@mrunalp
Copy link
Contributor

mrunalp commented Nov 10, 2016

Closing this. @RobDolinMS please re-open if you disagree.

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.

6 participants