Add reader/writer for oci-archive multi image support #1381

umohnani8 · 2021-09-20T18:33:23Z

Add reader/writer with helpers to allow podman save/load multi oci-archive images.
Allow read oci-archive using source_index to point to the index from oci-archive manifest.
Also reimplement ociArchiveImage{Source,Destination} to support this.

Taking over #1072
Fixes #1116

Signed-off-by: Qi Wang qiwan@redhat.com
Signed-off-by: Urvashi Mohnani umohnani@redhat.com

umohnani8 · 2021-09-20T18:36:37Z

@vrothberg @mtrmac PTAL
Looked through #1072 and most of the reviews were addressed, did some fixes for the few comments that weren't already fixed - please let me know if I missed any.
Will open up a podman vendor + patch PR once this is in.

vrothberg · 2021-09-22T12:30:51Z

Will look into it now. Note that it will require some massaging into c/common/libimage prior to Podman.

vrothberg

Before merging, we need some tests somewhere. For multi-image docker archives, we added them to Podman and those have since been moved to libimage (see https://github.com/containers/common/blob/main/libimage/save_test.go and https://github.com/containers/common/blob/main/libimage/load_test.go and https://github.com/containers/common/tree/main/libimage/testdata).

I suggest opening a PR against c/common vendoring in this PR. Once everything's green, we can merge here, then merge into libimage and then let it bubble up into Podman.

docs/containers-transports.5.md

oci/internal/oci_util_test.go

oci/archive/oci_transport.go

oci/archive/oci_dest.go

oci/archive/reader.go

mtrmac

WIP, so far I’ve just read parts of oci/archive.Reader.

Structurally it seems clean to me for oci/archive.ociArchiveImageSource to be always built on top of an oci/archive.Reader, and similarly for destination/Writer. I do feel somewhat strongly about this, OTOH the current approach around the existing tempDirOCIRef is probably less churn and easier to review, for now.

oci/archive/reader.go

mtrmac · 2021-10-05T20:00:34Z

oci/archive/reader.go

+	}
+	succeeded = true
+	return &reader, nil
+}


docker/archive.Reader needs to provide a NewReaderForReference for libimage, AFAICS to turn an user-supplied docker-archive:…@0 into a tag used in c/storage on podman load.

Will something like that be necessary here? (Cc: @vrothberg )

I update NewReader to take in a reference, if that is wrong I can break them apart tot two different functions NewReader and NewReaderForReference`.

I think it would definitely make sense to provide a NewReader that accepts a path, so that direct usage of a Reader is simpler.

As to the NewReaderForReference, that requires looking at what c/common needs. The docker-archive variant requires NewReaderForReference also to return a reader-bound equivalent of the input reference (because we don’t otherwise expose ref.index and ref.sourceIndex, so the caller can’t do that itself), and Reader.ManifestTagsForReference. Looking at the nameFromAnnotations call in c/common, something vaguely like that might be necessary here.

oci/archive/reader.go

oci/archive/oci_transport.go

oci/archive/oci_src.go

oci/archive/oci_dest.go

oci/layout/oci_transport.go

oci/archive/oci_dest.go

oci/archive/reader.go

vrothberg · 2021-12-13T09:01:20Z

As noted by @mtrmac, this PR should have a similar logic as in containers/common#853 (review) to account for archives generated with buildkit.

umohnani8 · 2022-02-08T12:10:45Z

@vrothberg @mtrmac I think I addressed most of the comments here, please let me know if I accidentally missed any.

The c/common PR for this is containers/common#921

oci/layout/oci_transport.go

umohnani8 · 2022-02-21T21:37:43Z

Can we please re-run the failed tests - failure looks like a flake.

vrothberg · 2022-02-22T08:37:04Z

Can we please re-run the failed tests - failure looks like a flake.

Retrigger ✔️

umohnani8 · 2022-02-24T16:01:32Z

This is ready for review, c/common PR is green containers/common#921.
@vrothberg @mtrmac PTAL

Also test failure looks like a flake again

vrothberg · 2022-02-28T10:43:50Z

Can you reply to https://github.com/containers/common/pull/921/files#r815770324?

oci/archive/oci_dest.go

vrothberg · 2022-02-28T10:39:24Z

oci/archive/reader.go

+	}
+	succeeded = true
+	return &reader, nil
+}


oci/archive/reader.go

oci/layout/oci_transport.go

mtrmac · 2022-02-26T17:31:20Z

oci/layout/oci_transport.go

 	var d *imgspecv1.Descriptor
+
+	if ref.sourceIndex != -1 {


Non-blocking: WIth three cases, this could be a switch (and perhaps with an explicit “internal error” case if both image and sourceIndex are set).

Will do in a follow up PR

mtrmac · 2022-02-26T17:32:47Z

oci/layout/oci_transport.go

+			d = &index.Manifests[ref.sourceIndex]
+			return *d, nil


Isn’t this just return index.Manifests[ref.sourceIndex]?

(Overall the d variable seems basically unnecessary — everything can just return a value, and the d == nil case only possible after the for loop`. OTOH cleaning up the pre-existing code is not really in scope, this PR is big enough.)

