manifest: Explicitly make 'layers' optional

[wking]

Most folks will distribute images containing layers, but the specified behavior applies cleanly to the layer-less case too. The unpacked rootfs will just be an empty directory with unspecified attributes.
Folks might want to use that sort of thing for namespace pinning, distributing configs, setting up containers that mount the meat from the host, etc.

Spun off from #318 to focus on a single point.

[glestaris]

+1 from me.

I realise there were other issues/PRs trying to make layers list optional that are now closed. What is the main argument against this change?

[jonboulle]

LGTM. What do you think about adding some language to one of the documents describing the use cases?

[wking]

On Fri, Oct 21, 2016 at 03:45:12AM -0700, George Lestaris wrote:

What is the main argument against this change?

As far as I can tell, just uncertainty about the usefulness. I agree that there aren't extremely convincing use cases, but there are a few (referenced in the topic post here). And in the absence of a good reason to impose the current restriction, I think we should lift it.

On Fri, Oct 21, 2016 at 03:54:58AM -0700, Jonathan Boulle wrote:

LGTM. What do you think about adding some language to one of the documents describing the use cases?

I can do that if you like. But I imagine folks who want to make images without layers will already have a use case in mind, and folks who want to make images with layers won't care about layer-less use cases, so I'm also fine leaving it as it stands (with some use cases in the commit message to motivate the change, but none in the diff itself).

[wking]

On Fri, Oct 21, 2016 at 03:54:58AM -0700, Jonathan Boulle wrote:

LGTM. What do you think about adding some language to one of the
documents describing the use cases?

I can do that if you like. But I imagine folks who want to make
images without layers will already have a use case in mind, and folks
who want to make images with layers won't care about layer-less use
cases, so I'm also fine leaving it as it stands (with some use cases
in the commit message to motivate the change, but none in the diff
itself).

[jonboulle]

@wking the reason I asked that is because given the countner-argument is
about usefulness, it might be better to clearly justify the usefulness ;-)

On 21 October 2016 at 16:47, W. Trevor King notifications@github.com
wrote:

On Fri, Oct 21, 2016 at 03:45:12AM -0700, George Lestaris wrote:

What is the main argument against this change?

As far as I can tell, just uncertainty about
#313 (review) the
usefulness
#318 (comment).
I agree that there aren't extremely convincing use cases, but there are a
few (referenced in the topic post here). And in the absence of a good
reason to impose the current restriction
#313 (comment),
I think we should lift it.

On Fri, Oct 21, 2016 at 03:54:58AM -0700, Jonathan Boulle wrote:

LGTM. What do you think about adding some language to one of the documents
describing the use cases?

I can do that if you like. But I imagine folks who want to make images
without layers will already have a use case in mind, and folks who want to
make images with layers won't care about layer-less use cases, so I'm also
fine leaving it as it stands (with some use cases in the commit message to
motivate the change, but none in the diff itself).

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#407 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACewNw1iKSXinSdIN5Eg3g-NR1sW56hKks5q2NCAgaJpZM4KcbbA
.

[wking]

Lots of earlier discussion in #313, and also around the end of #318.

[stevvooe]

@wking Could you sum those up into a cohesive argument, rather than linking us to 30 scattered comments?

[wking]

On Tue, Oct 25, 2016 at 11:32:59AM -0700, Stephen Day wrote:

@wking Could you sum those up into a cohesive argument, rather than
linking us to 30 scattered comments?

My cohesive argument is in the topic post here.

[wking]

On Tue, Oct 25, 2016 at 11:25:31AM -0700, Jonathan Boulle wrote:

@wking the reason I asked that is because given the countner-argument is
about usefulness, it might be better to clearly justify the usefulness ;-)

I agree that it is good to justify the usefulness in this PR and
commit message to motivate the change in the spec. I'm just not
clear that it's worth justifying the lack of restriction in the spec
itself. For many optional settings we don't have in-spec wording
justifying the optionallity.

If we're going to add it, we probably have to go into more details
about the use cases I float in the topic post than I do there (linking
to GitHub comments is fine for commit messages and PR discussion, but
seems less appropriate in the spec itself ;). If you find my “I don't
think we want the use-cases in the spec” arguments unconvincing, would
you like me to unpack all of that in the ‘layers’ entry itself, or do
you want me to put them somewhere else where they're less distracting
to folks who care more about the normative spec than the informative
background?

[stevvooe]

@wking Ok, so you ship a container without a filesystem. In docker parlance, we run the following:

$ docker run -it scratch

What could this actually do?

Following from that, let's assume you convinced me the above is not silly:

docker run -it -v /usr/bin:/usr/bin scratch sh

What did we gain versus an empty tar file that makes the empty layers explicit?

[wking]

On Thu, Oct 27, 2016 at 02:33:59PM -0700, Stephen Day wrote:

@wking Ok, so you ship a container without a filesystem. In docker
parlance, we run the following:

$ docker run -it scratch

What could this actually do?

Probably nothing, although there are currently gaping holes in the
image → runtime-config translation (1 and #87). If those holes are
filled with something that configures linux.namespaces to create new
namespaces (for all known namespace types?), then the container you
create above would pin its mount namespace 2. You could launch a
sub-container joining that pinned mount namespace, have the
subcontainer do something interesting, and collect the results from
the pinned mount namespace after the subcontainer died. Without
namespace pinning, the interesting results might get removed when the
kernel reaped the mount namespace.

Or maybe the scratch config sets up networking and a volume mount to
bind the host's BusyBox and a home directory, and uses BusyBox's httpd
command to serve that bind-mounted home directory.

And if/when the image spec starts distributing runtime-spec configs
instead of the current image-spec configs, this sort of thing will no
longer need to rely on unpack-time namespace injection or run-time
volume-mount completion.

Following from that, let's assume you convinced me the above is not
silly:

docker run -it -v /usr/bin:/usr/bin scratch sh

What did we gain versus an empty tar file that makes the empty
layers explicit?

I can create a manifest that contains no layers using jq. I'd need to
also pull in tar or dd if I wanted to make an empty tarball. And gzip
unless something like #332 or #388 lands. And that layer blob is one
extra round-trip when pushing or pulling from the registry. So
including an empty layer is not the end of the world, but it seems
like a silly hoop to jump through to satisfy a “must contain at least
one layer” constraint that has no known purpose.

[stevvooe]

If layers is optional, is rootfs optional, as well?

[wking]

On Wed, Nov 16, 2016 at 02:54:10PM -0800, Stephen Day wrote:

If layers is optional, is rootfs optional, as well?

I am in favor of an optional ‘rootfs’ 1, but I see that as an
orthogonal issue. With an empty layers, the unpacked bundle would
have a rootfs directory attributes are unspecified 2. The
currently-required root.path would point at that directory 3, which
must sit next to the config.json 4.

What the runtime does with that root.path is not an image-spec
concern, but if you're creating a new mount namespace (most likely for
configs unpacked from images), the runtime will pivot into that empty
directory. If you're joining an existing mount namespace or leaving
the container process in the host mount namespace, the runtime will
presumably ignore root.path [5](although “not creating a new mount
namespace” is not a concept that the current runtime-spec addresses
clearly).

 Subject: Dropping the rootfs requirement and restoring arbitrary bundle
  content
 Date: Wed, 26 Aug 2015 12:54:47 -0700
 Message-ID: <20150826195447.GX21585@odin.tremily.us>

[stevvooe]

From your response, it looks like you have no clue what field I'm talking about. Apologies for not being more precise.

I'm referring to the rootfs field on image config.

[wking]

On Wed, Nov 16, 2016 at 06:19:15PM -0800, Stephen Day wrote:

I'm referring to the rootfs field on image config.

Ah, thanks for clarifying. I see a few possible approaches. In order
of decreasing preference:

a. Land #451. Freeze application/vnd.oci.image.config.v1+json (which
currently requires rootfs, rootfs.type, and rootfs.diff_ids. After
a few years, remove the requirement that runtimes support
application/vnd.oci.image.config.v1+json and drop it from the spec.

b. Drop rootfs.type (#224). Make rootfs and rootfs.diff_ids optional,
regardless of whether layers is set. When both layers and
rootfs.diff_ids are set, require a one-to-one mapping between
entries.

c. Keep rootfs and rootfs.type required (the current spec). Make
rootfs.diff_ids optional, regardless of whether layers is set.
When both layers and rootfs.diff_ids are set, require a one-to-one
mapping between entries.

d. Keep rootfs, rootfs.type, and rootfs.diff_ids required (the current
spec). Require a one-to-one mapping between entries in
rootfs.diff_ids and layers. When layers is not set, require an
empty array for rootfs.diff_ids.

(d) is pretty much where this PR stands now, although I can add
clarification wording to the diff_ids docs if that's the approach we
want to take.

Personally I don't have a strong opinion on the future of
application/vnd.oci.image.config.v1+json (since I see it being
replaced by the runtime config, #451). Do you have a preference among
those approaches?

[stevvooe]

Rejected.

Since this clearly hasn't been thought through, I propose that we close this PR.

@wking How can you possibly propose that runtime config could be a viable replacement for image config? Runtime config is for runtime.

[wking]

On Thu, Nov 17, 2016 at 02:26:50PM -0800, Stephen Day wrote:

Since this clearly hasn't been thought through, I propose that we
close this PR.

What is the missing piece? If it is config rootfs handling, are none
of the options I lay out in 1 acceptable? (a) and (b) have separate
PRs, and the (b) PR has already been rejected, but can you explain
your concerns with (c) and (d)?

@wking How can you possibly propose that runtime config could be a
viable replacement for image config? Runtime config is for runtime.

This is #451. I think that's a solid proposal, but for the purpose of
this PR lets say it's not on the table. That still leaves (c) and (d)
on the table 1, and those both sound like reasonable ways forward to
me.

[stevvooe]

@wking Do you even have an analysis of doing nothing? That seems like the option with the smallest impact. It seems that you've only suggested options which you possibly agree with. If (c) or (d) are an option, we may be under-specified. The spec should read towards (d), but requires due diligence before doing so.

This is #451. I think that's a solid proposal, but for the purpose of
this PR lets say it's not on the table. That still leaves (c) and (d)
on the table [1], and those both sound like reasonable ways forward to
me.

#451 continues to propagate the fallacy that image config and runtime config are equivalent entities and that having them be so tightly related leads to solid design. I have left a rejection message on the issue outlining the main problems with the proposal.

[wking]

On Mon, Nov 21, 2016 at 12:24:14PM -0800, Stephen Day wrote:

@wking Do you even have an analysis of doing nothing?

As in “closing this PR”? That just means folks who want an empty
rootfs need to jump through some extra hoops 1. I agree that it has
low impact on the spec (tautologically?), but I see no reason to
require that hoop jumping.

If (c) or (d) are an option, we may be under-specified. The spec
should read towards (d), but requires due diligence before doing so.

I think the current spec is clear enough on this point, requiring both
rootfs.diff_ids (in the config) and layers (in the manifest). This PR
is interested in making layers optional. I don't really care if
rootfs.diff_ids becomes optional or not (which is why (c) and (d) are
both on the table). As this PR stands, rootfs.diff_ids is still
clearly required 2. If you like (d) better than (c), I can add
wording like:

If the manifest's layers is empty or unset, diff_ids MUST be an
empty array.

But that's mostly to help orient people. I think the spec would be
unambigous (but with slightly less context) without such an addition.

[stevvooe]

@wking What can you exec in the container if there is no rootfs?

[wking]

On Wed, Jan 18, 2017 at 04:16:06PM -0800, Stephen Day wrote: @wking What can you exec in the container if there is no rootfs?

You could exec in that container with execveat(2), although there's not a convenient runtime-spec syntax to invoke that (ccon supports it [1]). However, I expect a more common use-case will be for a wrapping container that has a nested container launched inside it [2] (although #492 currently has nothing to say about which namespaces get setup in the generated runtime-spec config). In this case, you are unlikely to ever call ‘start’ on the wrapper container. Or the meat of the filesystem might be provided by a volume mount [2], in which case you can exec anything you mounted in. [1]: https://github.com/wking/ccon/tree/v0.4.0#host [2]: #407 (comment)

[stevvooe]

@wking I don't see the value in rehashing this pre-1.0.

Let's revisit this after a release.

[wking]

On Fri, Jan 20, 2017 at 03:48:55PM -0800, Stephen Day wrote: @wking I don't see the value in rehashing this pre-1.0. Let's revisit this after a release.

Then maybe leave this PR open, milestone it post-v1.0.0, and tag it priority/Pmaybe?

[vbatts]

I feel that this should not be explicitly called out. If a use-case is binding mounting root from the host, then they can use a 512byte+1024byte empty tar archive (header + nil bytes).

[stevvooe]

Closing per discussion on OCI meeting, 3/8.

[wking]

On Wed, Mar 08, 2017 at 03:00:58PM -0800, Vincent Batts wrote: I feel that this should _not_ be explicitly called out.

Explicitly called out by using OPTIONAL [1]? That's not shining a particularly large spotlight. And there's no way to allow layer-less images without making that change, although…

If a use-case is binding mounting root from the host, then they can use a 512byte+1024byte empty tar archive (header + nil bytes).

You don't even need the record, you can just use 1024 zero bytes (‘dd’ invocation for creating that in [2]). I just think that's a pretty silly hoop to make folks jump through, and @glestaris [3], @jonboulle [4], @runcom (#308), and @coolljt0725 (#313) all at one point seemed in favor of dropping the hoop. @runcom seems to have made the change in favor of consistency [5], and I expect doesn't really care whether ‘layers’ is optional as long as everyone's consistent. But whatever, if that's a hoop you want to make layer-less folks jump through, it's not a huge hoop. [1]: https://github.com/opencontainers/image-spec/pull/407/files#diff-bf8d9e5146f89ef0a8b9bf8cd1026901R44 [2]: #593 (comment) [3]: #407 (comment) [4]: #407 (comment) [5]: #313 (comment)

[glestaris]

@vbatts workaround seems viable TBH. It's a bit of an ugly workaround but I don't have a real use case that opposes it right now.

[vbatts]

@glestaris good to hear