Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix saving in oci format #6814

Merged
merged 1 commit into from
Jul 9, 2020
Merged

Conversation

QiWang19
Copy link
Contributor

@QiWang19 QiWang19 commented Jun 29, 2020

Signed-off-by: Daniel J Walsh [email protected]
Signed-off-by: Qi Wang [email protected]

@QiWang19
Copy link
Contributor Author

replace #6562

@@ -273,7 +274,13 @@ func (r *Runtime) LoadImage(ctx context.Context, name, inputFile string, writer
newImages, err = r.ImageRuntime().LoadFromArchiveReference(ctx, src, signaturePolicy, writer)
}
if err != nil {
return "", errors.Wrapf(err, "error pulling %q", name)
src, err := layout.NewReference(inputFile, "")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtrmac PTAL
src, err := layout.NewReference(inputFile, ""), why thepodman load can work with "", but error if pass to name?
unable to pull oci:input_file:alpine: Error initializing source oci:input_file:alpine: no descriptor found for reference "alpine"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@QiWang19 "" means “verify that the index contains only one manifest, and use that one”. Passing a name allows reading from an index that contains any number of manifests, but the name must match the org.opencontainers.image.ref.name annotation exactly (that annotation is a string with allowed characters and no defined normalization semantics).

So, what’s the value of that annotation in the created index? Is it there at all? Does it perhaps say localhost/alpine or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm,yes, if I load from the oci-dir it was saved, it says localhost/alpine

$ podman save --format oci-dir -o podman-dir alpine
$ podman load -i podman-dir alpine

$ podman images
REPOSITORY            TAG     IMAGE ID      CREATED      SIZE
localhost/podman-dir  latest  0f5f445df8cc  4 weeks ago  5.85 MB
localhost/alpine      latest  0f5f445df8cc  4 weeks ago  5.85 MB

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s messy… the local image is named localhost/alpine in c/storage, and we do want to find that image when the user says alpine. So, using the localhost/alpine name when writing to the archive makes some sense.

OTOH when reading from the archive it’s not at all clean to automatically change the name, because the names in OCI don’t have any semantics like that.

Conceptually I think it would be clean to use just alpine when writing to oci-dir:, and because it is basically new with this PR, we could do that — but it is more important to me that oci-dir: and oci-archive: behave consistently, and we can’t quite change the behavior of oci-archive: now.

I suppose we could, when reading from the archive, try both the user input unmodified and user input with localhost/ prepended if there is no registry component.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing a name allows reading from an index that contains any number of manifests, but the name must match the org.opencontainers.image.ref.name annotation exactly (that annotation is a string with allowed characters and no defined normalization semantics).

The passing name error happens if org.opencontainers.image.ref.name contains a tag, it this expected?

podman save --format oci-dir -o podman-dir docker.io/library/alpine:3.5
podman load -i podman-dir docker.io/library/alpine:3.5
Error initializing source oci:podman-dir:docker.io/library/alpine: no descriptor found for reference "docker.io/library/alpine"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn’t look expected — but it also suggests that the tag was stripped somewhere in Podman.

@rhatdan
Copy link
Member

rhatdan commented Jul 1, 2020

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 1, 2020
libpod/runtime_img.go Outdated Show resolved Hide resolved
libpod/runtime_img.go Outdated Show resolved Hide resolved
libpod/runtime_img.go Outdated Show resolved Hide resolved
libpod/runtime_img.go Outdated Show resolved Hide resolved
@QiWang19
Copy link
Contributor Author

QiWang19 commented Jul 2, 2020

@mtrmac PTAL

@QiWang19 QiWang19 force-pushed the oci-dir branch 2 times, most recently from 85dcc38 to 42ef200 Compare July 7, 2020 20:48
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, just nits.

libpod/runtime_img.go Outdated Show resolved Hide resolved
libpod/runtime_img.go Outdated Show resolved Hide resolved
libpod/runtime_img.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

- fix saving&loading oci format. Close containers#6544
- support loading using image name without "localhost/" prefix when reading from ociarchive/dir saved from this semantics

Signed-off-by: Daniel J Walsh <[email protected]>
Signed-off-by: Qi Wang <[email protected]>
@QiWang19
Copy link
Contributor Author

QiWang19 commented Jul 9, 2020

@rhatdan @mheon @vrothberg PTAL

@rhatdan
Copy link
Member

rhatdan commented Jul 9, 2020

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2020
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: QiWang19, rhatdan

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-merge-robot openshift-merge-robot merged commit f86782c into containers:master Jul 9, 2020
@github-actions 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 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Podman save oci-dir produces non-OCI layout
5 participants