Will do in a follow up PR

oci/layout/oci_transport_test.go

docs/containers-transports.5.md

mtrmac · 2022-02-26T23:01:16Z

oci/archive/oci_transport.go

@@ -144,7 +178,7 @@ func (ref ociArchiveReference) NewImageDestination(ctx context.Context, sys *typ

 // DeleteImage deletes the named image from the registry, if supported.
 func (ref ociArchiveReference) DeleteImage(ctx context.Context, sys *types.SystemContext) error {
-	return errors.Errorf("Deleting images not implemented for oci: images")
+	return fmt.Errorf("Deleting images not implemented for oci: images")
 }

 // struct to store the ociReference and temporary directory returned by createOCIRef


(If LoadManifestDescriptor* is reimplemented) I imagine no users of tempDirOCIRef should remain — just Readers and Writers, now that they “own” their temporary directories. Maybe with a shared helper to create an oci/layout reference from a (temp dir, archive reference).

will do in a follow up PR

oci/archive/reader.go

oci/archive/oci_src.go

oci/archive/reader.go

oci/archive/oci_src.go

umohnani8 · 2022-03-09T15:41:25Z

@mtrmac addressed all your comments PTAL

mtrmac

Thanks! Looks great overall.

The one real blocker is #1381 (comment) / containers/common#921 (comment) .

oci/archive/oci_dest.go

oci/archive/oci_src.go

oci/archive/reader.go

tanguofu · 2022-06-05T03:32:30Z

@mtrmac addressed all your comments PTAL

@umohnani8 could u has some update here? we look for this API canbe published, many thanks,

Add reader/writer with helpers to allow podman save/load multi oci-archive images. Allow read oci-archive using source_index to point to the index from oci-archive manifest. Also reimplement ociArchiveImage{Source,Destination} to support this. Signed-off-by: Qi Wang <qiwan@redhat.com> Signed-off-by: Urvashi Mohnani <umohnani@redhat.com>

bea231 · 2022-10-05T10:21:06Z

Hi, any updates here?)

mtrmac

Good, there is now a NewReaderForReference — but it would be useful to have a at least a proof of concept in containers/common#921 before definitely committing to the API.

Let me just try to sketch what that would look like…

mtrmac · 2022-10-06T18:49:39Z

oci/archive/oci_dest.go

-		if err := tempDirRef.deleteTempDir(); err != nil {
-			return nil, perrors.Wrapf(err, "deleting temp directory %q", tempDirRef.tempDirectory)
-		}
+		archive.Close()


This should only close individualWriterOrNil if it is set, not the longer-term ref.archiveWriter.

(in both cases.)

mtrmac · 2022-10-06T18:51:58Z

oci/archive/oci_dest.go

 }

 // newImageDestination returns an ImageDestination for writing to an existing directory.
