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

OCI-archive multi-manifest support POC #1178

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Oct 6, 2022

This is containers/image#1677 + #921, updated to merge on top of current main, + an attempt to resolve review comments, and a fairly intrusive set of changes to actually implement pulling as expected.

Note that this depends on LoadManifestDescriptor being able to benefit from archive.Reader. Alternatively, we could introduce some other API with a similar effect (have NewReaderForReference directly return the manifest descriptor?)

umohnani8 and others added 10 commits March 2, 2022 19:58
Signed-off-by: Urvashi Mohnani <umohnani@redhat.com>
Signed-off-by: Urvashi Mohnani <umohnani@redhat.com>
…i-poc

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
It can only accept a path, so don't round-trip through an ImageReference.

(Alternatively, this could use a similar heuristic to loadMultiImageDockerArchive,
stat()in the path. But even in that case it should first make a decision and _then_
potentially create a reference.)

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Model it more directly on the docker-archive logic.

Pulls specify a single ref, and use all parts of that to choose a single image.
Loads load all of the archive.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Introduce copyFromOCIArchiveReaderReferenceAndManifestDescriptor and
storageReferenceFromOCIArchiveDescriptor , and use that so that
we don't need to load manifest descriptors which we already have
readily available.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 6, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mtrmac
Once this PR has been reviewed and has the lgtm label, please assign luap99 for approval by writing /assign @luap99 in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@mtrmac mtrmac marked this pull request as draft October 6, 2022 21:43
@mtrmac
Copy link
Collaborator Author

mtrmac commented Oct 6, 2022

⚠️ Absolutely untested in practice.

@mtrmac
Copy link
Collaborator Author

mtrmac commented Oct 6, 2022

See containers/image#1381 (comment) .

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@openshift-merge-robot
Copy link
Collaborator

@mtrmac: 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.

@rhatdan
Copy link
Member

rhatdan commented Nov 5, 2023

@mtrmac should this be closed or updated?

@mtrmac
Copy link
Collaborator Author

mtrmac commented Nov 6, 2023

The feature is 80–90 % done, so abandoning it seems like a waste.

OTOH it has been a long time, and by now at least the c/image part requires a non-trivial rebase.

@rhatdan
Copy link
Member

rhatdan commented Dec 16, 2023

Now that @flouthoc is not with Red Hat any longer, do you still think we are going to go forward with this, rather then just changing the default to zstd:chunked?

@mtrmac
Copy link
Collaborator Author

mtrmac commented Dec 20, 2023

@rhatdan This has no relationship to zstd (of any kind) at all.

I think it’s a useful feature, but features get added one at a time depending on priorities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants