[WIP] load/save oci-archive multi images #8218

QiWang19 · 2020-11-02T15:08:27Z

Allow load/save oci-archive format multi images.

$ bin/podman save -o two-images.tar -m --format oci-archive gcr.io/distroless/java docker.io/library/alpine
$ bin/podman load -i two-images.tar

close #4646

Signed-off-by: Qi Wang [email protected]

openshift-ci-robot · 2020-11-02T15:08:31Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: QiWang19
To complete the pull request process, please assign jwhonce after the PR has been reviewed.
You can assign the PR to them by writing /assign @jwhonce in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

zhangguanzhang · 2020-11-03T01:00:50Z

libpod/image/image.go

+			img := imageMap[id]
+			copyOptions := getCopyOptions(sys, writer, nil, nil, SigningOptions{RemoveSignatures: removeSignatures}, "", img.tags)
+			copyOptions.DestinationCtx.SystemRegistriesConfPath = registries.SystemRegistriesConfPath()


need table format

vrothberg

Early feedback from testing:

We need to distinguish IDs from names. Currently, IDs are treated as names and stored in annotations which leads to:

$ podman load -i ...
Loaded image(s): localhost/79fd58dc7611, localhost/d6e46aa2470d

vrothberg

I dropped some comments on the c/image PR: containers/image#1072 (review)

I think there are two things we need to address:

We need to support multiple names or none.
We need/should be consistent with the reference syntax of docker-archive which identifies images by index: https://github.com/containers/image/blob/master/docs/containers-transports.5.md#docker-archivepathdocker-referencesource-index

QiWang19 · 2020-11-23T15:49:09Z

@containers/podman-maintainers @vrothberg @mtrmac PTAL

mtrmac

From a very quick skim, this results in a very large function that is going to be very hard to maintain (just read, but primarily keep consistent). It would be nice to share more of the format-independent infrastructure.

Most importantly the namesOrIDs parsing code should be shared if at all possible, to ensure the semantics of that parameter is consistent across archive formats — even if the actual copy implementation needs to be different because one can benefit from DockerArchiveAdditionalTags. But also the things like converting imageNames to newImages would probably benefit from sharing (or, well, eliminating; the copy already has a storage reference so converting it into a name and back is pointless extra work), and I worry a bit about newImageEvent which is not mechanically essential, it might be too easy to forget to update a copy.

(Very tentative: it might be worth splitting the archive save/load code, along with the archive-specific helpers, into a separate file that can be easier to understand as a whole — if you do that, please make it a separate commit that changes nothing in the code.)

mtrmac · 2020-11-24T09:43:35Z

libpod/runtime_img.go

+		newImages, err = r.ImageRuntime().LoadAllImagesFromOCIArchive(ctx, inputFile, signaturePolicy, writer)
+		if err == nil {
+			return getImageNames(newImages), nil
+		}


These “try N different formats, succeed on any one” functions have frequently caused user confusion.

Would it make sense to have the user specify the format? I guess auto-detection is a desirable feature…

The error reporting definitely needs improving. If the user submits a docker-archive format that is mostly (or completely) valid but the load fails, the docker-archive-related error must be available to the user.

(Extra thought, probably not for this PR: It might be interesting to have an “autodetect archive format” helper so that we can detect the format separately (or report a clear error that the format is unrecognized), and then just do one load attempt with a simple error reporting structure. Or, well, we could go even further and share a single “decompress and untar” operation across all formats — right now we would decompress the multiple gigabytes separately for each format.)

… actually, the fact that we have to decompress the multiple-gigabyte archive in every format until we find one that works is a pretty big cost, and IMHO a fairly strong reason to at least allow the user to specify the format, even we still wanted to support auto-detection.

zhangguanzhang · 2020-12-03T01:15:59Z

Conflicting files

libpod/runtime_img.go

QiWang19 · 2020-12-03T20:08:56Z

@mtrmac PTAL, Added an archivehelper to decide the format according to the manifest.json to index.json, it this a way to detect the format?

mtrmac · 2020-12-03T21:17:24Z

@mtrmac PTAL, Added an archivehelper to decide the format according to the manifest.json to index.json, it this a way to detect the format?

