Issue #155 Making sure existing OCI index gets not overwritten #326

hferentschik · 2017-08-11T18:54:50Z

Addresses issue #155

hferentschik · 2017-08-11T18:57:23Z

oci/layout/oci_transport_test.go

+	assert.Equal(t, fullyExplicitPath, ns[0])
+	assert.Equal(t, filepath.Dir(fullyExplicitPath), ns[1])
+
+	//// Test with a known path which should exist. Test just one non-canonical


This test does not work on macOS. I am not sure whether there is a directory which could be used across platform to make this work.

Yeah, this is a repeated problem with the tests I have also noticed recently. Maybe we can replace $TMPDIR with the expanded version of it (i.e. ending up with /private/var/… instead of /var/… IIRC); that should be harmless for any other uses of $TMPDIR, shouldn’t it?

… Oops, I didn’t read the related code before answering, sorry.

I hope we can find something, I can take a look at least for macOS and Linux. If someone wanted to run this on Windows, I have no idea.

The $TMPDIR issue is real, and I’d prefer a general solution to that instead of every test having magic knowledge of that like fullyExplicitPath, err := explicitfilepath.ResolvePathToFullyExplicit(tmpDir). But that’s strictly speaking unrelated to this PR.

Actually, after the /var/tmp vs. /private/var/tmp ambiguity is resolved somehow, this test can just as well use ioutil.TempDir(…) instead of relying on a pre-existing directory in the OS.

The /etc/skel problem should have been …resolved… by #336.

mtrmac

