bundle: Remove rootfs requirement #469
Conversation
Recently b2e9154 (Remove requirement for rootfs path to be relative, 2016-04-22, opencontainers#394) relaxed the relative-path requirement on root.path to allow absolute paths (see also discussion in [1]). The motivation being that, while most folks will want a relative-path pointing into the bundle directory, there are valid use cases that don't do that, and we want to support them too [2,3]. However, allowing absolute paths but requiring them to point to a directory lying next to config.json seems to defeat the purpose. This commit removes the "root.path must point to a directory next to config.json" restriction, so folks who want absolute paths in root.path can use them without an unnecessary restriction. And again, most folks will put their rootfs alongside config.json or elsewhere inside the bundle directory. But this list was about "This MUST include the following artifacts", and we don't want to require *everyone* to do that. [1]: opencontainers#389 [2]: opencontainers#389 (comment) [3]: opencontainers#394 (comment) Signed-off-by: W. Trevor King <[email protected]>
|
Oh, and while I think the |
|
Rejected |
|
Because the original sentences are correct and describe exactly what should happen/ it should be. |
|
On Fri, May 27, 2016 at 03:51:05PM -0700, Michael Crosby wrote:
Following references from #389 takes me to moby/moby#22256. I Or more concretely, with /var/oci/config.json and the following a. "rootfs" Which are valid? Before #394, I think it was a, b, c, and d. Since And from my rough understanding of moby/moby#21969, I expect Does that all sound accurate to you? Once we agree on the facts, it |
|
On Fri, May 27, 2016 at 05:06:01PM -0700, W. Trevor King wrote:
ocitools adds another option to this list k. "." I think that is currently illegal as well because of the “MUST all be Is my understanding of that incorrect? Is k somehow legal with the |
|
On Fri, May 27, 2016 at 05:06:01PM -0700, W. Trevor King wrote:
I setup a container with Docker version 1.11.2 (build b9f10c9) and jq .bundle /run/containerd/a88d4b17c0f9b0347eb21c3f46ae124b2b38c7cf83495d3f3f60a08972bf1616/state.json"/var/run/docker/libcontainerd/a88d4b17c0f9b0347eb21c3f46ae124b2b38c7cf83495d3f3f60a08972bf1616" BUNDLE=$(jq -r .bundle /run/containerd/a88d4b17c0f9b0347eb21c3f46ae124b2b38c7cf83495d3f3f60a08972bf1616/state.json)ROOT=$(jq -r .root.path $BUNDLE/config.json)echo $BUNDLE/var/run/docker/libcontainerd/a88d4b17c0f9b0347eb21c3f46ae124b2b38c7cf83495d3f3f60a08972bf1616 echo $ROOT/var/lib/docker/btrfs/subvolumes/924b6be428d9dff1b206fb1ff6d4f217608860bfc367cd2052fbb0154e04fa0a realpath --relative-to $BUNDLE $ROOT../../../../var/lib/docker/btrfs/subvolumes/924b6be428d9dff1b206fb1ff6d4f217608860bfc367cd2052fbb0154e04fa0a So I still think we want this PR to drop the requirement that |
|
@crosbymichael Do @wking's comments address your concern or do you still think this should be rejected? |
|
after chatting about this: this is not behavior folks would expect. there is no sane assumption is there is no rootfs specified. It's a bad idea to assume it would default to '/' on the host. |
|
On Wed, Jan 18, 2017 at 02:23:56PM -0800, Vincent Batts wrote:
this is not behavior folks would expect…
Having a root.path point at a non-sibling is what Docker does (or did
as of 1.11.2 [1]).
there is no sane assumption is there is no rootfs specified.
With this PR, you still have to set root.path (because that's
specified in config.md, which I don't touch). What this PR removes is
the requirement that the root.path be a sibling of the on-disk
config.json.
[1]: #469 (comment)
|
Test the spec's [1]: While these artifacts MUST all be present in a single directory on the local filesystem, ... which is a condition it imposes on config.json and the directory referenced by root.path. I think we should drop that restriction from the spec, but my attempt to remove the restriction was rejected [2]. If a future spec drops the restriction, we can revert this commit. Using path/filepath for the path manipulation will break when validating cross-platform configs (e.g. trying to validate a Windows bundle on a Linux machine). But that's a bigger issue than this commit, so I've left it alone for now. [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc5/bundle.md#container-format [2]: opencontainers/runtime-spec#469 Signed-off-by: W. Trevor King <[email protected]>
Test the spec's [1]: While these artifacts MUST all be present in a single directory on the local filesystem, ... which is a condition it imposes on config.json and the directory referenced by root.path. I think we should drop that restriction from the spec, but my attempt to remove the restriction was rejected [2]. If a future spec drops the restriction, we can revert this commit. Using path/filepath for the path manipulation will break when validating cross-platform configs (e.g. trying to validate a Windows bundle on a Linux machine). But that's a bigger issue than this commit, so I've left it alone for now. [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc5/bundle.md#container-format [2]: opencontainers/runtime-spec#469 Signed-off-by: W. Trevor King <[email protected]>
Test the spec's [1]: While these artifacts MUST all be present in a single directory on the local filesystem, ... which is a condition it imposes on config.json and the directory referenced by root.path. I think we should drop that restriction from the spec, but my attempt to remove the restriction was rejected [2]. If a future spec drops the restriction, we can revert this commit. Using path/filepath for the path manipulation will break when validating cross-platform configs (e.g. trying to validate a Windows bundle on a Linux machine). But that's a bigger issue than this commit, so I've left it alone for now. [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc5/bundle.md#container-format [2]: opencontainers/runtime-spec#469 Signed-off-by: W. Trevor King <[email protected]>
Anyone setting 'rootfs.path' should be aware of this advice,
regardless of whether they're the ones composing the bundle or not.
Use the RFC 2119 "SHOULD" (vs. the old lowercase "should") for this
recommendation. The SHOULD semantics make sense and using SHOULD
avoids confusing readers ("did they mean to SHOULD this?").
Also drop the "While the name of this directory may be arbitrary"
caveat, because SHOULD already implies "but you can pick another
directory name if you want".
Note that the "MUST be present in a single directory" line which is
still in bundle.md forbids you from picking "foo/bar", "../foo", or
other paths that do not point at a direct child of the bundle
directory. I don't like that direct-child restriction [1], but I'm
not touching it in this commit.
[1]: opencontainers#469
Signed-off-by: W. Trevor King <[email protected]>
Instead of repeating the config.md conditions, just make the linkage more clear. Now the chain is: 1. Bundles REQUIRE a config.json which is a bundle artifact. 2. If that config.json has a root.path entry (as specified in config.md), then add the referenced directory to the set of bundle artifacts. The config.md requirements include "If defined, a directory MUST exist at the path declared by the field". 3. Apply the "MUST all be present in a single directory" condition to all bundle artifacts. I don't like that direct-child restriction [1], but I'm not touching it in this commit. So these are the same requirements as before this commit, but with less redundancy and fewer words. [1]: opencontainers#469 Signed-off-by: W. Trevor King <[email protected]>
Instead of repeating the config.md conditions, just make the linkage more clear. Now the chain is: 1. Bundles REQUIRE a config.json which is a bundle artifact. 2. If that config.json has a root.path entry (as specified in config.md), then add the referenced directory to the set of bundle artifacts. The config.md requirements include "If defined, a directory MUST exist at the path declared by the field". 3. Apply the "MUST all be present in a single directory" condition to all bundle artifacts. I don't like that direct-child restriction [1], but I'm not touching it in this commit. So these are the same requirements as before this commit, but with less redundancy and fewer words. [1]: opencontainers#469 Signed-off-by: W. Trevor King <[email protected]>
Instead of repeating the config.md conditions, just make the linkage more clear. Now the chain is: 1. Bundles REQUIRE a config.json which is a bundle artifact. 2. If that config.json has a root.path entry (as specified in config.md), then add the referenced directory to the set of bundle artifacts. The config.md requirements include "If defined, a directory MUST exist at the path declared by the field". 3. Apply the "MUST all be present in a single directory" condition to all bundle artifacts. I don't like that direct-child restriction [1], but I'm not touching it in this commit. So these are the same requirements as before this commit, but with less redundancy and fewer words. [1]: opencontainers#469 Signed-off-by: W. Trevor King <[email protected]>
Instead of repeating the config.md conditions, just make the linkage more clear. Now the chain is: 1. Bundles REQUIRE a config.json which is a bundle artifact. 2. If that config.json has a root.path entry (as specified in config.md), then add the referenced directory to the set of bundle artifacts. The config.md requirements include "If defined, a directory MUST exist at the path declared by the field". 3. Apply the "MUST all be present in a single directory" condition to all bundle artifacts. I don't like that direct-child restriction [1], but I'm not touching it in this commit. So these are the same requirements as before this commit, but with less redundancy and fewer words. [1]: opencontainers#469 Signed-off-by: W. Trevor King <[email protected]>
Recently b2e9154 (Remove requirement for rootfs path to be relative,
2016-04-22, #394) relaxed the relative-path requirement on root.path
to allow absolute paths (see also discussion here). The
motivation being that, while most folks will want a relative-path
pointing into the bundle directory, there are valid use
cases that don't do that, and we want to support them too.
However, allowing absolute paths but requiring them to point to a
directory lying next to config.json seems to defeat the purpose. This
commit removes the "root.path must point to a directory next to
config.json" restriction, so folks who want absolute paths in
root.path can use them without an unnecessary restriction.
And again, most folks will put their rootfs alongside config.json or
elsewhere inside the bundle directory. But this list was about "This
MUST include the following artifacts", and we don't want to require
everyone to do that.
This is the narrowly-scoped version of #423 with the minimum we need
to make #394 a useful change.