That’s a neat idea, for the cases where this code does work it’s cheap enough, and for the expensive case (compressed archive) we can just not do that much work to guess. But because it can fail, AFAICS there still must be the fall-back logic to try all of the formats (well, all of the archive formats or all of the directory formats, no need for both).

mtrmac

I’m still rather unhappy about the length and duplication in Runtime.SaveImages, see earlier.

As mentioned a few times, if the guessing can fail, this still needs to try the relevant alternatives. Maybe the guessing could be a separate performance feature PR, I’m not sure whether that would help or hinder.

mtrmac · 2020-12-03T21:18:14Z

libpod/runtime_img.go

-				return getImageNames(newImages), nil
+// imageFormat detects the image foramt of the input archive or directory
+// returns "", if can not decide the format
+func imageFormat(inputFile string) (string, error) {


guessImageFormat maybe, to emphasize that it can fail to make a decision?

mtrmac · 2020-12-03T21:18:49Z

libpod/runtime_img.go

+// imageFormat detects the image foramt of the input archive or directory
+// returns "", if can not decide the format
+func imageFormat(inputFile string) (string, error) {
+	path, err := explicitfilepath.ResolvePathToFullyExplicit(inputFile)


Does this do anything that matters for this function?

mtrmac · 2020-12-03T21:19:46Z

libpod/runtime_img.go

+	if err != nil {
+		return "", errors.Wrapf(err, "error detecting input file format")
+	}
+	if !archive.IsArchivePath(inputFile) {


An os.Stat + IsDir would be a bit cheaper.

mtrmac · 2020-12-03T21:21:15Z

libpod/runtime_img.go

+			break
+		}
+		if err != nil {
+			return "", errors.Wrapf(err, "error detecting archive format")


Is this the path taken on a compressed archive? That should probably return "", nil (and the caller must handle that) to indicate that it didn’t make a guess.

mtrmac · 2020-12-03T21:22:21Z

libpod/runtime_img.go

+		if header.Name == "index.json" {
+			return image.OCIArchive, nil
+		}
+		if header.Name == "manifest.json" {


Apparently for compatibility with some docker-archive writers, path.Clean(header.Name) must be used; I guess using it for both cases wouldn’t hurt.

mtrmac · 2020-12-03T21:39:39Z

libpod/runtime_img.go

+	format, err := imageFormat(inputFile)
+	if err != nil {
+		return "", err
+	}


What if format can not be determined?

mtrmac · 2020-12-03T21:40:28Z

libpod/runtime_img.go

 		return getImageNames(newImages), nil
 	}
-	return "", err
+
+	return "", errors.Wrapf(err, "error pulling image")


This is incorrect if src == nil (both “couldn’t guess” and “coding error”)

mtrmac · 2020-12-03T21:42:49Z

libpod/runtime_img.go

+}
+
+// LoadImageFromSingleImageDir loads image from the archive of single image that inputFile points to.
+func (r *Runtime) LoadImageFromSingleImageDir(ctx context.Context, writer io.Writer, inputFile, signaturePolicy, format string) (string, error) {


This can be package-private AFAICS.

mtrmac · 2020-12-03T21:44:20Z

libpod/runtime_img.go

+		}
+		return strings.Join(newImageNames, ", "), nil
+	}
+	return "", errors.Errorf("error loading multipal images from the archive")


This does not say much about why things failed or what the user should do.

mtrmac · 2020-12-03T21:50:57Z

libpod/image/image.go

-			return nil, errors.Wrapf(err, "error retrieving local image after pulling %s", name)
-		}
-		newImages = append(newImages, newImage)
-	}


Nice.

Absolutely non-blocking: If I’m checking correctly, doing the same to LoadFromArchiveReference would allow most of the Load functions involved in this PR to return []string, and two of the callers of Runtime.LoadImage would eliminate a strings.Split (and the third one would now contain the only strings.Join).

mtrmac · 2020-12-03T21:53:03Z

@mtrmac PTAL, Added an archivehelper to decide the format according to the manifest.json to index.json, it this a way to detect the format?

(The detection is definitely useful, but not something I think must be in this PR; originally I only asked for not throwing away errors from all the attempts. If detection can fail and we still might end up guessing, the “not throwing away errors” part will continue to be necessary.)

zhangguanzhang · 2020-12-07T01:39:34Z

need gofmt the files

QiWang19 · 2020-12-08T15:51:20Z

