Skip to content

serialization: remove windows-specific layers+base rootfs #211

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.

Sign up for GitHub

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

Merged
jonboulle merged 1 commit into from
Aug 30, 2016
Merged

serialization: remove windows-specific layers+base rootfs #211

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion serialization.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,6 @@ Note: whitespace has been added to this example for clarity. Whitespace is OPTIO
<ul>
<li>
<code>type</code> is usually set to <code>layers</code>.
There is also a Windows-specific value <code>layers+base</code> that allows a base layer to be specified in a field of <code>rootfs</code> called <code>base_layer</code>.
</li>
Copy link
Contributor

@wking wking Aug 27, 2016

Choose a reason for hiding this comment

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

Are you leaving the “usually” language to allow extensions here to still be valid images? Do you have a plan for validating unknown values? It seems like a single option should be:

type MUST be set to layers.

That gives you space to extend the type in the future.

But if there's only a single legal value, it seems like a waste of space. Can't users look at the versioning information (the v1 in application/vnd.oci.image.serialization.config.v1+json?) and know that the rootfs type would be “layers” without having to specify it in the config? Or do you expect future versions of the v1 serliazation spec to add support for additional types?

Copy link
Contributor Author

@stevvooe stevvooe Aug 29, 2016

Choose a reason for hiding this comment

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

@wking Could you please focus your review of the PR on what can actually be changed? This is not the time for reviewing the docker image format, that is the baseline of the specification and those decisions have already be made.

This field has no relationship to content descriptors present in the manifest layer. This is the internal image structure represented by this particular format. The presence of these field allows it to internally evolve without much effort.

Please keep the scope of your comments focused.

Copy link
Contributor

@wking wking Aug 29, 2016

Choose a reason for hiding this comment

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

On Mon, Aug 29, 2016 at 04:11:52PM -0700, Stephen Day wrote:

@wking Could you please focus your review of the PR on what can
actually be changed?

If the intention here is to only allow the layers-type rootfses, I
don't see a problem with dropping the field from the spec. You could
handle the drop/restoration with the same code that translates Docker
↔ OCI media types. Is there a compatibility issue with that that I'm
missing?

The presence of these field allows it to internally evolve without
much effort.

So the idea is to provide space for alternative types that still use
the application/vnd.oci.image.serialization.config.v1+json type?
That's fine with me, but I'd rather you changed the current “usually
set” language to explain how users are supposed to handle configs that
use different values. MUST they error out if asked to unpack an image
whose rootfs.type they don't understand (I expect so)? Is there any
way to determine if your image-spec implementation is modern enough to
unpack a particular image (I expect not by comparing version numbers,
but you could probably do this by using your tooling to validate the
image)?

Copy link
Contributor Author

@stevvooe stevvooe Aug 29, 2016

Choose a reason for hiding this comment

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

If the intention here is to only allow the layers-type rootfses, I
don't see a problem with dropping the field from the spec.

I'm not sure that you have full knowledge to evaluate the usage of this field, nor is this PR the forum for that discussion.

These are two completely different components operating at separate layers of the specification. I don't know what you want me to say.

Copy link
Contributor

@wking wking Aug 29, 2016

Choose a reason for hiding this comment

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

On Mon, Aug 29, 2016 at 04:31:23PM -0700, Stephen Day wrote:

If the intention here is to only allow the layers-type rootfses, I
don't see a problem with dropping the field from the spec.

I'm not sure that you have full knowledge to evaluate the usage of
this field, nor is this PR the forum for that discussion.

I'm sure you're right and I don't have sufficient background to
evaluate the usage of this field. I would expect that this spec
(since it defines the field) would provide sufficient background on
how it is expected to be used, and what it means is the value is
‘layers’, or unset, or some other string. But if that discussion is
supposed to happen somewhere else, I'm happy to have the discussion
there. Can you point me at the appropriate forum?

Copy link
Contributor Author

@stevvooe stevvooe Aug 30, 2016

Choose a reason for hiding this comment

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

@wking This field was added as part of the modifications that came out with Docker 1.10 to address long standing image format issues. The rootfs provide image provenance over the root filesystem components that make up a given image. The type field provides some measure of algorithmic agility. Removing this field probably has unintended consequences.

Recently, this value for the field was removed from the docker landscape due to a better solution for identifying base image layers that could not be distributed. The field value would never have been seen in production ready versions of docker and has no place in OCI.

The scope of this PR was to remove the value for the field and bring it in line with real world usage. Any discussion around changes to the meaning of the field or whether the field exists at all are outside the scope of this PR.

Keeping the discussion focused will ensure that PRs move more quickly and don't end up in a state where the path forward is unclear.

Copy link
Contributor

@wking wking Aug 30, 2016

Choose a reason for hiding this comment

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

On Mon, Aug 29, 2016 at 05:41:40PM -0700, Stephen Day wrote:

Removing this field probably has unintended consequences.

“Unintended consequences” are hard to discuss ;). Can you float an
example of the sort of consequences you're concerned about?

Recently, this value for the field was removed from the docker
landscape due to a better solution for identifying base image layers
that could not be distributed. The field value would never have been
seen in production ready versions of docker and has no place in OCI.

Yeah, I think “we want to remove layers+base” is something that we
agree on.

The scope of this PR was to remove the value for the field and bring
it in line with real world usage. Any discussion around changes to
the meaning of the field or whether the field exists at all are
outside the scope of this PR.

Keeping the discussion focused will ensure that PRs move more
quickly and don't end up in a state where the path forward is
unclear.

Touching/removing a particular line seems like a good opportunity to
consider the sense of the surrounding material with less maintainer
context-switching. But if getting to the bottom of the rootfs.type
field as a whole is too much to bite off in this PR, I'm happy to punt
everything after dropping layers+base to follow-up PRs. It's possible
that a general increase in formality like that proposed in #220 will
also help clarify rootfs.type as well.

<li>
<code>diff_ids</code> is an array of layer content hashes (<code>DiffIDs</code>), in order from bottom-most to top-most.
Expand Down