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

WIP: oci/layout API extensions #2567

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

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Sep 10, 2024

Includes @sourceIndex code from #1677 .

Do not merge:

  • At least the transport part must have tests (probably mostly from 1677).
  • Do we want to add this to oci/archive at the same time? Structure oci/internal accordingly either way.

@mtrmac mtrmac force-pushed the layout-list branch 2 times, most recently from 13d3ebf to 41446dc Compare September 10, 2024 18:48
@mtrmac mtrmac added the kind/feature A request for, or a PR adding, new functionality label Sep 18, 2024
@mtrmac mtrmac mentioned this pull request Sep 20, 2024
@mtrmac mtrmac linked an issue Sep 20, 2024 that may be closed by this pull request
@mtrmac mtrmac changed the title WIP: Add oci/layout.List WIP: oci/layout API extensions Sep 24, 2024
@mtrmac
Copy link
Collaborator Author

mtrmac commented Sep 24, 2024

Added PutBlobFromLocalFile, just to sketch out what an API benefiting from reflinks and the like could look like.

}
defer reader.Close()

// This makes a full copy; instead, if possible, we could only digest the file and reflink (hard link?)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note to self: c/storage/drivers/copy.CopyRegular…

@baude
Copy link
Member

baude commented Sep 25, 2024

i accidentally deleted my earlier comment, but I have been using the layout.List portion of this PR and it is working nicely. Will test the latest addition here soon.

Port all tests from containers#1677 ,
and see what else!

Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
WIP: For this to actually make any sense, it should be able to
avoid the copy.

Signed-off-by: Miloslav Trmač <[email protected]>
// It computes, and returns, the digest and size of the used file.
//
// This function can be used instead of dest.PutBlob() where the ImageDestination requires PutBlob() to be called.
func PutBlobFromLocalFile(ctx context.Context, dest types.ImageDestination, file string) (digest.Digest, int64, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we need a means to configure the mime type of the blob to serve artifacts. Going forward, I anticipate users to create their custom layer types for LLM etc.

Copy link
Collaborator Author

@mtrmac mtrmac Nov 14, 2024

Choose a reason for hiding this comment

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

The way the ImageDestination API works, PutBlob doesn’t typically need to worry about the MIME type: the MIME type comes via PutManifest.

While it is true that the PutBlobWithOptions gets a full types.BlobInfo, and

blobInfo, diffIDChan, err := ic.copyLayerFromStream(ctx, srcStream, types.BlobInfo{Digest: srcInfo.Digest, Size: srcBlobSize, MediaType: srcInfo.MediaType, Annotations: srcInfo.Annotations}, diffIDIsNeeded, toEncrypt, bar, layerIndex, emptyLayer)
forwards the MIME type value into the layer copy code, there are things like
stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info?
just dropping that; so, transports can’t rely on a MIME type being present (~instead, we have PutBlobOptions.IsConfig).

Also, specifically in the OCI transport, the PutBlob* code really doesn’t care about the MIME type. It is going to create a sha256-named file, and as far as the layout definition is concerned, it’s perfectly valid to refer to a single blob in two different manifests using two different MIME types. So, for this OCI-transport-specific function, I don’t think adding a MIME type is right.

@vrothberg
Copy link
Member

Opened #2633 to kick this PR over the finish line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature A request for, or a PR adding, new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add oci/layout.Reader
4 participants