@mtrmac Dropped the format detection from this PR. And the collections of the errors from LoadAllImageFromArchive and LoadImageFromSingleImageArchive using the appendErr, Could you take a look?

rhatdan · 2020-12-09T11:18:57Z

@QiWang19 needs rebase.

zhangguanzhang · 2020-12-10T04:57:06Z

still have Conflicting files

mtrmac · 2020-12-10T18:32:35Z

still have Conflicting files

This can not be merged as-is because it vendors an unmerged c/image branch, so that’s not an urgent concern at this point.

mtrmac

ACK in broad strokes, mostly “local” suggestions.

mtrmac · 2020-12-10T18:37:54Z

libpod/runtime_img.go

+	)
+	appendErr := func(e error) {
+		if latestErr != nil {
+			latestErr = errors.Wrapf(latestErr, "error pulling images: %v\n", e)


This will prefix some errors but not all? If I am reading this correctly,

error pulling images: error pulling images: error1 error2 error3

s/latestErr/allErrors/ maybe?

Most importantly, compare pkg/errorhandling, or maybe use it. I don’t know what are all of the coding conventions in Podman, but this does look like a problem appropriate for a common solution.

mtrmac · 2020-12-10T18:38:24Z

libpod/runtime_img.go

-		src, err := referenceFn()
+		if src, err = referenceFn(); err != nil {
+			appendErr(err)
+		}
 		if err == nil && src != nil {


(This can now be an else if to be a tiny bit clearer.)

mtrmac · 2020-12-10T18:41:54Z

libpod/runtime_img.go

 		return newImages, nil
 	}
+	logrus.Warnf("Error trying pulling multi-arch images: %v", err)


s/multi-arch images/multi-image archives/

For some formats like dir, LoadAllImageFromArchive will not even try, so it’s grating to output a warning and then succeed.

This is the only caller of LoadAllImageFromArchive as well as LoadImageFromSingleImageArchive, so it seems that there should only be one set of attempts, one list of errors (and the two archive formats should only be attempted once, not once via the multi-image reader and once via the non-reader path).

mtrmac · 2020-12-10T18:43:22Z

libpod/runtime_img.go

+	if err == nil {
+		return strings.Join(newImageNames, ", "), nil
+	}
+	appendErr(errors.Wrapf(err, "error trying multi docker aechive"))


“archive”, and this probably could benefit from a bit of rewording (users don’t care much about the “multi” part) — I haven’t tried this to see what the eventual full error text looks like.

(In both cases).

mtrmac · 2020-12-10T18:46:00Z

libpod/image/image.go

@@ -244,48 +240,143 @@ func (ir *Runtime) SaveImages(ctx context.Context, namesOrIDs []string, format s
 			if isTagged {
 				iData.tags = append(iData.tags, refTagged)
 			}
+		} else {
+			iData.destNames = []string{""}


This shouldn’t throw away any previous tags. Maybe add a single imageSpecifiedByID boolean?

mtrmac · 2020-12-10T18:53:32Z

libpod/image/image.go

 	// extend an image with additional tags
 	type imageData struct {
+		destNames []string


If “extend an image”, the *Image should probably remain first.

I’m not 100% happy with the two duplicate name arrays, but I also can’t see much of an alternative. (Moving the docker-archive tag parsing into the docker-archive specific loop would probably add too much complexity.)

mtrmac · 2020-12-10T18:55:39Z

libpod/image/image.go

+			img := imageMap[id]
+			// For copying, we need a source reference that we can create
+			// from the image.
+			src, err := is.Transport.NewStoreReference(img.imageruntime.store, nil, id)


This lookup can AFAICS also happen in the shared imageQueue construction code.

The src, storageReference is not exported. The imageQueue may not be able to hold a field for it.

Allow load/save oci-archive format multi images. Signed-off-by: Qi Wang <[email protected]>

Signed-off-by: Qi Wang <[email protected]>

QiWang19 · 2021-01-04T16:21:09Z

@mtrmac PTAL

github-actions · 2021-02-04T00:15:28Z

A friendly reminder that this PR had no activity for 30 days.

openshift-ci-robot · 2021-04-11T10:00:38Z

@QiWang19: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vrothberg · 2021-05-04T12:25:57Z

@QiWang19, do you still have time to tackle it? No worries in case not. I am going through some open PRs and issues at the moment.

QiWang19 · 2021-05-05T18:28:02Z

@QiWang19, do you still have time to tackle it? No worries in case not. I am going through some open PRs and issues at the moment.

Yes, I will find time to rebase this one and cooperate it with containers/image#1072.

vrothberg · 2021-05-31T13:13:58Z

Closing the PR as it must now be addressed in c/common/libimage. Thanks, @QiWang19, for working on this!

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 2, 2020

QiWang19 linked an issue Nov 2, 2020 that may be closed by this pull request

Error loading OCI archive with multiple manifest descriptors #4646

Open

QiWang19 mentioned this pull request Nov 2, 2020

Add reader/writer to for oci-archive multi image containers/image#1072

Closed

zhangguanzhang reviewed Nov 3, 2020

View reviewed changes

vrothberg reviewed Nov 3, 2020

View reviewed changes

QiWang19 force-pushed the multi-descriptor branch from b0d6409 to 62ec555 Compare November 4, 2020 21:57

QiWang19 force-pushed the multi-descriptor branch 3 times, most recently from 7a24e9b to bfb9c0b Compare November 17, 2020 20:47

QiWang19 force-pushed the multi-descriptor branch 6 times, most recently from 88cd693 to 4987527 Compare November 21, 2020 17:08

QiWang19 removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 22, 2020

mtrmac reviewed Nov 24, 2020

View reviewed changes

QiWang19 force-pushed the multi-descriptor branch 2 times, most recently from 35916a5 to 82ecd25 Compare December 2, 2020 17:25

QiWang19 force-pushed the multi-descriptor branch from 82ecd25 to 1977368 Compare December 3, 2020 19:49

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2020

QiWang19 force-pushed the multi-descriptor branch from 1977368 to 11a49f2 Compare December 3, 2020 19:55

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2020

QiWang19 force-pushed the multi-descriptor branch 2 times, most recently from 541e99c to 5c23ecf Compare December 3, 2020 20:04

mtrmac reviewed Dec 3, 2020

View reviewed changes

QiWang19 force-pushed the multi-descriptor branch 2 times, most recently from 1698b8f to 9d034a5 Compare December 6, 2020 16:02

QiWang19 force-pushed the multi-descriptor branch from 9d034a5 to ccbd8ce Compare December 7, 2020 17:03

QiWang19 force-pushed the multi-descriptor branch from ccbd8ce to 65565fb Compare December 9, 2020 19:40

mtrmac reviewed Dec 10, 2020

View reviewed changes

QiWang19 force-pushed the multi-descriptor branch from 65565fb to 852ed98 Compare December 23, 2020 00:46

QiWang19 added 2 commits December 23, 2020 12:28

load/save oci-archive multi images

a9f02e7

Allow load/save oci-archive format multi images. Signed-off-by: Qi Wang <[email protected]>

vendor c/image pr

45e424a

Signed-off-by: Qi Wang <[email protected]>

QiWang19 force-pushed the multi-descriptor branch from 852ed98 to 45e424a Compare December 23, 2020 17:30

github-actions bot added the stale-pr label Feb 4, 2021

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2021

github-actions bot removed the stale-pr label May 18, 2021

vrothberg closed this May 31, 2021

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023

[WIP] load/save oci-archive multi images #8218

[WIP] load/save oci-archive multi images #8218

Conversation

QiWang19 commented Nov 2, 2020 • edited Loading

openshift-ci-robot commented Nov 2, 2020

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

QiWang19 commented Nov 23, 2020

mtrmac left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangguanzhang commented Dec 3, 2020

QiWang19 commented Dec 3, 2020

mtrmac commented Dec 3, 2020

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 Dec 3, 2020

zhangguanzhang commented Dec 7, 2020

QiWang19 commented Dec 8, 2020

rhatdan commented Dec 9, 2020

zhangguanzhang commented Dec 10, 2020

mtrmac commented Dec 10, 2020

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

QiWang19 commented Jan 4, 2021

github-actions bot commented Feb 4, 2021

openshift-ci-robot commented Apr 11, 2021

vrothberg commented May 4, 2021

QiWang19 commented May 5, 2021

vrothberg commented May 31, 2021

QiWang19 commented Nov 2, 2020 •

edited

Loading

mtrmac left a comment •

edited

Loading