(Just a quick initial look for now, I’ll be back working in about two weeks.

The ”which manifests get overwritten” decision is the only non-trivial issue I can see right now.

mtrmac · 2017-08-12T00:26:15Z

oci/layout/oci_dest.go

+func (d *ociImageDestination) addManifest(desc *imgspecv1.Descriptor){
+	for i, manifest := range d.index.Manifests {
+		if manifest.Digest.String() == desc.Digest.String() {
+			// TODO Should there first be a cleanup based on the descriptor we are going to replace?


I guess this PR is still a clear improvement without the cleanup; but having a long-term TODO note here is nice.

fair enough

mtrmac · 2017-08-12T00:28:37Z

oci/layout/oci_dest.go

-			return err
+func (d *ociImageDestination) addManifest(desc *imgspecv1.Descriptor){
+	for i, manifest := range d.index.Manifests {
+		if manifest.Digest.String() == desc.Digest.String() {


Should this really be overwriting based on the manifest digest? If the manifest digest is the same, then the image by definition hasn’t changed (well, apart from the external metadata).

My initial impression is that this should replace images with the same org.opencontainers.image.ref.name annotation, if any; does that make sense?

Yeah, you're right @mtrmac. When you update an OCI image's "tag" you need to remove any existing descriptors in the manifest with the same org.opencontainers.image.ref.name and replace it with the new one (with that tag added). I already do this in umoci (with some more complicated handling because in umoci we mutate blobs which requires reconstructing the merkle tree).

Yes, makes sense. In fact when trying some of these things out now in Minishift (where we want to use this), I realized the problem. In fact I have a couple of questions around this.

As said, my aim is to store multiple images within the same OCI directory with the aim to share layers. Of course I need to also then be able to retrieve these images. And here is actually the rub. On a lower level, this pull request allows me to store multiple manifests to the index, but I realized now, that they won't be addressable. I think there are several reasons for that.

Let's quickly look at some example index.json which gets created:

{ "schemaVersion":2, "manifests":[ { "mediaType":"application/vnd.oci.image.manifest.v1+json", "digest":"sha256:bb1fb6e9d0509afa26e52b75bdf144faea07d2473c9d5c693427b6f09b6c3ba5", "size":823, "annotations":{ "org.opencontainers.image.ref.name":"v3.6.0" }, "platform":{ "architecture":"amd64", "os":"darwin" } } ] }

Notice, org.opencontainers.image.ref.name contains the tag of the image as it's name. Really I would expect the image name, eg openshift/origin as name and org.opencontainers.image.version should be used for the tag/version. Storing the tag in org.opencontainers.image.ref.name might work for the current single directory per image scenario, but I would think even there it is questionable.

I think the problem comes down to the ociReference struct (https://github.com/containers/image/blob/master/oci/layout/oci_transport.go#L79), as well as how an oci transport string is parsed (https://github.com/containers/image/blob/master/oci/layout/oci_transport.go#L92). Atm OCI specifies the target directory and a tag. Somehow the image name needs to get into this. Does this make sense? If so, I am wondering what the format should be for an oci reference then?

I think it comes down to how one would add the image name into the oci ref? How for example would one split between oci directory and the actual image name ,eg "oci:/:. Using '/' is problematic. How would you parse that?

I am happy to continue working on this, but do we agree that the way a oci reference is structured needs to be extended?

[See also #323 for discussion on the oci:(path, annotation) syntax.)

@cyphar

Lastly there is a question of spec interpretation. I would expect that if I want to push alpine:3.1 into an OCI index, the top level entry in the index should use 'org.opencontainers.image.ref.name' as name for the image, aka 'alpine' and 'org.opencontainers.image.ref.version' as version, aka 3.1.

This is actually a reasonable point, and AFAICT the OCI spec is silent on this. Is it reasonable / interoperable with umoci to expect users to set ….ref.name to unique values (i.e. embed Docker-like version tags in there), or should every consumer be matching on a pair of .ref.name+.ref.version, or perhaps on even more annotations, because single annotations or subsets of that are not expected to be unique?

@hferentschik What do you mean by "put alpine:3.1" in the index? Do you mean a refname of "alpine:3.1"? If so, you'd do something like (casext.Engine).AddReferences(context.Background(), "alpine:3.1", <descriptor that points to the manifest>). But I'm not fully sure what you're asking to be honest.

@mtrmac

Lastly there is a question of spec interpretation. I would expect that if I want to push alpine:3.1 into an OCI index, the top level entry in the index should use 'org.opencontainers.image.ref.name' as name for the image, aka 'alpine' and 'org.opencontainers.image.ref.version' as version, aka 3.1.

This is actually a reasonable point, and AFAICT the OCI spec is silent on this. Is it reasonable / interoperable with umoci to expect users to set ….ref.name to unique values (i.e. embed Docker-like version tags in there), or should every consumer be matching on a pair of .ref.name+.ref.version, or perhaps on even more annotations, because single annotations or subsets of that are not expected to be unique?

To be interoperable with umoci, umoci only looks at org.opencontainers.image.ref.name. It internally does support handling of multiple entries with the same name, but the UX doesn't support it (and so it does some not-very-nice things). As for the general concept, I'm not really sure which way is better. While I understand why you'd want to use different org.opencontainers.image.ref.* things, I would be careful about adding crucial information to annotations that other implementations might not know about. Do I think this is something that will need to be refined in future OCI updates? Most definitely, the arguments we had about issues like this before 1.0 are pages long.

Also, if you want to inter-operate with Docker (you'd have to ask @runcom to explain how the current implementation is going) then it's likely that you'll have to use ref.name only.

Great, .ref.name-only it is, then.

I was worried about having to invent some SQL-like query language, that would be a pretty bad UX (and a very horrible UX if every OCI consumer defined its own).

mtrmac · 2017-08-12T00:32:34Z

oci/layout/oci_dest.go

+}
+
+// Exists checks whether the specified path exists.
+// The implementation is opinionated, since in case of unexpected errors false is returned


(I’m not sure this is worth it; if os.Stat fails, then any other use of that directory will likely failing as well, and in that case we might just as well return the first encountered error (before the filesystem gets completely confused).

OTOH a separate exists function only simplifies things because it returns a simple bool, otherwise the caller handling the three states of exists might just as well call os.Stat itself, and handle the same three states. So I guess this is fine.)

mtrmac · 2017-08-12T00:32:54Z

oci/layout/oci_dest.go

+
+// Exists checks whether the specified path exists.
+// The implementation is opinionated, since in case of unexpected errors false is returned
+func exists(path string) bool {


Please name this pathExists, dirExists or something like that, though.

mtrmac · 2017-08-12T00:34:44Z

oci/layout/oci_dest_test.go

+	})
+
+	digest := "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad"
+	assert.Contains(t, paths, filepath.Join(tmpDir, "blobs", "sha256", digest), "The OCI directory does not contain the new manifest data")


(if this is checking for a single path with a known name, the code could just as well use the exists function and check for that specific name. But this is only a test, and it works, so whatever.)

mtrmac · 2017-08-12T00:36:04Z

oci/layout/oci_transport.go

+	defer indexJSON.Close()
 	if err != nil {
-		return imgspecv1.Descriptor{}, err
+		return index, err


I don’t think this should silently swallow I/O errors and return an empty index in that case as if everything was just fine and any pre-existing images inexplicably disappeared.

I was thinking about this one myself. So you reckon it should be nil, err?

Ah, it is actually returning a non-nil error value, I didn’t notice, sorry.

Anyway, yes, when the only caller ignores the index if err != nil, this function might as well return nil, err instead of pretending that returning index matters.

Still outstanding.

Ok, I'll return a *imgspecv1.Index. Not sure whether this is so much better though.

(Returning an imgspecv1.Index{} would be fine as well; the point is that the common case is that all other return values are ignored on a non-nil error return, and this makes a misleading impression that the return value matters.)

mtrmac · 2017-08-12T00:39:31Z

oci/layout/oci_transport_test.go

+	assert.Equal(t, fullyExplicitPath, ns[0])
+	assert.Equal(t, filepath.Dir(fullyExplicitPath), ns[1])
+
+	//// Test with a known path which should exist. Test just one non-canonical


… Oops, I didn’t read the related code before answering, sorry.

I hope we can find something, I can take a look at least for macOS and Linux. If someone wanted to run this on Windows, I have no idea.

The $TMPDIR issue is real, and I’d prefer a general solution to that instead of every test having magic knowledge of that like fullyExplicitPath, err := explicitfilepath.ResolvePathToFullyExplicit(tmpDir). But that’s strictly speaking unrelated to this PR.

mtrmac · 2017-08-12T00:45:08Z

oci/layout/oci_transport_test.go

 	defer os.RemoveAll(tmpDir)

-	assert.Equal(t, tmpDir+":tagValue", ref.PolicyConfigurationIdentity())
+	fullyExplicitPath, err := explicitfilepath.ResolvePathToFullyExplicit(tmpDir)


I do know that this is needed for macOS right now, but this is a bit weird to have in tests. This workaround gets copied all over the place…

At the very least, a comment explaining the /private/var situation would help make sure a next Linux-only maintainer does not break everything again.

Can we instead add a shared workaround, something like

func init() { // comment about what this is for os.Setenv("TMPDIR", filepath.EvalSymlinks(os.Getenv("TMPDIR"))) }

or something like that? Perhaps even a helper function, IIRC there are tests in multiple subpackages which need a similar workaround.

I do know that this is needed for macOS right now, but this is a bit weird to have in tests.

I know. I am also not quite sure what the best way to handle this. Unfortunately, on macOS thinks like /var /tmp and even /etc are symbolic links, so a lot of tests fail.

At the very least, a comment explaining the /private/var situation would help make sure a next Linux-only maintainer does not break everything again.

+1

Can we instead add a shared workaround, something like ...

Ahhh, that's an interesting one. Let me think about this one for a bit.

This should have been resolved by #336.

cyphar · 2017-08-12T10:02:18Z

oci/layout/oci_dest.go

+	if os.IsNotExist(err) {
+		return false
+	}
+	return true


This can be implemented as

func exists(path string) bool { _, err := os.Stat(path) return err != nil && os.IsNotExist(err) }

I also think that you could even remove the err != nil check (os.IsNotExist(nil) == false).

sure, I am just not sure whether more condensed code is always the better option. One needs to do much more logical operations in ones head in this version. My version is more verbose, but arguably easier to take in.

cyphar · 2017-08-12T10:07:17Z

oci/layout/oci_dest.go

 }
+
+func ensureDirectoryExists(path string) error {
+	if _, err := os.Stat(path); err != nil && os.IsNotExist(err) {


Shouldn't this be using the exist function you wrote?

hallyn · 2017-09-29T04:48:43Z

Hi, just wanted to check - is this still being worked on?

hferentschik · 2017-10-19T19:56:37Z

Hi, just wanted to check - is this still being worked on?

It is. In fact I just rebased the changes on top of master. I would love to get this integrated as well.

mtrmac

Also see the test suite failures – locally, make should be passing for you.

mtrmac · 2017-10-24T16:36:18Z

oci/archive/oci_transport.go

 // createOCIRef creates the oci reference of the image
 func createOCIRef(image string) (tempDirOCIRef, error) {
-	dir, err := ioutil.TempDir("/var/tmp", "oci")
+	dir, err := ioutil.TempDir("", "oci")


Why does this PR need this change? It seems unrelated.

(Using /var/tmp on is necessary on Linux because /tmp may be tmpfs, inappropriate for storing multi-gigabyte container images.)

This is indeed unrelated, however, I came across this while rebasing my original pull request. OCI is one of the few transports which would work across OS. /var/tmp will of course not work on Windows. IMO the basedir should not be specified, but I can revert this, if that means we can get this merged.

Yes, please split this out. Having as much as possible work on Windows would be very nice, but the fix should

Be in a clearly separate commit, with an explanation of what is fixing. (At least I, with my Linux blinders on, could not guess that this improves Windows.)

Not break Linux/macOS to fix Windows.

Ideally, be comprehensive. See the other two instances of temporaryDirectoryForBigFiles; I guess this would end up as a tiny sub-package.

@mtrmac - I created a new issue - #368. I agree this should be properly tracked since it is not directly related to this pull request.

Not sure whether I have the bandwidth to work on this, but either way I would like to see my others pull requests merged/addressed first. We need them merged in order to start using containers/image in Minishift.

@mtrmac - I created a new issue - #368.

Thanks!

mtrmac · 2017-10-24T16:38:59Z

oci/layout/oci_dest.go

 		},
 	}
+
+	if indexExists(ref.dir) {


This would be cleaner as if indexExists() { index = … } else { index = … }, rather than always setting index only to overwrite it.

fair enough

mtrmac · 2017-10-24T16:41:32Z

oci/layout/oci_dest.go

-			return err
+func (d *ociImageDestination) addManifest(desc *imgspecv1.Descriptor) {
+	for i, manifest := range d.index.Manifests {
+		if manifest.Digest.String() == desc.Digest.String() {


The replacement needs to be based on the org.opencontainers.image.ref.name annotation, not on the digest; see the earlier conversation in the PR.

mtrmac · 2017-10-24T16:43:39Z

oci/layout/oci_dest.go

+// Exists checks whether an index file exists at the specified path.
+// The implementation is opinionated, since in case of unexpected errors false is returned
+func indexExists(path string) bool {
+	_, err := os.Stat(filepath.Join(path, "index.json"))


This should call the existing ociReference.indexPath, at least to make it trivial to find all uses of that path.

Not sure what you mean here. Could you clarify this?

func (ref ociReference) indexExists() bool { _, err := os.Stat(ref.indexPath()) …

mtrmac · 2017-10-24T16:46:14Z

oci/layout/oci_transport.go

 	"github.com/opencontainers/go-digest"
+	imgspec "github.com/opencontainers/image-spec/specs-go"
 	imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1"
+


(Nit: Please drop this line.)

which one exactly? The empty one?

(Yes, exactly as you did.)

mtrmac · 2017-10-24T16:47:04Z

oci/layout/oci_transport.go

+	defer indexJSON.Close()
 	if err != nil {
-		return imgspecv1.Descriptor{}, err
+		return index, err


Still outstanding.

mtrmac · 2017-10-24T16:47:44Z

oci/layout/oci_transport.go

+	}
+
 	indexJSON, err := os.Open(ref.indexPath())
+	defer indexJSON.Close()


Moving the defer up is incorrect; indexJSON may be nil at this point.

mtrmac · 2017-10-24T16:48:29Z

oci/layout/oci_transport.go


-func (ref ociReference) getManifestDescriptor() (imgspecv1.Descriptor, error) {
+func (ref ociReference) getIndex() (imgspecv1.Index, error) {
+	index := imgspecv1.Index{


(After the return values are modified to return nil, err, this should just an empty value.)

ahh, I guess this relates to returning *imgspecv1.Index and nil in the case an error occurs. Fair enough

This refers to #326 (comment) .

hferentschik · 2017-10-25T18:34:11Z

@mtrmac - I pushed an update. I addressed your comments, except one which I am not quite clear about what it means. If it is still an issue in please clarify what you mean.

Also see the test suite failures – locally, make should be passing for you.

I have no idea what the dangling whitespace error was in the commit. I had to drop the commit and re-create it. Seems it is gone now.

mtrmac · 2017-10-25T21:01:10Z

oci/layout/oci_transport.go

-	if err := json.NewDecoder(indexJSON).Decode(&index); err != nil {
+
+	index := &imgspecv1.Index{
+		Versioned: imgspec.Versioned{


Why is the Versioned field initialized? Shouldn’t that come from indexJSON as well?

mtrmac · 2017-10-25T21:03:16Z

oci/layout/oci_transport.go

+	defer indexJSON.Close()
 	if err != nil {
-		return imgspecv1.Descriptor{}, err
+		return index, err


(Returning an imgspecv1.Index{} would be fine as well; the point is that the common case is that all other return values are ignored on a non-nil error return, and this makes a misleading impression that the return value matters.)

mtrmac · 2017-10-25T21:11:45Z

I have no idea what the dangling whitespace error was in the commit.

A space/tab at the end of the line; most editors etc. don’t highlight them by default, and formatters like gofmt drop them, so they may seem to ”surprisingly” disappear. IIRC the error message indicates the line, but I might be wrong. At least git show etc. highlight them in red.

mtrmac · 2017-10-31T14:02:08Z

👍

Thanks!

runcom · 2017-10-31T14:15:24Z

lgtm

runcom · 2017-10-31T14:15:57Z

mmm rebase?

Signed-off-by: Hardy Ferentschik <hardy@hibernate.org>

hferentschik · 2017-10-31T14:27:59Z

mmm rebase?

done

runcom · 2017-10-31T14:32:09Z

waiting Travis to merge

hferentschik · 2017-10-31T16:51:48Z

Awesome, thanks for the merge

hferentschik mentioned this pull request Aug 11, 2017

Implement sync semantic between source and destination image repository #155

Closed

hferentschik commented Aug 11, 2017

View reviewed changes

mtrmac reviewed Aug 12, 2017

View reviewed changes

cyphar reviewed Aug 12, 2017

View reviewed changes

hferentschik force-pushed the issue-155 branch from e3b0b96 to 3e7e1f4 Compare August 12, 2017 19:20

hferentschik mentioned this pull request Aug 22, 2017

Provide caching of docker images across VM instances minishift/minishift#952

Closed

mtrmac mentioned this pull request Sep 1, 2017

skopeo copy does not merge OCI indexes containers/skopeo#405

Closed

hferentschik force-pushed the issue-155 branch 2 times, most recently from 1751a75 to 04ca45b Compare October 19, 2017 19:55

hferentschik changed the title ~~[WIP] Issue #155 - Making sure existing OCI index gets not overwritten~~ Issue #155 - Making sure existing OCI index gets not overwritten Oct 19, 2017

hferentschik changed the title ~~Issue #155 - Making sure existing OCI index gets not overwritten~~ Issue #155 Making sure existing OCI index gets not overwritten Oct 19, 2017

hferentschik mentioned this pull request Oct 20, 2017

Use OCI storage for current image cache implementation minishift/minishift#1575

Closed

hferentschik force-pushed the issue-155 branch from 04ca45b to b4937bb Compare October 24, 2017 09:33

mtrmac reviewed Oct 24, 2017

View reviewed changes

hferentschik force-pushed the issue-155 branch from b4937bb to 446f913 Compare October 25, 2017 18:30

mtrmac reviewed Oct 25, 2017

View reviewed changes

hferentschik force-pushed the issue-155 branch 2 times, most recently from e0298a1 to 759c61c Compare October 27, 2017 08:27

Issue containers#155 Making sure existing OCI index gets not overwritten

7ae7031

Signed-off-by: Hardy Ferentschik <hardy@hibernate.org>

hferentschik force-pushed the issue-155 branch from 759c61c to 7ae7031 Compare October 31, 2017 14:27

runcom merged commit 3b15e3c into containers:master Oct 31, 2017

hferentschik deleted the issue-155 branch November 1, 2017 10:08

mtrmac mentioned this pull request May 7, 2019

oci transport will remove existing images #306

Closed

Issue #155 Making sure existing OCI index gets not overwritten #326

Issue #155 Making sure existing OCI index gets not overwritten #326

Uh oh!

Conversation

hferentschik commented Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyphar Aug 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hferentschik commented Aug 11, 2017 •

edited

Loading

cyphar Aug 12, 2017 •

edited

Loading