update test vendor to image-spec rc5 #241

runcom · 2017-02-25T10:40:46Z

Signed-off-by: Antonio Murdaca runcom@redhat.com

runcom · 2017-02-25T10:42:02Z

This is WIP since I need to rework the OCI transports for RC5 - just a placeholder for now

cyphar · 2017-03-03T05:12:36Z

@runcom We need to talk about how we are going to handle references given the discussion I've had upstream especially with opencontainers/image-spec#588 -- effectively the way it should be handled is like SQL queries.

Hopefully oci/layout can be implemented using umoci's libraries so you don't have to reimplement it.

runcom · 2017-03-03T10:45:38Z

Hopefully oci/layout can be implemented using umoci's libraries so you don't have to reimplement it.

mmm I don't understand why we went for using another library (umoci's) for OCI support in c/image (which is already a library)
but ok...OCI support will probably drop from c/image eventually (I guess) and skopeo can directly use umoci's library

mtrmac · 2017-03-03T20:05:30Z

@runcom We need to talk about how we are going to handle references given the discussion I've had upstream especially with opencontainers/image-spec#588 -- effectively the way it should be handled is like SQL queries.

(*sigh* that seems rather overengineered and very ambiguous (how does one do matching on os.version, or variant (is arm8 ≥ arm6l?); oh well, it does define enough to be usable I guess.)

So far we’ve handled the similar docker schema2 functionality in image/docker_list.go, i.e. when reading the image, we resolve the list into a single manifest at the earliest opportunity. But that’s all that is supported, there is no support for manifest list destinations, and copy.go explicitly refuses to copy multi-images.

but ok...OCI support will probably drop from c/image eventually (I guess) and skopeo can directly use umoci's library

I don’t particularly care about how the OCI transport is implemented (e.g. I would be perfectly fine with a set of one-liners calling umoci), but it does seem valuable to me to have OCI go through the types.ImageTransport abstraction so that signatures and the unified copy pipeline etc. can be used, and so that skopeo does not have to build a private abstraction over containers/image and umoci; that would be a bit ridiculous.

erikh · 2017-03-03T20:08:54Z

If I may jump in I think there's a valuable discussion in making containers/image (and perhaps parts of containers/storage) bank off of umoci for many of its operations. I spoke to @cyphar over twitter about making this a thing that could happen, the editing features I need are coming to umoci and the interface in containers/image from a lib perspective is very nice. Perhaps getting in a position where we can consume umoci as a lib would be the best option? I am very, very happy to do this work if desired.

…

On Fri, Mar 3, 2017 at 12:05 PM, Miloslav Trmač ***@***.***> wrote: @runcom <https://github.com/runcom> We need to talk about how we are going to handle references given the discussion I've had upstream especially with opencontainers/image-spec#588 <opencontainers/image-spec#588> -- effectively the way it should be handled is like SQL queries. (*sigh* that seems rather overengineered and very ambiguous (how does one do matching on os.version, or variant (is arm8 ≥ arm6l?); oh well, it does define enough to be usable I guess.) So far we’ve handled the similar docker schema2 functionality in image/docker_list.go, i.e. when reading the image, we resolve the list into a single manifest at the earliest opportunity. But that’s all that is supported, there is no support for manifest list destinations, and copy.go explicitly refuses to copy multi-images. but ok...OCI support will probably drop from c/image eventually (I guess) and skopeo can directly use umoci's library I don’t particularly care about how the OCI transport is implemented (e.g. I would be perfectly fine with a set of one-liners calling umoci), but it does seem valuable to me to have OCI go through the types.ImageTransport abstraction so that signatures and the unified copy pipeline etc. can be used, and so that skopeo does not have to build a private abstraction over containers/image and umoci; that would be a bit ridiculous. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#241 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ69FlCuSZcLsPnfFYppLrB-1a9KWQks5riHKKgaJpZM4ML-f9> .

erikh · 2017-03-03T20:11:04Z

I have code to do a lot of what umoci does for docker images, and have outstanding patches like moby/moby#28923 that can finish the work for docker-specific image work.

…

On Fri, Mar 3, 2017 at 12:08 PM, Erik Hollensbe ***@***.***> wrote: If I may jump in I think there's a valuable discussion in making containers/image (and perhaps parts of containers/storage) bank off of umoci for many of its operations. I spoke to @cyphar over twitter about making this a thing that could happen, the editing features I need are coming to umoci and the interface in containers/image from a lib perspective is very nice. Perhaps getting in a position where we can consume umoci as a lib would be the best option? I am very, very happy to do this work if desired. On Fri, Mar 3, 2017 at 12:05 PM, Miloslav Trmač ***@***.***> wrote: > @runcom <https://github.com/runcom> We need to talk about how we are > going to handle references given the discussion I've had upstream > especially with opencontainers/image-spec#588 > <opencontainers/image-spec#588> -- effectively > the way it should be handled is like SQL queries. > > (*sigh* that seems rather overengineered and very ambiguous (how does one > do matching on os.version, or variant (is arm8 ≥ arm6l?); oh well, it > does define enough to be usable I guess.) > > So far we’ve handled the similar docker schema2 functionality in > image/docker_list.go, i.e. when reading the image, we resolve the list > into a single manifest at the earliest opportunity. But that’s all that is > supported, there is no support for manifest list destinations, and > copy.go explicitly refuses to copy multi-images. > > but ok...OCI support will probably drop from c/image eventually (I guess) > and skopeo can directly use umoci's library > > I don’t particularly care about how the OCI transport is implemented > (e.g. I would be perfectly fine with a set of one-liners calling umoci), > but it does seem valuable to me to have OCI go through the > types.ImageTransport abstraction so that signatures and the unified copy > pipeline etc. can be used, and so that skopeo does not have to build a > private abstraction over containers/image and umoci; that would be a bit > ridiculous. > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > <#241 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AABJ69FlCuSZcLsPnfFYppLrB-1a9KWQks5riHKKgaJpZM4ML-f9> > . >

mtrmac · 2017-03-03T20:30:45Z

If I may jump in I think there's a valuable discussion in making containers/image (and perhaps parts of containers/storage) bank off of umoci for many of its operations.

Honestly I don’t know all that much about the wider runc / OCI / … ecosystem¹, @runcom ?

My guess after a five-minute look at umoci is that c/image currently tends to deal with images as a single ~immutable unit, so converting between image formats and probably config formats makes sense as part of that, but editing layers and especially individual files inside is conceptually somewhat different (notably it really needs a semi-persistent staging area for in-progress multi-step edits). That’s not to say that the two shouldn’t be very easily interoperable, or they may even be a single repo/project—at the very least types.ImageReference or something like that would be a nice thing to share.

But then…
¹ I came here for signatures; source/destination abstractions, copy.Image, and format conversions all just grew from that and were not originally anticipated.

Really, the project will grow in whichever way contributors want to take it :)

Actually the containers-storage: transport is already somewhat awkward to handle using our abstraction (assuming that a layer put into an ImageDestination can be copied back out from an ImageSource unmodified, which is not true for containers-storage), and if I understand umoci correctly, having containers/image know about “local extracted images” (which are not image-at-a-time sources/destinations) might make all of this much clearer.

Perhaps the copy abstraction which completely hides the difference between “push”/“pull”/“copy between servers” is just untenable…

* mitr shuts up now to stop displaying even more of his ignorance.

erikh · 2017-03-04T08:46:48Z

I dunno. It just seems like there's a ton of overlap and that maybe a sink for some of these core image/storage operations would be best. Perhaps I'm too eager to unify something that doesn't need to be.

…

On Fri, Mar 3, 2017 at 12:30 PM, Miloslav Trmač ***@***.***> wrote: If I may jump in I think there's a valuable discussion in making containers/image (and perhaps parts of containers/storage) bank off of umoci for many of its operations. Honestly I don’t know all that much about the wider runc / OCI / … ecosystem¹, @runcom <https://github.com/runcom> ? My *guess* after a five-minute look at umoci is that c/image currently tends to deal with images as a single ~immutable unit, so converting between image formats and probably config formats makes sense as part of that, but editing layers and especially individual files inside is conceptually somewhat different (notably it really needs a semi-persistent staging area for in-progress multi-step edits). That’s not to say that the two shouldn’t be very easily interoperable, or they may even be a single repo/project—at the very least types.ImageReference or something like that would be a nice thing to share. But then… ¹ I came here for signatures; source/destination abstractions, copy.Image, and format conversions all just grew from that and were not originally anticipated. Really, the project will grow in whichever way contributors want to take it :) Actually the containers-storage: transport is already somewhat awkward to handle using our abstraction (assuming that a layer put into an ImageDestination can be copied back out from an ImageSource unmodified, which is not true for containers-storage), and if I understand umoci correctly, having containers/image know about “local extracted images” (which are not image-at-a-time sources/destinations) might make all of this much clearer. Perhaps the copy abstraction which completely hides the difference between “push”/“pull”/“copy between servers” is just untenable… * mitr shuts up now to stop displaying even more of his ignorance. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#241 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ6yU43OlG0EyNGKzM8wLzbQQyeNvYks5riHh2gaJpZM4ML-f9> .

cyphar · 2017-03-04T14:37:04Z

@mtrmac

(sigh that seems rather overengineered and very ambiguous (how does one do matching on os.version, or variant (is arm8 ≥ arm6l?); oh well, it does define enough to be usable I guess.)

If you'd like to continue the good fight within the spec, that's your decision. Stephen Day made clear that in his opinion the tagging system of Docker (and thus all of the similar systems) is too simple and that a tagging system is much more useful for people.

I don’t particularly care about how the OCI transport is implemented (e.g. I would be perfectly fine with a set of one-liners calling umoci)

umoci implements a set of libraries, so you wouldn't be using it as a binary. Effectively oci/layout would become oci/ and it would just be a thin wrapper around umoci's libraries.

but editing layers and especially individual files inside is conceptually somewhat different (notably it really needs a semi-persistent staging area for in-progress multi-step edits).

That's the front-end of umoci. The libraries don't have that requirement (unless you're generating a new diff layer, which requires you to have a directory with files in it that can be used to source metadata and data). I'm not anticipating that c/image requires everything that umoci implements -- I only really expect that c/image needs oci/cas and oci/casext.

cyphar · 2017-03-04T14:38:51Z

@runcom

mmm I don't understand why we went for using another library (umoci's) for OCI support in c/image (which is already a library)

So you don't have to reimplement the OCI CAS interface. I mean, it's up to you ultimately, but I'm a bit concerned that skopeo is going to end up with an incompatible reference interface to umoci (which, aside from being bad for the OCI community, will actually impact us at SUSE). As long as we have discussions on how the reference API should be handled, I don't mind all that much whether you re-use my work or reimplement it.

mtrmac · 2017-03-06T15:48:18Z

(sigh that seems rather overengineered and very ambiguous (how does one do matching on os.version, or variant (is arm8 ≥ arm6l?); oh well, it does define enough to be usable I guess.)

If you'd like to continue the good fight within the spec, that's your decision.

I'm afraid I can’t spend time on that now. It does seem that a subset of that is easy enough to use in portable code, so I guess that’s what will end up happening… Or, *shrug*, I could just be wrong and this could be the right thing to do.

I don’t particularly care about how the OCI transport is implemented (e.g. I would be perfectly fine with a set of one-liners calling umoci)

umoci implements a set of libraries, so you wouldn't be using it as a binary. Effectively oci/layout would become oci/ and it would just be a thin wrapper around umoci's libraries.

Sure, one-liners calling a library. Executing a binary would be more lines :)

but editing layers and especially individual files inside is conceptually somewhat different (notably it really needs a semi-persistent staging area for in-progress multi-step edits).

That's the front-end of umoci. The libraries don't have that requirement (unless you're generating a new diff layer, which requires you to have a directory with files in it that can be used to source metadata and data). I'm not anticipating that c/image requires everything that umoci implements -- I only really expect that c/image needs oci/cas and oci/casext.

Ah, OK. That is indeed much closer to the oci: transport in this repo. I have no opinion on implementing the oci: transport through umoci, or not, in that case; whichever ends up being easier and more maintainable.

lucab · 2017-03-08T15:29:28Z

@runcom barring the unfortunate refs-as-annotations situation in -rc5 and the single test failing in here, is there anything else blocking this? I did a quick roundtrip of this through skopeo and seems fine on the surface, but I may be missing some details.

lucab · 2017-04-28T12:52:07Z

Bump on my last comment.

runcom · 2017-04-28T12:57:50Z

@lucab, not sure, I was waiting on @cyphar I guess

runcom · 2017-04-28T12:57:58Z

I'll fix the test meanwhile

cyphar · 2017-04-28T13:19:11Z

I've got a branch in umoci for this (https://github.com/openSUSE/umoci/tree/imagespec-v1.0.0-rc5) that I haven't worked on for a couple of days (mainly because I'm working on some other related image-spec stuff related to distribution). I'm going to need to finish it ASAP in order to implement some of the stuff I'm planning in distribution, but I wanted to wait to meet with @steveooe first (which I did at DockerCon though we didn't get a chance to run through the walker implementation).

Basically I think the best way of moving forward is that skopeo implement the most "simple" tagging implementation (which can be done with the current state of my umoci branch's OCI CAS library) -- more complicated manifest trees aren't necessary for the "simple" ca se. There are some complications with "updating" a tag, but umoci's updating implementation will probably never really want to work with non-unique tags (we'll see though).

The kinda nice thing about rc5 is that I can implement references in casext (so references are implemented completely separately from the CAS and you can implement different reference resolution methods externally). I still need to make casext more interface-friendly though...

cyphar · 2017-04-28T13:20:02Z

I definitely have put umoci integration on my list of things that "would be a good thing" in containers/image though. When I get a chance I will actually write that code.

runcom · 2017-04-29T02:13:27Z

@cyphar sorry, I didn't get if this PR still stand on his own and could be moved forward

mtrmac · 2017-04-29T02:20:27Z

manifest/manifest.go

@@ -54,7 +54,7 @@ func GuessMIMEType(manifest []byte) string {
 	}

 	switch meta.MediaType {
-	case DockerV2Schema2MediaType, DockerV2ListMediaType, imgspecv1.MediaTypeImageManifest, imgspecv1.MediaTypeImageManifestList: // A recognized type.
+	case DockerV2Schema2MediaType, DockerV2ListMediaType: // A recognized type.


I vaguely remember there was an earlier conversation about this, but I can't remember the details: why is guessing that a manifest is OCI no longer applicable? Because a manifest can never be a stand-alone object with OCi without an index now? Or just because OCI decided not to include the type in the JSON?

No mime type in Json anymore

To expand on the above, it's because types are now implemented using descriptor types (so objects are no longer self-descriptive).

Either way, this is awkward.

dirImageSource and storageImageSource depend on GuessMIMEType to work; otherwise c/i/image assumes schema1. (Not trivially fixable.)

dockerImageDestination depends on GuessMIMEType for setting Content-Type on upload. (This one is probably fixable by adding an explicit parameter, and using the c/i/image handler to get the type we are using.)

Is there another heuristic we can use? e.g. schemaVersion == 2 && mediaType missing? Or perhaps together with config.mediaType?

fixed I think

Thanks. Could the if meta.SchemaVersion == 2 code be unified with case 2 below?

runcom · 2017-05-01T16:39:56Z

ping @cyphar about #241 (comment)

cyphar

Looks good as a first step for v1.0.0-rc5. My main worry was inconsistency with other referencing implementations but after some discussions with Steven the main point is that skopeo doesn't need to support all possible image structures (though umoci will try to implement that in a more unopinionated way).

cyphar · 2017-05-01T17:06:10Z

oci/layout/oci_transport.go

+			d = &md
+			break
+		}
+	}


Yeah, this logic works for the basic case and was the main thing I was worried about. Once I have a nice implementation in umoci, skopeo should be able to handle more complicated cases.

lucab · 2017-05-02T08:23:34Z

Out of partially related curiosity, what's the golang libraries plan to deal with opencontainers/image-spec#581 (comment)?

runcom · 2017-05-03T09:23:43Z

Tests fixed and rebased, please take a look @mtrmac @cyphar @lucab

mtrmac

I’d be much happier with GuestManifestMIMEType working as expected. I guess I could be persuaded to give up on that, but it seems fairly possible to handle…

mtrmac · 2017-05-03T19:29:18Z

oci/layout/oci_dest.go

+	annotations := make(map[string]string)
+	annotations["org.opencontainers.ref.name"] = d.ref.tag
+	desc.Annotations = annotations
+	d.index.Manifests = append(d.index.Manifests, imgspecv1.ManifestDescriptor{


This gives a bit of an impression that the code adds more manifests to an existing index, but we actually never read the original file and blindly overwrite it if it exists.

I guess the intent is to perhaps add the “update existing index” over time? If so, this code does make enough sense; if not, and the intent is to always do a simple overwrite, the code structure could probably be a bit simpler (do nothing in Commit, just build a single ImageIndex local variable in PutManifest).

yup, the intent is to "update existing index"

yup, the intent is to "update existing index"

As in, with this PR, or at some later time? Because it is not updating an existing index now.

if you call PutManifest many times and at the end Commit you'll have a multi-manifest index with this code, am I stupidly missing something?

Ah, true. I wasn’t considering that because we have no tools to do that (and the semantics of doing that + PutSignatures is unclear — what manifest are the signatures attached to?).

Anyway, that can be useful for someone who explicitly expects that oci: behavior, and it doesn’t break anything, so fair enough.

mtrmac · 2017-05-03T19:32:59Z

oci/layout/oci_transport.go

+	if err != nil {
+		return nil, err
+	}
+	src := newImageSource(ref, d)


This duplication between NewImage and NewImageSource rather suggests that getManifestDescriptor should be called within newImageSource instead of supplied from the outside.

(That still allows getManifestDescriptor to be a method on ociReference if you want.)

mtrmac · 2017-05-03T19:36:51Z

oci/layout/oci_transport.go

+	}
+	defer indexJSON.Close()
+	index := imgspecv1.ImageIndex{}
+	if err := json.NewDecoder(indexJSON).Decode(&index); err != nil {


This is shorter than ReadAll + Unmarshal, but doesn’t it also silently accept garbage (or a valid JSON?) after the processed object? And does that matter? (Quite possibly it doesn’t…)

shouldn't matter afaict

mtrmac · 2017-05-03T19:38:33Z

oci/layout/oci_transport_test.go

@@ -239,6 +258,14 @@ func TestReferenceOCILayoutPath(t *testing.T) {
 	assert.Equal(t, tmpDir+"/oci-layout", ociRef.ociLayoutPath())
 }

+func TestReferenceIndexJSON(t *testing.T) {


Should this be TestReferenceIndexPath?

runcom · 2017-05-08T07:55:46Z

@mtrmac PTAL

mtrmac · 2017-05-09T17:38:41Z

(skopeo needs updating as well go keep CI happy.)

runcom · 2017-05-10T08:42:14Z

@mtrmac updated skopeo as well

mtrmac · 2017-05-10T08:45:54Z

LGTM pending tests / a skopeo update which passes tests.

Signed-off-by: Antonio Murdaca <runcom@redhat.com>

runcom · 2017-05-10T09:11:57Z

LGTM pending skopeo tests

runcom changed the title ~~update test vendor to image-spec rc5~~ [WIP] update test vendor to image-spec rc5 Feb 25, 2017

runcom mentioned this pull request Feb 27, 2017

oci: update to image-spec v1 RC5 containers/skopeo#313

Merged

runcom force-pushed the image-spec-rc5 branch 2 times, most recently from 5acde2a to 6fac654 Compare February 27, 2017 19:35

mtrmac mentioned this pull request Mar 2, 2017

Revert "manifest: remove not needed mediatype" #245

Merged

mtrmac reviewed Apr 29, 2017

View reviewed changes

cyphar approved these changes May 1, 2017

View reviewed changes

runcom force-pushed the image-spec-rc5 branch from 6fac654 to 783cf03 Compare May 3, 2017 09:23

mtrmac reviewed May 3, 2017

View reviewed changes

runcom force-pushed the image-spec-rc5 branch from 783cf03 to cf7006c Compare May 8, 2017 07:55

runcom force-pushed the image-spec-rc5 branch from cf7006c to ff7c88b Compare May 8, 2017 07:56

runcom force-pushed the image-spec-rc5 branch from ff7c88b to 5876b64 Compare May 10, 2017 08:40

runcom changed the title ~~[WIP] update test vendor to image-spec rc5~~ update test vendor to image-spec rc5 May 10, 2017

runcom added 2 commits May 10, 2017 11:11

oci: layout: update to image-spec RC5

df66618

Signed-off-by: Antonio Murdaca <runcom@redhat.com>

oci: layout: write oci layout file in Commit

088dc7d

Signed-off-by: Antonio Murdaca <runcom@redhat.com>

runcom force-pushed the image-spec-rc5 branch from 5876b64 to 088dc7d Compare May 10, 2017 09:11

runcom merged commit b25e6dd into containers:master May 10, 2017

runcom deleted the image-spec-rc5 branch May 10, 2017 09:34

update test vendor to image-spec rc5 #241

update test vendor to image-spec rc5 #241

Conversation

runcom commented Feb 25, 2017

runcom commented Feb 25, 2017

cyphar commented Mar 3, 2017

runcom commented Mar 3, 2017

mtrmac commented Mar 3, 2017

erikh commented Mar 3, 2017 via email

erikh commented Mar 3, 2017 via email

mtrmac commented Mar 3, 2017

erikh commented Mar 4, 2017 via email

cyphar commented Mar 4, 2017

cyphar commented Mar 4, 2017

mtrmac commented Mar 6, 2017

lucab commented Mar 8, 2017

lucab commented Apr 28, 2017

runcom commented Apr 28, 2017

runcom commented Apr 28, 2017

cyphar commented Apr 28, 2017

cyphar commented Apr 28, 2017

runcom commented Apr 29, 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

runcom commented May 1, 2017

cyphar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucab commented May 2, 2017 • edited Loading

runcom commented May 3, 2017

mtrmac left a 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

runcom commented May 8, 2017

mtrmac commented May 9, 2017

runcom commented May 10, 2017

mtrmac commented May 10, 2017 • edited by runcom Loading

runcom commented May 10, 2017 • edited Loading

lucab commented May 2, 2017 •

edited

Loading

mtrmac commented May 10, 2017 •

edited by runcom

Loading

runcom commented May 10, 2017 •

edited

Loading