-
Notifications
You must be signed in to change notification settings - Fork 372
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
Add reader/writer to for oci-archive multi image #1072
Conversation
oci/archive/reader.go
Outdated
tempDirOCIRef | ||
} | ||
|
||
func CreateUntarTempDirReader(src string, sys *types.SystemContext) (*reader, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs are missing
oci/archive/reader.go
Outdated
|
||
// List returns a list of ReferenceWrapper for images in the reader. | ||
// the ImageReferences of the ReferenceWrapper are valid only until the Reader is closed. | ||
func (r *reader) List() ([]ReferenceWrapper, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at docker-archive
, I think we should not create a custom ReferenceWrapper
but return a [][]types.ImageReference
. With the first array determining the amount of images and the second one the "names".
docker-archive
supports loading by index and by name and I think that's what we should do here as well to be consistent. The docker-archive parts are in docker/archive/reader.go
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vrothberg Could you elaborate on what's loading by index and name?
lloading by index, the usage is load -i images.tar:@source-index
, the source-index should be one of the digests from manifests in index.json, like sha256:e2cb60c4b307f3253254276da1d93e5edb32c3ddc0c40b40333def88eb48bf5f
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A poadman load -i
always expects a file, so we can't use the transport syntax. But we can individually pull/extract images from an archive. To extract an image (note we can do this with podman pull
as well), we need to specify which one. We can specify the image either via a docker-reference (without digest) or by it's index in the list of manifests.
Examples are the man page (https://github.com/containers/image/blob/master/docs/containers-transports.5.md#docker-archivepathdocker-referencesource-index) or the tests in podman that exercise this code (containers/podman@7fea46752cbf#diff-36d4ee0b4d8927b38ddbff8a5648df51ebf0e9289fcb0d37e0dedc2ffa3ee161R173).
194951a
to
3ef46bb
Compare
@vrothberg @mtrmac PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, @QiWang19
oci/archive/oci_transport.go
Outdated
if strings.HasPrefix(image, "@") { | ||
idx, err := strconv.Atoi(image[1:]) | ||
if err != nil { | ||
return nil, errors.Wrapf(err, "Invalid source index %s", image) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, errors.Wrapf(err, "Invalid source index %s", image) | |
return nil, errors.Wrapf(err, "Invalid source index @%s: not an integer", image[1:]) |
oci/layout/oci_transport.go
Outdated
} | ||
|
||
// NewReference returns an OCI reference for a directory and a image. | ||
// | ||
// We do not expose an API supplying the resolvedDir; we could, but recomputing it | ||
// is generally cheap enough that we prefer being confident about the properties of resolvedDir. | ||
func NewReference(dir, image string) (types.ImageReference, error) { | ||
func NewReference(dir, image string, sourceIndex int) (types.ImageReference, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's technically breaking the API. Can we let this default to 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a new NewReferenceWithIndex
?
3ef46bb
to
52ca48d
Compare
oci/archive/writer.go
Outdated
"os" | ||
|
||
"github.com/containers/image/v5/internal/tmpdir" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not have blank line.
oci/layout/oci_transport.go
Outdated
// | ||
// We do not expose an API supplying the resolvedDir; we could, but recomputing it | ||
// is generally cheap enough that we prefer being confident about the properties of resolvedDir. | ||
func NewReferenceWithIndex(dir, image string, sourceIndex int) (types.ImageReference, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NewReference() Should be calling this function, rather then duplicating the code.
52ca48d
to
1e6dc9b
Compare
@vrothberg @mtrmac PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is a definitely a welcome feature.
bf2318e
to
94e6444
Compare
@mtrmac PTAL |
85815ec
to
2196004
Compare
@mtrmac PTAL |
2196004
to
8f570d1
Compare
Add read/writer with helpers to allow podman save/load multi oci-archive images. Allow read oci-archive using source_index to point to the an inedx from oci-archive manifest. Signed-off-by: Qi Wang <qiwan@redhat.com>
8f570d1
to
e3f17d3
Compare
@mtrmac PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick partial read for now I’m afraid, focusing at least on the public API.
I’d still prefer to re-implement ociArchiveImage{Source,Destination}
in terms of the Reader
/Writer
, rather than making Reader
/Writer
a completely separate path that creates oci/layout references, per #1072 (comment) .
|
||
// Close converts the data about images in the temp directory to the archive and | ||
// deletes temporary files associated with the Writer | ||
func (w *Writer) Close() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this have a context.Context
, to eventually allow aborting that write? OTOH that would make this not conform to io.Closer
.
Ugh.
Alternatively, pass a context.Context
to NewWriter
would be an option, violating the usual rules of use of context.Context
(OTOH it wouldn’t be the first time IIRC).
*shrug* Maybe we should defer solving that for now, we can always add a CloseWithContext
or something like that when actually adding a cancellable implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A full review now.
633420a
to
e8670de
Compare
e8670de
to
b7df36a
Compare
Signed-off-by: Qi Wang <qiwan@redhat.com>
b7df36a
to
bdf582f
Compare
@mtrmac PTAL |
@vrothberg PTAL |
Add read/writer with helpers to allow podman save/load multi oci-archive images.
work with PR: containers/podman#8218
close: containers/podman#4646
Signed-off-by: Qi Wang qiwan@redhat.com