-
Notifications
You must be signed in to change notification settings - Fork 554
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: runtime SHOULD create mountpoints and cwd #928
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
cc @jhowardmsft @darrenstahlmsft @jstarks Is this applicable for Windows as well? |
There is some earlier discussion of this behavior in #591, which
initially started out the other way around (trying to forbid
destination auto-creation). The consensus after discussion there was
that the runtime should attempt to create a missing destination and
error when it couldn't, but that a spec MUST like the one you're
adding might be hard to compliance-test [1].
I agree that validating error *messages* is not possible, because we
don't specify how or even if errors are logged [2]. But we can test
whether ‘create’ errors as a boolean [3], and when it succeeds we can
test the permissions and ownership of any auto-created destination.
So I'm in favor of something like the wording you have in 1e5f050.
However:
* SHOULD doesn't really help callers. If they're going to rely on
this behavior, they'll need a runtime MUST. If they can't rely on
this behavior, they'll have to implement their own workarounds
regardless of runtime support. If they decide to rely on runtime
support based on a particular runtime implementation, then we don't
need a spec entry at all. I'm personally in favor of either MUST
(so callers can rely on support) or either a MAY or leaving the spec
as it stands (so callers realize they cannot rely on support in a
generic compliant runtime), but I'm not a maintainer.
* It seems strange to specify mode but not owner/group. I'd rather
leave all of that up to the runtime. Folks who care can chmod/chown
post-create (possibly via a hook). That lets us avoid specifying an
owner/group, which could be sticky for runtimes called by
unprivileged users (which may or may not be using a privileged
daemon or set(u|g)id runtime for creation).
* “if the directory does not exist” seems much cleaner than “if the
destination is a directory and it does not exist”. If the
destination does not exist, it's not going to be a directory.
Perhaps you were getting at file mounts, e.g. binding
/etc/resolv.conf. In that case, I think you want explicit wording
to cover the case, like:
If a [stat(3)][4] call for ‘source’ has ‘S_IFDIR’ set in ‘mode_t’
[5], the runtime MUST create a directory at ‘destination’.
Otherwise, the runtime MUST create an ‘S_IFREG’ file at
‘destination’. In both cases, the runtime MUST create any missing
ancestor directories. …bla bla ownership, etc.…
* nit: I'd rather not have a Linux/Solaris entry in ‘destination’,
followed by a Windows entry, followed by another Solaris entry.
Can we stick to alphabetical order and move the current trailing
Solaris entry up to follow the Linux/Solaris entry you have?
* nit: We currently use “POSIX platforms” [6] or “systems that support
POSIX {feature}” [7] to avoid explicitly listing POSIX platforms,
with the idea being that it will be easier to add a BSD platform (or
some such) in the future [8] (although this may not be a maintainer
consensus, see [9]).
[1]: #591 (comment)
[2]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/runtime.md#errors
[3]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/runtime.md#create
[4]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/stat.html
[5]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_stat.h.html
[6]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#posix-platform-user
[7]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#posix-process
[8]: #835 (comment)
[9]: #838 (comment)
|
@opencontainers/runtime-spec-maintainers Should this be SHOULD or MUST, or MAY? In this PR I chose SHOULD, but MUST would be fine (when ENOENT && !EROFS) |
Any thought? |
More discussions on #955 |
@alban |
house keeping: see comment #955 (comment) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This corresponds to the runc implementation.
cc @crosbymichael @cyphar
Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp