manifest: Explicitly base manifest.layers on an empty directory #318

wking · 2016-09-16T17:46:16Z

And point out that layer authors interested in the ownership and other attributes of the unpacked root directory should explicitly set an entry for it. I expect folks will mostly use ./, but have made that an example in case they want to use other spellings (which unpackers should respect).

I've used "match"” instead of "be" to allow folks to apply via a union filesystem or whatever. As long as the finished product is right, compliance testers and users should be satisfied.

Spun off from this comment to answer this request, cc @jonboulle.

wking · 2016-09-16T17:52:07Z

As this stands with 1472012, this PR focuses on the empty-directory base and is largely agnostic on whether layers[0] must exist. I don't see a reason to require it (more discussion on this in #313), but if we want to drop the requirement this PR would also need these changes. And if we want to keep the requirement, this PR would need a line like:

layers MUST contain at least one entry.

wking · 2016-09-16T17:58:19Z

Better wording for the unspecified initial directory in 1472012 → 7b92852.

stevvooe · 2016-09-16T20:31:45Z

manifest.md

@@ -140,9 +140,8 @@ Unlike the [Manifest List](#manifest-list), which contains information about a s
 - **`layers`** *array*

    Each item in the array MUST be a [descriptor](descriptor.md).
-    The array MUST have the base image at index 0.
-    Subsequent layers MUST then follow in the order in which they are to be layered on top of each other.
-    The algorithm to create the final unpacked filesystem layout MUST be to first unpack the layer at index 0, then index 1, and so on.


Could we preserve the clarity of this wording?

@wking Please respond inline on the review.

Do
you have a particular feature in the lines I'm removing that you feel
it doesn't cover?

You've changed the emphasis and meaning of the clause.

Before, the emphasis was on ordering, where now it is focusing on the nature of the base layer.

wking · 2016-09-16T20:36:57Z

On Fri, Sep 16, 2016 at 01:31:50PM -0700, Stephen Day wrote:

@@ -140,9 +140,8 @@ Unlike the Manifest List, which contains information about a s

layers array
 Each item in the array MUST be a [descriptor](descriptor.md).
The array MUST have the base image at index 0.

Subsequent layers MUST then follow in the order in which they are to be layered on top of each other.

The algorithm to create the final unpacked filesystem layout MUST be to first unpack the layer at index 0, then index 1, and so on.

Could we preserve the clarity of this wording?

One man's clarity is another's redundancy ;). The “MUST be … in the
listed order” is used in several runtime-spec locations [1,2,3]. Do
you have a particular feature in the lines I'm removing that you feel
it doesn't cover?

jonboulle · 2016-09-16T20:41:23Z

manifest.md

@@ -140,9 +140,8 @@ Unlike the [Manifest List](#manifest-list), which contains information about a s
 - **`layers`** *array*

    Each item in the array MUST be a [descriptor](descriptor.md).


must the descriptor point to a layer mediatype?

wking · 2016-09-16T20:50:29Z

I've pushed 7b92852 → 809ff0e with a new commit to make layers
optional, as requested here [1](I think I'm reading that right).
This pulls in my suggestions from 2 except for the change from #308.
#308 addresses content since spun off into image-tools, and that
change should land there if/when this PR lands with the OPTIONAL
semantics.

wking · 2016-09-16T20:52:15Z

On Fri, Sep 16, 2016 at 01:41:23PM -0700, Jonathan Boulle wrote:

 Each item in the array MUST be a [descriptor](descriptor.md).
must the descriptor point to a layer mediatype?

Yes, but I think that's orthogonal (#304).

wking · 2016-09-16T21:00:07Z

On Fri, Sep 16, 2016 at 01:51:32PM -0700, Stephen Day wrote:

@@ -140,9 +140,8 @@ Unlike the Manifest List, which contains information about a s

layers array
 Each item in the array MUST be a [descriptor](descriptor.md).
The array MUST have the base image at index 0.

Subsequent layers MUST then follow in the order in which they are to be layered on top of each other.

The algorithm to create the final unpacked filesystem layout MUST be to first unpack the layer at index 0, then index 1, and so on.

@wking Please respond inline on the review.

I'm just replying to GitHub's mail, and it's sending a lot since this
new review feature landed. I'll experiment with which message I
respond to and see if I can find a pattern to keep things grouped on
the PR.

Do you have a particular feature in the lines I'm removing that
you feel it doesn't cover?

You've changed the emphasis and meaning of the clause.

I tried to add meaning (for the initial state) without removing
anything from the ordering. What ordering meaning did I change?

Before, the emphasis was on ordering, where now it is focusing on
the nature of the base layer.

We obviously want to emphasize both. There's a bit more to say about
the initial state here, because layer application is covered
externally in layer.md. But maybe if I turn the sentence around:

The root filesystem MUST match the result of applying
the entries in the listed order to an empty directory.

I would rather start with the empty directory in the sentence, since
that's where implementations will start during unpacking, but putting
applications first does increase the changes that readers see “in the
listed order” before they get bored and wander off (or whatever they
were doing with the directory-first order ;).

wking · 2016-09-16T21:44:07Z

I've pushed 809ff0e → c22efd8 changing:

The root filesystem MUST match an empty directory onto which the
layers have been applied in the listed order.

to:

The root filesystem MUST match the result of
applying the entries to an intially empty
directory in the listed order.

which I like more mostly because it avoid “onto which” and “have been
applied” ;). Still has “in the listed order” at the end, but the idea
seems clear to me. If there are clearer rephrasings that avoid
repeating ourselves, I'm happy to roll them in. I'd just rather not
have “start at 0”, “follow in the order…”, and “first unpack the
layer at index 0, then index 1, and so on”.

jonboulle · 2016-09-16T21:52:09Z

manifest.md

-    The array MUST have the base image at index 0.
-    Subsequent layers MUST then follow in the order in which they are to be layered on top of each other.
-    The algorithm to create the final unpacked filesystem layout MUST be to first unpack the layer at index 0, then index 1, and so on.
+    The root filesystem MUST match the result of [applying](layer.md#applying) the entries to an intially empty directory in the listed order.


initially (but I'd rather just scrap this word)

jonboulle · 2016-09-16T21:52:49Z

Getting better, though I'm not quite sure we're at the right wording yet...

wking · 2016-09-16T22:02:50Z

I've pushed c22efd8 → 43d2e9a dropping “intially” 1. It still has
“the initial empty directory” in the second line, and I think that
between that and the context it's clear that the directory only needs
to be empty before you start applying layers ;).

jonboulle · 2016-09-16T22:28:19Z

LGTM (good enough for now :-)

stevvooe · 2016-09-20T19:16:51Z

@crosbymichael @mrunalp Could you please make sure this makes sense in the context of runc?

In docker, we do have a "scratch" layer but I think we explicitly call that out. I am not sure about the effect of adding an implied empty layer.

crosbymichael · 2016-09-20T20:17:03Z

I don't think having a required empty is needed or a good thing. If I specify one layer in the array i should only have 1 dir, not two.

wking · 2016-09-20T20:53:23Z

On Tue, Sep 20, 2016 at 01:17:04PM -0700, Michael Crosby wrote:

I don't think having a required empty is needed or a good thing. If
I specify one layer in the array i should only have 1 dir, not two.

How would you have two? If you specify one layer in the array, you'll
have a rootfs that is just that layer tarball unpacked into an
initially empty directory. The wording I'm floating here just ensures
that the runtime doesn't unpack the layer tarball into a non-empty
directory.

With:

$ tar -tf layer.tar
./
./abc

This would be the legal result of the unpacking:

$ mkdir rootfs
$ tar -C rootfs -xf layer.tar # applying the layer to an initially empty directory
$ tree rootfs/
rootfs/
└── abc

0 directories, 1 file

But this would not be legal:

$ mkdir rootfs
$ touch rootfs/def
$ tar -C rootfs -xf layer.tar # applying the layer to an initially non-empty directory
$ tree rootfs/
rootfs/
├── abc
└── def

0 directories, 2 files

crosbymichael · 2016-09-20T20:55:33Z

Then I guess the wording is super confusing and hard to tell what is meant.

wking · 2016-09-20T21:17:46Z

On Tue, Sep 20, 2016 at 01:55:34PM -0700, Michael Crosby wrote:

Then I guess the wording is super confusing and hard to tell what is meant.

“MUST match the result of applying the entries to an empty directory
in the listed order” seems fairly clear to me. Can you explain what
your two-directory interpretation 1 looked like and/or propose
alternative wording to express 2?

wking · 2016-10-01T04:23:46Z

The bulk of the first commit here (which explicitly required an
initially empty target directory) were picked up in #349. I've pushed
43d2e9a → 28a4a9d rebasing around #349, and the only remaining
changes are explicitly setting the attributes of the initial empty
directory unspecified and allowing an empty or unset layers array
(which seemed to be the consensus opinion after #313 [1,2]).

jonboulle · 2016-10-01T09:23:02Z

lgtm

vbatts · 2016-10-06T17:55:15Z

manifest.md

    Each item in the array MUST be a [descriptor](descriptor.md).
-    The array MUST have the base image at index 0.
-    Subsequent layers MUST then follow in stack order (i.e. from `layers[0]` to `layers[len(layers)]`).
+    Layers MUST must be [applied](layer.md#applying) in stack order (i.e. from `layers[0]` to `layers[len(layers)]`).


my mistake, but layers[len(layers)] -> layers[len(layers)-1]`

…ectory Point out that the layer's initial empty directory has unspecified attributes. Layer authors interested in the ownership and other attributes of the unpacked root directory should explicitly set an entry for it. This commit originally focused on requiring the initial directory to be empty before the layers are unpacked into it. But most of that was picked up in fed7241 (manifest: clarify the order of layers, 2016-09-29, opencontainers#349). I'd recommended "match" instead of "be" to allow folks to apply via a union filesystem or whatever. As long as the finished product is right, compliance testers and users should be satisfied. Signed-off-by: W. Trevor King <wking@tremily.us>

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 [1], distributing configs [2], setting up containers that mount the meat from the host [2], etc. [1]: opencontainers/runtime-spec#395 (comment) [2]: opencontainers#313 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

wking · 2016-10-06T18:02:55Z

Rebased around #363 with 28a4a9d → ded80c0.

vbatts · 2016-10-06T18:12:44Z

i think this LGTM
But i'm not certain that having layers as optional has a real use case.

(as an aside, it seems almost like the "layers" ought to just be "blobs" or "objects", as the config will reference the diff_ids as well in the expected order)

wking · 2016-10-06T18:17:55Z

On Thu, Oct 06, 2016 at 11:12:45AM -0700, Vincent Batts wrote:

(as an aside, it seems almost like the "layers" ought to just be
"blobs" or "objects", as the config will reference the diff_ids as
well in the expected order)

The manifest links to the config blob too, and that blob is not
referenced from the layers array. So I prefer the current name to
either ‘blobs’ or ‘objects’.

vbatts · 2016-10-19T16:32:58Z

LGTM

jonboulle · 2016-10-19T16:35:29Z

LGTM but needs approval from steeeeve

stevvooe · 2016-10-20T00:48:55Z

While this is elegant, I am slightly worried there is nothing practical here. From a conceptual point of few, it is interesting that layers are applied to an empty directory.

I did investigate this and ended up filing moby/moby#27568.

I think other language in this PR is sufficient for a merge.

wking · 2016-10-20T03:49:54Z

On Wed, Oct 19, 2016 at 05:48:56PM -0700, Stephen Day wrote:

From a conceptual point of few, it is interesting that layers are
applied to an empty directory.

There was previous discussion about an ./ entry in a layer in 1. If
three image-spec maintainers can have a multi-comment discussion about
whether ./ is useful, I think we need to have a spec entry that
clarifies that it is useful.

And the empty initial directory also protects from 2, which I think
is important for sanity ;).

stevvooe · 2016-10-20T17:46:45Z

@wking 100 image spec maintainers could agree and still be wrong. There are three things at issue:

What is the initial state to which layers are applied?
Clarification around ordering.
The allowance of a container with no layers.

For the most part, the discussion around 1 and 2, which is partially addressed in this PR, is important and relevant. Number 3 is novel, the use cases aren't clear and introduces something that is not already done in practice, the implications of which aren't clear.

wking · 2016-10-25T18:27:57Z

On Thu, Oct 20, 2016 at 10:46:46AM -0700, Stephen Day wrote:

What is the initial state to which layers are applied?

Clarification around ordering.

The allowance of a container with no layers.

For the most part, the discussion around 1 and 2, which is partially
addressed in this PR, is important and relevant.

Part of (1) and all of (2) has already been addressed by #349 using
language initially discussed in this PR. To keep discussion distinct,
I've spun off the two commits in this PR into their own PRs: #407 and
#408.

We doesn't define a `base image`, use `base layer` is correct. This issue first being addressed in pr opencontainers#312, and then being addressed in pr opencontainers#318, and then in pr opencontainers#407, but landing opencontainers#407 has a long way to go. We could addressed this first to avoid confusing. Signed-off-by: Lei Jitang <leijitang@huawei.com>

wking mentioned this pull request Sep 16, 2016

Replace 'base image' with 'base layer' #312

Closed

wking force-pushed the layer-base branch from 1472012 to 7b92852 Compare September 16, 2016 17:57

wking mentioned this pull request Sep 16, 2016

An image may have empty layers array #313

Closed

stevvooe requested changes Sep 16, 2016

View reviewed changes

jonboulle reviewed Sep 16, 2016

View reviewed changes

wking force-pushed the layer-base branch from 809ff0e to c22efd8 Compare September 16, 2016 21:36

jonboulle reviewed Sep 16, 2016

View reviewed changes

wking force-pushed the layer-base branch from c22efd8 to 43d2e9a Compare September 16, 2016 21:58

wking mentioned this pull request Sep 19, 2016

Make the order more clear #330

Closed

wking mentioned this pull request Sep 29, 2016

Clarify layer ordering #349

Merged

wking force-pushed the layer-base branch 2 times, most recently from 57eb55c to 28a4a9d Compare October 1, 2016 04:15

vbatts reviewed Oct 6, 2016

View reviewed changes

wking force-pushed the layer-base branch from 28a4a9d to 9270438 Compare October 6, 2016 17:59

wking force-pushed the layer-base branch from 9270438 to ded80c0 Compare October 6, 2016 18:00

This was referenced Oct 20, 2016

manifest: Explicitly make 'layers' optional #407

Closed

manifest: Explicitly unspecified attributes for the initial layer directory #408

Merged

wking closed this Oct 20, 2016

wking mentioned this pull request Nov 18, 2016

Support different levels validation for JSON reader stream #452

Closed

coolljt0725 mentioned this pull request Nov 18, 2016

manifest.md: use base layer instead of base image #463

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

manifest: Explicitly base manifest.layers on an empty directory #318

manifest: Explicitly base manifest.layers on an empty directory #318

wking commented Sep 16, 2016

wking commented Sep 16, 2016

wking commented Sep 16, 2016

stevvooe Sep 16, 2016

stevvooe Sep 16, 2016

wking commented Sep 16, 2016

jonboulle Sep 16, 2016

wking commented Sep 16, 2016

wking commented Sep 16, 2016

wking commented Sep 16, 2016 •

edited

Loading

wking commented Sep 16, 2016

jonboulle Sep 16, 2016

jonboulle commented Sep 16, 2016

wking commented Sep 16, 2016

jonboulle commented Sep 16, 2016 •

edited by caniszczyk

Loading

stevvooe commented Sep 20, 2016

crosbymichael commented Sep 20, 2016

wking commented Sep 20, 2016 •

edited

Loading

crosbymichael commented Sep 20, 2016

wking commented Sep 20, 2016

wking commented Oct 1, 2016

jonboulle commented Oct 1, 2016 •

edited by caniszczyk

Loading

vbatts Oct 6, 2016

wking commented Oct 6, 2016 via email

vbatts commented Oct 6, 2016

wking commented Oct 6, 2016

vbatts commented Oct 19, 2016 •

edited by caniszczyk

Loading

jonboulle commented Oct 19, 2016 •

edited by caniszczyk

Loading

stevvooe commented Oct 20, 2016

wking commented Oct 20, 2016

stevvooe commented Oct 20, 2016

wking commented Oct 25, 2016

		@@ -140,9 +140,8 @@ Unlike the [Manifest List](#manifest-list), which contains information about a s
		- `layers` array

		Each item in the array MUST be a [descriptor](descriptor.md).

manifest: Explicitly base manifest.layers on an empty directory #318

manifest: Explicitly base manifest.layers on an empty directory #318

Conversation

wking commented Sep 16, 2016

wking commented Sep 16, 2016

wking commented Sep 16, 2016

stevvooe Sep 16, 2016

Choose a reason for hiding this comment

stevvooe Sep 16, 2016

Choose a reason for hiding this comment

wking commented Sep 16, 2016

jonboulle Sep 16, 2016

Choose a reason for hiding this comment

wking commented Sep 16, 2016

wking commented Sep 16, 2016

wking commented Sep 16, 2016 • edited Loading

wking commented Sep 16, 2016

jonboulle Sep 16, 2016

Choose a reason for hiding this comment

jonboulle commented Sep 16, 2016

wking commented Sep 16, 2016

jonboulle commented Sep 16, 2016 • edited by caniszczyk Loading

stevvooe commented Sep 20, 2016

crosbymichael commented Sep 20, 2016

wking commented Sep 20, 2016 • edited Loading

crosbymichael commented Sep 20, 2016

wking commented Sep 20, 2016

wking commented Oct 1, 2016

jonboulle commented Oct 1, 2016 • edited by caniszczyk Loading

vbatts Oct 6, 2016

Choose a reason for hiding this comment

wking commented Oct 6, 2016 via email

vbatts commented Oct 6, 2016

wking commented Oct 6, 2016

vbatts commented Oct 19, 2016 • edited by caniszczyk Loading

jonboulle commented Oct 19, 2016 • edited by caniszczyk Loading

stevvooe commented Oct 20, 2016

wking commented Oct 20, 2016

stevvooe commented Oct 20, 2016

wking commented Oct 25, 2016

wking commented Sep 16, 2016 •

edited

Loading

jonboulle commented Sep 16, 2016 •

edited by caniszczyk

Loading

wking commented Sep 20, 2016 •

edited

Loading

jonboulle commented Oct 1, 2016 •

edited by caniszczyk

Loading

vbatts commented Oct 19, 2016 •

edited by caniszczyk

Loading

jonboulle commented Oct 19, 2016 •

edited by caniszczyk

Loading