-func newImageDestination(ctx context.Context, sys *types.SystemContext, ref ociArchiveReference) (private.ImageDestination, error) {
-	tempDirRef, err := createOCIRef(sys, ref.image)
+func newImageDestination(ctx context.Context, sys *types.SystemContext, ref ociArchiveReference) (types.ImageDestination, error) {


I think this should continue to return private.ID, not types.ID. (We do, ultimately, have a test for the return value implementing private.ID, but having it explicit here would be a bit more convenient.)

mtrmac · 2022-10-06T19:00:18Z

oci/archive/oci_src.go

+	} else {
+		layoutRef, err = layout.NewReference(archive.tempDirectory, ref.image)
+		if err != nil {
+			archive.Close()


archive should only be closed if it is individualReaderOrNil. (in all cases.)

mtrmac · 2022-10-06T19:04:35Z

oci/archive/oci_transport.go

-func createOCIRef(sys *types.SystemContext, image string) (tempDirOCIRef, error) {
-	dir, err := os.MkdirTemp(tmpdir.TemporaryDirectoryForBigFiles(sys), "oci")
+func createOCIRef(sys *types.SystemContext, image string, sourceIndex int) (tempDirOCIRef, error) {
+	dir, err := ioutil.TempDir(tmpdir.TemporaryDirectoryForBigFiles(sys), "oci")


(Merge problem) This should use the newer os.MkdirTemp name.

mtrmac · 2022-10-06T19:07:02Z

oci/archive/oci_transport_test.go

+// The caller should
+//   defer os.RemoveAll(tmpDir)
+func refToTempOCI(t *testing.T, sourceIndex bool) (ref types.ImageReference, tmpDir string) {
+	tmpDir, err := ioutil.TempDir("", "oci-transport-test")
+	require.NoError(t, err)


(Merge problem) The use of t.TempDir makes the caller cleaning up unnecessary.

mtrmac · 2022-10-06T19:10:39Z

oci/archive/reader.go

+	if err := archive.NewDefaultArchiver().Untar(arch, dst, &archive.TarOptions{NoLchown: true}); err != nil {
+		return nil, perrors.Wrapf(err, "error untarring file %q", dst)
+	}
+
+	indexJSON, err := os.Open(filepath.Join(dst, "index.json"))
+	if err != nil {
+		return nil, err
+	}
+	defer indexJSON.Close()
+	reader.manifest = &imgspecv1.Index{}
+	if err := json.NewDecoder(indexJSON).Decode(reader.manifest); err != nil {
+		return nil, err
+	}


On these failures, the temporary directory should be removed.

mtrmac · 2022-10-06T19:13:20Z

oci/archive/reader.go

+		if refName != "" {
+			index = 1
+		}
+		ref, err := newReference(r.path, refName, index, r, nil)


This looks rather incorrect:

It is setting index exactly in the case where it is not needed (and newReference would reject such an input)

The index needs to actually point at the right manifest, not a hard-coded 1.

mtrmac · 2022-10-06T19:15:30Z

oci/layout/oci_transport_test.go

+// The caller should
+//   defer os.RemoveAll(tmpDir)
+func refToTempOCI(t *testing.T, sourceIndex bool) (ref types.ImageReference, tmpDir string) {
+	tmpDir, err := ioutil.TempDir("", "oci-transport-test")
+	require.NoError(t, err)


(Merge problem) The use of t.TempDir() means callers don’t need to worry about cleanup.

mtrmac · 2022-10-06T19:18:10Z

oci/archive/oci_dest.go

@@ -160,32 +178,5 @@ func (d *ociArchiveImageDestination) Commit(ctx context.Context, unparsedTopleve
 	if err := d.unpackedDest.Commit(ctx, unparsedToplevel); err != nil {
 		return perrors.Wrapf(err, "storing image %q", d.ref.image)


pkg/errors are no longer used (throughout).

mtrmac · 2022-10-06T19:19:27Z

oci/archive/oci_src.go

+		archive = ref.archiveReader
+		individualReaderOrNil = nil
+	} else {
+		archive, _, err = NewReaderForReference(ctx, sys, ref)


I don’t see any benefit to calling NewReaderForReference if we are going to throw away that reference.

mtrmac · 2022-10-06T21:47:17Z

Prototyping this results in #1677 + containers/common#1178 . Note that the “pull” case still needs either LoadManifestDescriptor being able to benefit from a Reader-based reference (the way #1677 now does), or some other Reader API, to be able to correctly name the pulled image.

TomSweeneyRedHat · 2022-10-31T19:28:45Z

Needs a rebase at this point.
@umohnani8 are you going to push this forward or should we close?

vrothberg reviewed Sep 22, 2021

View reviewed changes

mtrmac reviewed Oct 5, 2021

View reviewed changes

mtrmac reviewed Dec 10, 2021

View reviewed changes

oci/archive/reader.go Outdated Show resolved Hide resolved

umohnani8 force-pushed the multi-manifest branch from 03499a2 to 9e932c2 Compare December 13, 2021 17:29

mtrmac mentioned this pull request Dec 23, 2021

Do we have any way to copy additional annotations while copying oci images containers/skopeo#1505

Closed

umohnani8 force-pushed the multi-manifest branch from 9e932c2 to 6b37365 Compare February 7, 2022 15:54

umohnani8 mentioned this pull request Feb 7, 2022

[WIP] Add multi manifest oci-archive support containers/common#921

Closed

umohnani8 force-pushed the multi-manifest branch from 6b37365 to 77ce5d8 Compare February 8, 2022 12:08

umohnani8 force-pushed the multi-manifest branch 3 times, most recently from e1b0dbd to e4964c0 Compare February 8, 2022 15:17

vrothberg reviewed Feb 16, 2022

View reviewed changes

oci/layout/oci_transport.go Outdated Show resolved Hide resolved

umohnani8 force-pushed the multi-manifest branch 3 times, most recently from 1165cd5 to 460f04d Compare February 21, 2022 17:55

vrothberg reviewed Feb 28, 2022

View reviewed changes

mtrmac reviewed Mar 1, 2022

View reviewed changes

umohnani8 force-pushed the multi-manifest branch from 460f04d to c4be24d Compare March 1, 2022 16:05

mtrmac reviewed Mar 1, 2022

View reviewed changes

oci/archive/reader.go Outdated Show resolved Hide resolved

mtrmac reviewed Mar 2, 2022

View reviewed changes

oci/archive/oci_src.go Show resolved Hide resolved

umohnani8 force-pushed the multi-manifest branch 2 times, most recently from 63283ce to 6ce3216 Compare March 3, 2022 01:19

umohnani8 force-pushed the multi-manifest branch 2 times, most recently from 2de595a to e6ece2b Compare March 9, 2022 15:39

mtrmac reviewed Mar 12, 2022

View reviewed changes

mtrmac mentioned this pull request Apr 11, 2022

fea(#1619): skopeo sync support use src or dest use oci format containers/skopeo#1620

Closed

mtrmac mentioned this pull request Jun 2, 2022

Add oci/layout.Reader #1563

Open

umohnani8 force-pushed the multi-manifest branch from e6ece2b to 44465c9 Compare July 12, 2022 17:14

mtrmac mentioned this pull request Jul 13, 2022

Switch to golang native error wrapping #1607

Merged

mtrmac mentioned this pull request Oct 6, 2022

Introduce oci/{archive,layout}.ImageNotFoundError #1675

Merged

mtrmac reviewed Oct 6, 2022

View reviewed changes

mtrmac mentioned this pull request Oct 6, 2022

OCI-archive multi-manifest support POC #1677

Draft

mtrmac mentioned this pull request Oct 6, 2022

OCI-archive multi-manifest support POC containers/common#1178

Draft

mtrmac added the kind/feature A request for, or a PR adding, new functionality label Dec 7, 2022

mtrmac mentioned this pull request Feb 2, 2023

[Feature]: digest support for OCI transport containers/podman#17308

Open

mwopfner mentioned this pull request Feb 6, 2023

Allow referring images in oci: (and oci-archive: ?) by manifest digest #1828

Open

mtrmac mentioned this pull request Jul 21, 2023

Oci image deletion #2003

Merged

umohnani8 closed this Feb 21, 2024

dekimsey mentioned this pull request Feb 23, 2024

RFE: create multi-arch manifests containers/skopeo#1136

Open

		@@ -160,32 +178,5 @@ func (d *ociArchiveImageDestination) Commit(ctx context.Context, unparsedTopleve
		if err := d.unpackedDest.Commit(ctx, unparsedToplevel); err != nil {
		return perrors.Wrapf(err, "storing image %q", d.ref.image)

Add reader/writer for oci-archive multi image support #1381

Add reader/writer for oci-archive multi image support #1381

Conversation

umohnani8 commented Sep 20, 2021

umohnani8 commented Sep 20, 2021

vrothberg commented Sep 22, 2021

vrothberg left a comment

Choose a reason for hiding this comment

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

vrothberg commented Dec 13, 2021

umohnani8 commented Feb 8, 2022

umohnani8 commented Feb 21, 2022

vrothberg commented Feb 22, 2022

umohnani8 commented Feb 24, 2022

vrothberg commented Feb 28, 2022

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

umohnani8 commented Mar 9, 2022

mtrmac left a comment

Choose a reason for hiding this comment

tanguofu commented Jun 5, 2022

bea231 commented Oct 5, 2022

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

Choose a reason for hiding this comment

mtrmac commented Oct 6, 2022

TomSweeneyRedHat commented Oct 31, 2022