serialization: remove windows-specific layers+base rootfs #211
Conversation
The `rootfs.type` field previously allowed a `type` of `layers+base` to signify that there the layers must be applied to a base image. This was required to support windows containers in Docker initially but has since been replaced with a more compatible mechanism. It has no relevence for OCI. Signed-off-by: Stephen J Day <[email protected]>
| <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>. |
There was a problem hiding this comment.
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:
typeMUST be set tolayers.
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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
|
Tagging @jstarks for a look / FYI |
|
LGTM |
|
LGTM (and +1 on #211 (comment)) |
With 0c52d5d (serialization: remove windows-specific layers+base rootfs, 2016-08-26, opencontainers#211) this field became a single-choice field. We could keep the field to make the current "layers" approach explicit in the face of future rootfs types, but we'd have to solidify the wording around it to cover what should happen when rootfs.type was not set, or if rootfs.type was an unrecognized value. It seems easier to remove it and make the "layers" approach implicit default, so that's what this commit does. We can restore the field later if/when we have another rootfs type without breaking forward compatibility. Code translating between Docker and OCI image renderings can drop/restore this field while it is translating Docker ↔ OCI media types, so I don't expect compatibility issues from this change. Signed-off-by: W. Trevor King <[email protected]>
With 0c52d5d (serialization: remove windows-specific layers+base rootfs, 2016-08-26, opencontainers#211) this field became a single-choice field. We could keep the field to make the current "layers" approach explicit in the face of future rootfs types, but we'd have to solidify the wording around it to cover what should happen when rootfs.type was not set, or if rootfs.type was an unrecognized value. It seems easier to remove it and make the "layers" approach implicit default, so that's what this commit does. We can restore the field later if/when we have another rootfs type without breaking forward compatibility. Code translating between Docker and OCI image renderings can drop/restore this field while it is translating Docker ↔ OCI media types, so I don't expect compatibility issues from this change. Signed-off-by: W. Trevor King <[email protected]>
With 0c52d5d (serialization: remove windows-specific layers+base rootfs, 2016-08-26, opencontainers#211) this field became a single-choice field. We could keep the field to make the current "layers" approach explicit in the face of future rootfs types, but we'd have to solidify the wording around it to cover what should happen when rootfs.type was not set, or if rootfs.type was an unrecognized value. It seems easier to remove it and make the "layers" approach implicit default, so that's what this commit does. We can restore the field later if/when we have another rootfs type without breaking forward compatibility. Code translating between Docker and OCI image renderings can drop/restore this field while it is translating Docker ↔ OCI media types, so I don't expect compatibility issues from this change. Signed-off-by: W. Trevor King <[email protected]>
I'd rather drop the field [1], but have been unable to convince Stephen that there would not be side effects of that approach. So we're back to my initial recommendation that we require the 'layers' value [2] and require implementations to error out if they see an unknown value [3]. The use of “unknown” vs. “another” allows image and implementation authors to collaborate on additional layer types if they see a need to do so while ensuring that users not party to such extensions don't get silently-broken behavior. This relies on extention types being suitably namespaced/unique so that two separate extension groups don't pick the same type string, but that seems like a reasonably safe bet. The spec does not provide any way to version this field, so users wondering "is my tooling modern enough to handle this image and any rootfs.type extensions it may contain?" should ask their tooling to validate the image. [1]: opencontainers#224 Subject: serialization: Drop rootfs.type (which had only one legal value) [2]: opencontainers#211 (comment) Subject: serialization: remove windows-specific layers+base rootfs [3]: opencontainers#211 (comment) Signed-off-by: W. Trevor King <[email protected]>
I'd rather drop the field [1], but have been unable to convince Stephen that there would not be side effects of that approach. So we're back to my initial recommendation that we require the 'layers' value [2] and require implementations to error out if they see an unknown value [3]. The use of "unknown" vs. "another" allows image and implementation authors to collaborate on additional layer types if they see a need to do so while ensuring that users not party to such extensions don't get silently-broken behavior. This relies on extention types being suitably namespaced/unique so that two separate extension groups don't pick the same type string, but that seems like a reasonably safe bet. The spec does not provide any way to version this field, so users wondering "is my tooling modern enough to handle this image and any rootfs.type extensions it may contain?" should ask their tooling to validate the image. [1]: opencontainers#224 Subject: serialization: Drop rootfs.type (which had only one legal value) [2]: opencontainers#211 (comment) Subject: serialization: remove windows-specific layers+base rootfs [3]: opencontainers#211 (comment) Signed-off-by: W. Trevor King <[email protected]>
With 0c52d5d (serialization: remove windows-specific layers+base rootfs, 2016-08-26, opencontainers#211) this field became a single-choice field. We could keep the field to make the current "layers" approach explicit in the face of future rootfs types, but we'd have to solidify the wording around it to cover what should happen when rootfs.type was not set, or if rootfs.type was an unrecognized value. It seems easier to remove it and make the "layers" approach implicit default, so that's what this commit does. We can restore the field later if/when we have another rootfs type without breaking forward compatibility. Code translating between Docker and OCI image renderings can drop/restore this field while it is translating Docker ↔ OCI media types, so I don't expect compatibility issues from this change. Signed-off-by: W. Trevor King <[email protected]>
I'd rather drop the field [1], but have been unable to convince Stephen that there would not be side effects of that approach. So we're back to my initial recommendation that we require the 'layers' value [2] and require implementations to error out if they see an unknown value [3]. The use of "unknown" vs. "another" allows image and implementation authors to collaborate on additional layer types if they see a need to do so while ensuring that users not party to such extensions don't get silently-broken behavior. This relies on extention types being suitably namespaced/unique so that two separate extension groups don't pick the same type string, but that seems like a reasonably safe bet. The spec does not provide any way to version this field, so users wondering "is my tooling modern enough to handle this image and any rootfs.type extensions it may contain?" should ask their tooling to validate the image. [1]: opencontainers/image-spec#224 Subject: serialization: Drop rootfs.type (which had only one legal value) [2]: opencontainers/image-spec#211 (comment) Subject: serialization: remove windows-specific layers+base rootfs [3]: opencontainers/image-spec#211 (comment) Signed-off-by: W. Trevor King <[email protected]>
I'd rather drop the field [1], but have been unable to convince Stephen that there would not be side effects of that approach. So we're back to my initial recommendation that we require the 'layers' value [2] and require implementations to error out if they see an unknown value [3]. The use of "unknown" vs. "another" allows image and implementation authors to collaborate on additional layer types if they see a need to do so while ensuring that users not party to such extensions don't get silently-broken behavior. This relies on extention types being suitably namespaced/unique so that two separate extension groups don't pick the same type string, but that seems like a reasonably safe bet. The spec does not provide any way to version this field, so users wondering "is my tooling modern enough to handle this image and any rootfs.type extensions it may contain?" should ask their tooling to validate the image. [1]: opencontainers/image-spec#224 Subject: serialization: Drop rootfs.type (which had only one legal value) [2]: opencontainers/image-spec#211 (comment) Subject: serialization: remove windows-specific layers+base rootfs [3]: opencontainers/image-spec#211 (comment) Signed-off-by: W. Trevor King <[email protected]>
I'd rather drop the field [1], but have been unable to convince Stephen that there would not be side effects of that approach. So we're back to my initial recommendation that we require the 'layers' value [2] and require implementations to error out if they see an unknown value [3]. The use of "unknown" vs. "another" allows image and implementation authors to collaborate on additional layer types if they see a need to do so while ensuring that users not party to such extensions don't get silently-broken behavior. This relies on extention types being suitably namespaced/unique so that two separate extension groups don't pick the same type string, but that seems like a reasonably safe bet. The spec does not provide any way to version this field, so users wondering "is my tooling modern enough to handle this image and any rootfs.type extensions it may contain?" should ask their tooling to validate the image. [1]: opencontainers/image-spec#224 Subject: serialization: Drop rootfs.type (which had only one legal value) [2]: opencontainers/image-spec#211 (comment) Subject: serialization: remove windows-specific layers+base rootfs [3]: opencontainers/image-spec#211 (comment) Signed-off-by: W. Trevor King <[email protected]>
I'd rather drop the field [1], but have been unable to convince Stephen that there would not be side effects of that approach. So we're back to my initial recommendation that we require the 'layers' value [2] and require implementations to error out if they see an unknown value [3]. The use of "unknown" vs. "another" allows image and implementation authors to collaborate on additional layer types if they see a need to do so while ensuring that users not party to such extensions don't get silently-broken behavior. This relies on extention types being suitably namespaced/unique so that two separate extension groups don't pick the same type string, but that seems like a reasonably safe bet. The spec does not provide any way to version this field, so users wondering "is my tooling modern enough to handle this image and any rootfs.type extensions it may contain?" should ask their tooling to validate the image. [1]: opencontainers/image-spec#224 Subject: serialization: Drop rootfs.type (which had only one legal value) [2]: opencontainers/image-spec#211 (comment) Subject: serialization: remove windows-specific layers+base rootfs [3]: opencontainers/image-spec#211 (comment) Signed-off-by: W. Trevor King <[email protected]>
I'd rather drop the field [1], but have been unable to convince Stephen that there would not be side effects of that approach. So we're back to my initial recommendation that we require the 'layers' value [2] and require implementations to error out if they see an unknown value [3]. The use of "unknown" vs. "another" allows image and implementation authors to collaborate on additional layer types if they see a need to do so while ensuring that users not party to such extensions don't get silently-broken behavior. This relies on extention types being suitably namespaced/unique so that two separate extension groups don't pick the same type string, but that seems like a reasonably safe bet. The spec does not provide any way to version this field, so users wondering "is my tooling modern enough to handle this image and any rootfs.type extensions it may contain?" should ask their tooling to validate the image. [1]: opencontainers/image-spec#224 Subject: serialization: Drop rootfs.type (which had only one legal value) [2]: opencontainers/image-spec#211 (comment) Subject: serialization: remove windows-specific layers+base rootfs [3]: opencontainers/image-spec#211 (comment) Signed-off-by: W. Trevor King <[email protected]>
This special case was added in 540c8e9 to handle situations where a Windows daemon would still have images in legacy format on Disk. For legal reasons, Windows base-images were not allowed to be distributed through registries, and because of that had to be pre-loaded on the host. Such images would have a RootFS with a special `layers+base` type. This type is no longer used and [removed from the OCI image spec][1], which now only allows a single type ("Layers"); from the [OCI image-spec][2]: > - **rootfs** _object_, REQUIRED > > The rootfs key references the layer content addresses used by the image. > This makes the image config hash depend on the filesystem hash. > > - **type** _string_, REQUIRED > > MUST be set to `layers`. > Implementations MUST generate an error if they encounter a unknown value while verifying or unpacking an image. The special handling was added in 2016 to help in the transition, but it's very unlikely such images still exist, so we can remove the special handling. This reverts commit 540c8e9. [1]: opencontainers/image-spec#211 [2]: https://github.com/opencontainers/image-spec/blob/v1.1.1/config.md#properties Signed-off-by: Sebastiaan van Stijn <[email protected]>
5 participants
The
rootfs.typefield previously allowed atypeoflayers+basetosignify that there the layers must be applied to a base image. This was
required to support windows containers in Docker initially but has since
been replaced with a more compatible mechanism. It has no relevence for
OCI.
Signed-off-by: Stephen J Day [email protected]
Closes #186.
@opencontainers/image-spec-maintainers PTAL