image-layout: ./refs/ -> index.json #533

vbatts · 2017-01-26T22:04:27Z

For media that pertains to an image, like AppStream metadata, may need to accompany a manifest-list, but is not a descriptor that points to a manifest (i.e. and XML file).

This enables non-manifest descriptors.

From the 2017-01-25 call we agreed to keep the manifest list as the entry point for accessing objects. The index concept is only a more generic use of the manifest-list. #438 (comment)

This change uses the manifest-list as an index that allows implementations to work around needing to walk/traverse the ./refs/ directories.

Obsoletes #438

vbatts · 2017-01-26T22:05:13Z

@opencontainers/image-spec-maintainers PTAL

stevvooe · 2017-01-26T22:34:49Z

image-layout.md

+  "elements": [
+    {
+      "mediaType": "application/xml",
+      "Asize": 7143,


Oh hah. I was messing with the schema validation... Clearly it works :-/

stevvooe · 2017-01-26T22:35:02Z

image-layout.md

+      }
+    }
+  ],
+  "elements": [


elements?

Elements of the index. Elements of the image.
"Channels" felt too much like providing alternate paths or manifests, which is not the point. "descriptors" seems obvious but also a bit redundant.

Ok, extensions?

(let's explore a few...)

My personal favorite is descriptors. It's boring and redundant, but obviously correct and it is very clearly not implying any semantic meaning about the content.

stevvooe · 2017-01-26T22:36:51Z

needing to walk/traverse the ./refs

Doesn't this obsolete ./refs?

Were we going to change the name of manifest-list or do you want to do that in a follow up?

wking · 2017-01-26T23:48:51Z

image-layout.md

@@ -156,4 +161,48 @@ $ cat ./blobs/sha256/e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7f
 [tar stream]
 ```

+## index.json
+
+This OPTIONAL file is for use indexing references and elements of the image-layout, where walking the `refs` and `blobs` is sub-optimal.


The index file has no effect on whether you need to or can walk blobs. Performance issues aside, it is a drop-in replacement for refs, except that the previous ref names are gone. So I think we should remove the blobs reference from this line.

Fair.
For the name, that is why I made up the example

"annotations": { "org.opencontainers.ref.name": "v1.0" }

For the name, that is why I made up the example [org.opencontainers.ref.name]

That works as a ref-name replacement, but if we actually want to use it (e.g. in image-tools), I think we want to define it here.

yes. I didn't do that because it was not my intention to anihilate the ./refs/ directory.
@stevvooe thoughts?

I didn't do that because it was not my intention to anihilate the ./refs/ directory.

Defining something like org.opencontainers.ref.name lets consumers use the index to access content by name (e.g. oci-unpack --ref v1.0 foo/ bar/) vs. having callers access it by annotation (e.g. oci-unpack --annotation org.opencontainers.ref.name=v1.0 foo/ bar/). But being able to use the index to access content by name doesn't mean you have to drop refs/ (which is another way to access content by name).

updated.

You're still referencing blobs in this line (which is what started the sub-thread on this line).

understandable

wking · 2017-01-26T23:50:50Z

image-layout.md

+```json,title=Manifest%20List&mediatype=application/vnd.oci.image.manifest.list.v1%2Bjson
+{
+  "schemaVersion": 2,
+  "mediaType": "application/vnd.oci.image.manifest.list.v1+json",


This line can go away (see #531 and #411).

Yeah... I was wrestling with the schema validation which is requiring it

wking · 2017-01-26T23:52:24Z

manifest-list.md

@@ -11,7 +11,8 @@ For the media type(s) that this document is compatible with, see the [matrix][ma
 - **`schemaVersion`** *int*

  This REQUIRED property specifies the image manifest schema version.
-  For this version of the specification, this MUST be `2` to ensure backward compatibility with older versions of Docker. The value of this field will not change. This field MAY be removed in a future version of the specification.
+  For this version of the specification, this MUST be `2` to ensure backward compatibility with older versions of Docker.
+  The value of this field will not change. This field MAY be removed in a future version of the specification.


If you're trying to break this into one sentence per line, you'll want to put another newline in between “change.” and “This”. But you can probably make this PR without touching the schemaVersion definition at all.

Missed this. I'll pick it up in a second

wking · 2017-01-26T23:55:00Z

manifest-list.md

@@ -108,5 +116,5 @@ For the media type(s) that this document is compatible with, see the [matrix][ma
 }
 ```

-[runtime-platform2]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc2/config.md#platform
+[runtime-platform2]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc3/config.md#platform


If we bump this to rc3, we probably want to update the config reference as well. But we can probably leave the rc2 → rc3 bump out of this PR.

i've updated the other references now.

wking · 2017-01-26T23:58:02Z

schema/content-descriptor.json

+    },
+    "annotations": {
+      "id": "https://opencontainers.org/schema/image/descriptor/annotations",
+      "$ref": "defs-image.json#/definitions/annotations"


This catches us up with #501 (and it needs an fs.go bump too). With the spec change already hashed out in #501, this can probably get spun off into its own PR and landed faster than the index pivot as a whole.

i've updated the fs.go now. But what needs to get spun off?

Nothing needs to get spun off. But adding annotations here is independent of the rest of this PR, so you could spin this whole hunk off into a separate PR if you wanted more, smaller PRs (vs. fewer, bigger PRs).

oh right. It could be independent. This PR makes an example of it though. So I fixed what I saw.

wking · 2017-01-27T00:01:35Z

schema/defs-image.json

@@ -11,14 +11,47 @@
      "type": "string",
      "pattern": "^[a-z0-9_+.-]+:[a-f0-9]+$"
    },
+    "elementDescriptor": {


Do you need this type (which drops urls?). I'd expect you could just use:

"$ref": "content-descriptor.json"

in the elements array (or whatever that ends up being called).

I tried that to no avail. Clearly there is some schema attention needed

vbatts · 2017-01-27T15:02:55Z

@stevvooe I reckon is does obsolete the ./refs, but not intentionally.
Also I will get to the rename, but want Ed to get this up for review first

vbatts · 2017-01-27T20:53:56Z

@stevvooe I think doing the manifest-list name and media-type change can happen in a separate thread than this PR.

vbatts · 2017-01-27T20:54:08Z

Alrighty, y'all. PTAL

wking · 2017-01-27T21:47:25Z

schema/fs.go

+`,
+	},
+
+	"/.defs-image.json.swp": {


Local editor stuff? Maybe we should -include=.*\.json instead of our current -ignore=.*go?

the -include did not do it. had to add another -ignore. fixed now.

the -include did not do it.

It worked for me (although I had to add a trailing $ to my earlier guess). I've filed the change as #538.

wking · 2017-01-27T21:51:23Z

manifest-list.md

@@ -11,7 +11,9 @@ For the media type(s) that this document is compatible with, see the [matrix][ma
 - **`schemaVersion`** *int*

  This REQUIRED property specifies the image manifest schema version.
-  For this version of the specification, this MUST be `2` to ensure backward compatibility with older versions of Docker. The value of this field will not change. This field MAY be removed in a future version of the specification.
+  For this version of the specification, this MUST be `2` to ensure backward compatibility with older versions of Docker.
+  The value of this field will not change


nit: missing a trailing period.

wking · 2017-01-27T21:56:19Z

manifest-list.md

@@ -64,6 +66,13 @@ For the media type(s) that this document is compatible with, see the [matrix][ma

        This OPTIONAL property specifies an array of strings, each specifying a mandatory CPU feature (for example `sse4` or `aes`).

+- **`elements`** *array of objects*
+
+  This OPTIONAL property contains a list of non-manifest objects.


In the “using the object as an index” use case, the entries here are likely to be manifests and manifest lists. In fact, folks using a manifest-list to switch between platforms could have an index where multiple elements (or whatever we end up calling them) have the same org.opencontainers.ref.name but differ in other metadata. That would shorten index → manifest-list → manifest to index → manifest. So I'd recommend something more generic, like:

This OPTIONAL property contains an array of descriptors. No restrictions on the referenced content are implied.

there is still the manifests field. Why would manifest and manifest-lists be put into this elements array?

there is still the manifests field. Why would manifest and manifest-lists be put into this elements array?

Maybe I'm not understanding right. I'd assumed that elements would be used when folks were looking for a single-file refs/ alternative and that manifests would only be used for traditional manifest-list stuff (e.g. selecting the best manifest based on platform matching) during some hopefully-short deprecation phase. In that case, the index would have an elements entry pointing at a manifest whenever you create a named ref for a manifest. And eventually, folks would only use elements and we could drop manifests.

But it sounds like you are intending all descriptors pointing at application/vnd.oci.image.manifest.v1+json go into manifests and everything else would go into elements? That works, I guess, although I'm not sure how you'd identify non-OCI types which count as manifests (e.g. a third-party application/vnd.foo.manifest).

So, the manifests array is intended for application/vnd.oci.image.manifest.v1+json and application/vnd.oci.image.manifest.list.v1+json. Any distinguishing of non-OCI manifest lists ... I'm not real sure. This field is also what Docker uses presently for their manifest-list and therefore needs some backwards-compat consideration.

This elements (or whatever) is intended for additional metadata like AppStream or other relevant repo-metadata (say XML or sqlite objects) that are facilitating this image-layout.

I would think it much cleaner to just have a single field like descriptors where they all live. Then this desire to distinguish goes away.

I would think it much cleaner to just have a single field like descriptors where they all live. Then this desire to distinguish goes away.

I'm not sure we can make this work. Migrating these top-level types are a huge pain and I'm not sure we gain anything except naming. I'd prefer to call this extensions, as they seem like extensions.

I don't follow why these things can't just go into the manifests list? We already have a namespaced mediaType for all of the descriptors so adding an extension field doesn't really buy much AFAICS.

wking · 2017-01-27T21:57:50Z

image-layout.md

@@ -156,4 +161,48 @@ $ cat ./blobs/sha256/e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7f
 [tar stream]
 ```

+## index.json
+
+This OPTIONAL file is for use indexing references and elements of the image-layout, where walking the `refs` and `blobs` is sub-optimal.


updated.

You're still referencing blobs in this line (which is what started the sub-thread on this line).

wking · 2017-01-27T21:58:56Z

image-layout.md

+./oci-layout
+./blobs/sha256/3588d02542238316759cbf24502f4344ffcc8a60c803870022f335d1390c13b4
+./blobs/sha256/4b0bc1c4050b03c95ef2a8e36e25feac42fd31283e8c30b3ee5df6b043155d3c
+./blobs/sha256/7968321274dc6b6171697c33df7815310468e694ac5be0ec03ff053bb135e768


Probably keep this alphabetized.

fair, but it's the output of find . -type f

updated the blobs reference too.

vbatts · 2017-01-28T02:50:28Z

updated.

To avoid accidentally including .swp, .un~, etc. [1]. Test with: $ touch schema/.defs-image.json.swp $ touch schema/.content-descriptor.json.un~ $ touch touch schema/foojson $ make schema-fs $ grep '"/' schema/fs.go "/config-schema.json": { "/content-descriptor.json": { "/defs-config.json": { "/defs-image.json": { "/defs.json": { "/image-layout-schema.json": { "/image-manifest-schema.json": { "/manifest-list-schema.json": { "/": { local: "/", [1]: opencontainers#533 (comment) Signed-off-by: W. Trevor King <[email protected]> Conflicts: schema/gen.go

To avoid accidentally including .swp, .un~, etc. [1]. Test with: $ touch schema/.defs-image.json.swp $ touch schema/.content-descriptor.json.un~ $ touch schema/foojson $ make schema-fs $ grep '"/' schema/fs.go "/config-schema.json": { "/content-descriptor.json": { "/defs-config.json": { "/defs-image.json": { "/defs.json": { "/image-layout-schema.json": { "/image-manifest-schema.json": { "/manifest-list-schema.json": { "/": { local: "/", [1]: opencontainers#533 (comment) Signed-off-by: W. Trevor King <[email protected]>

wking · 2017-01-28T05:18:07Z

schema/defs-image.json

@@ -78,6 +77,17 @@
              }
            }
          }
+        },
+        "annotations": {
+          "id": "https://opencontainers.org/schema/image/annotations",


This has the same id as the defintion below (here in master). It would be more DRY to reference that here:

"annotations": { "$ref": "#/definitions/annotations" }

wking · 2017-01-28T14:54:50Z

Makefile

@@ -76,12 +76,15 @@ $(OUTPUT_DIRNAME)/$(DOC_FILENAME).html: $(DOC_FILES) $(FIGURE_FILES)
 	ls -sh $(shell readlink -f $@)
 endif

-validate-examples:
+validate-examples: schema/fs.go


Travis doesn't actually call validate-examples, and it's not a dependency of the test target either. If we're trying to rebuild schema/fs.go before any tests, you probably need to have:

test: schema/fs.go

as well.

then i'll add validate-examples as a travis target.

then i'll add validate-examples as a travis target.

That will double those tests, since make test, which we already call, also runs the example tests. Other make test tests depend on fs.go as well, so I think you can leave Travis alone and add the fs.go dependency to test.

oh i see. ok.

maybe schema/fs.go doesn't even need to be commited then ... only generated. It often causes a lot of senseless rebasing.

wking · 2017-01-28T14:56:14Z

Makefile

+schema/fs.go: $(wildcard schema/*.json) schema/gen.go
+	cd schema && printf "%s\n\n%s\n" "$$(cat ../.header)" "$$(go generate)" > fs.go
+
+.PHONY: schema-fs


Quite a few .PHONY entries are collected at the bottom of the Makefile if you want to put this entry there. On the other hand, there are still a few single-entry .PHONYs scattered throughout the Makefile, so there is precedent if you want to keep it here.

i didn't notice the phony's getting grouped. I'll update mine. I'm used to just scattering them so you can clearly read the phony next to the target it is operating on.

wking · 2017-01-28T14:57:25Z

Makefile

 	go test -run TestValidate ./schema

-schema-fs:
+schema/fs.go: $(wildcard schema/*.json) schema/gen.go


I'm not sure how well make will handle this in a fresh Git checkout. Maybe Travis should touch schema/gen.go to ensure a newer-than-fs.go mtime?

presumably, a fresh checkout wouldn't need a fresh schema. the schema-fs ought to be commited when the JSON schema changes.

the schema-fs ought to be commited when the JSON schema changes.

But that doesn't always happen (it didn't in #411). Making sure it does always happen was what I was trying to do with #530. I'd thought the approach you were aiming for here was to always rebuild in CI so it didn't matter if the committed copy was stale.

right. If travis runs the validate-examples, then it would fail CI for their PR, then they update schema-fs, commit and re-push.

The restriction on names was removed by 0556a6b (image-layout: ./refs/ -> index.json, 2017-01-26, opencontainers#533) and the replacement (org.opencontainers.ref.name) has no equivalent limitation.

To make it clear what they are now that we have no "Refs" section (which was removed in 0556a6b, image-layout: ./refs/ -> index.json, 2017-01-26, opencontainers#533). Signed-off-by: W. Trevor King <[email protected]>

The restriction on names was removed by 0556a6b (image-layout: ./refs/ -> index.json, 2017-01-26, opencontainers#533) and the replacement (org.opencontainers.ref.name) has no equivalent limitation. Signed-off-by: W. Trevor King <[email protected]>

To make it clear what they are now that we have no "Refs" section (which was removed in 0556a6b, image-layout: ./refs/ -> index.json, 2017-01-26, opencontainers#533). Signed-off-by: W. Trevor King <[email protected]>

PR opencontainers#533 removed annotations by accident. (489e00e specifically). This adds that definition back. Signed-off-by: Vincent Batts <[email protected]>

To avoid accidentally including .swp, .un~, etc. [1]. Test with: $ touch schema/.defs-image.json.swp $ touch schema/.content-descriptor.json.un~ $ touch schema/foojson $ make schema-fs $ grep '"/' schema/fs.go "/config-schema.json": { "/content-descriptor.json": { "/defs-config.json": { "/defs-image.json": { "/defs.json": { "/image-layout-schema.json": { "/image-manifest-schema.json": { "/manifest-list-schema.json": { "/": { local: "/", [1]: opencontainers#533 (comment) Signed-off-by: W. Trevor King <[email protected]>

From 0556a6b (image-layout: ./refs/ -> index.json, 2017-01-26, opencontainers#533). Signed-off-by: W. Trevor King <[email protected]>

A common use case of `org.opencontainers.ref.name` is representing a `tag` for a container image. In docker, there is some restriction of the name see https://github.com/docker/docker/blob/master/vendor/github.com/docker/distribution/reference/regexp.go#L37. Without these restrictions, I'm afraid the tag of oci image is invalid for docker. These restrictions used to exist but removed in opencontainers#533. Signed-off-by: Lei Jitang <[email protected]>

To avoid accidentally including .swp, .un~, etc. [1]. Test with: $ touch schema/.defs-image.json.swp $ touch schema/.content-descriptor.json.un~ $ touch schema/foojson $ make schema-fs $ grep '"/' schema/fs.go "/config-schema.json": { "/content-descriptor.json": { "/defs-config.json": { "/defs-image.json": { "/defs.json": { "/image-layout-schema.json": { "/image-manifest-schema.json": { "/manifest-list-schema.json": { "/": { local: "/", [1]: opencontainers/image-spec#533 (comment) Signed-off-by: W. Trevor King <[email protected]>

stevvooe reviewed Jan 26, 2017

View reviewed changes

wking reviewed Jan 26, 2017

View reviewed changes

wking reviewed Jan 27, 2017

View reviewed changes

vbatts force-pushed the manifest-list-index branch 3 times, most recently from 0f29fa3 to 2ec9dbd Compare January 27, 2017 20:11

vbatts mentioned this pull request Jan 27, 2017

.travis.yml: Rebuild schema-fs in the Travis tests #530

Closed

vbatts force-pushed the manifest-list-index branch from 2ec9dbd to 64ebe65 Compare January 27, 2017 20:50

wking reviewed Jan 27, 2017

View reviewed changes

vbatts force-pushed the manifest-list-index branch 2 times, most recently from 28e7ed9 to 539b37b Compare January 28, 2017 02:43

wking mentioned this pull request Jan 28, 2017

schema/gen: Include .json instead of excluding .go #538

Merged

wking reviewed Jan 28, 2017

View reviewed changes

wking mentioned this pull request Jan 28, 2017

Makefile: Add .install.esc and make it part of install.tools #540

Merged

wking mentioned this pull request Feb 8, 2017

*: Catch up with refs/ removal (remove RefsRegexp, etc.) #561

Merged

xiekeyang mentioned this pull request Feb 9, 2017

cmd/oci-image-tool: is having empty refs/ a valid image? opencontainers/image-tools#15

Closed

This was referenced Feb 9, 2017

manifest-list: add back annotations #565

Merged

Bump version #566

Merged

coolljt0725 mentioned this pull request Feb 15, 2017

TODO: image tool needs to update after image-layout: ./refs/ -> index.json opencontainers/image-tools#115

Closed

wking added a commit to wking/image-spec that referenced this pull request Feb 16, 2017

image-layout: Fix "auxilary" -> "auxiliary" typo

1cf8122

From 0556a6b (image-layout: ./refs/ -> index.json, 2017-01-26, opencontainers#533). Signed-off-by: W. Trevor King <[email protected]>

This was referenced Feb 16, 2017

image-layout: Fix "auxilary" -> "auxiliary" typo #574

Merged

image-index: delete the extra fields #576

Merged

coolljt0725 mentioned this pull request Feb 23, 2017

Update version opencontainers/image-tools#121

Merged

cyphar mentioned this pull request Feb 23, 2017

index.json: make org.opencontainers.image.ref.name unique? #581

Open

lucab mentioned this pull request Mar 7, 2017

OCI: update for new -rc5 image-layout format containers/build#292

Open

coolljt0725 mentioned this pull request Mar 8, 2017

Add some restriction to "org.opencontainers.ref.name" #591

Closed

vbatts mentioned this pull request Mar 9, 2017

Image metadata without affecting the CAS #59

Closed

wking mentioned this pull request Aug 2, 2017

image-layout: make index.json OPTIONAL #719

Closed

wking mentioned this pull request Aug 30, 2017

doc.go: Perfect description opencontainers/image-tools#177

Merged

wking mentioned this pull request Nov 9, 2017

[rfc] cas: cmpxchg interface? opencontainers/umoci#216

Open

tommie-lie mentioned this pull request Feb 6, 2018

Converting local files from buildah fails "Could not find image" appc/docker2aci#257

Open

image-layout: ./refs/ -> index.json #533

image-layout: ./refs/ -> index.json #533

Conversation

vbatts commented Jan 26, 2017 • edited Loading

vbatts commented Jan 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevvooe commented Jan 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbatts commented Jan 27, 2017

vbatts commented Jan 27, 2017

vbatts commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking Jan 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbatts commented Jan 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbatts commented Jan 26, 2017 •

edited

Loading

wking Jan 27, 2017 •

edited

Loading

wking Jan 28, 2017 •

edited

Loading

wking Jan 27, 2017 •

edited

Loading